Skip to content

Support duplicate keys in Query #129

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

Open
paluh opened this issue Dec 19, 2018 · 8 comments
Open

Support duplicate keys in Query #129

paluh opened this issue Dec 19, 2018 · 8 comments

Comments

@paluh
Copy link
Contributor

paluh commented Dec 19, 2018

There are multiple issuess with current Query representation:

  • fullPath - doesn't return original value of uri. We are not able to call it "equivalent" from my perspecitve,

  • We are trying to start using HTTPure not only for API based endpoints and we want to be able to receive data from standard HTML forms. HTML allows you to send multiple values for single query key (like with <select multiple name="key">) and it is quite common to build group of checkboxes with the same key too as multichoice widget.

I have two propositions assuming that we don't want to support empty values (like key1&key2):

  • type Query = Array (Tuple String String) with additional helper which converts this to Map

  • type Query = Map String (Array String)

We are ready to work on this and provide a PR.

@paluh
Copy link
Contributor Author

paluh commented Dec 19, 2018

Additional options:

  • add original uri value to Request

  • leave query as a String

@cprussin
Copy link
Collaborator

cprussin commented Dec 19, 2018

I like the proposal to use a Map String (Array String). I had wanted to convert it to a Map String String anyways, and I think that's a clean way to handle multiple values while enabling things like the Lookup typeclass to be used naturally.

I'd suggest before getting too deep into implementation, take a look at this example and open the PR with the changes to that example that would be necessary for your proposal (including extending it to use multiple values for the same parameter) so we can discuss on sample code.

Regarding fullPath, the original purpose there was mostly for logging. It probably should be equivalent to the original request URL, but not identical. Can you speak to specific ways in which it is not equivalent, and if we fixed those, would that be sufficient or would you still need the original URL?

@paluh
Copy link
Contributor Author

paluh commented Dec 19, 2018

I'd suggest before getting too deep into implementation, take a look at this example (...)

Thanks for suggestions!

Regarding fullPath (..) Can you speak to specific ways in which it is not equivalent

I think that it is not equivalent because Query representation looses information in the current implementation. Even if we have Map it won't be equivalent because possible key order could be different etc. Even if we have Array there still be possiblity about + encoding etc. etc.

would that be sufficient or would you still need the original URL?

I think that we should provide original uri because uri RFC doesn't say anything specific about the shape of for example query value and parsing is somewhat an arbitrary choice from our side (for example purescript-uri in Extras provides different representation).
I think that we should leave users an option to interpret query in different manner (for example parsing it with purescript-uri etc.).

@cprussin
Copy link
Collaborator

Ok, those are good points, I've opened #130 to track exposing the original request URL.

@paluh
Copy link
Contributor Author

paluh commented Dec 20, 2018

I have additional design question.
Maybe we can extend low level API of serve' and serveSecure' so they accept additional configuration which can be used in our final fromHTTPRequest for parsing HTTP.Request parts. Then we can keep our nice default high level API but also allow users to provide their own parsers so they can avoid additional per request parsing from our side which they are not interested in.
What do you think?

@cprussin
Copy link
Collaborator

Yeah, I like that proposal. Let's split out a separate issue to track determining the right API to expose and implementing it

@paluh
Copy link
Contributor Author

paluh commented Dec 21, 2018

As a first step I've added additional tests for query parsing which are failing in the current implementation:

paluh@09311a6

Do you agree that we don't want to parse single equal sign as correct key definition or should we treat it as a "=" key with empty value?
I mean for example (pseudo notation and key=value added for "readability") query ?key=value&= do we want to have:

  • { "key": ["value"], "": [""]}
    or
  • { "key": ["value"] }
    or
  • { "key": ["value"], "=": [""]}
    ?

@cprussin
Copy link
Collaborator

I am leaning towards following what Express does here, which is to ignore the empty =:

const express = require('express');
const app = express();
app.get('/', (req, res) => res.send(req.query));
app.listen(3000, () => console.log('App up'));
$ curl "localhost:3000?foo=bar&="
{"foo":"bar"}

This isn't something I feel particularly strongly about and I can be convinced otherwise, but I tend to think that httpure should follow express conventions for this kind of thing.

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

No branches or pull requests

2 participants