Issue 1197 add memberof operator #1291

Chandrasekar-Rajasekar · 2020-04-27T20:19:52Z

Resolves #1197

Description

Add the member of operation for only Element Collection attributes for root entity or toOne relationship entity.
filter[book]=awards=hasmember='Booker Prize'
filter[book]=publisher.phoneNumbers=hasmember='999-999-9999'

Enables to filter on collection attribute

License

I confirm that this contribution is made under an Apache 2.0 license and that I have the authority necessary to make this contribution on behalf of its copyright owner.

aklish · 2020-04-27T20:55:23Z

elide-core/src/main/java/com/yahoo/elide/core/filter/FilterPredicate.java

@@ -59,6 +59,12 @@ public static boolean toManyInPathExceptLastPathElement(EntityDictionary diction
                .anyMatch(RelationshipType::isToMany);
    }

+    public static boolean checkLastPathElementType(EntityDictionary dictionary, Path path, Class<?> clz) {
+        int pathLength = path.getPathElements().size();
+        PathElement lastPathElement = path.getPathElements().get(pathLength - 1);


There is a method in Path to get the last element.

Check out in the same file:

public static boolean toManyInPath(EntityDictionary dictionary, Path path) { return path.getPathElements().stream() .map(element -> dictionary.getRelationshipType(element.getType(), element.getFieldName())) .anyMatch(RelationshipType::isToMany); } public static boolean toManyInPathExceptLastPathElement(EntityDictionary dictionary, Path path) { int pathLength = path.getPathElements().size(); return path.getPathElements().stream() .limit(pathLength - 1) .map(element -> dictionary.getRelationshipType(element.getType(), element.getFieldName())) .anyMatch(RelationshipType::isToMany); }

Let's add a method that looks like one of these (toManyInLastPathElement):

aklish · 2020-04-27T21:07:55Z

elide-core/src/main/java/com/yahoo/elide/core/filter/Operator.java

+                throw new InvalidPredicateException("HasMember can only take one argument");
+            }
+            Object val = getFieldValue(entity, field, requestScope);
+            String filterStr = CoerceUtil.coerce(values.get(0), String.class);


coerce into val.getClass(). We probably want to support lists of numbers too.

Object val will be an instance of Collection. To get the parameterized type of collection, I think we need to iterate over the path and get paremeterizedType from entityDict. I am not sure how to get parameterized type from Collection object

You are right, we'll need to iterate over it.
Another alternative is to convert field into a Path object in Elide. That will do all the type conversion for you.

aklish · 2020-04-27T21:09:52Z

elide-core/src/main/java/com/yahoo/elide/core/filter/dialect/DefaultFilterDialect.java

@@ -111,12 +112,19 @@ public FilterExpression parseGlobalExpression(String path, MultivaluedMap<String
                throw new ParseException(String.format("Invalid predicate: %s", filterPredicate));
            }

+            if ((filterPredicate.getOperator().equals(Operator.HASMEMBER)


This check seems redundant with the validation logic below. Do we also need this check?

This check is for Global Filter. The validation logic below is for the typed filter. Previously we did not have toMany relationship check for global filter. So I decided to include only Iscollection check for global filter.

Without this check in global filter, we allow filter expr like book?filter=title=hasmember='1' to pass through and jpql rejects these statement.

aklish · 2020-04-27T21:13:29Z

...re/elide-datastore-hibernate/src/main/java/com/yahoo/elide/core/filter/FilterTranslator.java

@@ -170,6 +172,21 @@
            return String.format("%s IS NOT EMPTY", columnAlias);
        });

+        operatorGenerators.put(HASMEMBER, (columnAlias, params) -> {
+            Preconditions.checkArgument(params.size() == 1);
+            String x =  String.format("%s MEMBER OF %s",


Can we remove the local variable here?

aklish · 2020-04-27T21:14:12Z

elide-datastore/elide-datastore-hibernate3/pom.xml

@@ -40,7 +40,7 @@

    <properties>
        <dataStoreHarness>com.yahoo.elide.datastores.hibernate3.HibernateDataStoreHarness</dataStoreHarness>
-        <excludeTags>emptyOnAttributeCollection</excludeTags>
+        <excludeTags>emptyOnAttributeCollection,memberOfOperation</excludeTags>


In Elide 5 - we make tags for stores (rather than features).

aklish · 2020-04-27T21:18:02Z

elide-integration-tests/src/test/java/com/yahoo/elide/tests/FilterIT.java

+        result = getAsNode(String.format("/book?filter[book.awards][hasmember]=%s", filterString));
+        assertEquals(awardBook.size(), result.get("data").size());
+        for (JsonNode book : result.get("data")) {
+            assertTrue(awardBook.contains(book.get("id")));


There is a much cleaner way to do these comparisons with hamcrest Matchers. There are some good examples in PaginationIT.

coveralls · 2020-04-28T22:13:54Z

Coverage increased (+0.03%) to 81.785% when pulling b31c5ac on ISSUE-1197_add_memberof_operator into 1ce3b3c on master.

aklish · 2020-04-30T20:55:41Z

elide-core/src/main/java/com/yahoo/elide/core/filter/FilterPredicate.java

@@ -59,6 +59,15 @@ public static boolean toManyInPathExceptLastPathElement(EntityDictionary diction
                .anyMatch(RelationshipType::isToMany);
    }

+    public static boolean checkLastPathElementType(EntityDictionary dictionary, Path path, Class<?> clz) {


Can we rename this method to something more specific about what it's checking?

How about isLastPathElementAssignableFrom

aklish · 2020-04-30T21:00:42Z

elide-core/src/main/java/com/yahoo/elide/core/filter/Operator.java

+                throw new InvalidPredicateException("HasMember can only take one argument");
+            }
+            Object val = getFieldValue(entity, field, requestScope);
+            String filterStr = CoerceUtil.coerce(values.get(0), String.class);


You are right, we'll need to iterate over it.
Another alternative is to convert field into a Path object in Elide. That will do all the type conversion for you.

aklish · 2020-05-01T20:50:12Z

elide-core/src/main/java/com/yahoo/elide/core/filter/Operator.java

+                throw new InvalidPredicateException("HasMember can only take one argument");
+            }
+            Object val = getFieldValue(entity, fieldPath, requestScope);
+            String filterStr = CoerceUtil.coerce(values.get(0), String.class);


We still need to fix the coercion.

aklish · 2020-05-04T19:20:56Z

elide-integration-tests/src/test/java/com/yahoo/elide/tests/FilterIT.java

+
+    @Test
+    @Tag("excludeOnHibernate3")
+    void testExceptionOnMemberOfOperator() throws IOException {


Can we also add error tests for book.authors and book.publisher?

Updated CVSS limit to 7 to unblock release.

aklish requested changes Apr 27, 2020

View reviewed changes

Chandrasekar-Rajasekar force-pushed the ISSUE-1197_add_memberof_operator branch from f9833ba to c9b9244 Compare April 27, 2020 23:27

aklish requested changes Apr 30, 2020

View reviewed changes

aklish reviewed May 1, 2020

View reviewed changes

aklish reviewed May 4, 2020

View reviewed changes

Chandrasekar Rajasekar and others added 13 commits May 7, 2020 14:19

Hasmember operation added for attribute collection

3ec3798

Allow memberof operation for only attributes and collection types

7c04f62

make tags to datastore

80d22b7

Add exclude tags to test

f81daa3

import error fix

9e7bb79

Refactor Operator.java

22cb181

refactor OperatorTest

4f79753

member of accepts all type

91ed767

Add extra test case

bdfc04d

Update FilterIT.java

3913423

Add unit test

e692575

unit test for filter dialect

7bb1fe7

new unist test for member of filter translator

43ced02

Chandrasekar-Rajasekar force-pushed the ISSUE-1197_add_memberof_operator branch from 7257fa0 to 43ced02 Compare May 7, 2020 19:22

Chandrasekar Rajasekar and others added 6 commits May 7, 2020 14:52

compilation fix

de2151b

Merge branch 'master' into ISSUE-1197_add_memberof_operator

707fd9f

Update pom.xml

68ebef9

Updated CVSS limit to 7 to unblock release.

Merge branch 'master' into ISSUE-1197_add_memberof_operator

4206477

Bug fix

c4dccc0

Merge branch 'master' into ISSUE-1197_add_memberof_operator

b31c5ac

aklish approved these changes May 8, 2020

View reviewed changes

aklish merged commit 21a8332 into master May 8, 2020

aklish deleted the ISSUE-1197_add_memberof_operator branch May 8, 2020 18:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 1197 add memberof operator #1291

Issue 1197 add memberof operator #1291

Chandrasekar-Rajasekar commented Apr 27, 2020

aklish Apr 27, 2020

aklish Apr 27, 2020

aklish Apr 27, 2020

Chandrasekar-Rajasekar Apr 28, 2020

aklish Apr 30, 2020

aklish Apr 27, 2020

Chandrasekar-Rajasekar Apr 27, 2020

aklish Apr 27, 2020

aklish Apr 27, 2020

aklish Apr 27, 2020

coveralls commented Apr 28, 2020 •

edited

Loading

aklish Apr 30, 2020

aklish Apr 30, 2020

aklish May 1, 2020

aklish May 4, 2020

Issue 1197 add memberof operator #1291

Issue 1197 add memberof operator #1291

Conversation

Chandrasekar-Rajasekar commented Apr 27, 2020

Description

License

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 28, 2020 •

edited

Loading