Skip to content

Filtering with "any" requires more than one entry in the set #1152

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

Closed
bjornharrtell opened this issue Apr 28, 2022 · 9 comments · Fixed by #1234
Closed

Filtering with "any" requires more than one entry in the set #1152

bjornharrtell opened this issue Apr 28, 2022 · 9 comments · Fixed by #1234

Comments

@bjornharrtell
Copy link
Contributor

If a single element is given it results in error message "The specified filter is invalid" and detail ", expected.".

@bkoelman
Copy link
Member

This is by design. A set must contain at least two elements. A single element is a scalar value, for which other operators already exist. And an empty set for this operator makes no sense.

@ZilberberhV
Copy link

ZilberberhV commented Dec 20, 2022

@bkoelman Just stumbled on the same issue. Filter syntax section of documentation indicates this filter should work with 1 and more values, please update accordingly. But it would be much more convenient to support 1 value as well when list of filters depends on user input.

@steveland83
Copy link

I'd be interested to learn why you expect a set to have two or more elements @bkoelman - granted its a long time since I've done any set theory, but I cant remember anything limiting the number of items in a valid set (not to mention the empty set, which is an entirely valid set, with no elements...)

@bkoelman
Copy link
Member

Filter syntax section of documentation indicates this filter should work with 1 and more values

I don't think that it does. It is defined as:

anyExpression: 'any' LPAREN fieldChain COMMA literalConstant ( COMMA literalConstant )+

Where ( ... )+ means: at least one occurrence.

The design consideration was to avoid providing multiple ways to achieve the same thing. For a single term, equals is already available. An empty set is meaningless in a filter, as it would never match anything. This is not about what sets are in general, it is about what kinds of sets this operator accepts. Constraining input reduces strange usage. It's kinda odd to say: "please choose one of these: item1". That's not much of a choice, is it?

@bkoelman
Copy link
Member

Let me elaborate a bit (at the risk of contradicting what I said before). The "any" function was added as a shorter way of or-ing multiple equality checks, simply because it's a commonly used pattern. For example, any(f,'A','B','C') is shorthand for or(equals(f,'A'),equals(f,'B'),equals(f,'C')). Query strings are best kept short due to server constraints. So it was never intended as a replacement for the equals function. Perhaps more relevant: any translates to a SQL IN expression. Databases may or may not recognize single-term usage and optimize that, so there's a potential performance impact as well.

What makes it problematic to choose between equals and any based on user input?

@bjornharrtell
Copy link
Contributor Author

It would be more end user friendly to not have to special case 1 element array for the filter construction. That it has performance implications when translated to SQL is leaking abstraction in my opinion and should be taken care of in the translation below the filter abstraction rather than make it end user responsibilty. That said, it's just a minor nuisance so I opted to not continue to nag about it.. but since more people are being affected perhaps it should be reconsidered.

@steveland83
Copy link

Thanks for the feedback @bkoelman - appreciate the consideration you have put into this.
That said I still think this should be changed to accept a single term, for two reasons:

  1. As you've clarified, the current implementation does not satisfy the specification (one or more VS two or more) which means that this is likely to be a fairly common stumbling block for consumers of the library.

  2. Consider the use case of a form with a multi select option (e.g. the brand filter from amazon, shown below). Filtering on a single item in a case like this is a common usage - and as the JsonApiDotNetCore implementation stands the only way to do this is to have two separate cases implemented to acheive this, depending on how many elements are selected.

The case I have is an integration API provided to client companies, who implement their own front ends - so we do not control the client code. As such our choices are to compell clients to code around this, or work around it ourselves with custom code or a fork, and I would much prefer to fix this in the official version, for everyone :)

image

@bkoelman
Copy link
Member

It turns out that EF Core translates single-element collections to = instead of IN(...), so the performance concern does not apply. Perhaps EF Core does so because not all databases allow single-element IN expressions, but I'm just guessing here.

@steveland83

  1. That's not what I intended to say. I disagree with you and believe the BNF is correct as-is. COMMA literalConstant ( COMMA literalConstant )+ means: at least two occurrences of COMMA literalConstant.
  2. I understand your concern better now. I've opened Allow at least one constant instead of two in any() filter function #1234 to enable this.

Thanks all for providing this feedback!

@bkoelman
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants