rom-rb / rom-http

Abstract HTTP adapter for ROM
https://rom-rb.org
MIT License
73 stars 18 forks source link

Patches Dataset and JSON handler: #43

Closed dchandekstark closed 3 years ago

dchandekstark commented 3 years ago
dchandekstark commented 3 years ago

IMO Dataset#absolute_path should either be removed or changed to return uri.path, but I left it untouched here since that discussion is not resolved.

AMHOL commented 3 years ago

@dchandekstark the idea with an override-able base path is to allow for querying from a relation when that resource is nested under another resource, i.e. being able to query blog_posts by authors at /authors/:id/blog_posts, see https://github.com/PetRescue/pr-pin/blob/master/lib/pr/pin/relations/charges.rb#L29-L31 for an example of this, having said that, the default request handler doesn't handle many cases well, it doesn't add request params for non-get requests, and doesn't preserve the gateway path, as you mentioned. It may be best to update it to something similar to https://github.com/PetRescue/pr-pin/blob/master/lib/pr/pin/adapter/handlers/json_request.rb, but even then it will probably require customisation in most cases.

dchandekstark commented 3 years ago

@AMHOL I get overriding base_path, but what Dataset#absolute_path does is nuke the base path below base_path.

AMHOL commented 3 years ago

@dchandekstark it doesn't in https://github.com/PetRescue/pr-pin/blob/master/lib/pr/pin/adapter/handlers/json_request.rb as that's just using the Dataset#uri method to resolve the URL, I think the use of #absolute_path in the default request handler is just an implementation error carried over from the early documentation of a request handler, we should probably update the default request handler to use Dataset#uri too, and handle non-get requests

dchandekstark commented 3 years ago

OK, thanks @AMHOL.

dchandekstark commented 3 years ago

I think this PR is now superseded by #44.

dchandekstark commented 3 years ago

Closing in deference to #44.