Make STI .joins() fix more consistent with existing tests #1

lorint · 2018-08-17T23:07:40Z

What do you think about this approach?

BTW it had worked fine with all the .count changes that had been made in the tests, and also works with the original .length and .size calls, so I reverted the examples to how they were originally. (Keeping your two .joins() tests in place of course!)

lorint · 2018-08-18T09:43:04Z

This last commit provides a slightly cleaner approach to allow the correct item_type to persist during create.

seanlinsley · 2018-08-19T00:05:34Z

lib/paper_trail/model_config.rb

+      # back to base_class at the final stages when Association#creation_attributes
+      # gets called.
+      has_manys[klass.versions_association_name.to_s].define_singleton_method(:collection?) do
+        active_record.descends_from_active_record?


How confident can we be that collection? will continue to be used by Rails in this way? Why does Rails have this method in the first place, just to return true?

How often does collection? end up getting called? I wonder if it's worth the effort to memoize the result of descends_from_active_record? so it's not calculated all the time.

BTW, when monkeypatching code I like to add a link to the version-specific lines of code like this: https://github.com/rails/rails/blob/v5.2.1/activerecord/lib/active_record/associations/association.rb#L198-L210

Since creation_attributes only sets the foreign key & type attributes, wouldn't it be more expressive to override it directly?

has_manys[klass.versions_association_name.to_s].singleton_class.prepend(Module.new { def creation_attributes if active_record.descends_from_active_record? super # sets `item_type` and `item_id` else {} # we set these elsewhere. Rails would incorrectly set them to the STI base class. end end })

That raises the question: why don't we always pass in the correct item_type via this monkeypatch? If we're going to patch Rails, it might as well be consistent.

CC @jaredbeck

How confident can we be that collection? will continue to be used by Rails in this way?

It's been used in this way since Rails v4.0, and before that a very similar test was used, albeit one that wouldn't work out with our override:

if reflection.macro.in?([:has_one, :has_many]) && !options[:through]

Since creation_attributes only sets the foreign key & type attributes, wouldn't it be more expressive to override it directly?

At first I had hoped to have a solution that utilised this approach ... but it requires a little more code. The only way I see it working is as a class method added to the HasManyAssociation class, and inside a test to see if we're dealing with a PaperTrail::Version. For fun I've put that together in the latest commit. 10 lines of stuff as opposed to this little 4-liner.

I'm certainly OK with having this either way -- both approaches establish the same net result.

why don't we always pass in the correct item_type via this monkeypatch?

It's very specific just to PaperTrail::Version objects, and only has effect for anything subclassed.

Re: paper-trail-gem#1137 (comment)

Are we okay with globally patching creation_attributes?

Good points on creation_attributes -- have gone back to directly patching collection? on the specific :verisons HasManyReflection objects that are actually subclassed. Much less intrusive.

lorint · 2018-08-19T19:08:54Z

(Your last review above became outdated, and if you expand it then you'll find three answers to the various points you've brought up.)

I'm certainly OK with having the fix for .create() events reverting back to base_class done either with an override to .collection?() or to .creation_attributes() -- both of these establish the same net result. In this latest commit I've done it the .creation_attributes() way.

Hmmm, that reminds me that I haven't put in a comment with a link to the original Rails code! Will do that now.

jaredbeck

This is just an initial review, and I'm jumping into this in the middle of the discussion, so please keep that in mind when reading my comments. Maybe I'm missing something. That said, I'm concerned by the direction this is going, and I haven't even reviewed Sean's main PR yet.

I'd like to support Sean's use-case, but not if it dramatically increases the complexity of PT.

jaredbeck · 2018-08-22T00:18:30Z

lib/paper_trail/model_config.rb

-          item_type = klass.paper_trail.send(:versions_association_item_type)
-          relation = relation.unscope(where: :item_type).where(item_type: item_type)
-          relation
+          order!(model.timestamp_sort_order)


order! is tagged :nodoc: meaning that it is private API. The rails team could remove it at any time without warning. I don't think we should use it.

jaredbeck · 2018-08-22T00:25:03Z

lib/paper_trail/model_config.rb

+          order!(model.timestamp_sort_order)
+          unless klass.descendants.any? && klass.columns.exclude?(klass.inheritance_column)
+            unscope(where: :item_type).where(item_type: klass.name)
+          end


I don't fully understand this unless condition and I could use an explanation comment, pretty please.

jaredbeck · 2018-08-22T00:27:39Z

lib/paper_trail/model_config.rb

+          super
+        end
+      end
+    })


I'm really uncomfortable with patching rails. PT does not currently patch rails at all and I don't want to start.

Completely agree -- I'm just trying to keep everyone happy here. Sean's project uses STI in a way in which he wants changes made to Management objects to end up with an item_type of Company instead, so he suggested a workaround where if there is no defined inheritance_column then it would revert back to pre-1108 behaviour. This patch is only necessary because of this edge case, combined with a hope to have .joins() work on the versions association.

I would LOVE to have total consistency in how PaperTrail operates, and zero reason to have to patch Rails. Perhaps instead of throwing the baby out with the bathwater in 1143 then we can encourage the use of STI as it was originally conceived -- along with a type column so that a single object doesn't temporarily masquerade as another.

jaredbeck · 2018-08-22T00:29:15Z

spec/models/management_spec.rb

+
+# Add a STI-ish class on-the-fly
+class Management < Customer
+end


Please put models in spec/dummy_app/app/models

jaredbeck · 2018-08-22T00:32:31Z

spec/dummy_app/app/models/callback_modifier.rb

@@ -1,6 +1,10 @@
 # frozen_string_literal: true

 class CallbackModifier < ActiveRecord::Base
+  # These guys need to look just a _little_ more like a real STI class in order for the tests to
+  # call modifier.versions().
+  attr_accessor :type


This seems highly unusual. Is this how your app works, Sean? I've never heard of this technique and I'm not surprised that PT was confused by it. My gut reaction is that the amount of complexity I'd be willing to introduce into PT to support this use-case is minimal. I certainly am not interested in patching rails to support this.

This is in relation to the workaround that triggers pre-1108 behaviour.

Perhaps we can have a flag so people can choose base_class or real class. I'm not totally onboard with us always storing both.

jaredbeck · 2018-08-22T00:39:03Z

Every year that goes by, I dislike STI more and more.

jaredbeck · 2018-08-22T00:48:41Z

Maybe we should switch gears and go with adding an item_reification_type to our versions table. Then we wouldn't have to jump through any of these hoops, right?

This partially reverts commit 58369e1. I have kept the specs, skipped. Per the following, this approach does not seem to be working: - #1135 - #1137 - seanlinsley#1

jaredbeck · 2018-08-22T04:35:52Z

Maybe we should switch gears and go with adding an item_reification_type to our versions table. Then we wouldn't have to jump through any of these hoops, right?

I have implemented this in paper-trail-gem#1143. Please let me know what you think there.

lorint · 2018-08-22T07:53:27Z

Every year that goes by, I dislike STI more and more.

Relational data sets never have expressed the concept of inheritance very cleanly. Props to DHH and the crew for giving it a go. Anything that can steer people away from the temptation of architecting a NoSQL thing is beneficial as a project matures. (Have seen so many that have to be rewritten away from Mongo, it's crazy!)

I will say that STI is usually a little less obnoxious than gratuitous use of polymorphism! When it's appropriate then it's good. I mean, with PaperTrail's Version it is obviously a big win. But wow, when people try to abstract problem statements with similar-sounding associations via polymorphic inheritance, it can get out of hand pretty quickly!

…ss gem

This partially reverts commit 58369e1. I have kept the specs, skipped. Per the following, this approach does not seem to be working: - paper-trail-gem#1135 - paper-trail-gem#1137 - seanlinsley#1

lorint added 3 commits August 17, 2018 23:40

Minimal changes necessary to support .joins()

b7a1dcf

A couple cops

f561e64

Stray item_type= in record_create

7489c3f

lorint changed the base branch from master to sti-join-bug August 17, 2018 23:08

lorint added 3 commits August 18, 2018 00:46

A little simpler logic for reverting to use base_class

a8583c3

Destroy can refer simply to the real class name

85b956b

Cleaner way to avoid .create() reverting to the base_class

cefac8b

Be happy, Rubocop

7372b8f

seanlinsley reviewed Aug 19, 2018

View reviewed changes

lorint mentioned this pull request Aug 19, 2018

Fix joins(:versions) paper-trail-gem/paper_trail#1137

Closed

Override creation_attributes instead of collection?

7ce28d8

Link to overridden HasManyAssociation#creation_attributes

b91ae7a

jaredbeck suggested changes Aug 22, 2018

View reviewed changes

jaredbeck mentioned this pull request Aug 22, 2018

New optional column: item_subtype paper-trail-gem/paper_trail#1143

Merged

lorint added 3 commits August 22, 2018 13:27

Updates based on Jared's review

dd5eab9

Removed order\!

cd8a5ca

Verify that all tests work properly along with the store_base_sti_cla…

ad549eb

…ss gem

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make STI .joins() fix more consistent with existing tests #1

Make STI .joins() fix more consistent with existing tests #1

lorint commented Aug 17, 2018 •

edited

Loading

lorint commented Aug 18, 2018

seanlinsley Aug 19, 2018

seanlinsley Aug 19, 2018

seanlinsley Aug 19, 2018

lorint Aug 19, 2018

seanlinsley Aug 21, 2018 •

edited

Loading

lorint Aug 22, 2018

lorint commented Aug 19, 2018

jaredbeck left a comment

jaredbeck Aug 22, 2018

jaredbeck Aug 22, 2018

jaredbeck Aug 22, 2018

lorint Aug 22, 2018

jaredbeck Aug 22, 2018

jaredbeck Aug 22, 2018

lorint Aug 22, 2018

jaredbeck commented Aug 22, 2018

jaredbeck commented Aug 22, 2018

jaredbeck commented Aug 22, 2018

lorint commented Aug 22, 2018

Make STI .joins() fix more consistent with existing tests #1

Are you sure you want to change the base?

Make STI .joins() fix more consistent with existing tests #1

Conversation

lorint commented Aug 17, 2018 • edited Loading

lorint commented Aug 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlinsley Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorint commented Aug 19, 2018

jaredbeck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredbeck commented Aug 22, 2018

jaredbeck commented Aug 22, 2018

jaredbeck commented Aug 22, 2018

lorint commented Aug 22, 2018

lorint commented Aug 17, 2018 •

edited

Loading

seanlinsley Aug 21, 2018 •

edited

Loading