hotwired / turbo-rails

Use Turbo in your Ruby on Rails app
https://turbo.hotwired.dev
MIT License
2.07k stars 319 forks source link

`Turbo::Streams::BroadcastStreamJob` raising `ActiveJob::SerializationError` with `ActiveSupport::SafeBuffer` #522

Open northeastprince opened 9 months ago

northeastprince commented 9 months ago

The callbacks defined using broadcasts_refreshes (from 2.0.0-beta.1) fail with

Failed enqueuing Turbo::Streams::BroadcastStreamJob to Async(default): ActiveJob::SerializationError (Unsupported argument type: ActiveSupport::SafeBuffer)
northeastprince commented 9 months ago

Huh, for some reason the error's not showing up on my app anymore.

janklimo commented 9 months ago

The error comes from using Sidekiq with strong arguments. It's still present.

seanpdoyle commented 9 months ago

The underlying issue here might've been resolved by https://github.com/rails/rails/pull/50122. It's only 1 week old at this point (Nov 30, 2023), so depending on your version of Rails, that might not be a meaningful fix for some time.

With that fix in mind, I wonder if it'd be viable for the engine to conditionally package an ActiveJob::Serializers::ObjectSerializer extension to special case ActiveSupport::SafeBuffer instances when Rails.version <= 7.2.

northeastprince commented 9 months ago

That must be why it was fixed for me! We're running on Rails edge. If there's a chance the Rails team will backport the fix, I could ask. If they probably won't, then I'll add the monkey-patch.

northeastprince commented 9 months ago

Ah, I'm getting this error again when using Sidekiq.

northeastprince commented 9 months ago

@seanpdoyle it looks like rails/rails#50122 didn't actually add a serializer for SafeBuffers and in Sidekiq their strict args checking (enabled by default) raises an error of the class itself isn't a string or a few other primitive types. I can open a PR but I can also see the Rails team not wanting a serializer for something that doesn't usually need to be serialized, in which case just calling to_str on the arg for the broadcast job might be the best option.

janklimo commented 9 months ago

It's possible I'm doing this wrong but adding the serializer didn't resolve it. Here's my initializer:

# frozen_string_literal: true

class SafeBufferSerializer < ActiveJob::Serializers::ObjectSerializer
  def serialize(argument)
    super({ 'value' => argument.to_s })
  end

  def deserialize(hash)
    ActiveSupport::SafeBuffer.new(hash['value'])
  end

  private

  def klass
    ActiveSupport::SafeBuffer
  end
end

ActiveJob::Serializers.add_serializers(SafeBufferSerializer)

And this still triggers the error:

Failed enqueuing Turbo::Streams::BroadcastStreamJob to Sidekiq(default): ArgumentError (Job arguments to Turbo::Streams::BroadcastStreamJob must be native JSON types, but "<turbo-stream action=\"refresh\"></turbo-stream>" is a ActiveSupport::SafeBuffer.
See https://github.com/sidekiq/sidekiq/wiki/Best-Practices
To disable this error, add `Sidekiq.strict_args!(false)` to your initializer.

Even if this somehow did work, it would invariably be slower than converting the ActiveSupport::SafeBuffer payload to a string in this gem before enqueueing the job so I'd vote for that instead.

Edit: the above wasn't working on 7.1.2. It does, however, work on Rails edge but to_s returns self, so you need to use argument.to_str or String.new(argument). Having said that, it's still a cleaner approach to let the gem make that conversion.

Petercopter commented 8 months ago

I used the workaround by @northeastprince

Sidekiq.strict_args!(false)

Just so I could play with this new toy. Wow! 🤯 I can't wait till this is production ready!