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

Use external multipart and retry middleware #1367

Merged
merged 4 commits into from
Jan 6, 2022

Conversation

iMacTia
Copy link
Member

@iMacTia iMacTia commented Jan 6, 2022

Description

Use external multipart and retry middleware.
It should allow external gems to support both Faraday v1 and v2

Additional notes

I had to bump Ruby min version to 2.6, but considering 2.6 is going EOL at the end of March, I guess that should be fine?

@iMacTia iMacTia self-assigned this Jan 6, 2022
Copy link
Member

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

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

👍 Getting rid of a 1000+ lines from this codebase is also very nice.

Also: Yes to the Ruby version support change.

@olleolleolle olleolleolle merged commit aa17e5b into 1.x Jan 6, 2022
@iMacTia iMacTia deleted the use-external-middleware branch January 6, 2022 15:36
@javierjulio
Copy link

I upgraded to Faraday v1.8 and implemented something new to use Faraday::ParamPart and Faraday::FilePart which worked (I'm new to using Faraday) but with this change our builds failed on a Dependabot update for Faraday v1.9.x which was unexpected.

Reviewing this, I remember now about the work around moving parts into external gems. If I understand right, would it be expected that I just have to now use Faraday::Multipart::ParamPart and Faraday::Multipart::FilePart (source) going forward? I'm already setting f.request :multipart (source) equivalent as expected.

@iMacTia
Copy link
Member Author

iMacTia commented Jan 7, 2022

@javierjulio those are the correct classes to use from now on, but the old one should have worked just the same! That was a regression in the first "standalone" version of faraday-multipart, which have now been fixed in the last release (1.0.2).

Could you try to check which version of the middleware you're using and if upgrading to the latest fixes the issue?

@javierjulio
Copy link

Thank you! In the meantime I'll make that update to use the Faraday::Multipart namespace versions instead.

Yes I can. I've confirmed the faraday-multipart gem was updated to 1.0.2. Below is the diff from the Dependabot PR we received to bump faraday from 1.8.0 to 1.9.3 and where our tests failed with the uninitialized constant Faraday::ParamPart error. That precedes the line using the Faraday::FilePart by the way, so I would assume the same error would occur there.

-    faraday (1.8.0)
+    faraday (1.9.3)
       faraday-em_http (~> 1.0)
       faraday-em_synchrony (~> 1.0)
       faraday-excon (~> 1.1)
-      faraday-httpclient (~> 1.0.1)
+      faraday-httpclient (~> 1.0)
+      faraday-multipart (~> 1.0)
       faraday-net_http (~> 1.0)
-      faraday-net_http_persistent (~> 1.1)
+      faraday-net_http_persistent (~> 1.0)
       faraday-patron (~> 1.0)
       faraday-rack (~> 1.0)
-      multipart-post (>= 1.2, < 3)
+      faraday-retry (~> 1.0)
       ruby2_keywords (>= 0.0.4)
     faraday-em_http (1.0.0)
     faraday-em_synchrony (1.0.0)
     faraday-excon (1.1.0)
     faraday-httpclient (1.0.1)
+    faraday-multipart (1.0.2)
+      multipart-post (>= 1.2, < 3)
     faraday-net_http (1.0.1)
     faraday-net_http_persistent (1.2.0)
     faraday-patron (1.0.0)
     faraday-rack (1.0.0)
+    faraday-retry (1.0.3)
     faraday_middleware (1.2.0)
       faraday (~> 1.0)

@iMacTia
Copy link
Member Author

iMacTia commented Jan 8, 2022

I just reviewed the code and it seems I missed ParamPart in the aliases.
I'll add it back as well and will ship a release 1.0.3 with it 👍
Thanks for raising this!

@iMacTia
Copy link
Member Author

iMacTia commented Jan 8, 2022

@javierjulio That's done 🙌. Once again feedback welcome if the fix works 🙏

@javierjulio
Copy link

@iMacTia you're welcome! I just ran our test against the 1.0.3 release, without any of the changes I mentioned before, and it passed so its fixed. Thank you!

To confirm, since we'd like to be ready for Faraday v2, we are now adding gem 'faraday-multipart' to our Gemfile and then using Faraday::Multipart namespace to access the ParamPart and FilePart classes instead.

@iMacTia
Copy link
Member Author

iMacTia commented Jan 10, 2022

@javierjulio wonderful!

To confirm, since we'd like to be ready for Faraday v2, we are now adding gem 'faraday-multipart' to our Gemfile and then using Faraday::Multipart namespace to access the ParamPart and FilePart classes instead.

That's great 🙌! Just bear in mind that if you want to support both Faraday v1 and v2 you'll need a few conditionals in your code. I'm still trying to find the time to put a guide together, but in short that would be:

  • faraday-multipart should be explicitly required by you, but only with Faraday 2
  • The Faraday::Multipart namespace doesn't exist in Faraday v1, so you'll have to stick to the old one or (even better) conditionally use the new one only with Faraday 2, and fallback to the old one otherwise

timrogers pushed a commit to restforce/restforce that referenced this pull request Jan 14, 2022
This commit limits compatible Faraday versions to versions between
`0.9.0` and `1.8.x`.

The recently released `1.9.x` series isn't compatible with some
magic we have in `lib/restforce/file_part.rb` to maintain
compatability with pre-1.0 versions - see
lostisland/faraday#1367 for the relevant
changes.
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