Issue 683 (#1384)

* Improved error handling * Tests * update test * Added logic to test collections against collections * Added operator in test that traverses to-many relationship * Converted text operators * Added support for comparator operators * Build passes * Added back failing Filter IT test * Updated dialect tests * Updateed FilterIT tests Co-authored-by: wcekan <[email protected]> Co-authored-by: Aaron Klish <[email protected]> Co-authored-by: Aaron Klish <[email protected]>
yahoo · Jul 9, 2020 · dae9b09 · dae9b09
1 parent fea3219
commit dae9b09
Show file tree

Hide file tree

Showing 14 changed files with 309 additions and 123 deletions.
diff --git a/elide-core/src/main/java/com/yahoo/elide/Elide.java b/elide-core/src/main/java/com/yahoo/elide/Elide.java
@@ -209,7 +209,6 @@ public ElideResponse post(String path, String jsonApiDocument,
      * @param accept the accept
      * @param path the path
      * @param jsonApiDocument the json api document
-     * @param queryParams the query params
      * @param opaqueUser the opaque user
      * @return Elide response object
      */

diff --git a/elide-core/src/main/java/com/yahoo/elide/core/DataStoreTransaction.java b/elide-core/src/main/java/com/yahoo/elide/core/DataStoreTransaction.java
@@ -270,7 +270,8 @@ default FeatureSupport supportsFiltering(Class<?> entityClass, FilterExpression
 
     /**
      * Whether or not the transaction can sort the provided class.
-     * @param entityClass
+     * @param entityClass The model type
+     * @param sorting Whether or not the store supports sorting for the given model
      * @return true if sorting is possible
      */
     default boolean supportsSorting(Class<?> entityClass, Sorting sorting) {

diff --git a/elide-core/src/main/java/com/yahoo/elide/core/RequestScope.java b/elide-core/src/main/java/com/yahoo/elide/core/RequestScope.java
@@ -169,7 +169,7 @@ public RequestScope(String path,
                             errorMessage = errorMessage + "\n" + e.getMessage();
                         }
 
-                        throw new BadRequestException(errorMessage);
+                        throw new BadRequestException(errorMessage, e);
                     }
                 }
             }

diff --git a/elide-core/src/main/java/com/yahoo/elide/core/exceptions/BadRequestException.java b/elide-core/src/main/java/com/yahoo/elide/core/exceptions/BadRequestException.java
@@ -14,4 +14,8 @@ public class BadRequestException extends HttpStatusException {
     public BadRequestException(String message) {
         super(HttpStatus.SC_BAD_REQUEST, message);
     }
+
+    public BadRequestException(String message, Throwable cause) {
+        super(HttpStatus.SC_BAD_REQUEST, message, cause, null);
+    }
 }
diff --git a/elide-core/src/main/java/com/yahoo/elide/core/filter/Operator.java b/elide-core/src/main/java/com/yahoo/elide/core/filter/Operator.java
@@ -19,8 +19,11 @@
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
+import java.util.Objects;
+import java.util.function.BiPredicate;
 import java.util.function.Function;
 import java.util.function.Predicate;
+import java.util.stream.Collectors;
 
 /**
  * Operator enum for predicates.
@@ -180,8 +183,7 @@ public <T> Predicate<T> contextualize(Path fieldPath, List<Object> values, Reque
         public <T> Predicate<T> contextualize(Path fieldPath, List<Object> values, RequestScope requestScope) {
             return entiry -> !hasMember(fieldPath, values, requestScope).test(entiry);
         }
-    }
-    ;
+    };
 
     public static final Function<String, String> FOLD_CASE = s -> s.toLowerCase(Locale.ENGLISH);
     @Getter private final String notation;
@@ -234,90 +236,90 @@ public static Operator fromString(final String string) {
     // In with strict equality
     private static <T> Predicate<T> in(Path fieldPath, List<Object> values, RequestScope requestScope) {
         return (T entity) -> {
-            Object val = getFieldValue(entity, fieldPath, requestScope);
+            BiPredicate predicate = (a, b) -> a.equals(b);
 
-            return val != null && values.stream()
-                    .map(v -> CoerceUtil.coerce(v, val.getClass()))
-                    .anyMatch(val::equals);
+            return evaluate(entity, fieldPath, values, predicate, requestScope);
         };
     }
 
     //
     // String-like In with optional transformation
     private static <T> Predicate<T> in(Path fieldPath, List<Object> values,
-                                       RequestScope requestScope, Function<String, String> transform) {
+            RequestScope requestScope, Function<String, String> transform) {
         return (T entity) -> {
-            Object fieldValue = getFieldValue(entity, fieldPath, requestScope);
 
-            if (fieldValue == null) {
-                return false;
-            }
+            BiPredicate predicate = (a, b) -> {
+                if (!a.getClass().isAssignableFrom(String.class)) {
+                    throw new IllegalStateException("Cannot case insensitive compare non-string values");
+                }
 
-            if (!fieldValue.getClass().isAssignableFrom(String.class)) {
-                throw new IllegalStateException("Cannot case insensitive compare non-string values");
-            }
+                String lhs = transform.apply((String) a);
+                String rhs = transform.apply(CoerceUtil.coerce(b, String.class));
+
+                return lhs.equals(rhs);
+            };
 
-            String val = transform.apply((String) fieldValue);
-            return val != null && values.stream()
-                    .map(v -> transform.apply(CoerceUtil.coerce(v, String.class)))
-                    .anyMatch(val::equals);
+            return evaluate(entity, fieldPath, values, predicate, requestScope);
         };
     }
 
     //
     // String-like prefix matching with optional transformation
     private static <T> Predicate<T> prefix(Path fieldPath, List<Object> values,
-                                           RequestScope requestScope, Function<String, String> transform) {
+            RequestScope requestScope, Function<String, String> transform) {
         return (T entity) -> {
             if (values.size() != 1) {
                 throw new BadRequestException("PREFIX can only take one argument");
             }
 
-            Object val = getFieldValue(entity, fieldPath, requestScope);
-            String valStr = CoerceUtil.coerce(val, String.class);
-            String filterStr = CoerceUtil.coerce(values.get(0), String.class);
+            BiPredicate predicate = (a, b) -> {
+                String lhs = transform.apply(CoerceUtil.coerce(a, String.class));
+                String rhs = transform.apply(CoerceUtil.coerce(b, String.class));
 
-            return valStr != null
-                    && filterStr != null
-                    && transform.apply(valStr).startsWith(transform.apply(filterStr));
+                return lhs != null && rhs != null && lhs.startsWith(rhs);
+            };
+
+            return evaluate(entity, fieldPath, values, predicate, requestScope);
         };
     }
 
     //
     // String-like postfix matching with optional transformation
     private static <T> Predicate<T> postfix(Path fieldPath, List<Object> values,
-                                            RequestScope requestScope, Function<String, String> transform) {
+            RequestScope requestScope, Function<String, String> transform) {
         return (T entity) -> {
             if (values.size() != 1) {
                 throw new BadRequestException("POSTFIX can only take one argument");
             }
 
-            Object val = getFieldValue(entity, fieldPath, requestScope);
-            String valStr = CoerceUtil.coerce(val, String.class);
-            String filterStr = CoerceUtil.coerce(values.get(0), String.class);
+            BiPredicate predicate = (a, b) -> {
+                String lhs = transform.apply(CoerceUtil.coerce(a, String.class));
+                String rhs = transform.apply(CoerceUtil.coerce(b, String.class));
+
+                return lhs != null && rhs != null && lhs.endsWith(rhs);
+            };
 
-            return valStr != null
-                    && filterStr != null
-                    && transform.apply(valStr).endsWith(transform.apply(filterStr));
+            return evaluate(entity, fieldPath, values, predicate, requestScope);
         };
     }
 
     //
     // String-like infix matching with optional transformation
     private static <T> Predicate<T> infix(Path fieldPath, List<Object> values,
-                                          RequestScope requestScope, Function<String, String> transform) {
+            RequestScope requestScope, Function<String, String> transform) {
         return (T entity) -> {
             if (values.size() != 1) {
                 throw new BadRequestException("INFIX can only take one argument");
             }
 
-            Object val = getFieldValue(entity, fieldPath, requestScope);
-            String valStr = CoerceUtil.coerce(val, String.class);
-            String filterStr = CoerceUtil.coerce(values.get(0), String.class);
+            BiPredicate predicate = (a, b) -> {
+                String lhs = transform.apply(CoerceUtil.coerce(a, String.class));
+                String rhs = transform.apply(CoerceUtil.coerce(b, String.class));
+
+                return lhs != null && rhs != null && lhs.contains(rhs);
+            };
 
-            return valStr != null
-                    && filterStr != null
-                    && transform.apply(valStr).contains(transform.apply(filterStr));
+            return evaluate(entity, fieldPath, values, predicate, requestScope);
         };
     }
 
@@ -355,7 +357,9 @@ private static <T> Predicate<T> isEmpty(Path fieldPath, RequestScope requestScop
         return (T entity) -> {
 
             Object val = getFieldValue(entity, fieldPath, requestScope);
-            if (val == null) { return false; }
+            if (val == null) {
+                return false;
+            }
             if (val instanceof Collection<?>) {
                 return ((Collection<?>) val).isEmpty();
             }
@@ -377,7 +381,9 @@ private static <T> Predicate<T> hasMember(Path fieldPath, List<Object> values, R
                     .map(last -> CoerceUtil.coerce(values.get(0), last.getFieldType()))
                     .orElse(CoerceUtil.coerce(values.get(0), String.class));
 
-            if (val == null) { return false; }
+            if (val == null) {
+                return false;
+            }
             if (val instanceof Collection<?>) {
                 return ((Collection<?>) val).contains(filterStr);
             }
@@ -390,7 +396,7 @@ private static <T> Predicate<T> hasMember(Path fieldPath, List<Object> values, R
     }
 
     /**
-     * Return value of field/path for given entity.  For example this.book.author
+     * Return value of field/path for given entity. For example this.book.author
      *
      * @param <T> the type of entity to retrieve a value from
      * @param entity Entity bean
@@ -407,18 +413,36 @@ private static <T> Object getFieldValue(T entity, Path fieldPath, RequestScope r
             if (val == null) {
                 break;
             }
-            val = PersistentResource.getValue(val, field.getFieldName(), requestScope);
+            if (val instanceof Collection) {
+                val = ((Collection) val).stream()
+                        .filter(Objects::nonNull)
+                        .map(target -> PersistentResource.getValue(target, field.getFieldName(), requestScope))
+                        .filter(Objects::nonNull)
+                        .collect(Collectors.toSet());
+            } else {
+                val = PersistentResource.getValue(val, field.getFieldName(), requestScope);
+            }
         }
         return val;
     }
 
     private static <T> Predicate<T> getComparator(Path fieldPath, List<Object> values,
-                                                  RequestScope requestScope, Predicate<Integer> condition) {
+            RequestScope requestScope, Predicate<Integer> condition) {
         return (T entity) -> {
             if (values.size() == 0) {
                 throw new BadRequestException("No value to compare");
             }
             Object fieldVal = getFieldValue(entity, fieldPath, requestScope);
+
+            if (fieldVal instanceof Collection) {
+                return ((Collection) fieldVal).stream()
+                        .anyMatch((fieldValueElement) -> {
+                            return fieldValueElement != null
+                                    && values.stream()
+                                    .anyMatch(testVal -> condition.test(compare(fieldValueElement, testVal)));
+                        });
+            }
+
             return fieldVal != null
                     && values.stream()
                     .anyMatch(testVal -> condition.test(compare(fieldVal, testVal)));
@@ -434,6 +458,26 @@ private static int compare(Object fieldValue, Object rawTestValue) {
         return fieldComp.compareTo(testComp);
     }
 
+    private static boolean evaluate(Object entity, Path fieldPath, List<Object> values,
+                             BiPredicate predicate, RequestScope requestScope) {
+        Class<?> valueClass = fieldPath.lastElement().get().getFieldType();
+
+        Object leftHandSide = getFieldValue(entity, fieldPath, requestScope);
+
+        if (leftHandSide instanceof Collection && !valueClass.isAssignableFrom(Collection.class)) {
+            return ((Collection) leftHandSide).stream()
+                    .anyMatch((leftHandSideElement) -> {
+                        return values.stream()
+                                .map(value -> CoerceUtil.coerce(value, valueClass))
+                                .anyMatch(value -> predicate.test(leftHandSideElement, value));
+                    });
+        } else {
+            return leftHandSide != null && values.stream()
+                    .map(value -> CoerceUtil.coerce(value, valueClass))
+                    .anyMatch(value -> predicate.test(leftHandSide, value));
+        }
+    }
+
     public Operator negate() {
         if (negated == null) {
             throw new InvalidOperatorNegationException();

diff --git a/elide-core/src/main/java/com/yahoo/elide/core/filter/dialect/DefaultFilterDialect.java b/elide-core/src/main/java/com/yahoo/elide/core/filter/dialect/DefaultFilterDialect.java
@@ -214,10 +214,6 @@ private void validateFilterPredicate(FilterPredicate filterPredicate) throws Par
             case HASNOMEMBER:
                 memberOfOperatorConditions(filterPredicate);
                 break;
-            default:
-                if (FilterPredicate.toManyInPath(dictionary, filterPredicate.getPath())) {
-                    throw new ParseException("Invalid toMany join: " + filterPredicate);
-                }
         }
     }
 

diff --git a/elide-core/src/main/java/com/yahoo/elide/core/filter/dialect/MultipleFilterDialect.java b/elide-core/src/main/java/com/yahoo/elide/core/filter/dialect/MultipleFilterDialect.java
@@ -68,7 +68,10 @@ private static <T, R> R parseExpression(List<T> dialects, ParseFunction<T, R> pa
                     log.trace("Parse Failure: {}", e.getMessage());
                 }
                 if (lastFailure != null) {
+                    ParseException prev = lastFailure;
                     lastFailure = new ParseException(e.getMessage() + "\n" + lastFailure.getMessage());
+                    lastFailure.addSuppressed(prev);
+                    lastFailure.addSuppressed(e);
                 } else {
                     lastFailure = e;
                 }

diff --git a/elide-core/src/main/java/com/yahoo/elide/core/filter/dialect/RSQLFilterDialect.java b/elide-core/src/main/java/com/yahoo/elide/core/filter/dialect/RSQLFilterDialect.java
@@ -166,7 +166,7 @@ public Map<String, FilterExpression> parseTypedExpression(String path, Multivalu
 
                 String expressionText = paramValues.get(0);
 
-                FilterExpression filterExpression = parseFilterExpression(expressionText, entityType, false);
+                FilterExpression filterExpression = parseFilterExpression(expressionText, entityType, true);
                 expressionByType.put(typeName, filterExpression);
             } else {
                 throw new ParseException(INVALID_QUERY_PARAMETER + paramName);
@@ -300,8 +300,7 @@ public FilterExpression visit(ComparisonNode node, Class entityType) {
             //handles '=isempty=' op before coerce arguments
             // ToMany Association is allowed if the operation is IsEmpty
             if (op.equals(ISEMPTY_OP)) {
-                if (FilterPredicate.toManyInPathExceptLastPathElement(dictionary, path)
-                        && !allowNestedToManyAssociations) {
+                if (FilterPredicate.toManyInPathExceptLastPathElement(dictionary, path)) {
                     throw new RSQLParseException(
                             String.format("Invalid association %s. toMany association has to be the target collection.",
                                     relationship));

diff --git a/elide-core/src/test/java/com/yahoo/elide/core/PersistentResourceTest.java b/elide-core/src/test/java/com/yahoo/elide/core/PersistentResourceTest.java
@@ -47,6 +47,7 @@
 import com.google.common.collect.Sets;
 
 import example.Author;
+import example.Book;
 import example.Child;
 import example.Color;
 import example.ComputedBean;
@@ -2232,6 +2233,27 @@ public void testFilterExpressionByType() {
         assertEquals("[Author].name", predicate.getPath().toString());
     }
 
+    @Test
+    public void testFilterExpressionCollection() {
+        MultivaluedMap<String, String> queryParams = new MultivaluedHashMap<>();
+
+        queryParams.add(
+                "filter[book.authors.name][infix]",
+                "Hemingway"
+        );
+
+        RequestScope scope = buildRequestScope("/", mock(DataStoreTransaction.class), new User(1), queryParams);
+
+        Optional<FilterExpression> filter = scope.getLoadFilterExpression(Book.class);
+        FilterPredicate predicate = (FilterPredicate) filter.get();
+        assertEquals("name", predicate.getField());
+        assertEquals("authors.name", predicate.getFieldPath());
+        assertEquals(Operator.INFIX, predicate.getOperator());
+        assertEquals(Arrays.asList("Hemingway"), predicate.getValues());
+        assertEquals("[Book].authors/[Author].name", predicate.getPath().toString());
+    }
+
+
     @Test
     public void testSparseFields() {
         MultivaluedMap<String, String> queryParams = new MultivaluedHashMap<>();