shioyama / mobility

Pluggable Ruby translation framework
MIT License
997 stars 82 forks source link

RFC - model save hooks and mobility #574

Open mrbrdo opened 2 years ago

mrbrdo commented 2 years ago

There seems to be an issue with attr readers in some configuration of model after_create/save hooks. See also original issue https://github.com/mrbrdo/spree_mobility/issues/6

Code triggering the issue (in Spree project if you are familiar):

    after_create :set_root
    after_save :set_root_taxon_name

    private

    def set_root
      self.root ||= Taxon.create!(taxonomy_id: id, name: name)
    end

    def set_root_taxon_name
      return unless saved_change_to_name?
      return if name.to_s == root.name.to_s

      root.update(name: name)
    end

At some point here, Taxon's name (for translation) is set to nil, although a non-nil value was given to base model. It has something to do with the save hooks, it seems. Since I have a validation on the translation model's name presence, the validation fails. Do you have any idea what could be the issue here?

mrbrdo commented 2 years ago

@shioyama I managed to isolate the behavior into a failing mobility spec: https://github.com/mrbrdo/mobility/blob/b87ac2f14e29c40a4d1aa7de15e7acd0a65abc59/spec/integration/active_record_compatibility_spec.rb#L163 (if you move the translates calls above the defintion of the hooks, the spec will pass)

Basically, if translates is not done before declaring model after_* hooks, it is possible to not get the translated attribute inside a hook (on an associated model that is). It seems that ActiveRecord will run the hooks before persisting the associations, leading to this issue, as in this case the associated model will fetch the main model from DB and it will not find the translations. Although it might look like a contrived example, it is exactly what happened with above code from Spree, so it is a concern (although admittedly the code is kinda bad). Do you have a suggestion for a fix?

shioyama commented 2 years ago

Just glancing at this I doubt this is something that can be fixed very easily.

mrbrdo commented 2 years ago

Yeah it seems so :(