updates_for: skip updates

[assuntaw]

Hey everyone! Big thanks for updates_for – it's been an absolute joy to work with 🙌

One issue that I've come across is that, because updates_for requires an open Redis connection, CRUDing records will fail where you wouldn’t ordinarily have started a Redis server: seeds and data migrations (using data-migrate) in my case.

Here’s an example:

# app/models/chapter.rb
class Chapter < ActiveRecord
  include CableReady::Updatable

  enable_updates
end 

# db/seeds.rb
puts “Destroying chapters…”
Chapter.destroy_all

puts “Creating chapters…”
Chapter.create!
# …

bin/rails db:seed

Destroying chapters…
rails aborted!
Redis::CannotConnectError: Error connecting to Redis on localhost:6379 (Errno::ECONNREFUSED)

Spinning up a Redis server to run seeds has been ok (though not desirable) for me, but not being able to run (and, more importantly, roll back) data migrations without one seems a little more ✨ wild ✨.

I’m wondering whether it might make sense to add an option a la “skip updates”. Here are two ideas from an initial brainstorm:

1. A global config option that skips the relevant callbacks for predefined paths:

config.skip_updates_from += [Rails.root.join(“/db”)]

I’d personally like this from a DX perspective as I can’t see a need to broadcast changes from anywhere in the /db directory, so a blanket exclusion would make sense.

I’m not 100% sure what a reliable implementation would look like though – intuition tells me that foraging through the callstack in an after_commit conditional could be very error-prone. It’s also a pretty rigid approach (though maybe that’s what you’d want here?).

2. A skip_updates method that yields a given block (credit to @andrewerlanger for this idea, which I also like love):

# db/seeds.rb

cable_ready.skip_updates do
  Chapter.destroy_all
  Chapter.create!
end

This would, of course, be way more flexible. On the other hand, it would require more thinking on the DX side, and thinking is hard.

I’d love to get your thoughts on this! 🤸‍♀️

[leastbad]

I like these ideas, but I admit that I'm similarly stumped in terms of their implementation.

Is this something we could do on an environment basis? When are you running seeds? eg. is this a testing thing?

I do have an idea how we could implement @andrewerlanger's idea that will likely make us all 🤮 a little bit, but here goes: there's only 4 actual instances of ActionCable.server.broadcast in updatable.rb and model_updatable_callbacks.rb... what if we:

1. Create a single method that calls ActionCable.server.broadcast and convert those four instances to use it.
2. Wrap the above broadcast call in a conditional check for a @@global Boolean variable that gets set by
3. A new CableReady::Updateable.without_updates method that would look sort of like:

def without_updates do
  @@what_could_go_wrong = true
  yield
  @@what_could_go_wrong = false
end

It's putting perfume on a pig, but I think it would allow us to brute force this without boiling the ocean.

[assuntaw]

@leastbad that sounds and looks like a solid idea – I'd be happy to give it a try early next week and see what 🤮 -potential I can reach.

One concern that springs to mind is the class variable being (re-)set before its time, like if someone were to try and pass an async task in the block, e.g.

cable_ready.without_updates { RaceConditionJob.perform_later }

What do you think?

P.S:

Is this something we could do on an environment basis? When are you running seeds? eg. is this a testing thing?

I'm afraid this is a bit of an env-agnostic issue. We run seeds in development, data migrations in development, staging and production. We use MockRedis in our test environment anyway so haven't had any issues related to updates_for there (without the mock server we would).

[leastbad]

I think that if they run an async job inside a helper that is literally an escape hatch for the developer of a site to very intentionally run processes that shouldn't flood the UI with updates... then they deserve what's coming to them.

It's all about the intentionality. We could embed <%= `rm -rf /` %> in our view templates, but we have to give developers enough guidance that they don't want to do that to themselves.

As I often say to Julian, it all comes down to good documentation, IMO!

I can't wait to see what you come up with. 😺

[julianrubisch]

Maybe we could take inspiration from how https://api.rubyonrails.org/classes/ActiveRecord/NoTouching/ClassMethods.html is implemented

[assuntaw]

@julianrubisch good idea, thanks!

I've been playing around with this and will hopefully submit a PR tonight or tomorrow morning CET (rm -rf / and all, @leastbad 😸 )

[internets]

I haven't used this feature yet, but it's an inspiring one so I've been digging in.

What if there were a configuration governing how Redis::CannotConnectError errors are handled during enable_updates broadcasts?

config.enable_updates_connection_errors = :log

where the available options are :log, :raise, or :silence

Other options in the future, potentially. Maybe crib some ideas from https://api.rubyonrails.org/classes/ActiveSupport/Deprecation/Behavior.html#method-i-behavior-3D

Another thing to consider is expanding the scope of the error handling beyond connection errors. If that's desirable there might be a better way to design the configuration options.

[leastbad]

@internets I like where you're going with this, Peter!

#168 has been merged (huzzah!) but I encourage you to open a draft PR for the API you'd like to see. 👍