Change DescendantLoader to handle non-AR classes in the models directory #16867
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Class is patched by ActiveSupport to add the descendants method, so this change
keeps our patching of ActiveSupport consistent across the board by also patching
Class. This was handled specifically for ActsAsArModel previously, but there
are other classes that also might need this functionality such as Authenticator.
Individually adding new classes is not maintainable and also not expected, so
this is handled generically via a patch to Class.
We still need to keep our ActiveRecord::Base patch because due to timing. By
the time we hit the patching lines, ActiveRecord::Base has already been further
modified with ActiveSupport::DescendantsTracker via prepend, and if we were to
rely on the Class patch, then we wouldn't be able to get ahead of
DescendantsTracker.
Visually, the singleton_class' ancestors chain looks like this for Object:
[
#<Class:Object>,
...
DescendantLoader::ArDescendantsWithLoader,
Class,
...
Object,
...
]
but looks like this for ActiveRecord::Base (if we were to rely on the
Class patch only):
[
#<Class:ActiveRecord::Base>,
...
ActiveSupport::DescendantsTracker,
...
#<Class:Object>,
...
DescendantLoader::ArDescendantsWithLoader,
Class,
...
Object,
...
]
Thus ActiveSupport::DescendantsTracker's definition of descedants wins, unless
we patch ActiveRecord::Base directly.
https://bugzilla.redhat.com/show_bug.cgi?id=1537299
|
Note that this just changes the DescendantLoader...there is a follow up PR incoming to change |
|
Checked commit Fryguy@104416d with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0 |
|
👍 LGTM Thank you @Fryguy |
|
I dislike autoload and the hackery we need to do 😢 . With that said, this looks like a much better solution than handling non-model classes one at a time. |
Fryguy
added a commit
to Fryguy/manageiq
that referenced
this pull request
Jan 23, 2018
After ManageIQ#16867, the DescendantLoader now works properly with non-AR models such as the Authenticator. Much of the code in the Authenticator, such as require_nested and force_load_authenticator_for is just workarounds over the issues from DescendantLoader, so these can be removed. Additionally, the only caller of the authenticator_class was the validator, so this commit moves the validation code into the Authenticator class directly, allow us to make all of those authenticator_class methods private. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1537299
Fryguy
added a commit
to Fryguy/manageiq
that referenced
this pull request
Jan 23, 2018
After ManageIQ#16867, the DescendantLoader now works properly with non-AR models such as the Authenticator. Much of the code in the Authenticator, such as require_nested and force_load_authenticator_for is just workarounds over the issues from DescendantLoader, so these can be removed. Additionally, the only caller of the authenticator_class was the validator, so this commit moves the validation code into the Authenticator class directly, allow us to make all of those authenticator_class methods private. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1537299
Fryguy
added a commit
to Fryguy/manageiq
that referenced
this pull request
Jan 23, 2018
After ManageIQ#16867, the DescendantLoader now works properly with non-AR models such as the Authenticator. Much of the code in the Authenticator, such as require_nested and force_load_authenticator_for is just workarounds over the issues from DescendantLoader, so these can be removed. Additionally, the only caller of the authenticator_class was the validator, so this commit moves the validation code into the Authenticator class directly, allow us to make all of those authenticator_class methods private. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1537299
simaishi
pushed a commit
that referenced
this pull request
Jan 23, 2018
…r_models Change DescendantLoader to handle non-AR classes in the models directory (cherry picked from commit d4ce76f) https://bugzilla.redhat.com/show_bug.cgi?id=1537788
|
Gaprindashvili backport details: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Class is patched by ActiveSupport to add the descendants method, so this change
keeps our patching of ActiveSupport consistent across the board by also patching
Class. This was handled specifically for ActsAsArModel previously, but there
are other classes that also might need this functionality such as Authenticator.
Individually adding new classes is not maintainable and also not expected, so
this is handled generically via a patch to Class.
We still need to keep our ActiveRecord::Base patch because due to timing. By
the time we hit the patching lines, ActiveRecord::Base has already been further
modified with ActiveSupport::DescendantsTracker via prepend, and if we were to
rely on the Class patch, then we wouldn't be able to get ahead of
DescendantsTracker.
Visually, the singleton_class' ancestors chain looks like this for Object:
but looks like this for ActiveRecord::Base (if we were to rely on the
Class patch only):
Thus ActiveSupport::DescendantsTracker's definition of descedants wins, unless
we patch ActiveRecord::Base directly.
https://bugzilla.redhat.com/show_bug.cgi?id=1537299
Paired on this with @jvlcek and @bdunne
@jrafanie Please review.