From a31028ee1990903164fc0c99948c6049f71a1e34 Mon Sep 17 00:00:00 2001 From: Avgustin Marinov Date: Mon, 9 Sep 2024 16:10:31 +0300 Subject: [PATCH] Slight improvements in RSQL to SQL logic (#1833) Signed-off-by: Marinov Avgustin --- hawkbit-core/pom.xml | 7 +- .../hawkbit/repository/FieldNameProvider.java | 10 ++- .../repository/FileNameFieldsTest.java | 48 +++++++++++++ .../rsql/AbstractFieldNameRSQLVisitor.java | 54 ++++++-------- .../jpa/rsql/JpaQueryRsqlVisitorG2.java | 72 +++++++------------ 5 files changed, 104 insertions(+), 87 deletions(-) create mode 100644 hawkbit-core/src/test/java/org/eclipse/hawkbit/repository/FileNameFieldsTest.java diff --git a/hawkbit-core/pom.xml b/hawkbit-core/pom.xml index 974ee4a1a..514ded4ba 100644 --- a/hawkbit-core/pom.xml +++ b/hawkbit-core/pom.xml @@ -39,7 +39,12 @@ ${commons-io.version} - + + + io.github.classgraph + classgraph + test + org.springframework.boot spring-boot-starter-test diff --git a/hawkbit-core/src/main/java/org/eclipse/hawkbit/repository/FieldNameProvider.java b/hawkbit-core/src/main/java/org/eclipse/hawkbit/repository/FieldNameProvider.java index bc3f0ac6f..9cdcf4c0b 100644 --- a/hawkbit-core/src/main/java/org/eclipse/hawkbit/repository/FieldNameProvider.java +++ b/hawkbit-core/src/main/java/org/eclipse/hawkbit/repository/FieldNameProvider.java @@ -46,14 +46,12 @@ public interface FieldNameProvider { default String[] getSubAttributes(final String propertyFieldName) { if (isMap()) { final String[] subAttributes = propertyFieldName.split(SUB_ATTRIBUTE_SPLIT_REGEX, 2); - // [0] field name | [1] key name + // [0] field name | [1] key name (could miss, e.g. for target attributes) final String mapKeyName = subAttributes.length == 2 ? subAttributes[1] : null; - if (ObjectUtils.isEmpty(mapKeyName)) { - return new String[] { getFieldName() }; - } - return new String[] { getFieldName(), mapKeyName }; + return ObjectUtils.isEmpty(mapKeyName) ? new String[] { getFieldName() } : new String[] { getFieldName(), mapKeyName }; + } else { + return propertyFieldName.split(SUB_ATTRIBUTE_SPLIT_REGEX); } - return propertyFieldName.split(SUB_ATTRIBUTE_SPLIT_REGEX); } /** diff --git a/hawkbit-core/src/test/java/org/eclipse/hawkbit/repository/FileNameFieldsTest.java b/hawkbit-core/src/test/java/org/eclipse/hawkbit/repository/FileNameFieldsTest.java new file mode 100644 index 000000000..3c964ed7e --- /dev/null +++ b/hawkbit-core/src/test/java/org/eclipse/hawkbit/repository/FileNameFieldsTest.java @@ -0,0 +1,48 @@ +/** + * Copyright (c) 2024 Contributors to the Eclipse Foundation + * + * This program and the accompanying materials are made + * available under the terms of the Eclipse Public License 2.0 + * which is available at https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + */ +package org.eclipse.hawkbit.repository; + +import io.github.classgraph.ClassGraph; +import io.github.classgraph.ClassInfo; +import io.github.classgraph.ScanResult; +import io.qameta.allure.Description; +import org.junit.jupiter.api.Test; + +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; + +public class FileNameFieldsTest { + + @Test + @Description("Verifies that fields classes are correctly implemented") + @SuppressWarnings("unchecked") + public void repositoryManagementMethodsArePreAuthorizedAnnotated() { + final String packageName = getClass().getPackage().getName(); + try (final ScanResult scanResult = new ClassGraph().acceptPackages(packageName).scan()) { + final List> matchingClasses = scanResult.getAllClasses() + .stream() + .filter(classInPackage -> classInPackage.implementsInterface(FieldNameProvider.class)) + .map(ClassInfo::loadClass) + .map(clazz -> (Class) clazz) + .toList(); + assertThat(matchingClasses).isNotEmpty(); + matchingClasses.forEach(providerClass -> { + assertThat(providerClass.getEnumConstants()).isNotEmpty(); + for (final FieldNameProvider provider : providerClass.getEnumConstants()) { + if (provider.isMap() && !provider.getSubEntityAttributes().isEmpty()) { + throw new UnsupportedOperationException( + "Currently sub-entity attributes for maps are not supported, alternatively you could use the key/value tuple, defined by SimpleImmutableEntry class"); + } + } + }); + } + } +} \ No newline at end of file diff --git a/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/rsql/AbstractFieldNameRSQLVisitor.java b/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/rsql/AbstractFieldNameRSQLVisitor.java index 540034bb0..d3c02456b 100644 --- a/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/rsql/AbstractFieldNameRSQLVisitor.java +++ b/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/rsql/AbstractFieldNameRSQLVisitor.java @@ -43,48 +43,36 @@ public abstract class AbstractFieldNameRSQLVisitor & FieldName protected String getAndValidatePropertyFieldName(final A propertyEnum, final ComparisonNode node) { final String[] subAttributes = propertyEnum.getSubAttributes(node.getSelector()); - validateMapParameter(propertyEnum, node, subAttributes); - // sub entity need minimum 1 dot - if (!propertyEnum.getSubEntityAttributes().isEmpty() && subAttributes.length < 2) { - throw createRSQLParameterUnsupportedException(node, null); - } - - final StringBuilder fieldNameBuilder = new StringBuilder(propertyEnum.getFieldName()); - for (int i = 1; i < subAttributes.length; i++) { - final String propertyField = getFormattedSubEntityAttribute(propertyEnum ,subAttributes[i]); - fieldNameBuilder.append(FieldNameProvider.SUB_ATTRIBUTE_SEPARATOR).append(propertyField); - - // the key of map is not in the graph - if (propertyEnum.isMap() && subAttributes.length == (i + 1)) { - continue; + if (propertyEnum.isMap()) { + // enum.key + if (subAttributes.length != 2) { + throw new RSQLParameterUnsupportedFieldException( + "The syntax of the given map search parameter field {" + node.getSelector() + "} is wrong. Syntax is: ."); } - - if (!propertyEnum.containsSubEntityAttribute(propertyField)) { + } else { + // sub entity need minimum 1 dot + if (!propertyEnum.getSubEntityAttributes().isEmpty() && subAttributes.length < 2) { throw createRSQLParameterUnsupportedException(node, null); } } + final StringBuilder fieldNameBuilder = new StringBuilder(propertyEnum.getFieldName()); + for (int i = 1; i < subAttributes.length; i++) { + final String propertyField = getFormattedSubEntityAttribute(propertyEnum, subAttributes[i]); + + if (!propertyEnum.containsSubEntityAttribute(propertyField)) { + if (i != subAttributes.length - 1 || !propertyEnum.isMap()) { + throw createRSQLParameterUnsupportedException(node, null); + } // otherwise - the key of map is not in the sub entity attributes + } + + fieldNameBuilder.append(FieldNameProvider.SUB_ATTRIBUTE_SEPARATOR).append(propertyField); + } + return fieldNameBuilder.toString(); } - private void validateMapParameter(final A propertyEnum, final ComparisonNode node, final String[] subAttributes) { - if (!propertyEnum.isMap()) { - return; - } - - if (!propertyEnum.getSubEntityAttributes().isEmpty()) { - throw new UnsupportedOperationException( - "Currently sub-entity attributes for maps are not supported, alternatively you could use the key/value tuple, defined by SimpleImmutableEntry class"); - } - - // enum.key - if (subAttributes.length != 2) { - throw new RSQLParameterUnsupportedFieldException("The syntax of the given map search parameter field {" + - node.getSelector() + "} is wrong. Syntax is: ."); - } - } - /** * @param node current processing node * @param rootException in case there is a cause otherwise {@code null} diff --git a/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/rsql/JpaQueryRsqlVisitorG2.java b/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/rsql/JpaQueryRsqlVisitorG2.java index 0bda5505b..82834f2c0 100644 --- a/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/rsql/JpaQueryRsqlVisitorG2.java +++ b/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/rsql/JpaQueryRsqlVisitorG2.java @@ -51,8 +51,7 @@ import org.springframework.util.CollectionUtils; import org.springframework.util.ObjectUtils; /** - * An implementation of the {@link RSQLVisitor} to visit the parsed tokens and - * build JPA where clauses. + * An implementation of the {@link RSQLVisitor} to visit the parsed tokens and build JPA where clauses. * * @param the enum for providing the field name of the entity field to filter on. * @param the entity type referenced by the root @@ -63,8 +62,8 @@ public class JpaQueryRsqlVisitorG2 & FieldNameProvider, T> public static final Character LIKE_WILDCARD = '*'; private static final char ESCAPE_CHAR = '\\'; - private static final List NO_JOINS_OPERATOR = List.of("!=", "=out="); private static final String ESCAPE_CHAR_WITH_ASTERISK = ESCAPE_CHAR +"*"; + private static final List NO_JOINS_OPERATOR = List.of("!=", "=out="); private final Root root; private final CriteriaQuery query; @@ -94,11 +93,7 @@ public class JpaQueryRsqlVisitorG2 & FieldNameProvider, T> @Override public List visit(final AndNode node, final String param) { final List children = acceptChildren(node); - if (children.isEmpty()) { - return toSingleList(cb.conjunction()); - } else { - return toSingleList(cb.and(children.toArray(new Predicate[0]))); - } + return Collections.singletonList(children.isEmpty() ? cb.conjunction() : cb.and(children.toArray(new Predicate[0]))); } @Override @@ -106,11 +101,7 @@ public class JpaQueryRsqlVisitorG2 & FieldNameProvider, T> inOr = true; try { final List children = acceptChildren(node); - if (children.isEmpty()) { - return toSingleList(cb.conjunction()); - } else { - return toSingleList(cb.or(children.toArray(new Predicate[0]))); - } + return Collections.singletonList(children.isEmpty() ? cb.conjunction() : cb.or(children.toArray(new Predicate[0]))); } finally { inOr = false; javaTypeToPath.clear(); @@ -143,11 +134,17 @@ public class JpaQueryRsqlVisitorG2 & FieldNameProvider, T> final Predicate mapPredicate = mapToMapPredicate(node, enumField, fieldPath); final Predicate valuePredicate = addOperatorPredicate(node, enumField, finalProperty, - getMapValueFieldPath(enumField, fieldPath), transformedValues, value); + getValueFieldPath(enumField, fieldPath), transformedValues, value); - return toSingleList(mapPredicate != null ? cb.and(mapPredicate, valuePredicate) : valuePredicate); + return Collections.singletonList(mapPredicate != null ? cb.and(mapPredicate, valuePredicate) : valuePredicate); + } + private Path getValueFieldPath(final A enumField, final Path fieldPath) { + if (enumField.isMap()) { + return enumField.getSubEntityMapTuple().map(Entry::getValue).map(fieldPath::get).orElse(fieldPath); + } else { + return fieldPath; + } } - @SuppressWarnings("unchecked") private Predicate mapToMapPredicate(final ComparisonNode node, final A enumField, final Path fieldPath) { if (!enumField.isMap()) { @@ -168,7 +165,6 @@ public class JpaQueryRsqlVisitorG2 & FieldNameProvider, T> "SimpleImmutableEntry are allowed. Neither of those could be found!")); return equal(fieldPath.get(keyFieldName), keyValue); } - private Predicate addOperatorPredicate(final ComparisonNode node, final A enumField, final String finalProperty, final Path fieldPath, final List transformedValues, final String value) { // only 'equal' and 'notEqual' can handle transformed value like enums. @@ -188,15 +184,14 @@ public class JpaQueryRsqlVisitorG2 & FieldNameProvider, T> "Operator symbol {" + operator + "} is either not supported or not implemented"); }; } - private Predicate getEqualToPredicate(final Path fieldPath, final Object transformedValue) { if (transformedValue == null) { - return cb.isNull(pathOfString(fieldPath)); + return cb.isNull(fieldPath); } if ((transformedValue instanceof String transformedValueStr) && !NumberUtils.isCreatable(transformedValueStr)) { if (ObjectUtils.isEmpty(transformedValue)) { - return cb.or(cb.isNull(pathOfString(fieldPath)), cb.equal(pathOfString(fieldPath), "")); + return cb.or(cb.isNull(fieldPath), cb.equal(pathOfString(fieldPath), "")); } if (isPattern(transformedValueStr)) { // a pattern, use like @@ -212,26 +207,25 @@ public class JpaQueryRsqlVisitorG2 & FieldNameProvider, T> private Predicate getNotEqualToPredicate(final A enumField, final String finalProperty, final Path fieldPath, final Object transformedValue) { if (transformedValue == null) { - return cb.isNotNull(pathOfString(fieldPath)); + return cb.isNotNull(fieldPath); } if ((transformedValue instanceof String transformedValueStr) && !NumberUtils.isCreatable(transformedValueStr)) { if (ObjectUtils.isEmpty(transformedValue)) { - return cb.and(cb.isNotNull(pathOfString(fieldPath)), cb.notEqual(pathOfString(fieldPath), "")); + return cb.and(cb.isNotNull(fieldPath), cb.notEqual(pathOfString(fieldPath), "")); } final String[] fieldNames = enumField.getSubAttributes(finalProperty); if (isSimpleField(fieldNames, enumField.isMap())) { if (isPattern(transformedValueStr)) { // a pattern, use like - return cb.or(cb.isNull(pathOfString(fieldPath)), notLike(pathOfString(fieldPath), toSQL(transformedValueStr))); + return cb.or(cb.isNull(fieldPath), notLike(pathOfString(fieldPath), toSQL(transformedValueStr))); } else { return toNullOrNotEqualPredicate(fieldPath, transformedValueStr); } } clearJoinsIfNotNeeded(); - return toNotExistsSubQueryPredicate(enumField, fieldNames, expressionToCompare -> isPattern(transformedValueStr) ? // a pattern, use like like(expressionToCompare, toSQL(transformedValueStr)) : @@ -246,12 +240,10 @@ public class JpaQueryRsqlVisitorG2 & FieldNameProvider, T> final String[] fieldNames = enumField.getSubAttributes(finalProperty); if (isSimpleField(fieldNames, enumField.isMap())) { - final Path pathOfString = pathOfString(fieldPath); - return cb.or(cb.isNull(pathOfString), cb.not(in(pathOfString, transformedValues))); + return cb.or(cb.isNull(fieldPath), cb.not(in(pathOfString(fieldPath), transformedValues))); } clearJoinsIfNotNeeded(); - return toNotExistsSubQueryPredicate(enumField, fieldNames, expressionToCompare -> in(expressionToCompare, transformedValues)); } @@ -261,8 +253,7 @@ public class JpaQueryRsqlVisitorG2 & FieldNameProvider, T> Path fieldPath = null; for (int i = 0, end = isMapKeyField ? split.length - 1 : split.length; i < end; i++) { final String fieldNameSplit = split[i]; - fieldPath = fieldPath == null ? - getPath(root, fieldNameSplit) : fieldPath.get(fieldNameSplit); + fieldPath = fieldPath == null ? getPath(root, fieldNameSplit) : fieldPath.get(fieldNameSplit); } if (fieldPath == null) { throw new RSQLParameterUnsupportedFieldException("RSQL field path cannot be empty", null); @@ -336,8 +327,8 @@ public class JpaQueryRsqlVisitorG2 & FieldNameProvider, T> return simpleTypeConverter.convertIfNecessary(value, javaType); } catch (final TypeMismatchException e) { throw new RSQLParameterSyntaxException( - "The value of the given search parameter field {" + node.getSelector() - + "} is not well formed. Only a boolean (true or false) value will be expected {", + "The value of the given search parameter field {" + node.getSelector() + + "} is not well formed. Only a boolean (true or false) value will be expected {", e); } } @@ -346,23 +337,14 @@ public class JpaQueryRsqlVisitorG2 & FieldNameProvider, T> final Object convertedValue = ((FieldValueConverter) fieldName).convertValue(fieldName, value); if (convertedValue == null) { throw new RSQLParameterUnsupportedFieldException( - "field {" + node.getSelector() + "} must be one of the following values {" - + Arrays.toString(((FieldValueConverter) fieldName).possibleValues(fieldName)) + "}", + "field {" + node.getSelector() + "} must be one of the following values {" + + Arrays.toString(((FieldValueConverter) fieldName).possibleValues(fieldName)) + "}", null); } else { return convertedValue; } } - private Path getMapValueFieldPath(final A enumField, final Path fieldPath) { - final String valueFieldNameFromSubEntity = enumField.getSubEntityMapTuple().map(Entry::getValue).orElse(null); - - if (!enumField.isMap() || valueFieldNameFromSubEntity == null) { - return fieldPath; - } - return fieldPath.get(valueFieldNameFromSubEntity); - } - private void clearJoinsIfNotNeeded() { if (!joinsNeeded) { root.getJoins().clear(); @@ -371,7 +353,7 @@ public class JpaQueryRsqlVisitorG2 & FieldNameProvider, T> private Predicate toNullOrNotEqualPredicate(final Path fieldPath, final Object transformedValue) { return cb.or( - cb.isNull(pathOfString(fieldPath)), + cb.isNull(fieldPath), transformedValue instanceof String transformedValueStr ? notEqual(pathOfString(fieldPath), transformedValueStr) : cb.notEqual(fieldPath, transformedValue)); @@ -467,10 +449,6 @@ public class JpaQueryRsqlVisitorG2 & FieldNameProvider, T> return split.length == 1 || (split.length == 2 && isMapKeyField); } - private static List toSingleList(final Predicate predicate) { - return Collections.singletonList(predicate); - } - @SuppressWarnings("unchecked") private static Path pathOfString(final Path path) { return (Path) path;