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

Fix dropped values from queries by using FlatParamsEncoder #115

Merged
merged 6 commits into from
Feb 17, 2017
Merged

Fix dropped values from queries by using FlatParamsEncoder #115

merged 6 commits into from
Feb 17, 2017

Conversation

ivoanjo
Copy link
Contributor

@ivoanjo ivoanjo commented Feb 17, 2017

The uri_template gem, which hyperclient uses to build urls, implements RFC 6570, that expands a parameter in the {?foo*} format as ?foo=a&foo=b&foo=c.

Unfortunately, faraday's default argument parser (Faraday::Utils.default_params_encoder) is set to the NestedParamsEncoder which only considers parameters to be arrays if they have [] in the key.

The result is that values for a parameter are dropped in this process:

require 'faraday'
require 'uri_template'

Faraday::Utils.default_params_encoder.decode(
  URITemplate.new('http://example.com/{?foo*}').expand(foo: ['a', 'b']).split('?').last
)
=> {"foo"=>"b"}

And thus the resulting faraday request will be missing some of the values.

The solution is to use faraday's other parameter encoder (the FlatParamsEncoder), that correctly encodes such requests.

Fixes #114

Ivo Anjo added 3 commits February 17, 2017 17:51
The `uri_template` gem, which hyperclient uses to build urls,
implements RFC6570, that expands a parameter in the `{?foo*}` format
as `?foo=a&foo=b&foo=c`.

Unfortunately, faraday's default argument parser
(`Faraday::Utils.default_params_encoder`)
[is set to the](https://github.com/lostisland/faraday/blob/master/lib/faraday/utils.rb#L213)
[`NestedParamsEncoder`](https://github.com/lostisland/faraday/blob/master/lib/faraday/parameters.rb#L4)
which only considers parameters to be arrays if they have `[]` in the key.

The result is that values for a parameter are dropped in this process:

```ruby
require 'faraday'
require 'uri_template'

Faraday::Utils.default_params_encoder.decode(
  URITemplate.new('http://example.com/{?foo*}').expand(foo: ['a', 'b']).split('?').last
)
=> {"foo"=>"b"}
```

And thus the resulting faraday request will be missing some of the
values.

The solution is to use faraday's other parameter encoder (the
`FlatParamsEncoder`), that correctly encodes such requests.

Fixes #114
This happens because EntryPoint extends Link, but does not call super
in its initializer so @resource is never defined.
@dblock
Copy link
Collaborator

dblock commented Feb 17, 2017

Thanks. Feel free to nuke or ignore jruby from the build and get it to green.

@dblock
Copy link
Collaborator

dblock commented Feb 17, 2017

The test doesn't seem to be testing the actual problem here, you should write a spec that demonstrates the issue at hand, please.

@ivoanjo
Copy link
Contributor Author

ivoanjo commented Feb 17, 2017

Thanks for the feedback. I tried to do so, but the stub_request also uses faraday to parse methods, so it also ignores multiple parameters. I will try to take a different approach and get back to you.

@ivoanjo
Copy link
Contributor Author

ivoanjo commented Feb 17, 2017

@dblock Updated! I ended up going for a spinach test, since those are using webmock, but still had to configure webmock to behave properly, otherwise its default config also drops values... The ruby ecosystem really doesn't like RFC 6570.

@dblock
Copy link
Collaborator

dblock commented Feb 17, 2017

Alright! Just need a passing build.

@dblock dblock merged commit 4441cf2 into codegram:master Feb 17, 2017
@dblock
Copy link
Collaborator

dblock commented Feb 17, 2017

Merged, thank you.

@ivoanjo
Copy link
Contributor Author

ivoanjo commented Feb 17, 2017

Thanks for the quick feedback!

ivoanjo added a commit that referenced this pull request May 16, 2017
In #115 we started configuring Faraday to use the FlatParamsEncoder to
avoid an issue with dropped parameters when building a request url.

The FlatParamsEncoder was only added on Faraday 0.9.0 and thus we must
require it explicitly, otherwise clients may get confusing messages
after upgrading to hyperclient 0.8.4.

Fixes #117
ivoanjo added a commit that referenced this pull request May 16, 2017
In #115 we started configuring Faraday to use the FlatParamsEncoder to
avoid an issue with dropped parameters when building a request url.

Unfortunately, as FlatParamsEncoder was only added on Faraday 0.9.0
we should require that version as a minimum, otherwise clients may get
confusing error messages after upgrading to the latest hyperclient
version.

Fixes #117
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.

2 participants