Allow empty filter expressions to be visited

[david-driscoll]

QueryableCombinator.TryCombineOperations never actually "tries" anything. It either succeeds or throws.

I'm working on integrating Raven Db with HotChocolate and everything is going fairly well however I ran into a strange case (unique to how Raven does things vs other databases). Spatial queries are done directly on the IQueryable instance instead of as part of the expression itself. I have solved this part of the problem using additional middleware. (If anyone is interested let me know, it was fun, I've solved spatial filtering and spatial sorting!). The problem here is that my middleware happens after the filter middleware, and for filtering to work today it must always have some expression added to the tree. Well Raven hates Expression.Constant(true), so I had to go digging.

The next problem I'm now running into is QueryableCombinator.TryCombineOperations never actually "tries" anything. Instead of setting the output parameter to null and returning false it simply throws if the collection is zero. This disallows filters that are simply empty root(where: {}) or in my case filters that are skipped and simply handled elsewhere in middleware. I also found this issue and it turned out to be related.

During testing it became apparent this was solving an issue and causing some side-effects, but as I reviewed the snapshots, they were errors which I think (hope! 🤣) are unexpected.

Please let me know if you have any questions or concerns. I wasn't sure if there was a branch to support targeting 12 or if it will just be version 13 forward, I can work with that too.

🥳 🎉

Closes #1705

[michaelstaib]

Hey @david-driscoll,

main is 13.

We have a branch called main-version-12 which which is for bug fixing version 12.

We are holding off on merging this PR for this week since we have a large PR coming in introducing the new V13 execution engine. I will merge your PR once we have #5168 in. Probably after the weekend.

[david-driscoll]

I'll wait for the review, but if this is a change that would benefit version 12 I can rebase the branch from there.

Is there a process to then get those changes merged up into main or is that something ya'll handle I assume.

[PascalSenn]

@david-driscoll sorry for this taking so long :)
The only issue i see is that it is semantically wrong to have where: {} return the all the documents.
The where filter is like a gate where only values go through that match the filter. Having a where clause but no conditions should therefore just return no objects at all (and of course no error message / a better error message)

I would like to know how you guys see this @david-driscoll @michaelstaib

Spatial queries are done directly on the IQueryable instance instead of as part of the expression itself. I have solved this part of the problem using additional middleware. (If anyone is interested let me know, it was fun, I've solved spatial filtering and spatial sorting!).

Can you share this? Maybe i can show you how to plug this into the filtering directly :) This would mean (if it works) you would not require a middleware and could directly alter the expression generation :)

[david-driscoll]

@PascalSenn A where with no conditions would effectively behave like true = true would it not? Or more specifically behave as if no expression were applied? I'm not super involved with the graphql spec, so I don't know if there is special meaning to where: {} however as a consumer that reads to me as "empty criteria" which would be same as doing nothing. If however, it should return zero objects, then that is better than a full-on error (even if I disagree with the behavior). It would also make doing dynamic filtering harder as described in #1705. Perhaps make the behavior configurable?

Now to the core problem of with queryable projection as it exists today is that it creates a single predicate to the applied to the Where linq method.

https://github.com/ChilliCream/hotchocolate/blob/80350d82253419a9ff631bbbaae0f640eb834753/src/HotChocolate/Data/src/Data/Projections/Expressions/QueryableProjectionProvider.cs#L78-L88

In my case RavenDB has a special Spatial extension method that is used for creating spatial queries, this method is applied to the queryable itself. Here are the docs for an example. The way the HotChocolate.Spatial methods work do not apply in the case of RavenDB, which is fine not everything works the same.

A copy of the code is here: https://github.com/david-driscoll/raven-hc-spatial

So in order to properly call this method, in the best way I could figure out.

• I've added a custom FilterFieldHandler that supports the spatial operations, this is explicitly does not add any expressions to the tree, by just popping on leave.
  • This allows me to tell HC to "ignore" the fields I'm looking to do spatial filtering with later.
• I have my own FilterVisitor that does some very bad things. It actually removes items from the parsed expression.
  • This breaks caching and has to be explicitly disabled for this to work.
  • What this does is essentially removes my fields from the where syntax,
• Then I have my middleware apply the Spatial Query if any of the field data was found.
  • To get around issues with the current filtering throwing an exception, I simply force the local context to think it's already handled the filtering here

The code change in this pull request was specifically targeted to do two things. It no longer throws an exception if the where clause is empty or if the where clause produced operation no expressions. The handlers and operations still have to be known by the graph schema. This then gives the flexibility to do something a little out of the ordinary with that expression data elsewhere in the pipeline.

[david-driscoll]

Sorting was done very similarly, however when sorting is applied to the queryable it does not throw in the same manner as QueryableCombiner. If there are zero expressions returned it just returns the source expression.

https://github.com/ChilliCream/hotchocolate/blob/80350d82253419a9ff631bbbaae0f640eb834753/src/HotChocolate/Data/src/Data/Sorting/Expressions/Extensions/QueryableSortContextExtensions.cs#L21-L48

[PascalSenn]

@david-driscoll
I see. As we also support [] in sorting and it generally make the UI implementation easier (no special check for empty array) i think we can go along with this.

Regarding the GraphQL specification, it does not really have a behaviour defined for these use cases. Filter is built on top of GraphQL.

I aligned the behaviour across the different providers (Mongodb and Neo4j). they also now allow where: {}.

@michaelstaib i did a breaking api change there as we do not longer need TryCreateQuery in mongodb and Neo4j. It's now just CreateQuery.
If you agree on this change then we can go ahead with this pr

[michaelstaib]

@PascalSenn if we want this change on 12 no breaking changes please.

[PascalSenn]

sure thing, this will only go to main

[PascalSenn]

if we want to backport this to 12 we can easily make it non breaking

[michaelstaib]

@PascalSenn for main I can merge it ... but david wants it on the v12 branch as well.

[michaelstaib]

/AzurePipelines run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[michaelstaib]

/AzurePipelines run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s), but failed to run 1 pipeline(s).

[michaelstaib]

/AzurePipelines run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[michaelstaib]

@PascalSenn can you have a look at the Data test errors?

Neo4J is OK, is source gen failed on build ... this is just a build issue.
AspNetCore is OK seems to be issues with the socket tests.
StrawberryShake is OK since we at the moment have a couple of errors on main here.

But Data has a single error that you have to review.

[michaelstaib]

CodeQL also has issues with the source generators ... this seams to be related to the .NET 7 Upgrade. We will fix that in a separate PR

[michaelstaib]

@david-driscoll pascal will port this one back to the version 12 branch.