Skip to content

Commit

Permalink
fixed circular ref false positive during property resolution with mut…
Browse files Browse the repository at this point in the history 
…liple references
  • Loading branch information
@ikasarov
ikasarov committed Aug 5, 2024
1 parent 22204c6 commit bf25337
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 29 deletions.
4 changes: 2 additions & 2 deletions multiapps-common-test/src/main/java/org/cloudfoundry/multiapps/common/test/Tester.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,8 @@ private void validateFailure(Expectation expectation, Exception e) {
}
String exceptionMessage = e.getMessage();
assertTrue(exceptionMessage.contains(expectation.getExpectationAsString()),
MessageFormat.format("Exception's message doesn't match up! Expected [{0}] to contain [{1}]!",
expectation.getExpectationAsString(), exceptionMessage));
MessageFormat.format("Exception's message doesn't match up! Expected [{0}] to contain [{1}]!", exceptionMessage,
expectation.getExpectationAsString()));
}

private Object loadResourceAsJsonObject(String resource) {
Expand Down
18 changes: 16 additions & 2 deletions multiapps-mta/src/main/java/org/cloudfoundry/multiapps/mta/resolvers/PropertiesResolver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ private Object resolveReferences(String key, String value) {
private StringBuilder resolveReferenceInPlace(String key, StringBuilder value, Reference reference) {
String matchedPattern = reference.getMatchedPattern();
int patternStartIndex = value.indexOf(matchedPattern);
Object resolvedReference = resolveReferenceInContext(key, reference);
Object resolvedReference = resolveReferenceInContext(key, reference, resolutionContext != null);
if (resolvedReference != null) {
return value.replace(patternStartIndex, patternStartIndex + matchedPattern.length(), resolvedReference.toString());
}
Expand All @@ -102,14 +102,24 @@ private boolean isSimpleReference(String value, List<Reference> references) {
}

protected Object resolveReferenceInContext(String key, Reference reference) {
return resolveReferenceInContext(key, reference, false);
}

protected Object resolveReferenceInContext(String key, Reference reference, boolean shouldBackupContext) {
boolean resolutionContextWasCreated = false;
HashSet<String> contextKeysBackup = null;
if (resolutionContext == null) {
resolutionContext = new ResolutionContext(NameUtil.getPrefixedName(prefix, key));
resolutionContextWasCreated = true;
} else if (shouldBackupContext) {
// if multiple refs are resolved sequentially in a value - backup and revert context after each ref
contextKeysBackup = new HashSet<>(resolutionContext.referencedKeys);
}
Object resolvedValue = resolveReference(reference);
if (resolutionContextWasCreated) {
resolutionContext = null;
} else if (contextKeysBackup != null) {
resolutionContext.setReferencedKeys(contextKeysBackup);
}
return resolvedValue;
}
Expand Down Expand Up @@ -220,7 +230,7 @@ private List<Reference> detectReferences(String line) {

private static class ResolutionContext {

private String rootKey;
private final String rootKey;
private Set<String> referencedKeys = new HashSet<>();

public ResolutionContext(String rootKey) {
Expand All @@ -234,6 +244,10 @@ public void addToReferencedKeys(String key) {
referencedKeys.add(key);
}

public void setReferencedKeys(Set<String> keys) {
this.referencedKeys = keys;
}

}

}
49 changes: 25 additions & 24 deletions ...ps-mta/src/test/java/org/cloudfoundry/multiapps/mta/resolvers/PropertiesResolverTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,41 +13,42 @@

class PropertiesResolverTest {

protected static final PropertiesResolver testResolver = new PropertiesResolver(null,
null,
null,
"test-",
false,
Collections.emptySet());
protected static final Map<String, Object> replacementValues = TestUtil.getMap("moduleProperties.yaml", PropertiesResolverTest.class);

private final Tester tester = Tester.forClass(getClass());

static Stream<Arguments> testResolve() {
return Stream.of(Arguments.of("moduleProperties.yaml", "test-map/a-list/1", new Expectation("second-item")),
Arguments.of("moduleProperties.yaml", "test-list/0/foo", new Expectation("@foo")),
Arguments.of("moduleProperties.yaml", "test-list/0/foo/", new Expectation("@foo")),
Arguments.of("moduleProperties.yaml", "test-list/1", new Expectation("{fizz=@fizz, buzz=@buzz}")),
Arguments.of("moduleProperties.yaml", "test-list/1/buzz", new Expectation("@buzz")),
Arguments.of("moduleProperties.yaml", "test-list/2", new Expectation("a string in list")),
Arguments.of("moduleProperties.yaml", "hosts/0/", new Expectation("some host")),
Arguments.of("moduleProperties.yaml", "hosts/1.0",
return Stream.of(Arguments.of("list-in-map", "${test-map/a-list/1}", new Expectation("second-item")),
Arguments.of("map-in-list", "${test-list/0/foo}", new Expectation("@foo")),
Arguments.of("map-in-list-extra-slash", "${test-list/0/foo/}", new Expectation("@foo")),
Arguments.of("complex-value", "${test-list/1}", new Expectation("{buzz=@buzz, fizz=@fizz}")),
Arguments.of("simple-value", "${test-list/2}", new Expectation("a string in list")),
Arguments.of("first-in-list", "${hosts/0/}", new Expectation("some host")),
Arguments.of("bad-index", "${hosts/1.0}",
new Expectation(Expectation.Type.EXCEPTION, "Unable to resolve \"test-#hosts/1.0\"")),
Arguments.of("moduleProperties.yaml", "test-list/10/foo/",
Arguments.of("too-high-index", "${test-list/10/foo/}",
new Expectation(Expectation.Type.EXCEPTION, "Unable to resolve \"test-#test-list/10/foo/\"")),
Arguments.of("moduleProperties.yaml", "test-list//foo/",
Arguments.of("missing-index", "${test-list//foo/}",
new Expectation(Expectation.Type.EXCEPTION, "Unable to resolve \"test-#test-list//foo/\"")),
Arguments.of("moduleProperties.yaml", "tricky-map/just a key", new Expectation("foo")),
Arguments.of("moduleProperties.yaml", "tricky-map/0/1", new Expectation("baz")),
Arguments.of("moduleProperties.yaml", "tricky-map/one/two/", new Expectation("why")),
Arguments.of("moduleProperties.yaml", "tricky-map/3/4/5", new Expectation("stop")));
Arguments.of("corner-case-whitespace", "${tricky-map/just a key}", new Expectation("foo")),
Arguments.of("corner-case-map-and-list", "${tricky-map/0/1}", new Expectation("baz")),
Arguments.of("corner-case-slash-1", "${tricky-map/one/two/}", new Expectation("why")),
Arguments.of("corner-case-slash-2", "${tricky-map/3/4/5}", new Expectation("stop")),
Arguments.of("9-qux", "${NO_CIRCULAR_REF}", new Expectation("qux-qux-qux-qux-qux-qux-qux-qux-qux")),
Arguments.of("9-qux-direct", "${foo}-${bar}-${baz}", new Expectation("qux-qux-qux-qux-qux-qux-qux-qux-qux")),
Arguments.of("2-ha", "${NO_CIRCULAR_REF_IN_MAP/a}", new Expectation("ha-ha")),
Arguments.of("circular-ref", "${CIRCULAR_REF}",
new Expectation(Expectation.Type.EXCEPTION, "Circular reference detected in \"test-#circular-ref\"")),
Arguments.of("circular-ref-map", "${CIRCULAR_REF_MAP_IN_MAP/key/key}", new Expectation(Expectation.Type.EXCEPTION,
"Circular reference detected in \"test-#circular-ref-map\"")));
}

@ParameterizedTest
@MethodSource
void testResolve(String modulePropertiesLocation, String parameterExpression, Expectation expectation) {
Map<String, Object> moduleProperties = TestUtil.getMap(modulePropertiesLocation, getClass());

tester.test(() -> testResolver.resolveReferenceInDepth(new Reference(null, parameterExpression), moduleProperties), expectation);
void testResolve(String parameterName, String parameterValue, Expectation expectation) {
PropertiesResolver testResolver = new PropertiesResolver(null, irrelevant -> replacementValues, ReferencePattern.PLACEHOLDER,
"test-", false, Collections.emptySet());
tester.test(() -> testResolver.visit(parameterName, parameterValue), expectation);
}

}
18 changes: 17 additions & 1 deletion ...pps-mta/src/test/resources/org/cloudfoundry/multiapps/mta/resolvers/moduleProperties.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,20 @@ tricky-map:
? "3/4"
: "5": stop
? "3/4/5"
: this is actually unreachable
: this is actually unreachable
foo: ${qux}-${baz}-${bar}-${baz}-${qux}
bar: ${baz}-${qux}
baz: ${qux}
qux: "qux"
NO_CIRCULAR_REF: ${foo}-${bar}-${baz}
NO_CIRCULAR_REF_IN_MAP:
a: ${NO_CIRCULAR_REF_IN_MAP/b}-${NO_CIRCULAR_REF_IN_MAP/c}
b: ${NO_CIRCULAR_REF_IN_MAP/c}
c: "ha"
head: ${baz}-${body}
body: ${baz}-${tail}
tail: ${baz}-${head}
CIRCULAR_REF: ${foo}-${head}
CIRCULAR_REF_MAP_IN_MAP:
key:
key: ${head}

0 comments on commit bf25337

Please sign in to comment.