bullet-train-pro / bullet_train-action_models

Other
4 stars 1 forks source link

`targets-one` raising `Could not find inverse_of` error #21

Closed gazayas closed 11 months ago

gazayas commented 1 year ago

Steps to reproduce

rails g model Listing team:references name:string
bin/super-scaffold crud Listing Team name:text_field --sidebar="ti.ti-world"
rails db:migrate
bin/super-scaffold action-model:targets-one Publish Listing Team
rails db:migrate

Try to publish a listing from the Listing index page, and the error shows up.

A Cable Ready error

It looks like Cable Ready is being flaky when defining inverse_of, which is set as inverse_association in the hash in the return value at the end of the method. Here's the code that defines inverse_association:

def enrich_association_with_updates(name, option, descendants = nil)
  reflection = reflect_on_association(name)

  inverse_of = reflection.inverse_of&.name&.to_s
  through_association = nil

  if reflection.through_reflection?
    inverse_of = reflection.through_reflection.inverse_of&.name&.to_s
    through_association = reflection.through_reflection.name.to_s.singularize
  end

  options = build_options(option)

  [reflection.klass, *descendants&.map(&:to_s)&.map(&:constantize)].each do |klass|
    klass.send(:include, CableReady::Updatable) unless klass.respond_to?(:cable_ready_collections)
    klass.cable_ready_collections.register({
      klass: self,
      foreign_key: reflection.foreign_key,
      name: name,
      inverse_association: inverse_of,
      through_association: through_association,
      options: options
    })
  end
end

and the error can be found here:

raise ArgumentError, "Could not find inverse_of for #{collection[:name]}" unless collection[:inverse_association]

What to do with inverse_of

inverse_of is populated when on the listings index page, but not on other pages. I don't know enough about Cable Ready to say what's going on here though, so I don't want to delve too deep for now. I can at least remove enable_updates: true for the time being to continue writing the system tests, so I'll continue focusing on that end of things.

gazayas commented 1 year ago

Really strange behavior, As I'm working on #19, the tests will pass and fail intermittently after just super scaffolding the new action and running the tests out of the box. Not quite sure what's going on here, would like to figure this one out though.

gazayas commented 1 year ago

One thing to note is the tests work when running just the one test file, but fail when running the entire unit test suite with bundle exec rails test. At this point I can't tell if it's a CableReady issue or a Rails issue.

What we declared by the has_many method in the actual model itself is there via reflection.options, the inverse reflection just hasn't been registered for some reason. So, I'm currently solving the issue by declaring the inverse_of variable below with what we have in the original model file:

def enrich_association_with_updates(name, option, descendants = nil)
  reflection = reflect_on_association(name)

  inverse_of = reflection.inverse_of&.name&.to_s

  # This line!
  inverse_of ||= reflection.options[:inverse_of].to_s

  ...
end

Since this line in the collection registry uses a String and not an ActiveRecord::Reflection object for collection[:inverse_association], we can just use the string provided in reflection.options to make this line in the collection registry work properly.

Worst case scenario, we can just monkey patch CableReady with the code here and keep an eye on their issues in case this comes up in the future.

gazayas commented 1 year ago

@andrewculver I racked my brain for a while on this one today and opened an issue over at CableReady: https://github.com/stimulusreflex/cable_ready/issues/258

Since Action Models is a private repository I made sure not to share any sensitive information, and I wanted to tag you since I know you and other people connected to Bullet Train have done some work on CableReady.

gazayas commented 1 year ago

Digging deep into this one again, and I think I've made some progress. It seems that the problem is with the caching of reflections and how the has_many override in Cable Ready is affecting it.

TLDR

Once all of our associations are loaded, we can access inverse_of associations for a class without a problem (for example after loading the environment with rails c).

However, with the bug we're experiencing here when trying to perform an action, we're trying to use inverse_of when the has_many associations are still being processed via the Cable Ready has_many override, so the cache that holds all the reflections for a class doesn't have all the reflections we need, resulting in nil for the inverse_of association.

Details

The ActiveRecord::Reflection class has a class attribute called _reflections. This stores the different reflections associated with that class.

In Cable Ready, we call inverse_of here which eventually tries to grab the association from _reflections:

def enrich_association_with_updates(name, option, descendants = nil)
  reflection = reflect_on_association(name)

  inverse_of = reflection.inverse_of&.name&.to_s
  ...
end

This calls the following two methods in Rails' ActiveRecord::Reflection class:

def inverse_of
  return unless inverse_name

  @inverse_of ||= klass._reflect_on_association inverse_name
end

def _reflect_on_association(association) # :nodoc:
  _reflections[association.to_s]
end

However, when we get to _reflections[association.to_s], the reflection we're looking for isn't in the cache, resulting in nil.

The thing is, when we run rails c and have everything fully loaded, this problem doesn't occur and we can get the reflection without a problem.

One thing to note here is that the inverse_of function populates @inverse_of if it is nil

This means that when we run the inverse_of method, the cache stores inverse_of reflections when they exist. The thing is, in Cable Ready, we're not running it for every has_many, but only for associations that have enable_updates: true. This means that (if my understanding is correct), we're trying to use inverse_of on select models without having the full context of inverse_of for the other models that exist.

Calling inverse_of on all has_many associations

A fix I found for this was to simply call inverse_of on all the has_many associations so we ensure that @inverse_of is populated inside the _reflections class variable for all of the classes that we're loading. So, I just added one line to the has_many override in CableReady, and the problem went away:

def has_many(name, scope = nil, **options, &extension)
  option = options.delete(:enable_updates)
  broadcast = option.present?
  result = super
  reflect_on_association(name).inverse_of # Added this line.
  enrich_association_with_updates(name, option) if broadcast
  result
end

We call inverse_of inside enrich_association_with_updates, but that method is fired only for associations with enable_updates: true (basically, when broadcast here is true). Since the inverse_of method doesn't overwrite @inverse_of if it's already populated, I think this is a safe way to handle the reflections without using a strange workaround to compensate.

julianrubisch commented 1 year ago

One thing to note is the tests work when running just the one test file, but fail when running the entire unit test suite with bundle exec rails test. At this point I can't tell if it's a CableReady issue or a Rails issue.

This is interesting. Can you tell me if the entire unit test suite is run with parallelization? I might have a hunch of what's the problem...

julianrubisch commented 1 year ago

And another question - do you know if this ever happened in production? I think it might have to do with lazy loading in development/test

julianrubisch commented 1 year ago

alright, could you try it out with this branch? https://github.com/stimulusreflex/cable_ready/tree/fix-inverse-of-resolution

this is more or less a reimplementation of @andrewculver 's patch here: https://github.com/stimulusreflex/cable_ready/compare/master...andrewculver:cable_ready:master?diff=split

but I took it a bit further because I didn't wanna do the work twice 😅

could you give me feedback if this works?

gazayas commented 1 year ago

Hey @julianrubisch, yes that branch works for me! I had to change the version in bullet_train-core/bullet_train since since it's locked to 5.0.0.pre9, but I was able to perform the action without any errors and all the tests are passing.

I had a feeling it had to do with the Rails' initialization process so I was going to look into the after_initialize callback for models, but the lines you wrote here work for me and return the inverse_of object properly, so it looks like I'll have to do my homework concerning Rails' lazy loading:

if collection.reflection&.through_reflection?
  collection.inverse_association ||= collection.reflection.through_reflection.inverse_of&.name&.to_s
  collection.through_association = collection.reflection.through_reflection.name.to_s.singularize
else
  # Specifically this line for the `Listing` and
  # `Listings::PublishAction` models I used as examples in this issue.
  collection.inverse_association ||= collection.reflection.inverse_of&.name&.to_s
end

Production

Also, I haven't tried this on production so I'm not sure if the same thing is happening after deployment.

Parallel testing

I'm not seeing any parallel testing configuration as per the Rails docs in our repository:

# Goes in test_helper.rb
class ActiveSupport::TestCase
  parallelize(workers: 2)
end
gazayas commented 11 months ago

Closing this since it was solved in https://github.com/stimulusreflex/cable_ready/issues/258.

Also, the example I provide at the beginning is the same as what we currently have in the Action Models system tests in the starter repository, and those tests have been passing without an issue.