Fix delegates to start respecting polymorphic associations #79
Conversation
kbrock
changed the title
kbrock
changed the title
| to_table = select_from_alias_table(to_ref.klass, src_model_id.relation) | ||
| to_model_id = to_ref.klass.arel_attribute(to_model_col_name, to_table) | ||
| to_column = to_ref.klass.arel_attribute(col, to_table) | ||
| arel = query.except(:select).select(to_column).arel | ||
| .from(to_table) | ||
| .where(to_model_id.eq(src_model_id)) | ||
|
|
||
| # :type is in the reflection definition (meaning it is polymorphic) |
There was a problem hiding this comment.
this is the only change
all other changes are either modifying the tests or test schema (model) for demonstrating this issue
Our code roughly maps to association_scope
AssociationScope is focused on associations (link between a model instance and another model) rather than a relation (link between 2 models - more generic). We need the latter, so we have duplicated their logic but it works with the generic form.
Now that we have added the type part, we have all the part implemented (though our from().where() construction is a little lacking)
in my laundry list is enhancing rails for association scope to work with our use case as well.
kbrock
force-pushed
the
bad_aggregate
branch
from
324552b
to
f2ba3e6
Compare
spec/db/models.rb
Outdated
| @@ -12,6 +12,8 @@ class Author < VirtualTotalTestBase | |||
| has_many :wip_books, -> { wip }, :class_name => "Book" | |||
| has_and_belongs_to_many :co_books, :class_name => "Book" | |||
| has_many :bookmarks, :class_name => "Bookmark", :through => :books | |||
| # who thinks I am targeting them. not who am I targeting | |||
| has_one :primary_target, -> { all.merge(Book.order(:name)) }, :as => :focus, :class_name => "Book" | |||
There was a problem hiding this comment.
I do worry about focused_on looking like a date field, but it does read better.
There was a problem hiding this comment.
the use case for this bug is a definite stretch.
it is not who I am focused on, but rather who I think is focused on my
and it can't be primary book because it is a polymorphic
There was a problem hiding this comment.
thanks. I went back to this and decided to introduce a new model, 'Photo'. It is a classic polymorphic model and I feel makes the code easier to understand.
spec/db/models.rb
Outdated
| @@ -94,6 +99,8 @@ class Book < VirtualTotalTestBase | |||
| belongs_to :author | |||
| has_and_belongs_to_many :co_authors, :class_name => "Author" | |||
| belongs_to :author_or_bookmark, :polymorphic => true, :foreign_key => "author_id", :foreign_type => "author_type" | |||
| # person or book currently focusing on this book. | |||
| belongs_to :focus, :polymorphic => true | |||
There was a problem hiding this comment.
And then this can be currently_focused_by
kbrock
force-pushed
the
bad_aggregate
branch
from
f2ba3e6
to
48d93e7
Compare
|
Checked commits kbrock/activerecord-virtual_attributes@fcdc26f~...48d93e7 with ruby 2.6.3, rubocop 0.69.0, haml-lint 0.28.0, and yamllint |
Before:
virtual delegates does not include the
typecolumn when fetching associated models.So if a model has a polymorphic relationship (e.g. points to either books or authors), it would bring back extra records(e.g. books with author ids or authors with book ids).
It is a little bit of an edge case, and that is why this bug was not found until now. But since all tables start with an id of 1, the overlap can happen all the time.
In this case, you have to delegate from the has one side to the belongs to, which is uncommon for us and was not even possible before nick added the
limit()andorder()fixes.In the bz example, the has_one only brings back one record, and it brought back a record with an id from a different class. So the limit (has one limits to one) brought back the wrong record.
After:
It properly constrains the records to the correct type.
Changes:
referencesI had looked into using
AssociationScope.scopeinstead. It was very close, but is a much bigger change and would require a bit of monkey patching.https://bugzilla.redhat.com/show_bug.cgi?id=1704644
ManageIQ/manageiq#20115
ManageIQ/manageiq#20494
/cc @NickLaMuro I'm changing the other va stuff as you read this