Skip to content
Original file line number Diff line number Diff line change
Expand Up @@ -93,11 +93,6 @@ private static Predicate<String> detectorAttributeFilter(
if (included == null && excluded == null) {
return ResourceFactory::matchAll;
}
// when "included" is omitted in configuration, we get an empty list
// in this context it should be interpreted as "include everything"
if (included != null && included.isEmpty()) {
included = null;
}
return IncludeExcludePredicate.createPatternMatching(included, excluded);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,7 @@ public View create(ViewStreamModel model, DeclarativeConfigContext context) {
if (attributeKeys != null) {
List<String> included = attributeKeys.getIncluded();
List<String> excluded = attributeKeys.getExcluded();
if (included != null || excluded != null) {
builder.setAttributeFilter(IncludeExcludePredicate.createExactMatching(included, excluded));
}
builder.setAttributeFilter(IncludeExcludePredicate.createExactMatching(included, excluded));
}
if (model.getAggregation() != null) {
builder.setAggregation(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ private static Stream<Arguments> createTestCases() {
private static final AttributeKey<String> HTTP_PATH = AttributeKey.stringKey("http.path");

@ParameterizedTest
@MethodSource("declarativeCOnfigSamplingPredicateArgs")
@MethodSource("declarativeConfigSamplingPredicateArgs")
void declarativeConfigSamplingPredicate(
DeclarativeConfigSamplingPredicate predicate,
Context context,
Expand All @@ -174,7 +174,7 @@ void declarativeConfigSamplingPredicate(
}

@SuppressWarnings("unused")
private static Stream<Arguments> declarativeCOnfigSamplingPredicateArgs() {
private static Stream<Arguments> declarativeConfigSamplingPredicateArgs() {
DeclarativeConfigSamplingPredicate matchAll =
new DeclarativeConfigSamplingPredicate(null, null, null, null);
DeclarativeConfigSamplingPredicate valuesMatcher =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,26 +38,28 @@ private IncludeExcludePredicate(
@Nullable Collection<String> excluded,
boolean globMatchingEnabled) {
this.globMatchingEnabled = globMatchingEnabled;
this.included = included == null ? null : new LinkedHashSet<>(included);
this.excluded = excluded == null ? null : new LinkedHashSet<>(excluded);
this.included = copyIfNotEmpty(included);
this.excluded = copyIfNotEmpty(excluded);
if (this.included != null && this.excluded != null) {
this.predicate =
includedPredicate(this.included, globMatchingEnabled)
.and(excludedPredicate(this.excluded, globMatchingEnabled));
} else if (this.included == null && this.excluded != null) {
} else if (this.excluded != null) {
this.predicate = excludedPredicate(this.excluded, globMatchingEnabled);
} else if (this.excluded == null && this.included != null) {
} else if (this.included != null) {
this.predicate = includedPredicate(this.included, globMatchingEnabled);
} else {
throw new IllegalArgumentException(
"At least one of includedPatterns or excludedPatterns must not be null");
// include everything by default if both included and excluded are null or empt
this.predicate = s -> true;
}
}

/**
* Create a (case-sensitive) exact matching include exclude predicate.
*
* @throws IllegalArgumentException if {@code included} AND {@code excluded} are null.
* <p>When {@code included} is empty or {@literal null}, all values are included by default.
*
* <p>When {@code excluded} is empty or {@literal null}, no value is excluded by default.
*/
public static Predicate<String> createExactMatching(
@Nullable Collection<String> included, @Nullable Collection<String> excluded) {
Expand All @@ -67,9 +69,11 @@ public static Predicate<String> createExactMatching(
/**
* Create a pattern matching include exclude predicate.
*
* <p>See {@link GlobUtil} for pattern matching details.
* <p>When {@code included} is empty or {@literal null}, all values are included by default.
*
* <p>When {@code excluded} is empty or {@literal null}, no value is excluded by default.
*
* @throws IllegalArgumentException if {@code included} AND {@code excluded} are null.
* <p>See {@link GlobUtil} for pattern matching details.
*/
public static Predicate<String> createPatternMatching(
@Nullable Collection<String> included, @Nullable Collection<String> excluded) {
Expand Down Expand Up @@ -119,4 +123,9 @@ private static Predicate<String> excludedPredicate(
}
return result;
}

@Nullable
private static Set<String> copyIfNotEmpty(@Nullable Collection<String> collection) {
return collection == null || collection.isEmpty() ? null : new LinkedHashSet<>(collection);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,13 @@
import static java.util.Collections.singletonList;
import static org.assertj.core.api.Assertions.assertThat;

import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.function.Predicate;
import java.util.stream.Stream;
import javax.annotation.Nullable;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;
Expand All @@ -26,8 +30,6 @@ class IncludeExcludePredicateTest {
IncludeExcludePredicate.createExactMatching(singletonList("foo"), singletonList("bar"));
private static final Predicate<String> EXACT_MULTI =
IncludeExcludePredicate.createExactMatching(asList("foo", "fooo"), asList("bar", "barr"));
public static final Predicate<String> EXACT_INCLUDE_NONE =
IncludeExcludePredicate.createExactMatching(Collections.emptyList(), null);

private static final Predicate<String> PATTERN_INCLUDE =
IncludeExcludePredicate.createPatternMatching(singletonList("f?o"), null);
Expand All @@ -37,8 +39,6 @@ class IncludeExcludePredicateTest {
IncludeExcludePredicate.createPatternMatching(singletonList("f?o"), singletonList("b?r"));
private static final Predicate<String> PATTERN_MULTI =
IncludeExcludePredicate.createPatternMatching(asList("f?o", "f?oo"), asList("b?r", "b?rr"));
public static final Predicate<String> PATTERN_INCLUDE_NONE =
IncludeExcludePredicate.createPatternMatching(Collections.emptyList(), null);

@ParameterizedTest
@MethodSource("testArgs")
Expand Down Expand Up @@ -75,8 +75,6 @@ private static Stream<Arguments> testArgs() {
Arguments.of(EXACT_MULTI, "bar", false),
Arguments.of(EXACT_MULTI, "barr", false),
Arguments.of(EXACT_MULTI, "baz", false),
// include none
Arguments.of(EXACT_INCLUDE_NONE, "foo", false),
// pattern matching
// include only
Arguments.of(PATTERN_INCLUDE, "foo", true),
Expand Down Expand Up @@ -106,9 +104,7 @@ private static Stream<Arguments> testArgs() {
Arguments.of(PATTERN_MULTI, "bAr", false),
Arguments.of(PATTERN_MULTI, "barr", false),
Arguments.of(PATTERN_MULTI, "bArr", false),
Arguments.of(PATTERN_MULTI, "baz", false),
// include none
Arguments.of(PATTERN_INCLUDE_NONE, "foo", false));
Arguments.of(PATTERN_MULTI, "baz", false));
}

@ParameterizedTest
Expand Down Expand Up @@ -140,4 +136,30 @@ private static Stream<Arguments> stringRepresentationArgs() {
PATTERN_MULTI,
"IncludeExcludePredicate{globMatchingEnabled=true, included=[f?o, f?oo], excluded=[b?r, b?rr]}"));
}

@Test
void emptyOrNullShouldIncludeAll() {
shouldIncludeAll(null, null);
shouldIncludeAll(Collections.emptyList(), null);
shouldIncludeAll(null, Collections.emptyList());
shouldIncludeAll(Collections.emptyList(), Collections.emptyList());
}

private static void shouldIncludeAll(
@Nullable Collection<String> included, @Nullable Collection<String> excluded) {
Arrays.asList("foo", "fooo", "fOo", "FOO", "bar", "bAr", "barr", "bArr", "baz")
.forEach(
s -> {
Predicate<String> exactMatching =
IncludeExcludePredicate.createExactMatching(included, excluded);
assertThat(exactMatching.test(s)).isTrue();
assertThat(exactMatching.toString())
.isEqualTo("IncludeExcludePredicate{globMatchingEnabled=false}");
Predicate<String> patternMatching =
IncludeExcludePredicate.createPatternMatching(included, excluded);
assertThat(patternMatching.test(s)).isTrue();
assertThat(patternMatching.toString())
.isEqualTo("IncludeExcludePredicate{globMatchingEnabled=true}");
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,11 @@ public ViewBuilder setAggregation(Aggregation aggregation) {
*/
public ViewBuilder setAttributeFilter(Set<String> keysToRetain) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[for reviewer] this is the only place where we use IncludeExcludePredicate to potentially remove all items, dealing with this as an exception allows to make most common use-case simpler to call without having to convert empty collection to null.

Objects.requireNonNull(keysToRetain, "keysToRetain");
if (keysToRetain.isEmpty()) {
// include/exclude predicate requires to include or exclude at least one, we have to skip it
// when we don't want to include any key.
return setAttributeFilter(s -> false);
}
return setAttributeFilter(IncludeExcludePredicate.createExactMatching(keysToRetain, null));
}

Expand Down
Loading