Skip to content

Feat: Filtering wildcard and multiple value #125

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

spike83
Copy link
Contributor

@spike83 spike83 commented Sep 29, 2016

This change introduces

  • wildcard filtering on strings
    • multiple comma separated values
    • date range filtering year, month, day, hour, minute, second (e.g. datetime within the day => 00:00:00 - 23:59:59)
    • more generic preparation of expressions

@csantero would be great if you could have a look and merge if ok. Thanks.

@csantero
Copy link
Collaborator

@spike83 This is a pretty big change. I'll try to find some time to look at it this weekend.

One thing I will say now is that because filtering syntax is not defined in the JSONAPI spec (beyond saying you must use the filter query param), we should document how to use this in our README. Would you mind adding some documentation with examples for usage?

@spike83
Copy link
Contributor Author

spike83 commented Oct 1, 2016

@csantero I added some documentation on filtering.
I'm not native so please have a look at the language ;-)

@csantero
Copy link
Collaborator

csantero commented Oct 2, 2016

@spike83 I've thought about this some more. Because your implementation (which looks very good, btw) uses its own non-standardized micro-syntax, I don't want to commit all users to it without an opt-in. It would make it harder to adopt other syntaxes in the future, such as some of the ones discussed in this forum thread. I want to leave the decision of what filtering syntax to use up to the user, but I also don't want to waste the work you've done in this PR.

I propose that you move these changes into a new implementation of IQueryableFilteringTransformer (don't know what to call it, maybe AdvancedQueryableFilteringTransformer?). Then to use it I think we'd need to add an option on IJsonApiConfiguration to allow you to specify your preferred queryable filtering transformer. That way the user can opt in to the new logic, and the system falls back to the old logic if they do nothing. How does that sound?

@spike83
Copy link
Contributor Author

spike83 commented Oct 3, 2016

@csantero Thanks for your review.
From an architectural view I absolutely agree. Sounds good.

I think most of the addition should be default behavior like multiple values and automatic transformation to DateTime ranges. I agree that the wildcard filter syntax is not standard and would extract this to a separate location.

I've seen this forum thread and wonder about how to integrate such things. As you could see in #124 there are other needs to evaluate query parameters and manipulate the predicate or the queryable as well.

To make the system as flexible as possible I would propose to make a chain of "consumers" of the url parameters wich can be configured by the customer. So if anyone would implement any specific syntax he can intercept the chain and "consume" the parameters which belong to his syntax. All parameters wich are not consumed will be passed to the next class in chain.

The last Instance in chain is always the DefaultFilteringTransformer, the DefaultSortingTransformer and DefaultPaginationTransformer as it is now. With such system all adaption of transforming the URL query parameters should be possible in a consistent way.

If you agree, I would do this:

  • Common interface of IParameterTransformers or IParameterConsumers to use for the implementation of chain members
  • Add a configuration mechanism to configure chaining
  • Merge this change with [WIP] Feat: Queryable resolver refs #121 #124 into a consistent solution
  • Add the missing functions on [WIP] Feat: Queryable resolver refs #121 #124 into a consistent solution
  • Move the micro syntax (wildcard string filtering) into a specific class wich one can opt in
  • Add doc examples how to use and extend the chaining with the wildcard filtering implementation

What do you think about that?

@csantero
Copy link
Collaborator

csantero commented Oct 3, 2016

@spike83

I know I haven't given you feedback on #124 yet, and I apologize. Preliminarily I'll say that your concept of the resolver seems to differ from what I had in mind, and I'd like to get that sorted out over in that PR separately from this discussion.

The big problem with filtering I've always had in my JSONAPI.NET consuming project, is that I need pluggability on a per-resource type basis. Not all attributes on every resource type are filterable, but the current implementation based around DefaultFilteringTransfromer doesn't know that. As far as it knows, every attribute is a candidate for filtering, and if you pass a parameter, it will try to manipulate the IQueryable to accommodate it. This may not always be possible, because some attributes may not come directly from your query at all, and are calculated from other data AFTER the main query has returned and you know the IDs of the page of resources (I do this frequently for expensive attributes).

I'd like to see filtering changed to be something that is configured on a per-endpoint basis. For example, you might have a Posts resource type with Title, CreatedDate, and NumLikes attributes. Perhaps Title is a property you can filter on a powerful micro-syntax against string, and CreatedDate can be filtered using a different syntax for dates. But NumLikes is a field that is calculated in a second query using something like SELECT PostId, COUNT(*) FROM PostLikes WHERE PostId IN (1, 2, 3, 4, 5) GROUP BY PostId. (yes I know this example could trivially be included as a correlated sub-query in the main query, but imagine something much more complicated.) Because this is a second query that operates off the data page returned from the first query, you cannot possibly filter the data page by this result - it's too late for that. Therefore NumLikes is not eligible for filtering, and if the client tries to do so using filter[num-likes]=43, they should get a 400 error. So Post needs to be configured to reject filtering NumLikes. This is a resource type-level concern, and not something that the application can set globally.

@spike83
Copy link
Contributor Author

spike83 commented Oct 5, 2016

@csantero ok, I agree, that filtering should be configurable globaly (default) and pre resource for handling such things. As you can see in #124 this is configurable per resource and I use it for fulltext search with special url parameter.

What do you think about chaining? How should we go on?

@csantero
Copy link
Collaborator

csantero commented Oct 8, 2016

@spike83 The smallest possible change would to be enable overriding the global filtering transformer at configuration-time, which would allow us to the work you've done in this PR sooner. We can then discuss adding support for per-resource filtering in a separate PR.

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

Successfully merging this pull request may close these issues.

2 participants