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

Add support for proxy servers (e.g. Nginx) URI rewrites #137

Merged
merged 3 commits into from
Jan 27, 2017

Conversation

iMacTia
Copy link
Contributor

@iMacTia iMacTia commented Nov 28, 2016

  • adds #original_uri method to all request drivers
  • use #original_uri instead of #request_uri if any value returned from the former
  • added RubyMine .idea folder to .gitignore

This addresses the issue highlighted in #95 .
I went for the header X-ORIGINAL-URI as this is a standard for proxy servers (in primis Nginx).
I've also added a test to show that the new test is working (plus, I've tested it with our application and is now working fine 👍 ).

If you need to configure Nginx to make this working, you just need to add:

proxy_set_header X-Original-URI $request_uri;

@kjg On a sidenote, while working on the code I realised that the request_drivers folder could be refactored as most of the implementation are just a copy-and-paste of the same methods.
Maybe it would be a good idea to introduce a superclass like BaseDriver and make all drivers inheriting from it? Im sure it would save a good amount of code

use #original_uri instead of #request_uri if any value returned from the former
added RubyMine .idea folder to .gitignore
@iMacTia iMacTia mentioned this pull request Nov 28, 2016
@iMacTia
Copy link
Contributor Author

iMacTia commented Dec 6, 2016

@kjg did you have a chance to take a look?
Unfortunately tests are failing because Faraday 0.10 was release without support for ruby 1.8.7, so not related to my changes 😄

@kjg
Copy link
Collaborator

kjg commented Dec 6, 2016 via email

@iMacTia
Copy link
Contributor Author

iMacTia commented Dec 6, 2016

Hi @kjg no worries I'm a maintainer as well (of Faraday, btw) so I know how it is :)
If you need any help with fixing the tests please do let me know.
Basically the choice is yours between:

  1. Fixing Faraday gem spec to '~> 0.9' (last version supporting Ruby 1.8.7)
  2. Drop support for Ruby 1.8.7

In Faraday, we decided to go for the 2nd choice as always more dependencies were dropping the support as well and to embrace the syntax improvements available in new versions.
Whatever your choice is, let me know when you've updated master and I'll pull changes in my branch as well.

@mgomes
Copy link
Owner

mgomes commented Dec 6, 2016

Thanks @iMacTia. I vote for dropping Ruby 1.8.7 support. What do you think @kjg?

@kjg
Copy link
Collaborator

kjg commented Dec 8, 2016

I think with the next major release it might make sense to drop support for 1.8.7

@iMacTia
Copy link
Contributor Author

iMacTia commented Dec 9, 2016

By major do you mean 3.0?

@iMacTia
Copy link
Contributor Author

iMacTia commented Jan 6, 2017

@mgomes @kjg I noticed you decided to change Faraday version according to the ruby version (which fixes the issue I was having with tests) and released a new version of the gem, but since you didn't tell me my PR remained behind 😞

I've now pulled changes from master into my branch and pushed again.
Tests are green everywhere apart for one specific test, where Travis couldn't install nokogiri successfully (so unrelated to my changes).

I think we're there and at this point it really takes nothing to review and merge my branch in.
I'm sure people lie @elliot-nelson from #95 will be happy to have it in the next release 👍

@kjg
Copy link
Collaborator

kjg commented Jan 25, 2017

can you please either rebase this against master or give me Push access on the PR by clicking the "Allow edits from maintainers" checkbox? I think I've fixed the travis failure.

Thanks for your patients and for the PR!

@iMacTia
Copy link
Contributor Author

iMacTia commented Jan 27, 2017

Thanks @kjg I'll try to rebase now and push the changes 👍

@iMacTia
Copy link
Contributor Author

iMacTia commented Jan 27, 2017

@kjg looks like your fix worked 😃!
Tests are green now

@kjg kjg merged commit 914a666 into mgomes:master Jan 27, 2017
fwininger pushed a commit to fwininger/api_auth that referenced this pull request Mar 7, 2017
use #original_uri instead of #request_uri if any value returned from the former
added RubyMine .idea folder to .gitignore
dunghuynh added a commit to dunghuynh/api_auth that referenced this pull request Oct 18, 2018
* master: (96 commits)
  Bump to v2.2.1
  Bump to version 2.2.0
  Add http.rb request driver (mgomes#164)
  [CI] Test against Ruby 2.5
  Fix mgomes#152 and 125
  Update rubocop configuration
  Update dependencies
  Add Rails 5.1 tests
  Change return type for request #content_md5 #timestamp #content_type
  uri edge case having another uri
  mention bundle_gemfile in readme
  Allow clock skew to be user-defined (mgomes#136)
  Drop support ruby 1.x, rails 2.x, rails 3.x (mgomes#141)
  adds #original_uri method to all request drivers (mgomes#137)
  Disable the Metrics/BlockLength cop in specs
  Update to latest bundler in travis before tests
  Fix faraday on ruby 1.8
  Update to v2.1.0 for feature release
  Add new test for far future requests
  Fix incorrect behaviour introduced with the request_too_old name change
  ...
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.

3 participants