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

Ignore nil headers in Env's Util::Headers' KeyMap for 0.9.x #728

Closed
wants to merge 1 commit into from

Conversation

f3ndot
Copy link
Contributor

@f3ndot f3ndot commented Sep 1, 2017

This prevents the net_http adapter from freaking out when encountering a header with a Nil value.

I am unsure where the best filtering logic should live. If you prefer a different class to be responsible for this, I'm happy to make changes to my approach.

Attempts to solve #727

This prevents the net_http adater from freaking out when encountering a header
with a Nil value.
@olleolleolle
Copy link
Member

@iMacTia Hi! This is a patch for the 0.9 release series.

@f3ndot Could you update the title of this PR to reflect the fact that this is about making 0.9.x work?

@f3ndot f3ndot changed the title Ignore nil headers in Env's Util::Headers' KeyMap Ignore nil headers in Env's Util::Headers' KeyMap for 0.9.x Sep 5, 2017
@f3ndot
Copy link
Contributor Author

f3ndot commented Sep 5, 2017

@olleolleolle I'm a little confused by the test suite. It was failing before I started my changes. Is it broken for 0.9.x branch?

@olleolleolle
Copy link
Member

olleolleolle commented Sep 5, 2017

@f3ndot Yes, it seems that the 0.9 needs to hold back /net-http-persistent-3.0.0 to before v 3.

See https://github.com/lostisland/faraday/blob/0.9/Gemfile#L17

@olleolleolle
Copy link
Member

Related: #729

@iMacTia
Copy link
Member

iMacTia commented Sep 7, 2017

Hi @f3ndot,
is there any reason why you don't want to update to the latest faraday version?
I'm asking because, differently from v 0.8 and 0.9, from v0.10 we tried our best to maintain backwards compatibility. My understanding is that your issue is already fixed on later versions, so what is preventing you from updating?

@f3ndot
Copy link
Contributor Author

f3ndot commented Sep 8, 2017

@iMacTia it's an issue with a legacy monolith we're trying to carve up and deprecate. Faraday has a dependency that, in 0.10 and later, resolve to a version who's minimum is greater than approximately 15 other gems utilized in our application (some of which are in house). iirc, it was mime-types's 3.0 major version release. We also used to use an older version of rest-client exhaustively which also relies on a mime-types < 3.0 as well.

I attempted to shave the yak of cascading gem updates before giving up and solving the issue I was experiencing directly in the 0.9.x branch

@iMacTia
Copy link
Member

iMacTia commented Sep 15, 2017

Seems like tests are failing for the same reason as #731.
I'm still trying to get my head around this issue.
Once tests will be green, we can eventually discuss if it makes sense to release a backport this fix into 0.9

@iMacTia
Copy link
Member

iMacTia commented Sep 15, 2017

Moreover, I'd ask you to provide more details about your gem dependencies issues. Faraday's only dependency seems to be 'multipart-post', '>= 1.2', '< 3', so in theory you should be able to fix mime-types to whatever you want unless I'm missing some strange dependency loop.

@iMacTia
Copy link
Member

iMacTia commented Oct 30, 2017

Hi @f3ndot, apologies for the delay but the issue on Travis was tricky to solve and took me some time! Please pull changes from master into your branch and we should see green tests 👍

Can you please update me on your situation? Are you still depending on Faraday 0.9 or were you able to update? Did you have a look at multipart-post dependency as I suggested?

@f3ndot
Copy link
Contributor Author

f3ndot commented Oct 30, 2017

Heya @iMacTia, so I was able to sidestep the issue personally with some tweaking in my application. We're still using Faraday 0.9 for the application.

For context, this was our particular problem:

  1. google_drive v1.0.6 has dep on google-api-client which at 0.8.2 uses faraday ~> 0.9.0
  2. google_drive v1.0.6 has dep on oauth2 which at 1.2.0 uses faraday ~> 0.9.0. Resolvable by updating to v1.4.0 (accomplishable)
  3. There is no 0.8.x version of google-api-client that moves onto Faraday 0.10 or greater so we must update google_drive
  4. The next available version of google_drive is a major bump to v2 which is an entirely different Google API implementation.

Interestingly, I didn't run into the issues with mime-types redoing this list for you. So that must've been user error earlier on my part. I probably tried to select a particular version of google_drive that had hard dependencies on mime-types that conflicted with our rest-client locks elsewhere.

In any event, we're not really in a position of changing over to a newer version of Google Drive API calls in our app just so I can get a newer version of Faraday-- Hence the submission of this PR.

I'm happy to close this out if you don't want the code cluttered.

@iMacTia
Copy link
Member

iMacTia commented Oct 30, 2017

That's OK, I don't mind this little change, but we should make the tests green again.
I'll see if I can change your branch directly.

@f3ndot
Copy link
Contributor Author

f3ndot commented Oct 30, 2017

Closing in favour of #739

@f3ndot f3ndot closed this Oct 30, 2017
@f3ndot f3ndot deleted the handle-nil-headers branch October 30, 2017 18:49
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

3 participants