Skip to content
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

Adding the initial prototype backported from yoke #2089

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from
Draft

Conversation

pmlopes
Copy link
Member

@pmlopes pmlopes commented Nov 16, 2021

Signed-off-by: Paulo Lopes [email protected]

Motivation:

Addressing the issue #2087

@pmlopes pmlopes marked this pull request as draft November 16, 2021 12:33
@vietj
Copy link
Contributor

vietj commented Nov 17, 2021

this should be merged when we consider doing 4.3

@pmlopes pmlopes requested a review from jponge November 24, 2021 10:58
Signed-off-by: Paulo Lopes <[email protected]>
Signed-off-by: Paulo Lopes <[email protected]>
@jponge
Copy link
Member

jponge commented Nov 25, 2021

Looks like a useful addition

@pmlopes
Copy link
Member Author

pmlopes commented Nov 26, 2021

@jponge I think ATM we have a concise and simple way to handle REST API implementations. I've one question for you.

The current draft assumes that users will always work with JsonObject as type, would it make sense to allow POJOs too?

This means that users need to have jackson-databind in the class-path so all the decoding and encoding happens behind the scenes?

Signed-off-by: Paulo Lopes <[email protected]>
Signed-off-by: Paulo Lopes <[email protected]>
Signed-off-by: Paulo Lopes <[email protected]>
@jponge
Copy link
Member

jponge commented Nov 27, 2021

@jponge I think ATM we have a concise and simple way to handle REST API implementations. I've one question for you.

The current draft assumes that users will always work with JsonObject as type, would it make sense to allow POJOs too?

This means that users need to have jackson-databind in the class-path so all the decoding and encoding happens behind the scenes?

Yes that'd make lots of sense to be able to map POJOs

@Jotschi
Copy link
Contributor

Jotschi commented Nov 28, 2021

I took a look at the code and have a few points I want to mention:

ETags

As far as I can tell does the crud handler implementation not take care of ETag handling. Please keep in mind that the ETag may not just be inferred from the entity id. The response information defines the etag value. A user response may for example also contain references to other resources. Deletion or addition of the referenced resource may also affect the user etag.
Personally I think this is a very complicated but required task since more and more REST APIs are directly used by browsers.

Handler parameters

The handler implementation does not provide the routing context. How can a implementation access the user to check if the required permissions are granted to access the resource?

Paging

Currently the start/end paging parameter pattern must be used. Another very common pattern is page / page size. These parameter can directly be used to infer the start, end values. (pagepageSize=start, pagepageSize+pageSize=end, 0 checks not included). It would be nice to have both options. Imho the page, pageSize variant is more common.

Architecture

For Mesh I always wished to be able to make the CrudHandlers 100% HTTP agnostic. This way I could better test them (no need to mock RoutingContext). The current approach seems to fulfill this. If you change parts I would try to keep it this way.

In Mesh there were only a few areas where this was not easy to do. A crud handler which returned files with nio needed to access the RoutingContext to use sendFile. ETag handlers needed to read and modify headers.

Query Parameters

I did not find the place where the CrudQuery was populated. How can custom query parameters be added to this query?
A client may request a shallow variant of a resource or a CRUD request for image handling may contain resize parameters.

@pmlopes
Copy link
Member Author

pmlopes commented Nov 30, 2021

@Jotschi good points!

ETags

Regarding etags I think it is a good addition. We can capture the response for the verbs that return data, GET in this case, and compute the etag, then other verbs like PUT/PATCH can compute the etag from the local storage and perform cache checks.

Not yet sure how to handle DELETE and POST though.

Parameters

My initial idea was using composition, I was thinking of:

router.route().handler(Oauth2Handler.create(...))
router.route("/persons").handler(CrudHandler.create(...))

Yet, you have a valid point, Oauth2 in this case is too coarse and the next handler CrudHandler may require specific rules per verb. In yoke, I had a pre[Create|Read|Update|Delete]Handler() and post[...]Handler() I was thinking we could live without them, but it seems there was a point after all. :)

Paging

Paging was using the Range header to avoid using the query string, because of the following question.

Query

The query is a JSON representation of the query string, for example:

HTTP GET /persons?name=P&active=true&sort(+foo,-bar)

Will create an object:

crudQuery = {
  query: {
    name: "P*",
    active: "true"
  },
  sort: {
    foo: 1
    bar: -1
  }
}

You may ask why this format? The reason was that yoke was heavily used on projects with https://dojotoolkit.org/reference-guide/1.10/dojo/store/JsonRest.html

The same way, the sort parameter is handled differently, we can also handle pagination properties. The query object is just a simplified representation of the request, so users can extract fields to build mongodb, couchdb or even SQL queries. There was intentionally no specification on what what the query means, or operators for this reason.

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

Successfully merging this pull request may close these issues.

4 participants