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

Reify on associations fails if using Single Table Inheritance #594

Closed
ksornberger opened this issue Aug 26, 2015 · 23 comments
Closed

Reify on associations fails if using Single Table Inheritance #594

ksornberger opened this issue Aug 26, 2015 · 23 comments

Comments

@ksornberger
Copy link

It appears as though if Single Table Inheritance is being used on an association, if you try to reify with has_many: true or has_one: true, it will not find and build the previous version of the association and instead always use the current version of the association when browsing previous versions.

I believe this is happening because if STI is used, the item_type is always stored as the base class. But when calling the method reify_has_many_directly, it is using the main class of the association. This builds a WHERE clause that is looking for the wrong value in the item_type column.

ex:

class Document < ActiveRecord::Base
  has_paper_trail
  has_one :authorship
end

class Assignment < ActiveRecord::Base
  belongs_to :document 
end

class Authorship < Assignment
  has_paper_trail
end

@document.reify(has_many: true, has_one: true)

The last line there will produce a SQL query like this:

SELECT  `versions`.* FROM `versions` 
INNER JOIN `version_associations` ON `version_associations`.`version_id` = `versions`.`id` 
WHERE (version_associations.foreign_key_name = 'document_id') 
AND (version_associations.foreign_key_id = 1) 
AND (versions.item_type = 'Authorship') 
AND (created_at >= '2015-08-26 20:07:28.000000' OR transaction_id = 933)  
ORDER BY versions.id ASC LIMIT 1

But item_type will never be 'Authorship', it is always stored as Assignment. I'm still working on figuring out a solution to this if I can, but this seems to be what is happening from my understanding.

Modifying the database manually and changing the item_type column from Assignment to Authorship results in being able to view the correct versions of the association as I browse through the document history.

@jaredbeck
Copy link
Member

Sounds like you're onto something. Thanks for the bug report.

I'm still working on figuring out a solution to this ..

Any luck? Even a failing test would be welcome, thanks.

@jaredbeck
Copy link
Member

Still working on this, Kevin? Were you able to write a failing test? Thanks.

@ksornberger
Copy link
Author

Hey, sorry.
I'm still planning on looping back to take another look at this, just haven't had the time to get my head into the right space to try to sort this out again. I'll try to make it a priority soon and keep you updated!

@jaredbeck
Copy link
Member

I recently adapted the AR bug report template for use in PaperTrail. Maybe it will help you write your failing test? Please check it out: https://github.com/airblade/paper_trail/blob/master/doc/bug_report_template.rb

I hope it helps!

@jaredbeck
Copy link
Member

Kevin, I think I've reproduced your issue: https://gist.github.com/jaredbeck/4664e45abe3aa09416ae

Does that look like the issue you're seeing?

@daneshd
Copy link

daneshd commented Jan 8, 2016

I worked with Kevin on this, your gist does reproduce the issue. Here's another one too:

https://gist.github.com/daneshd/f0e61744075e1617dbcb

I was working on a fix for this issue but I haven't had the time to finish it. One thing I noticed in your report is you'd like the item_type to be Authorship. I went down that path when patching paper_trail but it didn't work out. I eventually found this

Using polymorphic associations in combination with single table inheritance (STI) is a little tricky. In order for the associations to work as expected, ensure that you store the base model for the STI models in the type column of the polymorphic association.

Saving item_type to the child model instead of the base model caused issues with how it was loaded.

I tried to change the way the lookup was done to grab all the versions whose class is the base model here. I only started on the reify_has_ones.

- where("#{version_table_name}.item_type = ?", assoc.class_name).
+ where("#{version_table_name}.item_type = ?", assoc.klass.base_class.name).

This isn't enough though. You could have more than 1 record saved with the base class if your model uses other has_one associations of the same STI base model (e.g. the sponsorship model in my bug report). Say I have a user that has an authorship and a sponsorship, then there would be 2 version records whose base class is Assignment and whose foreign key is my user's id.

I tried iterating through the found versions to lookup and filter these by their type. I wasn't sure about this approach because I had to force PT to never skip/ignore the type attributes, and it meant serializing the object data to figure out which to use.

Our implementation for versioning things caused some other issues for me so I had to put this fix on hold to figure that out. I never got around to reify_has_manys.

jaredbeck added a commit that referenced this issue May 4, 2016
[Related to #594]

[ci skip]
@jaredbeck
Copy link
Member

This just came up again recently. What do you think about ecd1df7 as a recommendation?

@jaredbeck
Copy link
Member

jaredbeck commented May 4, 2016

What do you think about ecd1df7 as a recommendation?

Nevermind, I forgot about 23ffbdc. Also, I don't think ecd1df7 would help with reifying associations, would it?

@daneshd
Copy link

daneshd commented May 6, 2016

The STI related documentation from ecd1df7 looks good to me. Reifying the polymorphic associations are currently bugging out though. I think it may be better to say that it's a known issue instead. What do you think?

@jaredbeck
Copy link
Member

jaredbeck commented May 6, 2016

I think it may be better to say that it's a known issue instead.

https://github.com/airblade/paper_trail#5a-single-table-inheritance-sti now mentions this issue (#594) as a known issue.

devonestes pushed a commit to devonestes/paper_trail that referenced this issue May 9, 2016
devonestes pushed a commit to devonestes/paper_trail that referenced this issue May 9, 2016
@jaredbeck
Copy link
Member

I was working on a fix for this issue ..

Danesh, have you done any more work on this since January? Is it still an issue with the latest version (currently 5.1.1)? Thanks.

@andre1810
Copy link
Contributor

It´s still an issue with the latest version (8.1.1).

@jaredbeck
Copy link
Member

jaredbeck commented Dec 18, 2017

Adding a type column to the assignments table seems to fix the problem. Docs:

Using polymorphic associations in combination with single table inheritance (STI) ... there must be a type column in the posts table.
http://api.rubyonrails.org/classes/ActiveRecord/Associations/ClassMethods.html#module-ActiveRecord::Associations::ClassMethods-label-Polymorphic+Associations

and

If you don't have a type column defined in your table, single-table inheritance won't be triggered. In that case, it'll work just like normal subclasses with no special magic for differentiating between them or reloading the right type ..
http://api.rubyonrails.org/v5.1.4/classes/ActiveRecord/Inheritance.html

Here is an example with the required type column: https://gist.github.com/jaredbeck/0d4abefcf1a778cba40aa1b8a3645d42

It seems that no changes to PT are necessary. Please try adding the type column and let me know if that works for you.

@andre1810
Copy link
Contributor

andre1810 commented Dec 18, 2017

My tests were done with type column as I used the gist of @daneshd and added a has_many test aswell: https://gist.github.com/andre1810/3cc1cbfaa8344a8ad8a840fbb9f91890
@jaredbeck your test isn´t covering the case with two has_one or has_many associations.

I implemented a fix for this issue by adding a item_sub_type column to the versions table which is queried ad reify as you can see at https://github.com/andre1810/paper_trail/tree/issue_594

@jaredbeck
Copy link
Member

My tests were done with type column as I used the gist of @daneshd and added a has_many test aswell: https://gist.github.com/andre1810/3cc1cbfaa8344a8ad8a840fbb9f91890

Thanks for the clarification, Andre, I'll check out your gist later.

I implemented a fix for this issue by adding a item_sub_type column to the versions table which is queried ad reify as you can see at https://github.com/andre1810/paper_trail/tree/issue_594

Hmmm. I would like to fix this issue without adding a new column, if at all possible. I would almost rather drop support for STI than change our schema.

@andre1810
Copy link
Contributor

@jaredbeck I would like to fix this without adding a new column, too.

item_type is always filled with the base type e.g. assignment in your gist. Maybe we can change the way it is set to avoid adding a new column for the subtype.

@jaredbeck
Copy link
Member

item_type is always filled with the base type e.g. assignment in your gist. Maybe we can change the way it is set to avoid adding a new column for the subtype.

Yeah, I thought about that, but I think it's ActiveRecord who sets the item_type, not us.

@daneshd
Copy link

daneshd commented Dec 18, 2017

Danesh, have you done any more work on this since January?

@jaredbeck I realize this is a very, very late response but no, sorry I never circled back to revisit this.

@daneshd
Copy link

daneshd commented Dec 18, 2017

I don't think you can get around saving item_type as the child type because that caused issues at a lower level as I recall. The lookups were not loading the expected result because it was not respecting an STI expectation/requirement Rails had.

The only solution that came to mind back when I was working on it was to use the base type for the lookup, let all the results load, and then filter through them until it found the correct one which matched the current record's type attribute. I don't think that's an elegant solution though since it requires loading extra records. There were also the issues that I ran into that I mentioned at the end of my first comment.

@andre1810
Copy link
Contributor

@daneshd that is the only solution without adding a item_sub_type column filled if item is not a base class. This column would only be used for version lookup.

@andre1810
Copy link
Contributor

andre1810 commented Dec 20, 2017

@jaredbeck I need this feature working for versioning my models correctly as soon as possible. I found that changing the class name on reification to the base class name fixes my issue. I changed that and opened an PR. This will not fix the reification issues mentioned by @daneshd on having multiple has_one associations on the same model pointing to a polymorphic model but has_many_through associations to polymorphic models.

@jaredbeck
Copy link
Member

Thanks to André's work in #1028 this issue is partially fixed and released as 8.1.2. Also thanks to André, we have a failing (skipped) test in person_spec.rb that should demonstrate the remaining issue.

@jaredbeck
Copy link
Member

jaredbeck commented Mar 11, 2018

Also thanks to André, we have a failing (skipped) test in person_spec.rb that should demonstrate the remaining issue.

Closed by 06a5e30 (will release in 9.0.0) which improves the message of the error raised. Because it's a very rare issue on an experimental feature, I don't intended to work on it further (thus, closing this) but PRs are welcome.

lorint added a commit to lorint/paper_trail that referenced this issue Jun 5, 2018
lorint added a commit to lorint/paper_trail that referenced this issue Jun 6, 2018
lorint added a commit to lorint/paper_trail that referenced this issue Jun 7, 2018
lorint added a commit to lorint/paper_trail that referenced this issue Jun 7, 2018
lorint added a commit to lorint/paper_trail that referenced this issue Jul 23, 2018
lorint added a commit to lorint/paper_trail that referenced this issue Jul 23, 2018
lorint added a commit to lorint/paper_trail that referenced this issue Jul 24, 2018
jaredbeck pushed a commit that referenced this issue Jul 30, 2018
See the changes to the changelog and readme for details.
aried3r pushed a commit to aried3r/paper_trail that referenced this issue Dec 14, 2020
…se STI (paper-trail-gem#1108)

See the changes to the changelog and readme for details.
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

No branches or pull requests

5 participants
@ksornberger @jaredbeck @daneshd @andre1810 and others