Getting Katharsis to adhere to the Json api specification for query parameters

[masterspambot]

From @dustinstanley on July 27, 2016 2:52

Hi,

I am working on getting katharsis to adhere more strictly to the official specification, especially with regards to the parsing of query parameters. I have reviewed Enhancement #209 which appeared as though it would allow extension of the framework to enable custom parsing of the query parameters, but this appears to be only a partial solution.

I am able to create my own QueryParamsParser and QueryParamsBuilder, but unfortunately KatharsisInvoker depends on the use of the QueryParams object which is not extensible in the same way, and QueryParams contains additional logic for parsing query parameters that I cannot override easily. For example, the setSorting method in QueryParams assumes that the sorting parameter is of the form "sort[resource name][field name]", presumably to allow sorting of different types of resources. However, this diverges from the spec as the examples show that the resource name is not to be specified as a query parameter. http://jsonapi.org/format/#fetching-sorting

Similarly, there is a problem with inclusion of related resources as the spec does not appear to support including the top level resource name as a query parameter, as it is instead assumed by the path in the URL. However, if the resource name is not present as a query parameter, this causes an issue with IncludedRelationshipExtractor as it appears to need the resource name as a query parameter and it is unable to identify the parent resource for which to load the relationships. I could not find a way to grab this from the URL and feed it into the extractor given the current design.

The katharsis parameters approach does appear to be more flexible than the spec which is good but we are concerned about clients using libraries that may strictly adhere to the spec, which could cause issues when calling katharsis services.

Perhaps this is something I am just missing here, but is it possible to make katharsis adhere to the json api specification taking the changes for #209 into account?

Copied from original issue: katharsis-project/katharsis-core#373

[patrykorwat]

I agree with you @dustinstanley about following the specification and possibly even the specific client library requirements. It would be good to loosen the requirements on the query parameters type, but keep in mind that Katharsis internally uses QueryParam object and in order to provide custom query params you would be required to create an adapter which would provide an interface from a custom query param classes to QueryParam structure.

[dustinstanley]

I can try and work on a PR for this when I get some free time, hopefully within the next few weeks.

[masterspambot]

Referral issue target same problem: #48

[patrykorwat]

From gitter katharsis-framework channel by @remmeier

I think QueryParams is quite generic and powerful, but could provide some support to ease simpler use cases. The nested maps make that a bit hard. Some potential improvements:

• instead of SortingParams, FilterParams, etc. classes a new class ResourceQueryParams could be introduced. QueryParams then contains a map of ResourceQueryParams. And ResourceQueryParams would hold the sorting, filtering, etc. for a given resource type. This can ease the implementation because usually people work with one given resourceType at a time and could just make use of the respective ResourceQueryParams.
• QueryParams.getRoot() would return the ResourceQueryParams object for the currently requested resourceType. In many cases that might be the only one used/supported by a repository.
• Sorting should be a list of (attributePath : List, direction) pairs instead of a map to ensure the proper order for multi-attribute sorting (did come up before)
• Each Filter should be represented by a Filter object with "attributePath : List", "operation : String", "value : Object". Meaning the QueryParamsParser would do a little bit more work to ease the implementation afterwards within the repositories. To make this possible, the parser would need an API to register possible operations and String -> Object converters for values. There should also be a default operation if no operation is specified.
• util ResourceQueryParams.apply(List) to apply sorting, filtering, paging, etc. on the provided list in-memory. Would require a set of recommended operations like "equals", "like", etc by Katharsis. Used for simple use cases where no DB sorting or similar is necessary.
• with the upcoming katharsis-client implementation it will also become important to have a fluent api to build QueryParams.

[remmeier]

BTW: katharsis-jpa does this already to some degree with FilterSpec, OrderSpec within DefaultQueryParamsProcessor. The parsing part (QueryParams => FilterSpec,OrderSpec) could be moved out somewhere. And some more flexibility would be necessary.

[dustinstanley]

I was thinking that the parsing logic currently in QueryParams could be moved to DefaultQueryParamsParser. It appears that the responsibilities for parsing the query parameters are split between QueryParams and DefaultQueryParamsParser (with the majority of the parsing logic in QueryParams), but it seems like that logic would be more appropriate in the QueryParamsParser implementations only. QueryParamsParsers would then return the TypedParams objects.

Doing this would allow some more flexibility in creating a new QueryParamsParser implementation that could adhere to the spec, and it won't change the getters on QueryParams so the code using QueryParams wouldn't be affected.

The above doesn't address any of the refactoring changes mentioned by @remmeier, however I think the above changes shouldn't make those refactorings any more difficult to apply in the future (and maybe they are outside of the scope of this specific issue/enhancement anyways).

I could work on the above change if you think it is a reasonable approach...any thoughts on that?

[remmeier]

sounds good to me,

maybe QueryParamsParser can be further simplified to

QueryParams parse(QueryParserContext);

QueryParserContext

• getParameterValue
• getParameterNames
• get(Root/Requested?)ResourceInformation

The resourceInformation of the requested resource should be available. Something that is currently missing. Something like the context could provide future extensiblity.

Not sure about the refactorings. QueryParams could potentially become an interface. Or an "adapter repository" could sit between the application and the actual repository and transforms the "raw" QueryParams into something else. Or QueryParams gets refactored which would be a major major breaking change and should be avoided. Or QueryParams gets extended and supports two different apis to access it. In general katharsis might follow the json api specification and does not enforce a particular "QueryParams" since it is use case specific and might be handled differently for different application. In this regard the interface approach could be reasonable.

[dustinstanley]

Regarding the QueryParserContext, it sounds interesting.

Just so I understand the suggestion correctly: You're suggesting to replace the existing methods on the QueryParamsParser interface with a single method that returns a QueryParams object, using the new QueryParserContext. I'm a little unclear on how the context would be implemented, it sounds like maybe it would be a wrapper over the request? Something that had access to the query parameters in the request as well as the root resource name, and maybe the context would need a different implementation for each of the different integrations (spring, jax-rs, servlet)? I suppose the context would also need a reference to the ResourceRegistry, in order to look up the appropriate ResourceInformation objects associated to the request.

It also seems like QueryParamsBuilder would no longer be necessary, as the parser would be returning the QueryParams object directly.

Does that sound right?

[remmeier]

sounds perfect :-)

on the client side there will be a need to setup a new QueryParams programmatically. I guess then the QueryParamsBuilder will be used again in a bit of a different setting.

[masterspambot]

I agree with @Remo logic. Sounds reasonable for me.

wt., 23.08.2016 o 10:53 użytkownik Remo notifications@github.com napisał:

sounds perfect :-)

on the client side there will be a need to setup a new QueryParams
programmatically. I guess then the QueryParamsBuilder will be used again in
a bit of a different setting.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#4 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAjeAYlj72O0ENFRCweSb2NaQgBW7aw1ks5qirUmgaJpZM4JYHtA
.

[dustinstanley]

That sounds good then. I started working on this and I'll submit a PR once it's done.

[remmeier]

regarding the refactorings, a commited some things to a new branch here: https://github.com/katharsis-project/katharsis-framework/tree/query/katharsis-core/src/main/java/io/katharsis/repository/base.

It is implemented without breaking API changes using a BaseResourceRepository that converts the low-level QueryParams to a higher-level and maybe more approchable (for default use cases) QuerySpec.

• QuerySpecParser will do the conversion from QueryPArams to QuerySpec.
• InMemoryEvaluator can be used to evaluate the params in memory against a list (for simple use cases).
• Feedback welcomed
• can also be added to an independent katharsis module.
• not yet functional
• looking for a better name for BaseResourceRepository and the "base" package...

[dustinstanley]

I've made a commit regarding the simplification of the QueryParamsParser / adding of a QueryParamsParserContext. If you get a chance, take a quick look and see if we're on the same page: dustinstanley@2b85052

Regarding your changes, is the intention to eventually replace QueryParams with QuerySpec? It seems like that would be ideal, then we wouldn't need to implement a BaseResourceRepository (I agree, the name seems off, but I can't think of a better one off the top of my head). Ideally we could change the ResourceRepository interface to only use QuerySpec, but that seems like a huge change.

QuerySpec really does seem like a "QueryParamsv2.0", and it looks like there is some precedent in the library for adding a "V2" suffix to some class names (e.g. "KatharsisFilterV2") but doing that for something as visible as ResourceRepository seems...wrong :)

[patrykorwat]

I think more meaningful name will be better and annotation based repos could be easily changed to add your query params object. Tommorow I'll try to do sth about this.

[remmeier]

in general I would favor QuerySpec over QueryParams2. At some point in the future the 2 would become meaningless or involve a second migration to remove it and go back to QueryParams.

direct support for the annotation-based repos sounds good. I think we should keep in mind to keep the entire thing pluggable. A imagine some data stores may have other requirements.

you can let me know if you have any input for the datastructures themselves. Otherwise I would cleanup/finalize the implementation and add test cases. Maybe in the meantime we find better names.

[remmeier]

I committed a new version of the QuerySpec to look at. This one is functional and tested. There are a couple of changes, like the introduction of a FilterOperatorRegistry. I removed the "base" naming in favor of "QuerySpec" all the way.

[patrykorwat]

Thanks for all the debate and implementation of the query parameters! It's a good idea to add a new layer to the currently existing solution, but I think there is still lots to do. I added two new issues:
#109 - Add documentation about new query params
#110 - Add support of new query params to annotated repositories

[dustinstanley]

@masterspambot @remmeier I'm not sure that the recent commit was enough to have this issue closed. remmeier's commit added support for QuerySpec but from what I understand, we still need to create the QuerySpec objects from QueryParams (we are still dependent on QueryParams). We still need a way to create QueryParams such that they work with requests that match the official JSON-API specification, which is what this initial issue was about. Unless I'm missing something, it seems like this would still be an issue.

I am still working on making a PR to fix the issue with QueryParams parsing, so that we can create them from requests that adhere to the JSON-API specification. I think once we have that, then we can close this issue.

[patrykorwat]

But on the other hand this PR was massive, and I don't like so big ones. Since it just adds a new functionality and doesn't break current implementation it's better to merge it.

I think it should be better to open a new issue to improve the implementation.

[remmeier]

yes, my commit is not enough, I still would like to see

GET /people?sort[people]=age,name

or just

GET /people?sort=age,name

being supported with proper multi-column ordering. That needs to be fixed in QueryParams first.

[patrykorwat]

It might be a bigger issue since the type passed in query params is used in katharsis, but let's create a new issue!

[remmeier]

maybe SortingParams.params should be deprecated and replaced by a list.

[remmeier]

there already is #12

[dustinstanley]

Ok @remmeier, it sounds like we are on the same page. I agree that in order to truly adhere to the spec, the ordering for sort needs to be enforced. I think SortingParams.getParams could return a LinkedHashMap to guarantee the order, or we could return a list of objects of a higher level abstraction that contain both the sort direction and the column name.

@meshuga, are you suggesting that we open a new issue for adhering JSON-API specification?

I can continue working on the QueryParams parsing, as long as we are all on the same page still.

[patrykorwat]

#12 is more about preserving sorting params, but what you mentioned is about passing the type

[patrykorwat]

@dustinstanley I would be more specific ;)

[dustinstanley]

So you're saying, I should open a new issue around specifically improving the parsing of QueryParams, to enable us to adhere to the JSON-API spec more strictly? Not a problem, just want to make sure we are on the same page :)