Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow enabling for only certain STI models #33

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

TylerRick
Copy link

@TylerRick TylerRick commented Jan 12, 2018

For example:

  ActiveRecord::Base.store_sti_classes = ['Person']

All other models will use the default behavior of storing the STI base class.

…rd(record) patch, which, unlike the other patches, is only used when store_base_sti_class is *true*:

            if ActiveRecord::Base.store_base_sti_class
              if options[:source_type]
                through_record.send("#{source_reflection.foreign_type}=", options[:source_type])
              end
            end
For example:

  ActiveRecord::Base.store_sti_classes = ['Person']

All other models will use the default behavior of storing the STI base class.
@TylerRick
Copy link
Author

Have only changed it for Rails 5.1 so far. I can update the other patches too if there is interest and if this is likely to get merged.

more intuitive what it does and that it expects an array of models
@TylerRick
Copy link
Author

The reason I wanted this feature, in case it wasn't clear, was because:

  1. In general, we should always try to use the official/standard Rails behavior as much as possible, and only override/tamper with it to the extent needed to solve the problem at hand.

Storing the STI class for all models was causing it to deviate from the standard behavior for a lot of models that had no need to use something other than the default Rails behavior.

One of the benefits for storing the base class is preventing duplication of the STI type in both records.

  1. Enabling it for all models would have meant writing a migration for every model that had previously been storing the base class: for every record with a polymorphic foreign key, I'd have to look up the correct STI subtype, and update its _type column for that key.

  2. There was only a specific STI submodel that where I apparently needed to make it store the STI subtype.

In my case when I was trying to do some joins and eager_loads involving going through that polymorphic association, it seemed to only be aware of the associations defined in the base class, so it was giving an error like:

         ActiveRecord::ConfigurationError:
           Can't join 'SpecialOrg' to association named 'some_assocation_only_on_special_org'; perhaps you misspelled it?

I maybe could have moved those associations to the base class, but they really only made sense in that subclass, so I'd prefer to keep them there.

But in order to use those associations through the polymorphic association, I apparently needed that association to store the actual subclass so that it could correctly look up its associations and avoid this error...

@bboe
Copy link
Contributor

bboe commented Apr 11, 2018

I think this PR is very interesting. I wonder if there is instead an approach where classes that the features provided by this gem can explicitly provide:

store_sti_classes = false

I haven't tested it, but I wonder if that's already feasible.

Thoughts?

@TylerRick
Copy link
Author

TylerRick commented Aug 23, 2018

Great idea, @bboe. If I'm understand you correctly, you would prefer to configure how the STI class is stored directly within each model, instead of some global configuration done in an initializer. I think that would be a much better API than the current one, and more consistent with how ActiveRecord's own API for setting an inheritance_column:

self.inheritance_column = "species"

as well as more in line with how other acts_as type plugins work. I only did it this way because it was an easier change to make (and I hadn't thought of that idea).

If we took that approach, though, I would suggest we use a more descriptive name. store_sti_classes is too ambiguous: we'll always be storing some STI class name in the case of STI models, it's just a matter of which one (that of the base STI model, or the sublcass STI model). Maybe call it store_base_sti_class like the name of the gem (which is misnamed, by the way, since it does the exact opposite)?

Perhaps you would enable these features by setting store_base_sti_class to false (the default being true). For example:

class Person
  self.store_base_sti_class = false
end

(or if you'd rather, by setting store_sti_subclass to true... and rename the gem 😄 )

Sounds feasible to me... though I doubt I will be able to help implement that change.

I've been using this branch in production since January without problem, but now I discovered that store_base_sti_class is causing a problem with paper_trail (#37).

In order to avoid fighting against standard Rails behavior of always storing the base class in the inheritance_column, and risk more things breaking without warning in the future, I'm looking into abandoning this plugin altogether (if I can get everything working)... it's usually more trouble than it's worth to fight against Rails conventions.

@bboe
Copy link
Contributor

bboe commented Jun 30, 2021

@TylerRick I think I'm going to close this PR due to inactivity. Is it worth updating for the current version?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants