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

Customize query encoding scheme #80

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

rgrinberg
Copy link
Member

my modest attempt at tackling #65

rgrinberg added 2 commits July 9, 2015 13:06
Uri.t can now be created with an optional `query_scheme` parameter that
controls the encoding of the query string. If it's omitted then the
encoding defaults to the uri's own scheme.

An implementation is given for query encoding according to amazon's
standards to be used when writing clients for AWS.
@dsheets
Copy link
Member

dsheets commented Jul 10, 2015

Thanks for taking the initiative on this.

I would like to find a solution that didn't require exceptions or schemes-which-aren't-schemes. In particular, it would be nice to override encoding of particular components without mentioning the rest. I don't particularly like special-casing the query component here. This would create a new type of value, an encoding, of which each scheme has at least a default. It could just be a record with fields for each component. What do you think?

Also, we still support old OCaml versions so we can't use |> in the test cases.

@rgrinberg
Copy link
Member Author

You're right about the exception bit but I'm not quite sure how would records help us with easier overriding of particular components. Isn't it already easy enough to override the way you describe:

let my_scheme = function
  | `Override_this -> ...
  | x -> scheme_were_overriding x

As for the exception I can just remove it and leave it to default to http for everything else.

I think I understand what you're saying. Instead of a ?query_scheme parameter we'd have an ?encoding parameter that can be set by the user. Otherwise it would be selected based on the scheme. The ?scheme parameter will no longer be used for encoding and would only be used for the xxx:// bit.

One thing that I think we can improve is to expose these different encodings via some type rather than a string. Of course this still doesn't let the user create her own encodings but it's fine for now.

Rather than setting a `query_scheme`, a user may choose an `encoding`.
If an `encoding` isn't chosen then it will be picked from `scheme`
instead.
@rgrinberg
Copy link
Member Author

@dsheets OK I've modified the interface to be a bit closer to what I think you want. The next step IMO would be to create an abstract type for an encoding and just expose a few of the default values for it (aws, http, etc.). This would let us be pretty flexible with the implementation under the hood.

@rgrinberg
Copy link
Member Author

@dsheets Friendly bump

I'd be happy to work on this.

@dsheets
Copy link
Member

dsheets commented Jul 13, 2015

Ok. Sorry for the delay. I will take a look at this soon. I have been traveling and am now quite crunched and jetlagged.

@rgrinberg
Copy link
Member Author

No problem, take your time. Sorry for the eagerness.

@dsheets
Copy link
Member

dsheets commented Aug 6, 2015

Could you create a type to encapsulate the percent-encoded set? I believe it is a property of the serialization of the URI only and not inherent to the identifier (but perhaps it affects deserialization somehow?).

@zbroyar
Copy link

zbroyar commented Feb 28, 2016

@dsheets @rgrinberg

Friendly bump.

I also hit the problem with pct encoding: Amazon doesn't allow char '=' in query values and I don't see any workaround except patching uri.ml to make '=' unsafe in query values.

@AngryLawyer
Copy link

@dsheets @rgrinberg
Bump - I'm also hitting walls with servers requiring clients to follow slightly nonstandard encoding schemes

@rgrinberg
Copy link
Member Author

@AngryLawyer If you're using the latest version of cohttp you can set the request path ("resource") as a string and encode it however you want. Is that not enough?

@AngryLawyer
Copy link

I did not know that. I should probably read the API a bit deeper

@hcarty
Copy link
Contributor

hcarty commented Sep 7, 2017

@rgrinberg How would you do that with Cohttp_lwt_unix.Client for example? All of the functions take a Uri.t. I don't see a way to get around Uri's encoding rules.

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

Successfully merging this pull request may close these issues.

5 participants