Deprecation warning about ::UploadIO constant when used without faraday-multipart gem

[jimeh]

Basic Version Info

Faraday Version: 2.8.1
Ruby Version: 3.2.2

Issue description

If your project has the multipart-post gem loaded, and uses faraday-retry for POST requests without the faraday-multipart gem, the Faraday::Retry::Middleware#rewind_files method triggers a deprecation warning:

.../faraday-retry-2.2.0/lib/faraday/retry/middleware.rb:216: warning: constant ::UploadIO is deprecated

We specifically noticed this via the acme-client gem, which performs post requests with retries enabled.

Most people may not notice this however, as it requires you to set Warning[:deprecated] = true for Ruby to emit these deprecation warnings. We've only discovered due to our Ruby 2 -> 3 and Rails 6 -> 7 upgrades, as we've enabled notifications for all deprecation warnings during this process.

Actual behavior

This is because the Middleware class is referring to the Faraday::UploadIO constant from the faraday-multipart gem. If that gem is not part of the dependency tree however, it will instead treat it as ::UploadIO, which is marked as a deprecated constant here in the multipart-post gem.

Expected behavior

No deprecation warning is triggered.

Also, faraday-retry probably should not rely on / expect a constant from another gem (faraday-multipart) which it does not depend upon.

Steps to reproduce

# frozen_string_literal: true

Warning[:deprecated] = true

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'

  gem 'faraday'
  gem 'faraday-retry'
  gem 'multipart-post'
end

count = 0
expected = 5
retry_options = {
  max: expected,
  interval: 0.1,
  methods: [:post],
  retry_statuses: [503],
  retry_block: proc { count += 1 }
}

conn = Faraday.new do |conn|
  conn.request :retry, **retry_options
  conn.adapter :test do |stub|
    stub.post '/fail' do
      [503, { 'Content-Type' => 'text/plain' }, ['Service Unavailable']]
    end
  end
end

conn.post('/fail', payload: { 'name' => 'John Doe' })

exit 0 if count == expected

warn "Retried #{count} times, expected #{expected}"
exit 1

[olleolleolle]

Right, the code in there is actually using the UploadIO constant, to check whether something in there is rewindable.

      def rewind_files(body)
        return unless defined?(UploadIO)
        return unless body.is_a?(Hash)


        body.each do |_, value|
          value.rewind if value.is_a?(UploadIO)
        end
      end

EDIT: I filed lostisland/faraday-multipart#12 which is about getting around this deprecation warning by releasing a new major version of faraday-multipart which does not refer to old aliases.

[iMacTia]

I remember there was a fix in faraday-multipart around the issue with a change in multipart-post classes, the deprecation warning should originate from that.

This gem does not directly depend on multipart-post or faraday-multipart but has an "optional integration" that kicks in if multipart files are detected.

I believe the fix here would be to replace UploadIO with Faraday::UploadIO, since the latter will automatically point to the correct constant based on the multipart-post version in use.

The side-effect of this change though it that the rewind will only work if you also have the faraday-multipart gem loaded, as otherwise Faraday::UploadIO won't be defined.
However I believe that's a fair assumption to make, as this was probably the original intention behind the integration.

@olleolleolle what do you think?

[olleolleolle]

@iMacTia Yes, following that assumption is good.

To make things slow, long and predictable, we may want to do it in a few phases.

• We can release something which emits deprecation warnings with steps to take "depend on this gem, too".
• We can then release something which removes support.
• We can then release Signpost issue: release a 2.0 version faraday-multipart#12 - which removes the old constant aliases.

[iMacTia]

That looks like a sound plan, and will definitely avoid surprises for anyone who is somehow relying on UploadIO payload without the faraday-multipart gem. I don't really think that's a thing, but have no way to confirm this hunch so better safe than sorry!

[jimeh]

I don't think that the faraday-multipart gem actually has anything to do with this. As the rewind method triggers the deprecation warning when said gem is not present.

Since the #rewind method is specifically looking for a UploadIO-based object, maybe a simpler solution is to just decouple from Faraday::UploadIO defined in faraday-multipart, and just handle the different constants directly?

Something like this:

      def upload_io_const
        @upload_io_const ||= if defined?(::Multipart::Post::UploadIO)
                               ::Multipart::Post::UploadIO
                             elsif defined?(::UploadIO)
                               ::UploadIO
                             end
      end

      def rewind_files(body)
        return unless upload_io_const
        return unless body.is_a?(Hash)

        body.each_value do |value|
          value.rewind if value.is_a?(upload_io_const)
        end
      end

Alternatively, the implementation from faraday-multipart could just be duplicated into faraday-retry, though maybe as Faraday::Retry::UploadIO to keep it internal.

[iMacTia]

Yeah so I was trying to avoid replicating the same logic in faraday-retry and instead suggesting we reuse the one we have in faraday-multipart. That would cause the "dependency" (still, a soft one because we check with defined? first) with faraday-multipart, which we think would be OK.

Basically if you want faraday-retry to work automatically with file uploads, you'd need to also use faraday-multipart.
I haven't used the latter in a while, so I'm unsure if you can actually pass an UploadIO object to a Faraday request and get that correctly sent without having faraday-multipart in your stack. If that's not possible anyway, then the soft dependency looks like a reasonable solution

[iMacTia]

OK so I've just tried the following and was able to confirm that using multipart-post with Faraday WITHOUT faraday-multipart is basically useless 😄

require 'faraday'
require 'multipart/post'

file = UploadIO.new(File.new('./.gitignore'), 'text', '.gitignore')
conn = Faraday.new('https://httpbingo.org')
conn.post('/anything/echo', { file: file })

which returns the following response body

{
  "args": {},
  "headers": { ... },
  "method": "POST",
  "url": "https://httpbingo.org/anything/echo",
  "data": "file=%23%3CMultipart%3A%3APost%3A%3AUploadIO%3A0x000000011c694ed0%3E",
  "files": {},
  "form": {
    "file": [
      "#<Multipart::Post::UploadIO:0x000000011c694ed0>"
    ]
  },
  "json": null
}

Based on this, I don't think we need the gradual changes or backwards compatibility that was suggested in this comment, and instead we can just change the code to check for Faraday::UploadIO instead of UploadIO.

@jimeh @olleolleolle am I missing anything?

[jimeh]

@iMacTia Yep, that all makes sense to me :)

No point in trying to be compatible with just the multipart-post gem if it's utterly broken without faraday-multipart... lol

[olleolleolle]

Nice, nice!