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

Remove Addressable::URI query hack #184

Merged
merged 1 commit into from
Jan 22, 2019
Merged

Conversation

sdogruyol
Copy link

@sdogruyol sdogruyol commented Jan 21, 2019

This has caused a really hard leaky spec for us while working with Restforce and Addressable.

We explicitly use the cache function of Restforce which relies on FaradayMiddleware::Caching and actually that breaks Addressable::URI query params ordering because of https://github.com/lostisland/faraday_middleware/blob/master/lib/faraday_middleware/addressable_patch.rb

That's why I propose to remove this monkey patch indefinitely 👍

@iMacTia
Copy link
Member

iMacTia commented Jan 21, 2019

@sdogruyol I had a quick look at why Mislav introduced this patch and it looks like the reason is that Addressable::URI can't compare different paths unless the query params are in the correct order.
With this patch, params order won't affect the comparison and I agree with Mislav this should be the correct behaviour as the params order doesn't really matter in HTTP calls.

As you can see, removing the patch breaks our tests because of that, so I can't possibly merge this PR.
Moreover, I'd be interested to know how this is causing issues for you?

@iMacTia
Copy link
Member

iMacTia commented Jan 21, 2019

aaaah it's a bundler issue!
Ignore my comment above then 😅!
It looks like bundler 2.0 was released since the last PR, I'm checking how to fix this but suggestions are welcome

@iMacTia
Copy link
Member

iMacTia commented Jan 21, 2019

@sdogruyol for the time being, I'd suggest changing this line to before_install: gem install bundler -v '<2'

@sdogruyol
Copy link
Author

Updated and specs are ✔️

Copy link
Member

@iMacTia iMacTia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, I was expecting some tests to fail, but it seems like we don't test the scenario with different params order 🤦‍♂️. Anyway, I'm not a fan of patches hanging around for 8 years so happy to remove it for now as it's not causing any harm 👍

@iMacTia iMacTia merged commit 9798b90 into lostisland:master Jan 22, 2019
@sdogruyol
Copy link
Author

Thank you @iMacTia 👍

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.

None yet

2 participants