Skip to content

Commit 2834fb8

Browse files
fsteegfsteeg
committed
Remove path replacement workarounds, only set and append paths
Append to path when adding values, set base path for nested values. This seems to resolves the issue in hbz/lobid-resources#1354, see unit and integration tests, which are now passing, but tweaked (unit test for output structure, integration test for field order).
1 parent f257f31 commit 2834fb8

File tree

12 files changed

+70
-108
lines changed

12 files changed

+70
-108
lines changed

‎metafix/src/main/java/org/metafacture/metafix/FixMethod.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -120,11 +120,11 @@ public void apply(final Metafix metafix, final Record record, final List<String>
120120
public void apply(final Metafix metafix, final Record record, final List<String> params, final Map<String, String> options) {
121121
final String oldName = params.get(0);
122122
final String newName = params.get(1);
123-
Value.asList(record.get(oldName), a -> a.forEach(newValue -> {
124-
record.addNested(newName, newValue);
125-
newValue.updatePathRename(newName);
123+
Value.asList(record.get(oldName), a -> a.forEach(oldValue -> {
124+
record.addNested(newName, oldValue); // we're actually aliasing
126125
}));
127126
}
127+
128128
},
129129
format {
130130
@Override

‎metafix/src/main/java/org/metafacture/metafix/FixPath.java

Lines changed: 10 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,6 @@
2222
import java.util.Arrays;
2323
import java.util.HashMap;
2424
import java.util.Map;
25-
import java.util.regex.Pattern;
26-
import java.util.stream.Collectors;
2725

2826
/**
2927
* Our goal here is something like https://metacpan.org/pod/Catmandu::Path::simple
@@ -35,8 +33,6 @@
3533
*/
3634
/*package-private*/ class FixPath {
3735

38-
/*package-private*/ static final Pattern RESERVED_FIELD_PATTERN = Pattern.compile(String.format("(?:%s)",
39-
Arrays.stream(ReservedField.values()).map(f -> Pattern.quote(f.name())).collect(Collectors.joining("|"))));
4036
private static final String ASTERISK = "*";
4137
private String[] path;
4238

@@ -81,13 +77,13 @@ private FixPath(final String[] path) {
8177
if (findInValue != null) {
8278
findInValue.matchType()
8379
// flatten result arrays (use Value#path for structure)
84-
.ifArray(a -> a.forEach(resultArray::add))
85-
.orElse(c -> resultArray.add(findInValue));
80+
.ifArray(a -> a.forEach(t -> resultArray.add(t, false)))
81+
.orElse(c -> resultArray.add(findInValue, false));
8682
}
8783
}));
8884
}
8985
else if (isReference(currentSegment)) {
90-
final Value referencedValue = getReferencedValue(array, currentSegment);
86+
final Value referencedValue = getReferencedValue(array, currentSegment, null);
9187
if (referencedValue != null) {
9288
result = findInValue(referencedValue, tail(path));
9389
}
@@ -262,7 +258,7 @@ private void removeNestedFrom(final Value value) {
262258
array.forEach(value -> insertInto(value, mode, newValue.copy(), field, tail(path)));
263259
}
264260
else if (isReference(field)) {
265-
insertInto(getReferencedValue(array, field), mode, newValue, field, tail(path));
261+
insertInto(getReferencedValue(array, field, newValue.getPath()), mode, newValue, field, tail(path));
266262
}
267263
}
268264
return new Value(array);
@@ -280,22 +276,21 @@ else if (isReference(field)) {
280276
}
281277
insertInto(hash.get(field), mode, newValue, field, tail(path));
282278
}
283-
284279
return new Value(hash);
285280
}
286281

287282
private Value insertInto(final Value value, final InsertMode mode, final Value newValue, final String field,
288283
final String[] tail) {
289284
if (value != null) {
290285
final FixPath fixPath = new FixPath(tail);
291-
newValue.updatePathAddBase(value, field);
286+
newValue.withPathSet(value.getPath());
292287
return value.extractType((m, c) -> m
293288
.ifArray(a -> c.accept(fixPath.insertInto(a, mode, newValue)))
294289
.ifHash(h -> c.accept(fixPath.insertInto(h, mode, newValue)))
295290
.orElseThrow());
296291
}
297292
else {
298-
throw new IllegalArgumentException("Using ref, but can't find: " + field + " in: " + value);
293+
throw new IllegalArgumentException("Can't find: " + field + " in: " + value);
299294
}
300295
}
301296

@@ -323,7 +318,7 @@ private boolean isReference(final String field) {
323318
}
324319

325320
// TODO replace switch, extract to method on array?
326-
private Value getReferencedValue(final Array array, final String field) {
321+
private Value getReferencedValue(final Array array, final String field, final String p) {
327322
Value referencedValue = null;
328323
if (Value.isNumber(field)) {
329324
final int index = Integer.valueOf(field) - 1;
@@ -333,15 +328,14 @@ private Value getReferencedValue(final Array array, final String field) {
333328
if (reservedField != null) {
334329
switch (reservedField) {
335330
case $first:
336-
referencedValue = getReferencedValue(array, "1");
331+
referencedValue = getReferencedValue(array, "1", p);
337332
break;
338333
case $last:
339-
referencedValue = getReferencedValue(array, String.valueOf(array.size()));
334+
referencedValue = getReferencedValue(array, String.valueOf(array.size()), p);
340335
break;
341336
case $append:
342-
referencedValue = Value.newHash(); // TODO: append non-hash?
337+
referencedValue = Value.newHash().withPathSet(p); // TODO: append non-hash?
343338
array.add(referencedValue);
344-
referencedValue.updatePathAppend(String.valueOf(array.size()), "");
345339
break;
346340
default:
347341
break;

‎metafix/src/main/java/org/metafacture/metafix/Metafix.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -127,8 +127,7 @@ private void init(final Map<String, String> newVars) {
127127
@Override
128128
public void literal(final String name, final String value) {
129129
final String[] split = Value.split(name);
130-
addValue(split[split.length - 1], new Value(value, name));
131-
// TODO could this help with https://github.com/metafacture/metafacture-fix/issues/147?
130+
addValue(split[split.length - 1], new Value(value));
132131
// TODO use full path here to insert only once?
133132
// new FixPath(name).insertInto(currentRecord, InsertMode.APPEND, new Value(value));
134133
}
@@ -236,9 +235,10 @@ private void addValue(final String name, final Value value) {
236235
}
237236
else {
238237
final Value entity = entities.get(index);
238+
value.withPathSet(entity.getPath());
239239
entity.matchType()
240-
.ifArray(a -> a.add(value.updatePathAddBase(entity, name)))
241-
.ifHash(h -> h.add(name, value.updatePathAddBase(entity, name)))
240+
.ifArray(a -> a.add(value))
241+
.ifHash(h -> h.add(name, value))
242242
.orElseThrow();
243243
}
244244
}

‎metafix/src/main/java/org/metafacture/metafix/Record.java

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -209,9 +209,7 @@ public void transform(final String field, final UnaryOperator<String> operator)
209209
toDelete.addFirst(insertPath);
210210
}
211211
else {
212-
final Value newValue = new Value(newString);
213-
insertPath.insertInto(this, InsertMode.REPLACE, newValue);
214-
newValue.setPath(insertPath.toString());
212+
insertPath.insertInto(this, InsertMode.REPLACE, new Value(newString));
215213
}
216214
}
217215
toDelete.forEach(path -> path.removeNestedFrom(this));

‎metafix/src/main/java/org/metafacture/metafix/Value.java

Lines changed: 34 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -100,17 +100,12 @@ public Value(final Map<String, Value> hash) {
100100
}
101101
}
102102

103-
public Value(final String string) {
104-
this(string, null);
105-
}
106-
107103
public Value(final int integer) {
108104
this(String.valueOf(integer));
109105
}
110106

111-
public Value(final String string, final String path) {
107+
public Value(final String string) {
112108
this(string != null ? Type.String : null, null, null, string);
113-
this.path = path;
114109
}
115110

116111
public static Value newArray() {
@@ -204,35 +199,6 @@ public Value asList(final Consumer<Array> consumer) {
204199
}
205200
}
206201

207-
/*package-private*/ Value updatePathRename(final String newName) {
208-
if (path != null) {
209-
path = FixPath.RESERVED_FIELD_PATTERN.matcher(newName).replaceAll(Matcher.quoteReplacement(split(path)[0]));
210-
}
211-
212-
return this;
213-
}
214-
215-
/*package-private*/ Value updatePathAddBase(final Value container, final String fallback) {
216-
if (container.path != null) {
217-
final String[] pathSegments = split(path != null ? path : fallback);
218-
final String lastSegment = pathSegments[pathSegments.length - 1];
219-
this.path = container.path + "." + lastSegment;
220-
}
221-
222-
return this;
223-
}
224-
225-
/*package-private*/ Value updatePathAppend(final String suffix, final String fallback) {
226-
if (path != null) {
227-
path = path + suffix;
228-
}
229-
else {
230-
path = fallback + suffix;
231-
}
232-
233-
return this;
234-
}
235-
236202
public TypeMatcher matchType() {
237203
return new TypeMatcher(this);
238204
}
@@ -286,8 +252,22 @@ public String getPath() {
286252
return path;
287253
}
288254

289-
/*package-private*/ void setPath(final String path) {
290-
this.path = path;
255+
/*package-private*/ Value withPathSet(final String p) {
256+
this.path = p;
257+
return this;
258+
}
259+
260+
/*package-private*/ Value withPathAppend(final int i) {
261+
return withPathAppend(String.valueOf(i));
262+
}
263+
264+
/*package-private*/ Value withPathAppend(final String field) {
265+
if (path == null || path.isEmpty()) {
266+
return this.withPathSet(field);
267+
}
268+
else {
269+
return this.withPathSet(path + "." + field);
270+
}
291271
}
292272

293273
/*package-private*/ Value copy() {
@@ -395,8 +375,12 @@ private Array() {
395375
}
396376

397377
public void add(final Value value) {
378+
add(value, true);
379+
}
380+
381+
public void add(final Value value, final boolean appendToPath) {
398382
if (!isNull(value)) {
399-
list.add(value);
383+
list.add(appendToPath ? value.withPathAppend(list.size() + 1) : value);
400384
}
401385
}
402386

@@ -454,6 +438,7 @@ public void remove(final int index) {
454438

455439
/*package-private*/ void set(final int index, final Value value) {
456440
list.set(index, value);
441+
value.withPathAppend(index + 1);
457442
}
458443

459444
/*package-private*/ void removeIf(final Predicate<Value> predicate) {
@@ -549,8 +534,12 @@ public int size() {
549534
* @param value the metadata value
550535
*/
551536
public void put(final String field, final Value value) {
537+
put(field, value, true);
538+
}
539+
540+
/*package-private*/ void put(final String field, final Value value, final boolean appendToPath) {
552541
if (!isNull(value)) {
553-
map.put(field, value);
542+
map.put(field, appendToPath ? value.withPathAppend(field) : value);
554543
}
555544
}
556545

@@ -583,8 +572,8 @@ public Value get(final String field) {
583572

584573
return set.isEmpty() ? null : set.size() == 1 ? getField(set.iterator().next(), enforceStringValue) :
585574
newArray(a -> set.forEach(f -> getField(f, enforceStringValue).matchType()
586-
.ifArray(b -> b.forEach(a::add))
587-
.orElse(a::add)
575+
.ifArray(b -> b.forEach(t -> a.add(t, false)))
576+
.orElse(t -> a.add(t, false))
588577
));
589578
}
590579

@@ -628,15 +617,13 @@ public void add(final String field, final Value newValue) {
628617
put(field, newValue);
629618
}
630619
else {
620+
final String basePath = oldValue.getPath();
631621
if (!oldValue.isArray()) { // repeated field: convert single val to first in array
632-
oldValue.updatePathAppend(".1", field);
622+
oldValue.withPathAppend(1);
633623
}
634624

635-
put(field, oldValue.asList(oldVals -> newValue.asList(newVals -> {
636-
for (int i = 0; i < newVals.size(); ++i) {
637-
oldVals.add(newVals.get(i).updatePathAppend("." + (i + 1 + oldVals.size()), field));
638-
}
639-
})));
625+
put(field, oldValue.asList(oldVals -> newValue
626+
.asList(newVals -> newVals.forEach(newVal -> oldVals.add(newVal.withPathSet(basePath))))));
640627
}
641628
}
642629

‎metafix/src/test/java/org/metafacture/metafix/MetafixLookupTest.java

Lines changed: 4 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@
3333
* @author Fabian Steeg
3434
*/
3535
@ExtendWith(MockitoExtension.class)
36-
@ExtendWith(MetafixToDo.Extension.class)
3736
public class MetafixLookupTest {
3837

3938
private static final String CSV_MAP = "src/test/resources/org/metafacture/metafix/maps/test.csv";
@@ -789,7 +788,6 @@ public void shouldLookupInNestedArrays() {
789788
}
790789

791790
@Test
792-
@MetafixToDo("See https://github.com/hbz/lobid-resources/pull/1354")
793791
public void shouldLookupInCopiedNestedArrays() {
794792
MetafixTestHelpers.assertFix(streamReceiver, Arrays.asList(
795793
"put_map('rswk-indicator', s: 'SubjectHeading')",
@@ -819,22 +817,10 @@ public void shouldLookupInCopiedNestedArrays() {
819817
o.get().startEntity("1");
820818
o.get().startEntity("type[]");
821819
o.get().literal("1", "SubjectHeading");
822-
f.apply(2).endEntity();
823-
o.get().startEntity("2");
824-
o.get().startEntity("type[]");
825-
o.get().literal("1", "SubjectHeading");
826-
f.apply(2).endEntity();
827-
o.get().startEntity("3");
828-
o.get().startEntity("type[]");
829-
o.get().literal("1", "SubjectHeading");
830-
f.apply(2).endEntity();
831-
o.get().startEntity("4");
832-
o.get().startEntity("type[]");
833-
o.get().literal("1", "SubjectHeading");
834-
f.apply(2).endEntity();
835-
o.get().startEntity("5");
836-
o.get().startEntity("type[]");
837-
o.get().literal("1", "SubjectHeading");
820+
o.get().literal("2", "SubjectHeading");
821+
o.get().literal("3", "SubjectHeading");
822+
o.get().literal("4", "SubjectHeading");
823+
o.get().literal("5", "SubjectHeading");
838824
f.apply(5).endEntity();
839825
o.get().endRecord();
840826
}

‎metafix/src/test/java/org/metafacture/metafix/MetafixMethodTest.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1964,7 +1964,7 @@ public void copyFieldToSubfieldOfArrayOfObjectsWithIndexExplicitAppend() {
19641964
@Test
19651965
// We currently fail on unresolved references, see MetafixRecordTests#assertThrowsOnEmptyArray
19661966
public void addFieldIntoArrayOfObjectsWithLastWildcardMissingError() {
1967-
MetafixTestHelpers.assertProcessException(IllegalArgumentException.class, "Using ref, but can't find: $last in: null", () -> {
1967+
MetafixTestHelpers.assertProcessException(IllegalArgumentException.class, "Can't find: $last in: null", () -> {
19681968
MetafixTestHelpers.assertFix(streamReceiver, Arrays.asList(
19691969
"add_field('animals[].$last.key', 'value')"
19701970
),

‎metafix/src/test/java/org/metafacture/metafix/MetafixRecordTest.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1113,7 +1113,7 @@ public void addFieldToObjectByIndexMissing() {
11131113
}
11141114

11151115
private void assertThrowsOnEmptyArray(final String index) {
1116-
MetafixTestHelpers.assertProcessException(IllegalArgumentException.class, "Using ref, but can't find: " + index + " in: null", () -> {
1116+
MetafixTestHelpers.assertProcessException(IllegalArgumentException.class, "Can't find: " + index + " in: null", () -> {
11171117
MetafixTestHelpers.assertFix(streamReceiver, Arrays.asList(
11181118
"add_field('animals[]." + index + ".kind','nice')"
11191119
),

‎metafix/src/test/java/org/metafacture/metafix/RecordTest.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -350,8 +350,8 @@ public void shouldPreserveOrderWhenTransformingOptionalArraySubfield() {
350350
}));
351351
a.add(Value.newHash(h -> {
352352
h.put(FIELD, VALUE);
353-
// For proper transformation, we need to explicitly set the path (as in Metafix.java):
354-
h.put(OTHER_FIELD, new Value(OTHER_VALUE.asString(), String.format(path, a.size() + 1)));
353+
// For proper transformation, we have to explicitly set the path (as in Metafix.java):
354+
h.put(OTHER_FIELD, OTHER_VALUE.withPathSet(String.format(path, a.size() + 1)), false);
355355
}));
356356
}));
357357

‎metafix/src/test/resources/org/metafacture/metafix/integration/lookup/fromXml/toJson/lookupInDeeplyNestedArrayOfObjects/expected.json

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,6 @@
4646
},
4747
"type" : [ "SubjectHeading" ]
4848
} ],
49-
"label" : [ ],
5049
"type" : [ "ComplexSubject" ]
5150
} ]
5251
}

0 commit comments

Comments
 (0)