Skip to content

Commit

Permalink
Replace path heuristic with actual value check in lookup (#170)
Browse files Browse the repository at this point in the history 
To reliably reject string operations on non-string fields
  • Loading branch information
@fsteeg
fsteeg committed Apr 7, 2022
1 parent 0dfcfb8 commit 2b476e2
Show file tree
Hide file tree
Showing 4 changed files with 98 additions and 19 deletions.
21 changes: 7 additions & 14 deletions metafix/src/main/java/org/metafacture/metafix/FixPath.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,16 +45,20 @@ private FixPath(final String[] path) {
}

/*package-private*/ Value findIn(final Hash hash) {
return findIn(hash, false);
}

/*package-private*/ Value findIn(final Hash hash, final boolean enforceStringValue) {
final String currentSegment = path[0];
final FixPath remainingPath = new FixPath(tail(path));
if (currentSegment.equals(ASTERISK)) {
// TODO: search in all elements of value.asHash()?
return remainingPath.findIn(hash);
return remainingPath.findIn(hash, enforceStringValue);
}
final Value value = hash.get(currentSegment);
final Value value = hash.get(currentSegment, enforceStringValue && path.length == 1);
return value == null || path.length == 1 ? value : value.extractType((m, c) -> m
.ifArray(a -> c.accept(remainingPath.findIn(a)))
.ifHash(h -> c.accept(remainingPath.findIn(h)))
.ifHash(h -> c.accept(remainingPath.findIn(h, enforceStringValue)))
.orElseThrow()
);
}
Expand Down Expand Up @@ -118,17 +122,6 @@ public String toString() {
// try to replace this with consistent usage of Value#getPath
// (e.g. take care of handling repeated fields and their paths)

/*package-private*/ void throwIfNonString(final Value value) {
final boolean isNonString = value != null &&
// basic idea: path is only set on literals/strings
value.getPath() == null &&
// but with wildcards, we might still point to literals/strings
!hasWildcard();
if (isNonString) {
value.asString();
}
}

/*package-private*/ FixPath to(final Value value, final int i) {
final FixPath result;
// One *, no matching path: replace with index of current result
Expand Down
3 changes: 1 addition & 2 deletions metafix/src/main/java/org/metafacture/metafix/Record.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,8 +200,7 @@ public void retainFields(final Collection<String> fields) {
*/
public void transform(final String field, final UnaryOperator<String> operator) {
final FixPath findPath = new FixPath(field);
final Value found = findPath.findIn(this);
findPath.throwIfNonString(found);
final Value found = findPath.findIn(this, true);
Value.asList(found, results -> {
final Deque<FixPath> toDelete = new LinkedList<>();
for (int i = 0; i < results.size(); ++i) {
Expand Down
12 changes: 11 additions & 1 deletion metafix/src/main/java/org/metafacture/metafix/Value.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -555,8 +555,18 @@ public void replace(final String field, final Value value) {
* @return the metadata value
*/
public Value get(final String field) {
return get(field, false);
}

/*package-private*/ Value get(final String field, final boolean enforceStringValue) { // TODO use Type.String etc.?
// TODO: special treatment (only) for exact matches?
final List<Value> list = findFields(field).map(this::getField).collect(Collectors.toList());
final List<Value> list = findFields(field).map(actualField -> {
final Value value = getField(actualField);
if (enforceStringValue) {
value.asString();
}
return value;
}).collect(Collectors.toList());
return list.isEmpty() ? null : list.size() == 1 ? list.get(0) : newArray(a -> list.forEach(v -> v.matchType()
.ifArray(b -> b.forEach(a::add))
.orElse(a::add)));
Expand Down
81 changes: 79 additions & 2 deletions metafix/src/test/java/org/metafacture/metafix/MetafixMethodTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -305,10 +305,32 @@ public void shouldNotTrimRepeatedField() {
);
}

@Test
public void shouldNotTrimIndexedArray() {
MetafixTestHelpers.assertExecutionException(IllegalStateException.class, "Expected String, got Hash", () ->
MetafixTestHelpers.assertFix(streamReceiver, Arrays.asList(
"trim('data.title')"
),
i -> {
i.startRecord("1");
i.startEntity("data");
i.startEntity("title");
i.literal("1", " marc ");
i.literal("2", " json ");
i.endEntity();
i.endEntity();
i.endRecord();
},
o -> {
}
)
);
}

@Test
// See https://github.com/metafacture/metafacture-fix/pull/133
public void shouldNotTrimStringInImplicitArrayOfHashes() {
MetafixTestHelpers.assertExecutionException(IllegalStateException.class, "Expected String, got Array", () ->
MetafixTestHelpers.assertExecutionException(NumberFormatException.class, "For input string: \"title\"", () ->
MetafixTestHelpers.assertFix(streamReceiver, Arrays.asList(
"trim('data.title')"
),
Expand Down Expand Up @@ -550,6 +572,39 @@ public void wildcard() {
});
}

@Test
public void wildcardNested() {
MetafixTestHelpers.assertFix(streamReceiver, Arrays.asList(
"trim('work.title-?')"),
i -> {
i.startRecord("1");
i.endRecord();

i.startRecord("2");
i.startEntity("work");
i.literal("title-1", " marc ");
i.literal("title-2", " json ");
i.endEntity();
i.endRecord();

i.startRecord("3");
i.endRecord();
}, o -> {
o.get().startRecord("1");
o.get().endRecord();

o.get().startRecord("2");
o.get().startEntity("work");
o.get().literal("title-1", "marc");
o.get().literal("title-2", "json");
o.get().endEntity();
o.get().endRecord();

o.get().startRecord("3");
o.get().endRecord();
});
}

@Test
public void characterClass() {
MetafixTestHelpers.assertFix(streamReceiver, Arrays.asList(
Expand Down Expand Up @@ -1171,7 +1226,6 @@ public void shouldNotPrependValueToArray() {
}

@Test
@MetafixToDo("See https://github.com/metafacture/metafacture-fix/pull/170")
public void shouldNotPrependValueToArrayWithWildcard() {
MetafixTestHelpers.assertExecutionException(IllegalStateException.class, "Expected String, got Array", () ->
MetafixTestHelpers.assertFix(streamReceiver, Arrays.asList(
Expand All @@ -1192,6 +1246,29 @@ public void shouldNotPrependValueToArrayWithWildcard() {
);
}

@Test
public void shouldNotPrependValueToArrayWithWildcardNested() {
MetafixTestHelpers.assertExecutionException(IllegalStateException.class, "Expected String, got Array", () ->
MetafixTestHelpers.assertFix(streamReceiver, Arrays.asList(
"prepend('some.animal?[]', 'the first one')"
),
i -> {
i.startRecord("1");
i.startEntity("some");
i.startEntity("animals[]");
i.literal("1", "dog");
i.literal("2", "cat");
i.literal("3", "zebra");
i.endEntity();
i.endEntity();
i.endRecord();
},
o -> {
}
)
);
}

@Test
public void shouldReplaceAllRegexes() {
MetafixTestHelpers.assertFix(streamReceiver, Arrays.asList(
Expand Down

0 comments on commit 2b476e2

Please sign in to comment.