DmitryTsepelev / store_model

Work with JSON-backed attributes as ActiveRecord-ish models
MIT License
1.04k stars 85 forks source link

Optional tracking of parent functionality #167

Closed paneq closed 4 months ago

paneq commented 5 months ago

Hey, thank you for this lovely gem.

While integrating it in one codebase we had an exception coming from this line https://github.com/DmitryTsepelev/store_model/blob/085ee5e9d9720a45a1c665dc7de6e4015ddb079b/lib/store_model/ext/parent_assignment.rb#L12

The reason for it was that there was an Active Record class backed by table which had a method column. Basically #121 and #122. We could easily workaround that, but it made me aware of something that I did not expect while reading the README. That this gem extends ActiveRecord and ActiveModel functionality quite deeply by extending getter and setter behavior.

As far as I checked by reading the code and running specs, this is only needed for parent tracking #35. We don't need this functionality and would prefer minimizing the risk of affecting rails behavior, especially in models that don't use StoreModel.

Would you be open to a PR which disables parent tracking functionality based on a configuration option?

I imagine it would work like

StoreModel.config.enable_parent_assignment = false # default would remain to be true

and then

config.to_prepare do |_app|
  ActiveSupport.on_load(:active_record) do
    if StoreModel.config.enable_parent_assignment
      ActiveModel::Attributes.prepend(Attributes)
      prepend(Base)
    end
  end
end

I tested it in my app and the attribute value (set by the app) is available during the extending phase.

Since the behavior is global, to test this functionality I would probably create a second dummy rails app in store_model specs.

DmitryTsepelev commented 5 months ago

Hey @paneq šŸ™‚ I'm all in for adding such a setting. You're totally right, this is only needed for tracking and I agree that not every app needs that.

paneq commented 5 months ago

@DmitryTsepelev Got it. Expect something within 3 weeks :)

paneq commented 5 months ago

@DmitryTsepelev Hey šŸ‘‹šŸ» We have a draft of PoC: https://github.com/DmitryTsepelev/store_model/pull/168/files . Can you please check if the solution and especially the testing approach are going in the right direction before we provide fully working version?

DmitryTsepelev commented 4 months ago

@paneq LGTM, let's proceed šŸ™‚