Skip to content

Comments

Register STI descendants for collection updates#204

Merged
leastbad merged 4 commits intomasterfrom
updatable-sti
May 19, 2022
Merged

Register STI descendants for collection updates#204
leastbad merged 4 commits intomasterfrom
updatable-sti

Conversation

@julianrubisch
Copy link
Contributor

@julianrubisch julianrubisch commented May 18, 2022 •
edited
Loading

Problem

I noticed that when working with STI models in a has_many relation, the collection callbacks would only trigger updates for the base class, not its descendants. So in short:

class Section
  has_many :blocks, enable_updates: true
end

class Block
  belongs_to :section
end

class Comment < Block; end
<%= updates_for @section, :blocks do %><%= render @section.blocks %><% end %>

would only ever trigger updates if I added an instance of Block to @section.blocks, not an instance of Comment.

Remedy

Registering collections for every descendant of a reflection.klass resolved this: https://github.com/stimulusreflex/cable_ready/compare/updatable-sti?expand=1#diff-90be2c3ebba84a166909a4f0528f7698ecea6f124f3d96c19a703e02320c38f9R93

However in the development and test environments, Block.descendants would return an empty array because of lazy loading. A hack compromise is to add an option descendants: to our has_many call:

https://github.com/stimulusreflex/cable_ready/compare/updatable-sti?expand=1#diff-90be2c3ebba84a166909a4f0528f7698ecea6f124f3d96c19a703e02320c38f9R93

https://github.com/stimulusreflex/cable_ready/compare/updatable-sti?expand=1#diff-90be2c3ebba84a166909a4f0528f7698ecea6f124f3d96c19a703e02320c38f9R44

This actually doesn't do anything else than putting the class constant into ObjectSpace. I.e. it doesn't matter where you call Comment, it just has to be before the collections are registered. Still, it feels like the most concise and documentable solution 💅

EDIT: now explicitly constantizing descendants from an array of symbols or strings:

class Section
   has_many :blocks, enable_updates: true, descendants: [:Comment]
end

@julianrubisch julianrubisch requested a review from leastbad May 18, 2022 09:44
leastbad
leastbad reviewed May 19, 2022
View reviewed changes
Copy link
Contributor

@leastbad leastbad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I still appreciate this solution, as off-beat as it is and I'm basically good to go with it. I would ask that you append a tiny example of what a has_many descendants: [Foo] would look like, just for documentation posterity.

However, I do have a thought: what if we actually didn't throw out the descendants array and actually used it as a list on line 93? This would allow the developer to specify a list of classes that might exclude some.

The other thing that just popped into my head: is there such a thing as dynamically created STI classes? Not in typical operation, but I'm trying to think if there's a pattern which a popular gem might use that arbitrarily creates model classes.

Given autoloading in production, maybe that's not an issue, either. Just trying to black-hat this fix with the weirdest fuzzing ideas I can muster.

@julianrubisch
Copy link
Contributor Author

julianrubisch commented May 19, 2022

what if we actually didn't throw out the descendants array and actually used it as a list on line 93?

I actually thought the same. Just for consistency, would we want to go with a string/symbol specification instead, like class_name?

@julianrubisch
Copy link
Contributor Author

julianrubisch commented May 19, 2022

trying to think if there's a pattern which a popular gem might use that arbitrarily creates model classes.

Hmm nothing that immediately springs to mind... every STI model has to write its type into the type column so... if we're talking about ActiveRecord, that would be quite backwards?

@leastbad
Copy link
Contributor

leastbad commented May 19, 2022

what if we actually didn't throw out the descendants array and actually used it as a list on line 93?

I actually thought the same. Just for consistency, would we want to go with a string/symbol specification instead, like class_name?

I think that your proposed syntax is already perfect. I was just hinting that it could be fed into the list as a splat, or such.

@leastbad
Copy link
Contributor

leastbad commented May 19, 2022

I feel like descendants: [Comment] is nicer than descendants: [:Comment], no? And I thought the point was to get them to evoke the true name of their class so it can take its rightful place in the Object Space.

If descendants: [Comment] isn't possible, might I suggest descendants: [:comment] instead?

Fussy, perhaps. But consistency is underrated.

@julianrubisch
Copy link
Contributor Author

julianrubisch commented May 19, 2022

If descendants: [Comment] isn't possible, might I suggest descendants: [:comment] instead?

very possible, but I was actually thinking it'd be more consistent with https://api.rubyonrails.org/classes/ActiveRecord/Associations/ClassMethods.html#method-i-has_many

:class_name
Specify the class name of the association. Use it only if that name can't be inferred from the association name. So has_many :products will by default be linked to the Product class, but if the real class name is SpecialProduct, you'll have to specify it with this option.

@leastbad
Copy link
Contributor

leastbad commented May 19, 2022

I thought we still have a need to get the constantized version into the call so that it's in object space when the method runs? or is that now moot because we're not using class.descendants?

I know that symbols are strings, ish, but in our docs (and tests) if we're emulating class_name then we should use "Comment" not :Comment

I have a perception that the standard Rails symbol idiom is to use downcase. Is that actually true?

@julianrubisch
Copy link
Contributor Author

julianrubisch commented May 19, 2022

I thought we still have a need to get the constantized version into the call so that it's in object space when the method runs? or is that now moot because we're not using class.descendants?

ah now I got you, yeah I actually got that mixed up.

I have a perception that the standard Rails symbol idiom is to use downcase. Is that actually true?

In the context of has_many/belongs_to class_name: "Comment" you actually have to specify the class_name, which is camel cased. String or symbol actually doesn't matter as long as we call to_s.

@leastbad leastbad merged commit c8ce6b4 into master May 19, 2022
@leastbad leastbad deleted the updatable-sti branch May 19, 2022 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@leastbad leastbad leastbad left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@julianrubisch @leastbad