Skip to content

Commit f01fc4e

Browse files
ndr-brtndr-brt
authored
fix: avoid hardcoded 'in' operator in validation and transformation (#5619)
1 parent 2909047 commit f01fc4e

15 files changed

Lines changed: 156 additions & 68 deletions

File tree

‎core/common/lib/api-lib/src/test/java/org/eclipse/edc/api/model/ApiCoreSchemaTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ class ApiCoreSchemaTest {
6060
@BeforeEach
6161
void setUp() {
6262
transformer.register(new JsonObjectToQuerySpecTransformer());
63-
transformer.register(new JsonObjectToCriterionTransformer());
63+
transformer.register(new JsonObjectToCriterionTransformer(criterionOperatorRegistry));
6464
transformer.register(new JsonValueToGenericTypeTransformer(typeManager, "test"));
6565
transformer.register(new JsonObjectToDataAddressTransformer());
6666
when(typeManager.getMapper("test")).thenReturn(objectMapper);

‎core/common/lib/query-lib/src/main/java/org/eclipse/edc/query/CriterionOperatorRegistryImpl.java‎

Lines changed: 25 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
package org.eclipse.edc.query;
1616

1717
import org.eclipse.edc.spi.query.Criterion;
18+
import org.eclipse.edc.spi.query.CriterionOperator;
1819
import org.eclipse.edc.spi.query.CriterionOperatorRegistry;
1920
import org.eclipse.edc.spi.query.OperatorPredicate;
2021
import org.eclipse.edc.spi.query.PropertyLookup;
@@ -34,29 +35,29 @@
3435
*/
3536
public class CriterionOperatorRegistryImpl implements CriterionOperatorRegistry {
3637

37-
private final Map<String, OperatorPredicate> operatorPredicates = new HashMap<>();
38+
private final Map<String, CriterionOperator> operators = new HashMap<>();
3839
private final List<PropertyLookup> propertyLookups = new ArrayList<>();
3940

4041
public static CriterionOperatorRegistry ofDefaults() {
4142
var registry = new CriterionOperatorRegistryImpl();
4243
registry.registerPropertyLookup(new ReflectionPropertyLookup());
43-
registry.registerOperatorPredicate(EQUAL, new EqualOperatorPredicate());
44-
registry.registerOperatorPredicate(NOT_EQUAL, not(new EqualOperatorPredicate()));
45-
registry.registerOperatorPredicate(IN, InOperatorPredicate.in());
46-
registry.registerOperatorPredicate(NOT_IN, InOperatorPredicate.notIn());
47-
registry.registerOperatorPredicate(LIKE, new LikeOperatorPredicate());
48-
registry.registerOperatorPredicate(ILIKE, new IlikeOperatorPredicate());
49-
registry.registerOperatorPredicate(CONTAINS, new ContainsOperatorPredicate());
50-
registry.registerOperatorPredicate(LESS_THAN, NumberStringOperatorPredicate.lessThan());
51-
registry.registerOperatorPredicate(LESS_THAN_EQUAL, NumberStringOperatorPredicate.lessThanEqual());
52-
registry.registerOperatorPredicate(GREATER_THAN, NumberStringOperatorPredicate.greaterThan());
53-
registry.registerOperatorPredicate(GREATER_THAN_EQUAL, NumberStringOperatorPredicate.greaterThanEqual());
44+
registry.registerOperator(EQUAL, Object.class, new EqualOperatorPredicate());
45+
registry.registerOperator(NOT_EQUAL, Object.class, not(new EqualOperatorPredicate()));
46+
registry.registerOperator(IN, Iterable.class, InOperatorPredicate.in());
47+
registry.registerOperator(NOT_IN, Iterable.class, InOperatorPredicate.notIn());
48+
registry.registerOperator(LIKE, Object.class, new LikeOperatorPredicate());
49+
registry.registerOperator(ILIKE, Object.class, new IlikeOperatorPredicate());
50+
registry.registerOperator(CONTAINS, Object.class, new ContainsOperatorPredicate());
51+
registry.registerOperator(LESS_THAN, Object.class, NumberStringOperatorPredicate.lessThan());
52+
registry.registerOperator(LESS_THAN_EQUAL, Object.class, NumberStringOperatorPredicate.lessThanEqual());
53+
registry.registerOperator(GREATER_THAN, Object.class, NumberStringOperatorPredicate.greaterThan());
54+
registry.registerOperator(GREATER_THAN_EQUAL, Object.class, NumberStringOperatorPredicate.greaterThanEqual());
5455
return registry;
5556
}
5657

5758
@Override
58-
public void registerOperatorPredicate(String operator, OperatorPredicate converter) {
59-
operatorPredicates.put(operator.toLowerCase(), converter);
59+
public void registerOperator(String operator, Class<?> rightOperandType, OperatorPredicate predicate) {
60+
operators.put(operator.toLowerCase(), new CriterionOperator(operator.toLowerCase(), rightOperandType, predicate));
6061
}
6162

6263
@Override
@@ -66,13 +67,13 @@ public void registerPropertyLookup(PropertyLookup propertyLookup) {
6667

6768
@Override
6869
public void unregister(String operator) {
69-
operatorPredicates.remove(operator.toLowerCase());
70+
operators.remove(operator.toLowerCase());
7071
}
7172

7273
@Override
7374
public <T> Predicate<T> toPredicate(Criterion criterion) {
74-
var predicate = operatorPredicates.get(criterion.getOperator().toLowerCase());
75-
if (predicate == null) {
75+
var operator = operators.get(criterion.getOperator().toLowerCase());
76+
if (operator == null) {
7677
throw new IllegalArgumentException(format("Operator [%s] is not supported.", criterion.getOperator()));
7778
}
7879

@@ -90,14 +91,19 @@ public <T> Predicate<T> toPredicate(Criterion criterion) {
9091
return false;
9192
}
9293

93-
return predicate.test(property, criterion.getOperandRight());
94+
return operator.predicate().test(property, criterion.getOperandRight());
9495
};
9596

9697
}
9798

9899
@Override
99100
public boolean isSupported(String operator) {
100-
return operatorPredicates.containsKey(operator.toLowerCase());
101+
return operators.containsKey(operator.toLowerCase());
102+
}
103+
104+
@Override
105+
public CriterionOperator get(String representation) {
106+
return operators.get(representation.toLowerCase());
101107
}
102108

103109
}

‎core/common/lib/query-lib/src/test/java/org/eclipse/edc/query/CriterionOperatorRegistryImplTest.java‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ class CriterionOperatorRegistryImplTest {
3131

3232
private final CriterionOperatorRegistryImpl registry = new CriterionOperatorRegistryImpl();
3333

34+
@Deprecated(since = "0.17.0")
3435
@Nested
3536
class IsSupported {
3637
@Test
@@ -42,7 +43,7 @@ void shouldReturnFalse_whenOperatorIsNotRegistered() {
4243

4344
@Test
4445
void shouldReturnTrue_whenOperatorIsRegistered() {
45-
registry.registerOperatorPredicate("operator", mock());
46+
registry.registerOperator("operator", Object.class, mock());
4647

4748
var result = registry.isSupported("operator");
4849

@@ -51,7 +52,7 @@ void shouldReturnTrue_whenOperatorIsRegistered() {
5152

5253
@Test
5354
void shouldReturnTrue_whenOperatorIsRegisteredWithDifferentCase() {
54-
registry.registerOperatorPredicate("OpERaTOr", mock());
55+
registry.registerOperator("OpERaTOr", Object.class, mock());
5556

5657
var result = registry.isSupported("OPERATOR");
5758

@@ -60,7 +61,7 @@ void shouldReturnTrue_whenOperatorIsRegisteredWithDifferentCase() {
6061

6162
@Test
6263
void shouldReturnFalse_whenOperatorHasBeenUnregistered() {
63-
registry.registerOperatorPredicate("operator", mock());
64+
registry.registerOperator("operator", Object.class, mock());
6465
registry.unregister("OPERATOR");
6566

6667
var result = registry.isSupported("OPERATOR");
@@ -82,7 +83,7 @@ void shouldThrowException_whenOperatorIsNotRegistered() {
8283
void shouldConvertUsingTheRegisteredConverter() {
8384
OperatorPredicate predicate = mock();
8485
when(predicate.test(any(), any())).thenReturn(true);
85-
registry.registerOperatorPredicate("operator", predicate);
86+
registry.registerOperator("operator", Object.class, predicate);
8687
registry.registerPropertyLookup((key, object) -> "propertyValue");
8788
var criterion = criterion("any", "operator", "operandRight");
8889

@@ -96,7 +97,7 @@ void shouldConvertUsingTheRegisteredConverter() {
9697
void shouldIgnoreOperatorCase() {
9798
OperatorPredicate predicate = mock();
9899
when(predicate.test(any(), any())).thenReturn(true);
99-
registry.registerOperatorPredicate("OPerATOr", predicate);
100+
registry.registerOperator("OPerATOr", Object.class, predicate);
100101
registry.registerPropertyLookup((key, object) -> "propertyValue");
101102
var criterion = criterion("any", "OPERATOR", "any");
102103

@@ -110,7 +111,7 @@ void shouldIgnoreOperatorCase() {
110111
void shouldReturnAlwaysFalsePredicate_whenPropertyCannotBeFound() {
111112
OperatorPredicate predicate = mock();
112113
when(predicate.test(any(), any())).thenReturn(true);
113-
registry.registerOperatorPredicate("operator", predicate);
114+
registry.registerOperator("operator", Object.class, predicate);
114115
registry.registerPropertyLookup((key, object) -> null);
115116
var criterion = criterion("any", "operator", "operandRight");
116117

@@ -129,7 +130,7 @@ void shouldUseLatestPropertyLookup() {
129130
registry.registerPropertyLookup((key, object) -> "firstOne");
130131
registry.registerPropertyLookup((key, object) -> "secondOne");
131132
OperatorPredicate operatorPredicate = mock();
132-
registry.registerOperatorPredicate("=", operatorPredicate);
133+
registry.registerOperator("=", Object.class, operatorPredicate);
133134

134135
registry.toPredicate(criterion("any", "=", "value")).test("any");
135136

@@ -141,7 +142,7 @@ void shouldUseLatestPropertyLookupThatDidNotReturnNull() {
141142
registry.registerPropertyLookup((key, object) -> "firstOne");
142143
registry.registerPropertyLookup((key, object) -> null);
143144
OperatorPredicate operatorPredicate = mock();
144-
registry.registerOperatorPredicate("=", operatorPredicate);
145+
registry.registerOperator("=", Object.class, operatorPredicate);
145146

146147
registry.toPredicate(criterion("any", "=", "value")).test("any");
147148

‎core/common/lib/transform-lib/src/main/java/org/eclipse/edc/transform/transformer/edc/to/JsonObjectToCriterionTransformer.java‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,19 +17,22 @@
1717
import jakarta.json.JsonObject;
1818
import org.eclipse.edc.jsonld.spi.transformer.AbstractJsonLdTransformer;
1919
import org.eclipse.edc.spi.query.Criterion;
20+
import org.eclipse.edc.spi.query.CriterionOperatorRegistry;
2021
import org.eclipse.edc.transform.spi.TransformerContext;
2122
import org.jetbrains.annotations.NotNull;
2223
import org.jetbrains.annotations.Nullable;
2324

24-
import static java.util.stream.Collectors.toList;
2525
import static org.eclipse.edc.spi.query.Criterion.CRITERION_OPERAND_LEFT;
2626
import static org.eclipse.edc.spi.query.Criterion.CRITERION_OPERAND_RIGHT;
2727
import static org.eclipse.edc.spi.query.Criterion.CRITERION_OPERATOR;
2828

2929
public class JsonObjectToCriterionTransformer extends AbstractJsonLdTransformer<JsonObject, Criterion> {
3030

31-
public JsonObjectToCriterionTransformer() {
31+
private final CriterionOperatorRegistry criterionOperatorRegistry;
32+
33+
public JsonObjectToCriterionTransformer(CriterionOperatorRegistry criterionOperatorRegistry) {
3234
super(JsonObject.class, Criterion.class);
35+
this.criterionOperatorRegistry = criterionOperatorRegistry;
3336
}
3437

3538
@Override
@@ -42,8 +45,8 @@ public JsonObjectToCriterionTransformer() {
4245
builder.operator(operator);
4346

4447
var operandRight = object.get(CRITERION_OPERAND_RIGHT);
45-
if ("in".equals(operator)) {
46-
builder.operandRight(operandRight.asJsonArray().stream().map(this::nodeValue).collect(toList()));
48+
if (Iterable.class.isAssignableFrom(criterionOperatorRegistry.get(operator).rightOperandClass())) {
49+
builder.operandRight(operandRight.asJsonArray().stream().map(this::nodeValue).toList());
4750
} else {
4851
builder.operandRight(transformGenericProperty(operandRight, context));
4952
}

‎core/common/lib/transform-lib/src/test/java/org/eclipse/edc/transform/transformer/edc/to/JsonObjectToCriterionTransformerTest.java‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@
1818
import jakarta.json.JsonValue;
1919
import org.eclipse.edc.jsonld.spi.JsonLdKeywords;
2020
import org.eclipse.edc.jsonld.util.JacksonJsonLd;
21+
import org.eclipse.edc.spi.query.CriterionOperator;
22+
import org.eclipse.edc.spi.query.CriterionOperatorRegistry;
2123
import org.eclipse.edc.spi.types.TypeManager;
2224
import org.eclipse.edc.transform.spi.TransformerContext;
2325
import org.junit.jupiter.api.BeforeEach;
@@ -38,9 +40,10 @@
3840

3941

4042
class JsonObjectToCriterionTransformerTest {
41-
private final TypeManager typeManager = mock();
4243

43-
private final JsonObjectToCriterionTransformer transformer = new JsonObjectToCriterionTransformer();
44+
private final TypeManager typeManager = mock();
45+
private final CriterionOperatorRegistry criterionOperatorRegistry = mock();
46+
private final JsonObjectToCriterionTransformer transformer = new JsonObjectToCriterionTransformer(criterionOperatorRegistry);
4447
private final JsonValueToGenericTypeTransformer genericTypeTransformer = new JsonValueToGenericTypeTransformer(typeManager, "test");
4548

4649
@BeforeEach
@@ -50,6 +53,7 @@ void setup() {
5053

5154
@Test
5255
void transform() {
56+
when(criterionOperatorRegistry.get(any())).thenReturn(new CriterionOperator("=", Object.class, (a, b) -> true));
5357
var context = mock(TransformerContext.class);
5458
when(context.transform(any(JsonValue.class), eq(Object.class))).thenAnswer(a -> genericTypeTransformer.transform(a.getArgument(0), context));
5559
var json = Json.createObjectBuilder()
@@ -68,7 +72,8 @@ void transform() {
6872
}
6973

7074
@Test
71-
void transform_shouldConsiderRightAsList_whenOperatorIsIn() {
75+
void transform_shouldConsiderRightAsList_whenOperatorRightOperandIsIterable() {
76+
when(criterionOperatorRegistry.get(any())).thenReturn(new CriterionOperator("in", Iterable.class, (a, b) -> true));
7277
var context = mock(TransformerContext.class);
7378
when(context.transform(any(JsonValue.class), eq(Object.class))).thenAnswer(a -> genericTypeTransformer.transform(a.getArgument(0), context));
7479
var json = Json.createObjectBuilder()
@@ -88,6 +93,7 @@ void transform_shouldConsiderRightAsList_whenOperatorIsIn() {
8893

8994
@Test
9095
void transform_rightOperandIsNumber() {
96+
when(criterionOperatorRegistry.get(any())).thenReturn(new CriterionOperator("=", Object.class, (a, b) -> true));
9197
var context = mock(TransformerContext.class);
9298
when(context.transform(any(JsonValue.class), eq(Object.class))).thenAnswer(a -> genericTypeTransformer.transform(a.getArgument(0), context));
9399
when(context.problem()).thenReturn(mock());

‎core/common/lib/validator-lib/src/main/java/org/eclipse/edc/validator/jsonobject/validators/model/CriterionValidator.java‎

Lines changed: 15 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626

2727
import java.util.Optional;
2828

29-
import static java.lang.String.format;
3029
import static org.eclipse.edc.jsonld.spi.JsonLdKeywords.VALUE;
3130
import static org.eclipse.edc.spi.query.Criterion.CRITERION_OPERAND_LEFT;
3231
import static org.eclipse.edc.spi.query.Criterion.CRITERION_OPERAND_RIGHT;
@@ -43,39 +42,35 @@ public static JsonObjectValidator.Builder instance(JsonObjectValidator.Builder b
4342
return builder
4443
.verify(CRITERION_OPERAND_LEFT, MandatoryValue::new)
4544
.verify(CRITERION_OPERATOR, MandatoryValue::new)
46-
.verify(CRITERION_OPERATOR, path -> new OperatorValidator(path, criterionOperatorRegistry))
4745
.verify(CRITERION_OPERAND_RIGHT, MandatoryArray.min(1))
48-
.verify(OperandRightValidator::new);
46+
.verify(path -> new OperatorValidator(path, criterionOperatorRegistry));
4947
}
5048

51-
private record OperandRightValidator(JsonLdPath path) implements Validator<JsonObject> {
49+
private record OperatorValidator(JsonLdPath path, CriterionOperatorRegistry criterionOperatorRegistry) implements Validator<JsonObject> {
5250

5351
@Override
5452
public ValidationResult validate(JsonObject input) {
5553
var operator = getOperator(input);
54+
if (operator == null) {
55+
return ValidationResult.success();
56+
}
57+
58+
var criterionOperator = criterionOperatorRegistry.get(operator);
59+
if (criterionOperator == null) {
60+
return ValidationResult.failure(violation("Operator %s is not supported"
61+
.formatted(operator), path.append(CRITERION_OPERATOR).toString(), operator));
62+
}
5663

57-
if (operator == null || "in".equals(operator)) {
64+
if (Iterable.class.isAssignableFrom(criterionOperator.rightOperandClass())) {
5865
return ValidationResult.success();
5966
}
6067

6168
return Optional.ofNullable(input.getJsonArray(CRITERION_OPERAND_RIGHT))
6269
.filter(it -> it.size() < 2)
6370
.map(it -> ValidationResult.success())
64-
.orElse(ValidationResult.failure(violation(format("%s cannot contain multiple values as the operator is not 'in'", path.append(CRITERION_OPERAND_RIGHT)), CRITERION_OPERAND_RIGHT)));
65-
}
66-
}
67-
68-
private record OperatorValidator(JsonLdPath path, CriterionOperatorRegistry criterionOperatorRegistry) implements Validator<JsonObject> {
69-
70-
@Override
71-
public ValidationResult validate(JsonObject input) {
72-
var operator = getOperator(input);
73-
74-
if (operator == null || criterionOperatorRegistry.isSupported(operator)) {
75-
return ValidationResult.success();
76-
} else {
77-
return ValidationResult.failure(violation("Operator %s is not supported".formatted(operator), path.toString(), operator));
78-
}
71+
.orElse(ValidationResult.failure(violation(
72+
"%s cannot contain multiple values as the operator %s cannot be applied on an Iterable right operand"
73+
.formatted(path.append(CRITERION_OPERAND_RIGHT), operator), CRITERION_OPERAND_RIGHT)));
7974
}
8075
}
8176

0 commit comments

Comments
 (0)