Skip to content

Commit

Permalink
Fix pipeline expression validation.
Browse files Browse the repository at this point in the history 
- Tolerate unresolved values: They are always possible due to unknown
  parameter values for functions such as fn:substring-after.

- Do not tolerate unsupported placeholders in any situation:
  They will always result in error.

Signed-off-by: Yufei Cai <yufei.cai@bosch-si.com>
  • Loading branch information
@yufei-cai
yufei-cai committed Nov 5, 2019
1 parent f0f5ae3 commit eafd28f
Show file tree
Hide file tree
Showing 10 changed files with 79 additions and 89 deletions.
79 changes: 38 additions & 41 deletions ...lders/src/main/java/org/eclipse/ditto/model/placeholders/ImmutableExpressionResolver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,16 @@

import static org.eclipse.ditto.model.placeholders.Expression.SEPARATOR;

import java.util.AbstractMap;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.UUID;
import java.util.function.Function;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import javax.annotation.Nullable;
import javax.annotation.concurrent.Immutable;
Expand Down Expand Up @@ -62,19 +61,19 @@ final class ImmutableExpressionResolver implements ExpressionResolver {
private static final Function<String, DittoRuntimeException> UNRESOLVED_INPUT_HANDLER = unresolvedInput ->
UnresolvedPlaceholderException.newBuilder(unresolvedInput).build();

private static final String DEFAULT_VALIDATION_STRING_ID = "justForValidation_" + UUID.randomUUID().toString();
private final String placeholderReplacementInValidation;
@Nullable private final String placeholderReplacementInValidation;

private final List<PlaceholderResolver<?>> placeholderResolvers;
private final Map<String, PlaceholderResolver<?>> placeholderResolvers;

ImmutableExpressionResolver(final List<PlaceholderResolver<?>> placeholderResolvers) {
this(placeholderResolvers, DEFAULT_VALIDATION_STRING_ID);
this(placeholderResolvers, null);
}

ImmutableExpressionResolver(final List<PlaceholderResolver<?>> placeholderResolvers,
final String stringUsedInPlaceholderValidation) {
this.placeholderResolvers = Collections.unmodifiableList(new ArrayList<>(placeholderResolvers));
@Nullable final String stringUsedInPlaceholderValidation) {
this.placeholderReplacementInValidation = stringUsedInPlaceholderValidation;
this.placeholderResolvers = placeholderResolvers.stream()
.collect(Collectors.toMap(PlaceholderResolver::getPrefix, Function.identity()));
}

@Override
Expand All @@ -84,22 +83,36 @@ public PipelineElement resolveAsPipelineElement(final String placeholderExpressi
if (isFirstPlaceholderFunction(firstPlaceholderInPipe)) {
return getPipelineFromExpressions(pipelineStagesExpressions, 0).execute(PipelineElement.unresolved(), this);
} else {
final PipelineElement pipelineInput = resolvePlaceholderWithOrWithoutPrefix(firstPlaceholderInPipe);
final PipelineElement pipelineInput = resolveSinglePlaceholder(firstPlaceholderInPipe);
return getPipelineFromExpressions(pipelineStagesExpressions, 1).execute(pipelineInput, this);
}
}

private PipelineElement resolvePlaceholderWithOrWithoutPrefix(final String placeholderInPipeline) {
return placeholderResolvers.stream()
.flatMap(resolver ->
resolvePlaceholderWithoutPrefixIfSupported(resolver, placeholderInPipeline)
.flatMap(p -> resolvePlaceholder(resolver, p))
.map(Stream::of)
.orElseGet(Stream::empty)
)
.findAny()
.map(PipelineElement::resolved)
.orElseGet(PipelineElement::unresolved);
private Optional<Map.Entry<PlaceholderResolver<?>, String>> findPlaceholderResolver(
final String placeholderInPipeline) {
return getPlaceholderPrefix(placeholderInPipeline)
.flatMap(prefix -> {
final String name = placeholderInPipeline.substring(prefix.length() + 1);
return Optional.ofNullable(placeholderResolvers.get(prefix))
.filter(resolver -> resolver.supports(name))
.map(resolver -> new AbstractMap.SimpleImmutableEntry<>(resolver, name));
});
}

private PipelineElement resolveSinglePlaceholder(final String placeholderInPipeline) {
final Map.Entry<PlaceholderResolver<?>, String> resolverPair = findPlaceholderResolver(placeholderInPipeline)
.orElseThrow(() -> UnresolvedPlaceholderException.newBuilder(placeholderInPipeline).build());

if (placeholderReplacementInValidation == null) {
// normal mode
return resolverPair.getKey()
.resolve(resolverPair.getValue())
.map(PipelineElement::resolved)
.orElseGet(PipelineElement::unresolved);
} else {
// validation mode: all placeholders resolve to dummy value.
return PipelineElement.resolved(placeholderReplacementInValidation);
}
}

private List<String> getPipelineStagesExpressions(final String template) {
Expand Down Expand Up @@ -137,30 +150,14 @@ private Pipeline getPipelineFromExpressions(final List<String> pipelineStagesExp
return new ImmutablePipeline(ImmutableFunctionExpression.INSTANCE, pipelineStages);
}

private Optional<String> resolvePlaceholderWithoutPrefixIfSupported(final PlaceholderResolver<?> resolver,
final String placeholder) {
private Optional<String> getPlaceholderPrefix(final String placeholder) {
final int separatorIndex = placeholder.indexOf(SEPARATOR);
if (separatorIndex == -1) {
throw UnresolvedPlaceholderException.newBuilder(placeholder).build();
return Optional.empty();
}
final String prefix = placeholder.substring(0, separatorIndex).trim();
if (prefix.equals(resolver.getPrefix())) {
return Optional.of(placeholder.substring(separatorIndex + 1).trim());
{
return Optional.of(placeholder.substring(0, separatorIndex).trim());
}
return Optional.empty();
}

private Optional<String> resolvePlaceholder(final PlaceholderResolver<?> resolver,
final String placeholderWithoutPrefix) {
return Optional.of(resolver)
.filter(p -> p.supports(placeholderWithoutPrefix))
.flatMap(p -> {
if (resolver.isForValidation()) {
return Optional.of(placeholderReplacementInValidation);
} else {
return resolver.resolve(placeholderWithoutPrefix);
}
});
}

@Override
Expand Down
16 changes: 3 additions & 13 deletions ...ders/src/main/java/org/eclipse/ditto/model/placeholders/ImmutablePlaceholderResolver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,25 +27,17 @@ final class ImmutablePlaceholderResolver<T> implements PlaceholderResolver<T> {

private final Placeholder<T> placeholder;
@Nullable private final T placeholderSource;
private final boolean forValidation;

ImmutablePlaceholderResolver(final Placeholder<T> placeholder, @Nullable final T placeholderSource,
boolean forValidation) {
ImmutablePlaceholderResolver(final Placeholder<T> placeholder, @Nullable final T placeholderSource) {
this.placeholder = placeholder;
this.placeholderSource = placeholderSource;
this.forValidation = forValidation;
}

@Override
public Optional<T> getPlaceholderSource() {
return Optional.ofNullable(placeholderSource);
}

@Override
public boolean isForValidation() {
return forValidation;
}

@Override
public Optional<String> resolve(final T placeholderSource, final String name) {
return placeholder.resolve(placeholderSource, name);
Expand Down Expand Up @@ -75,14 +67,13 @@ public boolean equals(final Object o) {
return false;
}
final ImmutablePlaceholderResolver<?> that = (ImmutablePlaceholderResolver<?>) o;
return forValidation == that.forValidation &&
Objects.equals(placeholder, that.placeholder) &&
return Objects.equals(placeholder, that.placeholder) &&
Objects.equals(placeholderSource, that.placeholderSource);
}

@Override
public int hashCode() {
return Objects.hash(placeholder, placeholderSource, forValidation);
return Objects.hash(placeholder, placeholderSource);
}


Expand All @@ -91,7 +82,6 @@ public String toString() {
return getClass().getSimpleName() + " [" +
"placeholder=" + placeholder +
", value=" + placeholderSource +
", forValidation=" + forValidation +
"]";
}
}
4 changes: 3 additions & 1 deletion ...ceholders/src/main/java/org/eclipse/ditto/model/placeholders/PipelineFunctionDefault.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ public PipelineElement apply(final PipelineElement value, final String paramsInc

// parse + resolve the specified default value for unresolved placeholders
// if previous stage does not resolve to a value. deleted pipeline elements remain deleted.
return value.onUnresolved(() -> parseAndResolveThrow(paramsIncludingParentheses, expressionResolver));
// evaluate parameter first to fail fast.
final PipelineElement parameter = parseAndResolveThrow(paramsIncludingParentheses, expressionResolver);
return value.onUnresolved(() -> parameter);
}

private PipelineElement parseAndResolveThrow(final String paramsIncludingParentheses,
Expand Down
4 changes: 2 additions & 2 deletions ...l/placeholders/src/main/java/org/eclipse/ditto/model/placeholders/PlaceholderFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public static TopicPathPlaceholder newTopicPathPlaceholder() {
*/
public static <T> PlaceholderResolver<T> newPlaceholderResolver(final Placeholder<T> placeholder,
@Nullable final T placeholderSource) {
return new ImmutablePlaceholderResolver<>(placeholder, placeholderSource, false);
return new ImmutablePlaceholderResolver<>(placeholder, placeholderSource);
}

/**
Expand All @@ -75,7 +75,7 @@ public static <T> PlaceholderResolver<T> newPlaceholderResolver(final Placeholde
* @return the created PlaceholderResolver instance
*/
public static <T> PlaceholderResolver<T> newPlaceholderResolverForValidation(final Placeholder<T> placeholder) {
return new ImmutablePlaceholderResolver<>(placeholder, null, true);
return new ImmutablePlaceholderResolver<>(placeholder, null);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion model/placeholders/src/main/java/org/eclipse/ditto/model/placeholders/PlaceholderFilter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ public static String applyOrElseRetain(final String template, final ExpressionRe
* function chain which is too complex (e.g. too much chained function calls)
*/
public static void validate(final String template, final Placeholder<?>... placeholders) {
doApply(template, PlaceholderFactory.newExpressionResolverForValidation(placeholders));
applyOrElseRetain(template, PlaceholderFactory.newExpressionResolverForValidation(placeholders));
}

/**
Expand Down
6 changes: 0 additions & 6 deletions .../placeholders/src/main/java/org/eclipse/ditto/model/placeholders/PlaceholderResolver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,6 @@
*/
public interface PlaceholderResolver<T> extends Placeholder<T> {

/**
* @return whether the placeholder is only used for validation (returning {@code true}) or if it is used for actual
* replacement.
*/
boolean isForValidation();

/**
* @return the source from which to resolve a placeholder with a {@code name}.
*/
Expand Down
39 changes: 20 additions & 19 deletions ...s/src/test/java/org/eclipse/ditto/model/placeholders/ImmutableExpressionResolverTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import java.util.Map;

import org.eclipse.ditto.model.base.headers.DittoHeaders;
import org.eclipse.ditto.model.connectivity.UnresolvedPlaceholderException;
import org.eclipse.ditto.model.things.ThingId;
import org.eclipse.ditto.protocoladapter.ProtocolFactory;
import org.eclipse.ditto.protocoladapter.TopicPath;
Expand Down Expand Up @@ -67,11 +68,11 @@ public static void setupClass() {
final TopicPath topic = ProtocolFactory.newTopicPath(KNOWN_TOPIC);

final ImmutablePlaceholderResolver<Map<String, String>> headersResolver =
new ImmutablePlaceholderResolver<>(PlaceholderFactory.newHeadersPlaceholder(), KNOWN_HEADERS, false);
new ImmutablePlaceholderResolver<>(PlaceholderFactory.newHeadersPlaceholder(), KNOWN_HEADERS);
final ImmutablePlaceholderResolver<CharSequence> thingResolver = new ImmutablePlaceholderResolver<>(
PlaceholderFactory.newThingPlaceholder(), THING_ID, false);
PlaceholderFactory.newThingPlaceholder(), THING_ID);
final ImmutablePlaceholderResolver<TopicPath> topicPathResolver = new ImmutablePlaceholderResolver<>(
PlaceholderFactory.newTopicPathPlaceholder(), topic, false);
PlaceholderFactory.newTopicPathPlaceholder(), topic);

underTest = new ImmutableExpressionResolver(Arrays.asList(headersResolver, thingResolver, topicPathResolver));
}
Expand Down Expand Up @@ -103,28 +104,28 @@ public void testSuccessfulPlaceholderResolution() {

@Test
public void testPlaceholderResolutionAllowingUnresolvedPlaceholders() {

// supported unresolved placeholders are retained
assertThat(underTest.resolve(UNKNOWN_HEADER_EXPRESSION, true))
.contains(UNKNOWN_HEADER_EXPRESSION);
assertThat(underTest.resolve(UNKNOWN_THING_EXPRESSION, true))
.contains(UNKNOWN_THING_EXPRESSION);
assertThat(underTest.resolve(UNKNOWN_TOPIC_EXPRESSION, true))
.contains(UNKNOWN_TOPIC_EXPRESSION);

// unsupported placeholders cause error
assertThatExceptionOfType(UnresolvedPlaceholderException.class)
.isThrownBy(() -> underTest.resolve(UNKNOWN_THING_EXPRESSION, true));
assertThatExceptionOfType(UnresolvedPlaceholderException.class)
.isThrownBy(() -> underTest.resolve(UNKNOWN_TOPIC_EXPRESSION, true));
}

@Test
public void testUnsuccessfulPlaceholderResolution() {
assertThat(underTest.resolve(UNKNOWN_HEADER_EXPRESSION, false)).isEmpty();
assertThat(underTest.resolve(UNKNOWN_THING_EXPRESSION, false)).isEmpty();
assertThat(underTest.resolve(UNKNOWN_TOPIC_EXPRESSION, false)).isEmpty();
}

@Test
public void testSuccessfulFunctionBasedOnPlaceholderInput() {

assertThat(underTest.resolve("{{ header:unknown | fn:default('fallback') }}", false))
.contains("fallback");
assertThat(underTest.resolve("{{ thing:bar | fn:default('bar') | fn:upper() }}", false))
assertThat(underTest.resolve("{{ header:unknown | fn:default('bar') | fn:upper() }}", false))
.contains("BAR");
assertThat(underTest.resolve("{{ thing:id | fn:substring-before(':') }}", false))
.contains(THING_NAMESPACE);
Expand All @@ -136,11 +137,11 @@ public void testSuccessfulFunctionBasedOnPlaceholderInput() {
.contains(" fallback-spaces ");

// verify different whitespace
assertThat(underTest.resolve("{{thing:bar |fn:default('bar')| fn:upper() }}", false))
assertThat(underTest.resolve("{{header:unknown |fn:default('bar')| fn:upper() }}", false))
.contains("BAR");
assertThat(underTest.resolve("{{ thing:bar | fn:default('bar') |fn:upper()}}", false))
assertThat(underTest.resolve("{{ header:unknown | fn:default('bar') |fn:upper()}}", false))
.contains("BAR");
assertThat(underTest.resolve("{{ thing:bar | fn:default( 'bar' ) |fn:upper( ) }}", false))
assertThat(underTest.resolve("{{ header:unknown | fn:default( 'bar' ) |fn:upper( ) }}", false))
.contains("BAR");
assertThat(underTest.resolve(
"{{ thing:id | fn:substring-before(\"|\") | fn:default('bAz') | fn:lower() }}", false))
Expand All @@ -149,23 +150,23 @@ public void testSuccessfulFunctionBasedOnPlaceholderInput() {

@Test
public void testSpecialCharactersInStrings() {
assertThat(underTest.resolve("{{ thing:bar | fn:default( ' \\s%!@/*+\"\\'上手カキクケコ' ) | fn:upper( ) }}", false))
assertThat(underTest.resolve("{{ header:unknown | fn:default( ' \\s%!@/*+\"\\'上手カキクケコ' ) | fn:upper( ) }}", false))
.contains(" \\S%!@/*+\"\\'上手カキクケコ");

assertThat(underTest.resolve("{{ thing:bar | fn:default( \" \\s%!@/*+'\\\"上手カキクケコ\" ) | fn:upper( ) }}", false))
assertThat(underTest.resolve("{{ header:unknown | fn:default( \" \\s%!@/*+'\\\"上手カキクケコ\" ) | fn:upper( ) }}", false))
.contains(" \\S%!@/*+'\\\"上手カキクケコ");
}

@Test
public void rejectUnsupportedPlaceholdersWithSpecialCharacters() {
assertThat(underTest.resolve("{{ thing:id\\s%!@/*+上手カキクケコ }}", false)).isEmpty();
assertThatExceptionOfType(UnresolvedPlaceholderException.class)
.isThrownBy(() -> underTest.resolve("{{ thing:id\\s%!@/*+上手カキクケコ }}", false));
}

@Test
public void testUnsuccessfulFunctionBasedOnPlaceholderInput() {

assertThat(underTest.resolve("{{ header:unknown }}", false)).isEmpty();
assertThat(underTest.resolve("{{ thing:bar | fn:upper() }}", false)).isEmpty();
assertThat(underTest.resolve("{{ header:unknown | fn:default(header:unknown) }}", false)).isEmpty();
}

@Test
Expand Down
6 changes: 3 additions & 3 deletions .../src/test/java/org/eclipse/ditto/model/placeholders/ImmutablePlaceholderResolverTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public void testPlaceholderResolvementBasedOnMap() {
inputMap.put("two", "2");

final ImmutablePlaceholderResolver<Map<String, String>> underTest = new ImmutablePlaceholderResolver<>(
PlaceholderFactory.newHeadersPlaceholder(), inputMap, false);
PlaceholderFactory.newHeadersPlaceholder(), inputMap);

assertThat(underTest.resolve("one"))
.contains("1");
Expand All @@ -66,7 +66,7 @@ public void testPlaceholderResolvementBasedOnThingId() {
final ThingId thingId = ThingId.of("org.eclipse.ditto", "foobar199");

final ImmutablePlaceholderResolver<CharSequence> underTest = new ImmutablePlaceholderResolver<>(
PlaceholderFactory.newThingPlaceholder(), thingId, false);
PlaceholderFactory.newThingPlaceholder(), thingId);

assertThat(underTest.resolve("id"))
.contains(thingId.toString());
Expand All @@ -82,7 +82,7 @@ public void testPlaceholderResolvementBasedOnTopic() {
final TopicPath topic = ProtocolFactory.newTopicPath(fullPath);

final ImmutablePlaceholderResolver<TopicPath> underTest = new ImmutablePlaceholderResolver<>(
PlaceholderFactory.newTopicPathPlaceholder(), topic, false);
PlaceholderFactory.newTopicPathPlaceholder(), topic);

assertThat(underTest.resolve("full"))
.contains(fullPath);
Expand Down
9 changes: 9 additions & 0 deletions ...laceholders/src/test/java/org/eclipse/ditto/model/placeholders/PlaceholderFilterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -218,10 +218,19 @@ public void testValidate() {
// pre/postfix and separators
PlaceholderFilter.validate("-----{{thing:namespace }}///{{ thing:name }}///{{header:device-id }}-----",
placeholders);

// placeholders with pipeline functions
PlaceholderFilter.validate("{{ thing:namespace }}:{{thing:id | fn:substring-after(':')}}",
placeholders);
}

@Test
public void testValidateFails() {
// unsupported placeholder in parameter position
assertThatExceptionOfType(UnresolvedPlaceholderException.class).isThrownBy(
() -> PlaceholderFilter.validate("{{ header:xxx | fn:default(ing:namespace) }}",
placeholders));

// illegal braces combinations
assertThatExceptionOfType(UnresolvedPlaceholderException.class).isThrownBy(
() -> PlaceholderFilter.validate("{{th{{ing:namespace }}{{ thing:name }}{{header:device-id }}",
Expand Down
Loading

0 comments on commit eafd28f

Please sign in to comment.