Added has_and_belongs_to_many reification #771

samboylett · 2016-04-08T14:29:04Z

HABTM associations are saved as attributes with the name
association_ids as an array of the associated model IDs. Changes
made to HABTM associations are stored on the model so on save the
original associations can be stored in the serialized object. On
reification, if the has_and_belongs_to_many option is not passed,
the serialized attributes are removed such that they are not
reified. If it is passed, the association is reified but if an
associated object no longer exists, it is not added to the
CollectionProxy.

jaredbeck · 2016-04-08T22:32:48Z

lib/paper_trail/reifier.rb

        enums = model.class.respond_to?(:defined_enums) ? model.class.defined_enums : {}
        model.class.unserialize_attributes_for_paper_trail! attrs

+        # Reifies habtm if the option is passed, otherwise sets the habtm
+        # attributes to nil so they are not reified
+        reify_has_and_belongs_to_many(model, attrs, options)


All of the other association types (has_one, has_many, and belongs_to) happen in reify_associations. Why should HABTM happen here, in reify_attributes? So that it can "[set] .. the attributes to nil so they are not reified"? Can you explain that further, please?

As the join model can't be versioned, the opposing habtm models ID is stored instead as an attribute in the serialised object. So has_and_belongs_to_many :bars would be saved as bar_ids = [1, 2, 3, etc.] in the serialised object column of the versions table. This means that upon reification of attributes, reify_attributes would attempt to set model.bar_ids = [1, 2, 3] even if the option to reify habtm is set to false. So these keys must be deleted from the attrs hash (they are deleted, not set to nil, my mistake)

So if the habtm option is not passed, the model retains the habtm associations from the latest version.

jaredbeck · 2016-04-08T22:47:16Z

Thanks for taking a stab at this @samboylett (Sam?). Rails has 6 association types:

    belongs_to
    has_one
    has_many
    has_many :through
    has_one :through
    has_and_belongs_to_many

Reification currently supports four or five of these, right? So adding HABTM support would be great!

@theRealNG (Ng?) recently added belongs_to support, in #730 so let's see if we can get him to look at this.

has_many reification was added by @bli, @lyfeyaj, and @NullVoxPopuli in #439 so maybe they can help too.

mikecmpbll · 2016-04-10T17:30:57Z

in our case we wanted to reify the associations and not the associated objects. to restore an object and restore it's associations, in our case, is a far more common requirement thatn to restore the object, it's associations, and the associated object. before this patch there wasn't any way to achieve this.

it was implemented as attributes because with HABTM there's no join model to version.

jaredbeck · 2016-04-10T21:01:33Z

Won't this load the live (current) records?

in our case we wanted to reify the associations and not the associated objects.

All of the existing assoc. reification methods restore the associated objects. I think it will be misleading if HABTM works differently from all the rest.

The end-user's expectation of reification is that the object will have exactly the same data as the time that the version record was created.

mikecmpbll · 2016-04-10T21:05:15Z

fair enough, maybe it needs renaming or something then. the lack of this feature is really limiting though. we found the same with the current way the HMT association works, we didn't find it possible to restore the has_many record, and not the has_many through target.

samboylett · 2016-04-11T09:31:59Z

I have added in a query to the versions table and some appropriate tests so HABTM should work as other association reifications (also added has_and_belongs_to_many: false to the options.merge for other reifiers)

jaredbeck · 2016-04-15T04:08:30Z

tl;dr Tests look good, but I still have concerns about implementation, specifically persistence.

.. HABTM should [now] work as other association reifications (also added has_and_belongs_to_many: false to the options.merge for other reifiers)

Great! I've read through the tests as of c0f15f2. They demonstrate the behavior I'd expect and seem thorough. Given these tests, I'm pretty sure we'll be able to add this feature, it's just a question of making sure we have the right implementation.

It's important that we find the right implementation here, especially regarding persistence, because it'll be very difficult to change the format once people start recording data in production.

If I understand correctly, this implementation stores a list of ids in the versions.object column. However, that column was only designed to persist attributes, not associations. Using it to store associations would also be inconsistent with other association types, which use the version_associations table.

create_table :version_associations do |t|
  t.integer  :version_id
  t.string   :foreign_key_name, null: false
  t.integer  :foreign_key_id
end

In a Has Many Through (HMT) association, I think we insert one record into version_associations for each record in the "through table", and, in turn, for each record in the "target table". (to use the terminology from Reifier#reify_has_many_through)

Couldn't we still use this table for a HABTM association?

bar1 = BarHabtm.create
bar2 = BarHabtm.create
FooHabtm.create(bar_habtms: [bar1])
FooHabtm.update_attributes(bar_habtms: [bar2]) # line 4

On line 4, when the update event is recorded in the versions table, we could insert a record into the versions_associations table, with a foreign_key_name of bar_habtms and a foreign_key_id equal to bar1.id.

It seems that in this way we could use the version_associations for all association types. This consistency is desirable.

samboylett · 2016-04-15T09:50:53Z

Hi, I have changed the implementation as per your suggestion, also added the option to save only the join table, such that the associated objects are shown in their current state if they are not paper trailed.

jaredbeck · 2016-04-15T16:48:04Z

lib/paper_trail/has_paper_trail.rb

+      end
+
+      def update_for_callback(name, callback, model, assoc)
+        paper_trail_habtm = model.instance_variable_get(:@paper_trail_habtm) || {}


Is there any way we can avoid using instance_variable_get? Could we, for example, add an instance accessor method?

jaredbeck · 2016-04-15T16:49:46Z

Please add a changelog entry.

HABTM associations are saved in the version_assocations table with the foreign_key_name set to the association name. They are saved if either the associated model is being paper trailed, or if the option join_tables: [:association_name] is passed to the has_paper_trail declaration. If the option is passed but the associated model is not paper trailed, only the join model will be saved. This means reification of HABTM would reify the associated objects but in their current state. If the associated model is paper trailed, this option does nothing, and the version of the model at the time is reified.

mikecmpbll · 2016-04-19T12:26:09Z

Commit updated 👍🏼

jaredbeck · 2016-04-19T15:42:31Z

Looks good to me. @batter any concerns?

CC: The people who have worked on associations: @theRealNG, @bli, @lyfeyaj, and @NullVoxPopuli your feedback still very much welcome.

tlynam · 2016-04-23T19:52:57Z

lib/paper_trail/has_paper_trail.rb

+
+      def update_for_callback(name, callback, model, assoc)
+        model.paper_trail_habtm ||= {}
+        model.paper_trail_habtm.reverse_merge!(name => { removed: [], added: [] })


Why not do this? It seems easier to understand

model.paper_trail_habtm[name] ||= {removed: [], added: []}

tlynam · 2016-04-23T20:44:45Z

lib/paper_trail/has_paper_trail.rb

+          assoc_ids =
+            send(a.name).to_a.map(&:id) +
+            (@paper_trail_habtm.try(:[], a.name).try(:[], :removed) || []) -
+            (@paper_trail_habtm.try(:[], a.name).try(:[], :added) || [])


What do you think of this?

history = @paper_trail_habtm || {} history = history[a.name] || Hash.new([]) assoc_ids = send(a.name).map(&:id) + history[:removed] - history[:added]

jaredbeck · 2016-04-26T01:29:27Z

It sounds like we're all generally happy with this, so I'm going to merge it.

@tlynam and @theRealNG Thanks for the review. Todd, good suggestions, feel like making a PR?

@samboylett and @mikecmpbll thanks for the contribution!

I thought we handled this in #771 but I guess not?

samboylett force-pushed the habtm branch 3 times, most recently from cf71db0 to 1de7298 Compare April 8, 2016 15:46

jaredbeck reviewed Apr 8, 2016
View reviewed changes

samboylett force-pushed the habtm branch from 1de7298 to 2a17d66 Compare April 11, 2016 09:28

samboylett force-pushed the habtm branch 3 times, most recently from 677e67a to c0f15f2 Compare April 11, 2016 14:05

samboylett force-pushed the habtm branch 2 times, most recently from c889f90 to c010ad9 Compare April 15, 2016 09:33

jaredbeck reviewed Apr 15, 2016
View reviewed changes

samboylett force-pushed the habtm branch from c010ad9 to b051f7a Compare April 18, 2016 08:23

samboylett force-pushed the habtm branch from b051f7a to 8b528ca Compare April 18, 2016 13:23

jaredbeck added this to the 5.0.0 milestone Apr 19, 2016

jaredbeck mentioned this pull request Apr 20, 2016

Has And Belongs To Many Support #466

Closed

jaredbeck added the Not a bug label Apr 21, 2016

tlynam reviewed Apr 23, 2016
View reviewed changes

jaredbeck merged commit 6fd1d0a into paper-trail-gem:master Apr 26, 2016

jaredbeck added a commit that referenced this pull request Apr 26, 2016

Update schema.rb with new test tables

1526620

I thought we handled this in #771 but I guess not?

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added has_and_belongs_to_many reification #771

Added has_and_belongs_to_many reification #771

samboylett commented Apr 8, 2016

jaredbeck Apr 8, 2016

samboylett Apr 11, 2016

jaredbeck commented Apr 8, 2016

mikecmpbll commented Apr 10, 2016

jaredbeck commented Apr 10, 2016

mikecmpbll commented Apr 10, 2016

samboylett commented Apr 11, 2016

jaredbeck commented Apr 15, 2016

samboylett commented Apr 15, 2016

jaredbeck Apr 15, 2016

jaredbeck commented Apr 15, 2016

mikecmpbll commented Apr 19, 2016

jaredbeck commented Apr 19, 2016

tlynam Apr 23, 2016

tlynam Apr 23, 2016

jaredbeck commented Apr 26, 2016

Added has_and_belongs_to_many reification #771

Added has_and_belongs_to_many reification #771

Conversation

samboylett commented Apr 8, 2016

jaredbeck Apr 8, 2016

Choose a reason for hiding this comment

samboylett Apr 11, 2016

Choose a reason for hiding this comment

jaredbeck commented Apr 8, 2016

mikecmpbll commented Apr 10, 2016

jaredbeck commented Apr 10, 2016

mikecmpbll commented Apr 10, 2016

samboylett commented Apr 11, 2016

jaredbeck commented Apr 15, 2016

samboylett commented Apr 15, 2016

jaredbeck Apr 15, 2016

Choose a reason for hiding this comment

jaredbeck commented Apr 15, 2016

mikecmpbll commented Apr 19, 2016

jaredbeck commented Apr 19, 2016

tlynam Apr 23, 2016

Choose a reason for hiding this comment

tlynam Apr 23, 2016

Choose a reason for hiding this comment

jaredbeck commented Apr 26, 2016