From 367b4e38babae444195c2f341aa816a6ba1a8c67 Mon Sep 17 00:00:00 2001 From: Fabian Steeg Date: Tue, 22 Feb 2022 17:55:30 +0100 Subject: [PATCH] Remove appendTo method, hide FixPath behind record (#102, #92, #89) Replace replace*/append* methods with insert* and InsertMode usage Use get / set / add / remove methods on Record for interaction --- .../java/org/metafacture/metafix/FixBind.java | 2 +- .../org/metafacture/metafix/FixMethod.java | 32 ++++--- .../java/org/metafacture/metafix/FixPath.java | 84 +++++++------------ .../java/org/metafacture/metafix/Record.java | 38 ++++++++- .../java/org/metafacture/metafix/Value.java | 16 ++-- .../metafacture/metafix/api/FixPredicate.java | 3 +- .../metafix/MetafixRecordTest.java | 11 +-- .../metafacture/metafix/util/TestContext.java | 6 +- .../metafix/util/TestFunction.java | 4 +- 9 files changed, 107 insertions(+), 89 deletions(-) diff --git a/metafix/src/main/java/org/metafacture/metafix/FixBind.java b/metafix/src/main/java/org/metafacture/metafix/FixBind.java index f79155e4..9fad7c9f 100644 --- a/metafix/src/main/java/org/metafacture/metafix/FixBind.java +++ b/metafix/src/main/java/org/metafacture/metafix/FixBind.java @@ -29,7 +29,7 @@ public enum FixBind implements FixContext { public void execute(final Metafix metafix, final Record record, final List params, final Map options, final List expressions) { final RecordTransformer recordTransformer = metafix.getRecordTransformer(); final String scopeVariable = options.get("var"); - Value.asList(new FixPath(options.get("path")).findIn(record), a -> { + Value.asList(record.get(options.get("path")), a -> { for (int i = 0; i < a.size(); ++i) { final Value value = a.get(i); diff --git a/metafix/src/main/java/org/metafacture/metafix/FixMethod.java b/metafix/src/main/java/org/metafacture/metafix/FixMethod.java index 7add5b81..ed36d0ff 100644 --- a/metafix/src/main/java/org/metafacture/metafix/FixMethod.java +++ b/metafix/src/main/java/org/metafacture/metafix/FixMethod.java @@ -99,7 +99,7 @@ public void apply(final Metafix metafix, final Record record, final List add_field { @Override public void apply(final Metafix metafix, final Record record, final List params, final Map options) { - new FixPath(params.get(0)).appendIn(record, params.get(1)); + record.add(params.get(0), new Value(params.get(1))); } }, array { // array-from-hash @@ -120,7 +120,9 @@ public void apply(final Metafix metafix, final Record record, final List copy_field { @Override public void apply(final Metafix metafix, final Record record, final List params, final Map options) { - record.copy(params); + final String oldName = params.get(0); + final String newName = params.get(1); + Value.asList(record.get(oldName), a -> a.forEach(v -> record.add(newName, v))); } }, format { @@ -149,8 +151,12 @@ public void apply(final Metafix metafix, final Record record, final List move_field { @Override public void apply(final Metafix metafix, final Record record, final List params, final Map options) { - record.copy(params); - new FixPath(params.get(0)).removeNestedFrom(record); + final String oldName = params.get(0); + final String newName = params.get(1); + Value.asList(record.get(oldName), a -> a.forEach(v -> { + record.add(newName, v); + record.remove(oldName); + })); } }, parse_text { @@ -195,10 +201,11 @@ public void apply(final Metafix metafix, final Record record, final List @Override public void apply(final Metafix metafix, final Record record, final List params, final Map options) { final String joinChar = options.get("join_char"); - new FixPath(params.get(0)).replaceIn(record, new Value(params.subList(1, params.size()).stream() - .filter(f -> literalString(f) || new FixPath(f).findIn(record) != null) - .map(f -> literalString(f) ? new Value(f.substring(1)) : Value.asList(new FixPath(f).findIn(record), null).asArray().get(0)) - .map(Value::asString).collect(Collectors.joining(joinChar != null ? joinChar : " ")))); + final Value newValue = new Value(params.subList(1, params.size()).stream() + .filter(f -> literalString(f) || record.get(f) != null) + .map(f -> literalString(f) ? new Value(f.substring(1)) : Value.asList(record.get(f), null).asArray().get(0)) + .map(Value::asString).collect(Collectors.joining(joinChar != null ? joinChar : " "))); + record.set(params.get(0), newValue); } private boolean literalString(final String s) { @@ -210,8 +217,7 @@ private boolean literalString(final String s) { public void apply(final Metafix metafix, final Record record, final List params, final Map options) { final String field = params.get(0); final int max = getInteger(params, 1); - - new FixPath(field).replaceIn(record, new Value(String.valueOf(RANDOM.nextInt(max)))); + record.set(field, new Value(String.valueOf(RANDOM.nextInt(max)))); } }, reject { @@ -268,13 +274,13 @@ public void apply(final Metafix metafix, final Record record, final List public void apply(final Metafix metafix, final Record record, final List params, final Map options) { final String field = params.get(0); final Value newValue = newArray(params.subList(1, params.size()).stream().map(Value::new)); - new FixPath(field).replaceIn(record, newValue); + record.set(field, newValue); } }, set_field { @Override public void apply(final Metafix metafix, final Record record, final List params, final Map options) { - new FixPath(params.get(0)).replaceIn(record, new Value(params.get(1))); + record.set(params.get(0), new Value(params.get(1))); } }, set_hash { @@ -282,7 +288,7 @@ public void apply(final Metafix metafix, final Record record, final List public void apply(final Metafix metafix, final Record record, final List params, final Map options) { final String field = params.get(0); final Value newValue = Value.newHash(h -> options.forEach((f, v) -> h.put(f, new Value(v)))); - new FixPath(field).replaceIn(record, newValue); + record.set(field, newValue); } }, vacuum { diff --git a/metafix/src/main/java/org/metafacture/metafix/FixPath.java b/metafix/src/main/java/org/metafacture/metafix/FixPath.java index 6b7adf45..92b0bc2e 100644 --- a/metafix/src/main/java/org/metafacture/metafix/FixPath.java +++ b/metafix/src/main/java/org/metafacture/metafix/FixPath.java @@ -35,12 +35,12 @@ * @author Fabian Steeg (fsteeg) * */ -public class FixPath { +/*package-private*/ class FixPath { private static final String ASTERISK = "*"; private String[] path; - public FixPath(final String path) { + /*package-private*/ FixPath(final String path) { this(Value.split(path)); } @@ -48,7 +48,7 @@ private FixPath(final String[] path) { this.path = path; } - public Value findIn(final Hash hash) { + /*package-private*/ Value findIn(final Hash hash) { final String currentSegment = path[0]; final FixPath remainingPath = new FixPath(tail(path)); @@ -101,38 +101,16 @@ private Value findInValue(final Value value, final String[] p) { ); } - public Value replaceIn(final Hash hash, final Value newValue) { - return new FixPath(path).insertInto(hash, InsertMode.REPLACE, newValue); - } - - public Value appendIn(final Hash hash, final String newValue) { - return new FixPath(path).insertInto(hash, InsertMode.APPEND, new Value(newValue)); - } - - /*package-private*/ void appendIn(final Hash hash, final Value v) { - // TODO: impl and call just value.append - if (v != null) { - v.matchType() - .ifString(s -> appendIn(hash, s)) - //.ifArray(a -> /* TODO: see MetafixMethodTest.moveToNestedArray */) - .ifHash(h -> { - if (path.length == 1) { - hash.add(path[0], v); - } - else { - appendIn(hash, new FixPath(tail(path)).findIn(h)); - } - }) - .orElseThrow(); - } - } - @Override public String toString() { return Arrays.asList(path).toString(); } - public void transformIn(final Hash hash, final UnaryOperator operator) { + /*package-private*/ int size() { + return path.length; + } + + /*package-private*/ void transformIn(final Hash hash, final UnaryOperator operator) { // basic idea: reuse findIn logic here? setIn(hash, operator.apply(findIn(hash))) final String currentSegment = path[0]; final String[] remainingPath = tail(path); @@ -152,7 +130,7 @@ public void transformIn(final Hash hash, final UnaryOperator operator) { if (operator != null) { value.matchType() - .ifString(s -> new FixPath(f).appendIn(hash, operator.apply(s))) + .ifString(s -> new FixPath(f).insertInto(hash, InsertMode.APPEND, new Value(operator.apply(s)))) .orElseThrow(); } } @@ -166,6 +144,25 @@ public void transformIn(final Hash hash, final UnaryOperator operator) { }); } + /* package-private */ enum InsertMode { + + REPLACE { + @Override + void apply(final Hash hash, final String field, final Value value) { + hash.put(field, value); + } + }, + APPEND { + @Override + void apply(final Hash hash, final String field, final Value value) { + hash.add(field, value); + } + }; + + abstract void apply(Hash hash, String field, Value value); + + } + /*package-private*/ void transformIn(final Hash hash, final BiConsumer> consumer) { final Value oldValue = findIn(hash); @@ -173,7 +170,7 @@ public void transformIn(final Hash hash, final UnaryOperator operator) { final Value newValue = oldValue.extractType(consumer); if (newValue != null) { - new FixPath(path).insertInto(hash, InsertMode.REPLACE, newValue); + insertInto(hash, InsertMode.REPLACE, newValue); } } } @@ -204,25 +201,6 @@ else if (Value.isNumber(currentSegment)) { array.removeIf(v -> Value.isNull(v)); } - private enum InsertMode { - - REPLACE { - @Override - void apply(final Hash hash, final String field, final Value value) { - hash.put(field, value); - } - }, - APPEND { - @Override - void apply(final Hash hash, final String field, final Value value) { - hash.add(field, value); - } - }; - - abstract void apply(Hash hash, String field, Value value); - - } - /*package-private*/ void removeNestedFrom(final Array array) { if (path.length >= 1 && path[0].equals(ASTERISK)) { array.removeAll(); @@ -253,7 +231,7 @@ else if (hash.containsField(field)) { } } - /*package-private*/ Value insertInto(final Array array, final InsertMode mode, final Value newValue) { + /*package-private*/ private Value insertInto(final Array array, final InsertMode mode, final Value newValue) { // basic idea: reuse findIn logic here? setIn(findIn(array), newValue) final String field = path[0]; if (path.length == 1) { @@ -282,7 +260,7 @@ else if (hash.containsField(field)) { //TODO: WDCD? insert into each element? } else { - (mode != null ? mode : InsertMode.APPEND).apply(hash, field, newValue); + mode.apply(hash, field, newValue); } } else { diff --git a/metafix/src/main/java/org/metafacture/metafix/Record.java b/metafix/src/main/java/org/metafacture/metafix/Record.java index b0805a18..575b79d4 100644 --- a/metafix/src/main/java/org/metafacture/metafix/Record.java +++ b/metafix/src/main/java/org/metafacture/metafix/Record.java @@ -16,9 +16,12 @@ package org.metafacture.metafix; +import org.metafacture.metafix.FixPath.InsertMode; + import java.util.Collection; import java.util.LinkedHashMap; import java.util.Map; +import java.util.function.BiConsumer; /** * Represents a metadata record, i.e., a {@link Value.Hash Hash} of fields @@ -112,7 +115,20 @@ public String toString() { */ @Override public Value get(final String field) { - return containsField(field) ? super.get(field) : virtualFields.get(field); + final Value result; + if (containsField(field)) { + result = super.get(field); + } + else { + final FixPath fixPath = new FixPath(field); + if (fixPath.size() > 1) { + result = fixPath.findIn(this); + } + else { + result = virtualFields.get(field); + } + } + return result; } /** @@ -129,10 +145,28 @@ public void add(final String field, final Value newValue) { super.add(field, newValue); } else { - put(field, newValue); + final FixPath fixPath = new FixPath(field); + if (fixPath.size() > 1) { + fixPath.insertInto(this, InsertMode.APPEND, newValue); + } + else { + put(field, newValue); + } } } + /** + * Sets a field/value pair to this record, replacing + * any previous association of the field with a value. + * + * @param field the field name + * @param newValue the new metadata value + */ + public void set(final String field, final Value newValue) { + final FixPath fixPath = new FixPath(field); + fixPath.insertInto(this, InsertMode.REPLACE, newValue); + } + /** * Retains only the given field/value pairs in this record. Turns * virtual fields into regular metadata fields if they're not already diff --git a/metafix/src/main/java/org/metafacture/metafix/Value.java b/metafix/src/main/java/org/metafacture/metafix/Value.java index 1f601bbb..7efa9030 100644 --- a/metafix/src/main/java/org/metafacture/metafix/Value.java +++ b/metafix/src/main/java/org/metafacture/metafix/Value.java @@ -537,7 +537,7 @@ public void addAll(final Hash hash) { * @param newValue the new metadata value */ public void add(final String field, final Value newValue) { - final Value oldValue = get(field); + final Value oldValue = new FixPath(field).findIn(this); put(field, oldValue == null ? newValue : oldValue.asList(a1 -> newValue.asList(a2 -> a2.forEach(a1::add)))); } @@ -547,19 +547,19 @@ public void add(final String field, final Value newValue) { * @param field the field name */ public void remove(final String field) { - modifyFields(field, this::removeField); + final FixPath fixPath = new FixPath(field); + if (fixPath.size() > 1) { + fixPath.removeNestedFrom(this); + } + else { + modifyFields(field, this::removeField); + } } public void removeField(final String field) { map.remove(field); } - public void copy(final List params) { - final String oldName = params.get(0); - final String newName = params.get(1); - asList(new FixPath(oldName).findIn(this), a -> a.forEach(v -> new FixPath(newName).appendIn(this, v))); - } - /** * Retains only the given field/value pairs in this hash. * diff --git a/metafix/src/main/java/org/metafacture/metafix/api/FixPredicate.java b/metafix/src/main/java/org/metafacture/metafix/api/FixPredicate.java index 7b7ebab1..d2c3cd4a 100644 --- a/metafix/src/main/java/org/metafacture/metafix/api/FixPredicate.java +++ b/metafix/src/main/java/org/metafacture/metafix/api/FixPredicate.java @@ -16,7 +16,6 @@ package org.metafacture.metafix.api; -import org.metafacture.metafix.FixPath; import org.metafacture.metafix.Metafix; import org.metafacture.metafix.Record; import org.metafacture.metafix.Value; @@ -43,7 +42,7 @@ default boolean testConditional(final Record record, final List params, final String field = params.get(0); final String string = params.get(1); - final Value value = new FixPath(field).findIn(record); + final Value value = record.get(field); return value != null && qualifier.test(value.asList(null).asArray().stream(), v -> v.extractType((m, c) -> m .ifString(s -> c.accept(conditional.test(s, string))) .orElse(w -> c.accept(false)) diff --git a/metafix/src/test/java/org/metafacture/metafix/MetafixRecordTest.java b/metafix/src/test/java/org/metafacture/metafix/MetafixRecordTest.java index e57fa6a2..af922f3c 100644 --- a/metafix/src/test/java/org/metafacture/metafix/MetafixRecordTest.java +++ b/metafix/src/test/java/org/metafacture/metafix/MetafixRecordTest.java @@ -426,7 +426,7 @@ public void complexAppendWithArrayOfStrings() { } @Test - @MetafixToDo("See https://github.com/metafacture/metafacture-fix/issues/92") + // See https://github.com/metafacture/metafacture-fix/issues/92 public void complexAppendWithArrayOfObjects() { MetafixTestHelpers.assertFix(streamReceiver, Arrays.asList( "copy_field('others', 'animals[].$append')", @@ -650,7 +650,7 @@ public void appendWithAsteriksWildcardAtTheEnd() { i.literal("ANIMALS", "dragon and unicorn"); i.endRecord(); }, - o -> { + (o, f) -> { o.get().startRecord("1"); o.get().literal("animals", "dog"); o.get().literal("animals", "cat"); @@ -666,9 +666,10 @@ public void appendWithAsteriksWildcardAtTheEnd() { o.get().literal("2", "cat"); o.get().literal("3", "zebra"); o.get().literal("4", "bunny"); - // TODO: Why is the hash (`animols`) not expected here? - // See also https://github.com/metafacture/metafacture-fix/issues/89#issuecomment-999433570 - o.get().endEntity(); + o.get().startEntity("5"); + o.get().literal("name", "bird"); + o.get().literal("type", "TEST"); + f.apply(2).endEntity(); o.get().endRecord(); } ); diff --git a/metafix/src/test/java/org/metafacture/metafix/util/TestContext.java b/metafix/src/test/java/org/metafacture/metafix/util/TestContext.java index 81a13c4e..f81cc8a9 100644 --- a/metafix/src/test/java/org/metafacture/metafix/util/TestContext.java +++ b/metafix/src/test/java/org/metafacture/metafix/util/TestContext.java @@ -16,9 +16,9 @@ package org.metafacture.metafix.util; -import org.metafacture.metafix.FixPath; import org.metafacture.metafix.Metafix; import org.metafacture.metafix.Record; +import org.metafacture.metafix.Value; import org.metafacture.metafix.api.FixContext; import org.metafacture.metafix.fix.Expression; @@ -32,9 +32,9 @@ public TestContext() { @Override public void execute(final Metafix metafix, final Record record, final List params, final Map options, final List expressions) { - new FixPath("BEFORE").appendIn(record, params.get(0)); + record.add("BEFORE", new Value(params.get(0))); metafix.getRecordTransformer().process(expressions); - new FixPath("AFTER").appendIn(record, options.get("data")); + record.add("AFTER", new Value(options.get("data"))); } } diff --git a/metafix/src/test/java/org/metafacture/metafix/util/TestFunction.java b/metafix/src/test/java/org/metafacture/metafix/util/TestFunction.java index 8d69e2df..5db0d7f1 100644 --- a/metafix/src/test/java/org/metafacture/metafix/util/TestFunction.java +++ b/metafix/src/test/java/org/metafacture/metafix/util/TestFunction.java @@ -17,9 +17,9 @@ package org.metafacture.metafix.util; import org.metafacture.metafix.FixMethod; -import org.metafacture.metafix.FixPath; import org.metafacture.metafix.Metafix; import org.metafacture.metafix.Record; +import org.metafacture.metafix.Value; import org.metafacture.metafix.api.FixFunction; import java.util.List; @@ -36,7 +36,7 @@ public void apply(final Metafix metafix, final Record record, final List params.set(0, "test"); FixMethod.add_field.apply(metafix, record, params, options); - options.forEach((k, v) -> new FixPath(k).appendIn(record, v)); + options.forEach((k, v) -> record.set(k, new Value(v))); } }