Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 1197 add memberof operator #1291

Merged
merged 19 commits into from
May 8, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,15 @@ public static boolean toManyInPathExceptLastPathElement(EntityDictionary diction
.anyMatch(RelationshipType::isToMany);
}

public static boolean isLastPathElementAssignableFrom(EntityDictionary dictionary, Path path, Class<?> clz) {
return path.lastElement()
.map(last ->
clz.isAssignableFrom(
dictionary.getType(last.getType(), last.getFieldName())
))
.orElse(false);
}

public FilterPredicate(PathElement pathElement, Operator op, List<Object> values) {
this(new Path(Collections.singletonList(pathElement)), op, values);
}
Expand Down Expand Up @@ -145,7 +154,7 @@ public <T> T accept(FilterExpressionVisitor<T> visitor) {

@Override
public Predicate apply(RequestScope dictionary) {
return operator.contextualize(getFieldPath(), values, dictionary);
return operator.contextualize(path, values, dictionary);
}

public boolean isMatchingOperator() {
Expand Down
173 changes: 106 additions & 67 deletions elide-core/src/main/java/com/yahoo/elide/core/filter/Operator.java

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import com.yahoo.elide.utils.coerce.CoerceUtil;

import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap;
import java.util.List;
import java.util.Locale;
Expand Down Expand Up @@ -111,12 +112,20 @@ public FilterExpression parseGlobalExpression(String path, MultivaluedMap<String
throw new ParseException(String.format("Invalid predicate: %s", filterPredicate));
}

if ((filterPredicate.getOperator().equals(Operator.HASMEMBER)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

|| filterPredicate.getOperator().equals(Operator.HASNOMEMBER))
&& !FilterPredicate.isLastPathElementAssignableFrom(
dictionary, filterPredicate.getPath(), Collection.class)) {
throw new ParseException("Invalid Path: Last Path Element has to be a collection type");
}

if (joinedExpression == null) {
joinedExpression = filterPredicate;
} else {
joinedExpression = new AndFilterExpression(joinedExpression, filterPredicate);
}
}

return joinedExpression;
}

Expand Down Expand Up @@ -201,6 +210,10 @@ private void validateFilterPredicate(FilterPredicate filterPredicate) throws Par
case NOTEMPTY:
emptyOperatorConditions(filterPredicate);
break;
case HASMEMBER:
case HASNOMEMBER:
memberOfOperatorConditions(filterPredicate);
break;
default:
if (FilterPredicate.toManyInPath(dictionary, filterPredicate.getPath())) {
throw new ParseException("Invalid toMany join: " + filterPredicate);
Expand All @@ -221,4 +234,14 @@ private void emptyOperatorConditions(FilterPredicate filterPredicate) throws Par
+ filterPredicate);
}
}

private void memberOfOperatorConditions(FilterPredicate filterPredicate) throws ParseException {
if (FilterPredicate.toManyInPath(dictionary, filterPredicate.getPath())) {
throw new ParseException("Invalid toMany join: member of operator cannot be used for toMany relationships");
}

if (!FilterPredicate.isLastPathElementAssignableFrom(dictionary, filterPredicate.getPath(), Collection.class)) {
throw new ParseException("Invalid Path: Last Path Element has to be a collection type");
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import cz.jirutka.rsql.parser.ast.RSQLVisitor;

import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
Expand All @@ -60,6 +61,8 @@ public class RSQLFilterDialect implements SubqueryFilterDialect, JoinFilterDiale
private static final Pattern TYPED_FILTER_PATTERN = Pattern.compile("filter\\[([^\\]]+)\\]");
private static final ComparisonOperator ISNULL_OP = new ComparisonOperator("=isnull=", false);
private static final ComparisonOperator ISEMPTY_OP = new ComparisonOperator("=isempty=", false);
private static final ComparisonOperator HASMEMBER_OP = new ComparisonOperator("=hasmember=", false);
private static final ComparisonOperator HASNOMEMBER_OP = new ComparisonOperator("=hasnomember=", false);

/* Subset of operators that map directly to Elide operators */
private static final Map<ComparisonOperator, Operator> OPERATOR_MAP =
Expand All @@ -68,6 +71,8 @@ public class RSQLFilterDialect implements SubqueryFilterDialect, JoinFilterDiale
.put(RSQLOperators.GREATER_THAN, Operator.GT)
.put(RSQLOperators.GREATER_THAN_OR_EQUAL, Operator.GE)
.put(RSQLOperators.LESS_THAN_OR_EQUAL, Operator.LE)
.put(HASMEMBER_OP, Operator.HASMEMBER)
.put(HASNOMEMBER_OP, Operator.HASNOMEMBER)
.build();


Expand All @@ -90,6 +95,8 @@ private static Set<ComparisonOperator> getDefaultOperatorsWithIsnull() {
Set<ComparisonOperator> operators = RSQLOperators.defaultOperators();
operators.add(ISNULL_OP);
operators.add(ISEMPTY_OP);
operators.add(HASMEMBER_OP);
operators.add(HASNOMEMBER_OP);
return operators;
}

Expand Down Expand Up @@ -291,7 +298,7 @@ public FilterExpression visit(ComparisonNode node, Class entityType) {
Path path = buildPath(entityType, relationship);

//handles '=isempty=' op before coerce arguments
// ToMany Association is allowed if the operation in Is Empty
// ToMany Association is allowed if the operation is IsEmpty
if (op.equals(ISEMPTY_OP)) {
if (FilterPredicate.toManyInPathExceptLastPathElement(dictionary, path)
&& !allowNestedToManyAssociations) {
Expand All @@ -302,6 +309,16 @@ public FilterExpression visit(ComparisonNode node, Class entityType) {
return buildIsEmptyOperator(path, arguments);
}

if (op.equals(HASMEMBER_OP) || op.equals(HASNOMEMBER_OP)) {
if (FilterPredicate.toManyInPath(dictionary, path)) {
throw new RSQLParseException(
"Invalid toMany join: member of operator cannot be used for toMany relationships");
}
if (!FilterPredicate.isLastPathElementAssignableFrom(dictionary, path, Collection.class)) {
throw new RSQLParseException("Invalid Path: Last Path Element has to be a collection type");
}
}

if (FilterPredicate.toManyInPath(dictionary, path) && !allowNestedToManyAssociations) {
throw new RSQLParseException(String.format("Invalid association %s", relationship));
}
Expand Down Expand Up @@ -406,5 +423,6 @@ private FilterExpression buildIsEmptyOperator(Path path, List<String> arguments)
throw new RSQLParseException(String.format("Invalid value for operator =isempty= '%s'", arg));
}
}

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,9 @@ public final boolean ok(T object, RequestScope requestScope, Optional<ChangeSpec
*/
public boolean applyPredicateToObject(T object, FilterPredicate filterPredicate, RequestScope requestScope) {
try {
String fieldPath = filterPredicate.getFieldPath();
com.yahoo.elide.core.RequestScope scope = coreScope(requestScope);
Predicate<T> fn = filterPredicate.getOperator()
.contextualize(fieldPath, filterPredicate.getValues(), scope);
.contextualize(filterPredicate.getPath(), filterPredicate.getValues(), scope);
return fn.test(object);
} catch (Exception e) {
log.error("Failed to apply predicate {}", filterPredicate, e);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -773,7 +773,7 @@ public String toString() {
assertFalse(isComputed(Editor.class, "badfield"));

assertEquals(
ImmutableSet.of("genre", "language", "title"),
ImmutableSet.of("awards", "genre", "language", "title"),
getFieldsOfType(Book.class, String.class));

assertTrue(isRelation(Book.class, "editor"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import org.junit.jupiter.api.Test;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
Expand Down Expand Up @@ -91,23 +92,26 @@ public InMemoryStoreTransactionTest() {
"English",
System.currentTimeMillis(),
Sets.newHashSet(author),
publisher1);
publisher1,
Arrays.asList("Prize1"));

book2 = new Book(2,
"Book 2",
"Science Fiction",
"English",
System.currentTimeMillis(),
Sets.newHashSet(author),
publisher1);
publisher1,
Arrays.asList("Prize1", "Prize2"));

book3 = new Book(3,
"Book 3",
"Literary Fiction",
"English",
System.currentTimeMillis(),
Sets.newHashSet(author),
publisher2);
publisher2,
Arrays.asList());

books.add(book1);
books.add(book2);
Expand Down
Loading