[#559] Add unit tests for new field name visitor and little refactoring

* Add JavaDocs
* Add static factory
* return unmodifiable set
* better namings
* and other minor simplifications.

Signed-off-by: Joel Bartelheimer <joel.bartelheimer@bosch.io>

concierge/service/src/main/java/org/eclipse/ditto/concierge/service/enforcement/ThingCommandEnforcement.java

@@ -786,11 +786,10 @@ static <T extends ThingCommand<T>> T authorizeByPolicyOrThrow(final Enforcer pol
                     policyEnforcer.hasPartialPermissions(thingResourceKey, authorizationContext, Permission.READ);
         }
 
-        dittoHeaders.getCondition().ifPresent(condition -> {
-            if (!(command instanceof CreateThing)) {
-                enforceReadPermissionOnCondition(condition, policyEnforcer, authorizationContext, dittoHeaders);
-            }
-        });
+        final var condition = dittoHeaders.getCondition();
+        if (!(command instanceof CreateThing) && condition.isPresent()) {
+            enforceReadPermissionOnCondition(condition.get(), policyEnforcer, dittoHeaders);
+        }
 
         if (commandAuthorized) {
             return AbstractEnforcement.addEffectedReadSubjectsToThingSignal(command, policyEnforcer);
@@ -801,18 +800,19 @@ static <T extends ThingCommand<T>> T authorizeByPolicyOrThrow(final Enforcer pol
 
     private static void enforceReadPermissionOnCondition(final String condition,
             final Enforcer policyEnforcer,
-            final AuthorizationContext authorizationContext,
             final DittoHeaders dittoHeaders) {
 
-        final RootNode rootNode = validateCondition(condition, dittoHeaders);
-        final Set<ResourceKey> resourceKeyList = determineResourceKeys(rootNode, dittoHeaders);
-        if (!policyEnforcer.hasUnrestrictedPermissions(resourceKeyList, authorizationContext, Permission.READ)) {
+        final var authorizationContext = dittoHeaders.getAuthorizationContext();
+        final var rootNode = tryParseRqlCondition(condition, dittoHeaders);
+        final var resourceKey = determineResourceKeys(rootNode, dittoHeaders);
+
+        if (!policyEnforcer.hasUnrestrictedPermissions(resourceKey, authorizationContext, Permission.READ)) {
             throw ThingConditionFailedException.newBuilderForInsufficientPermission(condition)
                     .build();
         }
     }
 
-    private static RootNode validateCondition(final String condition, final DittoHeaders dittoHeaders) {
+    private static RootNode tryParseRqlCondition(final String condition, final DittoHeaders dittoHeaders) {
         try {
             return RqlPredicateParser.getInstance().parse(condition);
         } catch (final ParserException e) {
@@ -823,23 +823,25 @@ private static RootNode validateCondition(final String condition, final DittoHea
     }
 
     private static Set<ResourceKey> determineResourceKeys(final RootNode rootNode, final DittoHeaders dittoHeaders) {
-        final FieldNamesPredicateVisitor visitor = new FieldNamesPredicateVisitor();
+        final var visitor = FieldNamesPredicateVisitor.getNewInstance();
         visitor.visit(rootNode);
-        final Set<String> extractedFieldNames = visitor.getFieldNames();
+        final var extractedFieldNames = visitor.getFieldNames();
 
         return extractedFieldNames.stream()
-                .map(fieldName -> {
-                    try {
-                        return PoliciesResourceType.thingResource(fieldName);
-                    } catch (JsonPointerInvalidException e) {
-                        throw ThingConditionInvalidException.newBuilder(fieldName, e.getDescription().orElse(""))
-                                .dittoHeaders(dittoHeaders)
-                                .build();
-                    }
-                })
+                .map(fieldName -> tryGetResourceKey(fieldName, dittoHeaders))
                 .collect(Collectors.toSet());
     }
 
+    private static ResourceKey tryGetResourceKey(final String fieldName, final DittoHeaders dittoHeaders) {
+        try {
+            return PoliciesResourceType.thingResource(fieldName);
+        } catch (final JsonPointerInvalidException e) {
+            throw ThingConditionInvalidException.newBuilder(fieldName, e.getDescription().orElse(""))
+                    .dittoHeaders(dittoHeaders)
+                    .build();
+        }
+    }
+
     private static boolean enforceMergeThingCommand(final Enforcer policyEnforcer,
             final MergeThing command, final ResourceKey thingResourceKey,
             final AuthorizationContext authorizationContext) {

rql/query/src/main/java/org/eclipse/ditto/rql/query/things/FieldNamesPredicateVisitor.java

@@ -12,6 +12,7 @@
  */
 package org.eclipse.ditto.rql.query.things;
 
+import java.util.Collections;
 import java.util.HashSet;
 import java.util.Set;
 
@@ -31,10 +32,23 @@
 @NotThreadSafe
 public final class FieldNamesPredicateVisitor implements PredicateVisitor {
 
-    private final Set<String> fieldNames = new HashSet<>();
+    private final Set<String> fieldNames;
 
+    private FieldNamesPredicateVisitor() {
+        fieldNames = new HashSet<>();
+    }
+
+    public static FieldNamesPredicateVisitor getNewInstance() {
+        return new FieldNamesPredicateVisitor();
+    }
+
+    /**
+     * Returns all names from all fields visited by this visitor.
+     *
+     * @return all field names.
+     */
     public Set<String> getFieldNames() {
-        return fieldNames;
+        return Collections.unmodifiableSet(fieldNames);
     }
 
     @Override

rql/query/src/test/java/org/eclipse/ditto/rql/query/things/FieldNamesPredicateVisitorTest.java

@@ -0,0 +1,47 @@
+/*
+ * Copyright (c) 2021 Contributors to the Eclipse Foundation
+ *
+ * See the NOTICE file(s) distributed with this work for additional
+ * information regarding copyright ownership.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License 2.0 which is available at
+ * http://www.eclipse.org/legal/epl-2.0
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ */
+package org.eclipse.ditto.rql.query.things;
+
+import java.util.Set;
+
+import org.assertj.core.api.JUnitSoftAssertions;
+import org.eclipse.ditto.rql.parser.internal.RqlPredicateParser;
+import org.junit.Rule;
+import org.junit.Test;
+
+/**
+ * Unit test for {@link FieldNamesPredicateVisitor}
+ */
+public final class FieldNamesPredicateVisitorTest {
+
+    @Rule
+    public final JUnitSoftAssertions softly = new JUnitSoftAssertions();
+
+    @Test
+    public void verifyCorrectFiltering() {
+        softly.assertThat(extractFieldsFromRql("and(eq(attributes/foo,1),exists(_created))"))
+                .containsExactlyInAnyOrder("attributes/foo", "_created");
+        softly.assertThat(extractFieldsFromRql("or(exists(attributes/foo),exists(attributes/foo2))"))
+                .containsExactlyInAnyOrder("attributes/foo", "attributes/foo2");
+        softly.assertThat(extractFieldsFromRql("eq(features/*/definition,\"t:t:1\")"))
+                .containsExactlyInAnyOrder("features/*/definition");
+        softly.assertThat(extractFieldsFromRql("eq(/attributes/complex,\"!#$%&'(features)*+,/:;=?@[\\\\]{|}\\\" äaZ0\")"))
+                .containsExactlyInAnyOrder("/attributes/complex");
+    }
+
+    private static Set<String> extractFieldsFromRql(final String filter) {
+        final FieldNamesPredicateVisitor fieldNameVisitor = FieldNamesPredicateVisitor.getNewInstance();
+        fieldNameVisitor.visit(RqlPredicateParser.parse(filter));
+        return fieldNameVisitor.getFieldNames();
+    }
+}