Comments (27)
That sounds good then. I started working on this and I'll submit a PR once it's done.
from katharsis-framework.
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.
from katharsis-framework.
I can try and work on a PR for this when I get some free time, hopefully within the next few weeks.
from katharsis-framework.
Referral issue target same problem: #48
from katharsis-framework.
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.
from katharsis-framework.
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.
from katharsis-framework.
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?
from katharsis-framework.
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.
from katharsis-framework.
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?
from katharsis-framework.
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.
from katharsis-framework.
I agree with @Remo logic. Sounds reasonable for me.
wt., 23.08.2016 o 10:53 użytkownik Remo [email protected] 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
.
from katharsis-framework.
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...
from katharsis-framework.
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 :)
from katharsis-framework.
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.
from katharsis-framework.
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.
from katharsis-framework.
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.
from katharsis-framework.
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
from katharsis-framework.
@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.
from katharsis-framework.
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.
from katharsis-framework.
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.
from katharsis-framework.
It might be a bigger issue since the type passed in query params is used in katharsis, but let's create a new issue!
from katharsis-framework.
maybe SortingParams.params should be deprecated and replaced by a list.
from katharsis-framework.
there already is #12
from katharsis-framework.
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.
from katharsis-framework.
#12 is more about preserving sorting params, but what you mentioned is about passing the type
from katharsis-framework.
@dustinstanley I would be more specific ;)
from katharsis-framework.
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 :)
from katharsis-framework.
Related Issues (20)
- when patch relationship with empty list, class BraveRepositoryFilter throw exception. HOT 1
- example for Katharsis 3.x for jersey
- Client/Server content type management incoherent and not compliant with JSON API spec HOT 3
- Homepage links to GitHub show 404 HOT 1
- Implementation of Resource.equals() completely ignores id and type
- spring boot example could not find class “io.katharsis.validation.ValidationModule” and “io.katharsis.brave.BraveModule” HOT 1
- Katharsis client: Ability to add custom exception mapper
- @JsonIgnore not working properly in hierarchies HOT 1
- serialization of float value adds many decimal places HOT 4
- Katharsis client expects content-type exactly equal to "application/vnd.api+json", but server appends "charset=utf-8"
- Unable to add ExceptionMappers with Katharsis-cdi HOT 1
- a problem we have when updating the version of Katharsis from 2.8.2 to 3.0.2
- No support for "https" in self links possible if Katharsis runs behind a reverse proxy
- io.katharsis.utils.parser.TypeParser.addParser
- Getting Started - dropwizard-simple-example can't find resource - broken links HOT 2
- If I wanted to *just* use this for serialization/deserialization, how would I do that? HOT 2
- Add save/delete implementations for many resources HOT 4
- Kathrasis is not extendable
- Katharsis + SpringBoot + Postgresql
- Make clear that this project is dead! HOT 4
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from katharsis-framework.