Patches Dataset and JSON handler:

[dchandekstark]

• Dataset#uri incorrectly omits params from non-GET requests.
• JSONRequest clobbers URI path (see
  https://discourse.rom-rb.org/t/rom-http-gateway-and-dataset-uri-paths/407)
• JSONRequest redundantly sets URI query (and clobbers Dataset#param_encoder
  by hard-coding URI.www_encode_form)

[dchandekstark]

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]

If I'm not mistaken, this will add all params as query params, even in a POST request, which could be bad if posting a blob of text for example, and is probably not desired in most scenarios. Perhaps it would be better to add a query_params option to Dataset?

[dchandekstark]

@AMHOL Oh, right. :) It so happens that the app I am cutting my rom-rb teeth on uses the request body to POST JSON data. I can amend this as you suggest.

[dchandekstark]

@AMHOL On further thought (what a day!) the use of both query_params and params seems ... confusing. I wonder if it would be too complicated to just POST params in the body if there is no other request data? E.g., in my app I have a request_data string var https://github.com/dchandekstark/solrbee/blob/main/lib/rom/solr/dataset.rb#L12 which gets dumped into the request body. I could see dumping the params into the body if request_data is nil or empty.

[AMHOL]

Yeah it could be confusing, I'm not sure of a better way to go about it myself, I like the idea of just having params sent as the request body though and having a separate query_params option does solve that, perhaps we could have a query_params option as well as a body_params option, probably best to wait for @solnic to weigh in on this though.

[solnic]

I like the idea of query_params and body_params, it would make things much more obvious.

[AMHOL]

#44 @solnic @dchandekstark

[AMHOL]

@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]

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

[AMHOL]

@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]

OK, thanks @AMHOL.

[dchandekstark]

I think this PR is now superseded by #44.

[dchandekstark]

Closing in deference to #44.