Skip to content

Support for query parameters on nested/deep resource endpoints #627

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
bart-degreed opened this issue Nov 21, 2019 · 9 comments
Closed

Support for query parameters on nested/deep resource endpoints #627

bart-degreed opened this issue Nov 21, 2019 · 9 comments

Comments

@bart-degreed
Copy link
Contributor

Description

For example, the URL:

https://localhost:5001/api/users/123/orders?fields[orders]=purchase-date

returns all attributes of orders, instead of only purchase-date.

@maurei
Copy link
Member

maurei commented Nov 21, 2019

I think the malfunctioning of paging, inclusion and sparse field (and probably the rest of the query parameters too) for nested resources have a common origin. Thanks for pointing this out

@maurei maurei changed the title Bug: Sparse fieldset in deep URL passes query validation, but is not applied Support for query parameters on nested/deep resource endpoints Nov 22, 2019
@maurei
Copy link
Member

maurei commented Nov 22, 2019

I looked into this and unfortunately this is all pretty complicated

Include

This was easy to support and is addressed in #634

Filter

Doing a filter on nested resource routes essentially the same as performing a EF Core filtered include which is not possible. Eg the following request

GET /articles/1/tags?filter[name]=jsonapi

would be translated to the currently not supported EF Core invocation

dbContext.People.Include(e => e.Tags.Where(t => t.Name.Contains("jsonapi")));

See the temp work around paragraph all the way at the bottom of this comment how this can be achieved in-memory without using custom repositories.

Paging

I suspect this is also not possible, see the following passage (source)

Currently, in EF you cannot filter or limit the Included related entities in any way.

Note that this is about EF, but if it wasn't supported in EF that is a good indicator that it isn't supported in EF Core either. I need to research this further to be sure, but even if it turns out that EF Core works smoothly with it, the same issue as with sparse field selection and sorting will be encountered (next paragraph).

Field selection, sorting

I think it is feasible to build expression trees that perform a sql SELECT and ORDER on the dataset, but this is currently not straight forward to implement due to design choice in JADNC. The problem is that the current expression tree building as performed in the IResourceRepository<Article> assumes that the table for which to perform a SELECT/ORDER is the "base" table of the query. This will not work for nested resource endpoints because it will always involve an INNER JOIN.

We could go ahead and change the expression tree building to be more flexible and support this. But from a separation-of-concerns point of view, I don't think having the IResourceService<Article> call the IResourceRepository<Article> to perform select/sort by fields defined on Tags (i.e. a completely different resource) is a good way forward. To properly implement this we need to seriously detangle this. I need to think about the best way forward here.

Temporary work around: in-memory

Nevertheless: paging, field selection, sort and even (included) filtering can be supported in-memory to ensure consistency in the API response. I'm currently pretty busy with other stuff but this definitely is high up in the backlog. I think currently a developer could already implement this themselves by accessing the query parameter services in a OnReturn and performing these operations himself.

For an example on how to do a filtered include using hooks see the example in the hooks docs. You would have to adjust this to be executed conditionally depending on the state of the IFilterService query parameter service.

For now I have changed the API to be explicit about this not being supported by returning a HTTP 400 Bad Request for unsupported query parameters, see #634.

@bart-degreed
Copy link
Contributor Author

bart-degreed commented Nov 25, 2019

First of all, thanks for putting effort in this. I can image these things are hard to get right.

But I'm surprised (and concerned) to find out these nested scenarios were never supported. This is what I expected the library to excel in: auto-generating smart SQL queries over relationships in various directions with unconstrained depth, based on the model/graph. Not having these features makes me doubt whether adopting this library for us is even an option.

I expected the library to translate a request to:

/articles/1/tags?filter[name]=jsonapi

into:

dbContext.Tags.Include(t => t.Article).Where(t => t.Article.Id.Equals(1) && t.Name.Equals("jsonapi"));

because tags is the primary data source in this request, just filtered by something related. This is semantically the same as:

/tags?filter[article.id]=1&filter[name]=jsonapi

and I expected the library to generate the same SQL statement for both of them.

I am not interested in the proposed in-memory workaround. And would recommend against it, because it creates false expectations. Better not have it, than remove it in vNext because it was never properly implemented and upset existing users.

I get that you are trying to finalize for the v4 release and a change like this involves a lot of effort and causes risk. However if this remains unsupported, it would help others to mention these limitations in the documentation.

@maurei
Copy link
Member

maurei commented Nov 25, 2019

Currently the library identifies Article as the primary data source for that request, which means the outer query is on Article instead of Tag (I do not know what the creators design consideration was to choose Article instead of Tag). That means that the current query would look like

db.Article.Include(a => a.Tags).Where(a => a.Id.Equals(1) &&  a.Tags.Any( t => t.Name.Equals("jsonapi")));

Which is a bit more tedious to build with expression trees, and rapidly becomes unmanageable when filtering on more deeply included resources.

I think if we were to identify Tag as the primary request resource, building the expression tree as in the example you gave will be significantly easier, and we should even be able to support nested filters on endpoints like this

/articles/1/tags?include=blogs&filter[articleType]=1&filter[blog.Type]=2

Unfortunately this is still not a small fix: treating Tag as the main resource instead of Article will require some work on this particular pipeline to treat it as such, but it should be more manageable in terms of separation of concerns and expression tree building. We would be staying away from excessively complex expression tree-building as these guys must have been doing. As such to elaborate on your following comment

This is what I expected the library to excel in: auto-generating smart SQL queries over relationships in various directions with unconstrained depth, based on the model/graph

I believe this is currently beyond the scope of JADNC: it does not aim to extend EF Core but rather to provide elimination of boilerplate code (for among which it uses EF Core). Filtered includes have been put on the roadmap for 5.0.0 so for generic, unconstrained usage I would prefer to wait for their support. But nevertheless it seems we could support it two levels deep using expression tree building.

I will further scope the size of this task in the course of this/next week. But note that the size of the list of feedback is growing faster than I can currently process (i.e. you are more than welcome to make a draft for this 🚀)

@bart-degreed
Copy link
Contributor Author

I've been thinking about this some more and did some experiments. I discovered that my expectations on relationships of unconstrained depth were based on wrong assumptions and me not understanding the full story.

I found that relationship requests only go one level deep at most. So there is no such thing as /users/1/orders/2/lines. And there does not need to be, because it means the same as /orders/2/lines.

I also realized that an endpoint like /users/1/documents is ambiguous in case the document resource contains both an owner and a creator of type user. Would it return all documents for owner 1, for creator 1, or would it return documents where the owner or creator is user 1? Therefore I believe that within our API implementation we should prefer the /documents?filter[owner.id]=1 format.

Then I tried to patch up parts of the library to transform requests to /users/1/orders into /orders?filter[user.id]=1, but eventually gave up on that. There are too many changes needed at all levels and I don't truly understand most of the JADNC internals yet.

My current workaround for the deep paging/filtering limitations is to perform a HTTP redirect in cases like /users/1/orders. I have a working prototype, but need to flesh out more details before it can be turned into a generic solution.

@maurei
Copy link
Member

maurei commented Dec 23, 2019

Such redirect feature sounds like a very interesting feature to add. Feel free to hit me up on our gitter channel if you would like to have a more direct communication line with me to address questions about JADNC internals

@bart-degreed
Copy link
Contributor Author

@maurei
Copy link
Member

maurei commented Feb 12, 2020

https://docs.microsoft.com/en-us/ef/core/what-is-new/ef-core-5.0/plan#filtered-include

Oh jeez, that's interesting. We'll have to do a lot of refactoring and improvements once this lands

@bart-degreed
Copy link
Contributor Author

Replaced by #751,

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

No branches or pull requests

2 participants