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

Added has_and_belongs_to_many reification #771

Merged
merged 1 commit into from
Apr 26, 2016

Conversation

samboylett
Copy link
Contributor

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.

@samboylett samboylett force-pushed the habtm branch 3 times, most recently from cf71db0 to 1de7298 Compare April 8, 2016 15:46
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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
Copy link

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
Copy link
Member

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
Copy link

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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.

end

def update_for_callback(name, callback, model, assoc)
paper_trail_habtm = model.instance_variable_get(:@paper_trail_habtm) || {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jaredbeck
Copy link
Member

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
Copy link

Commit updated 👍🏼

@jaredbeck
Copy link
Member

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.

@jaredbeck jaredbeck added this to the 5.0.0 milestone Apr 19, 2016

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not do this? It seems easier to understand

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

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) || [])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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!

@jaredbeck jaredbeck merged commit 6fd1d0a into paper-trail-gem:master Apr 26, 2016
jaredbeck added a commit that referenced this pull request Apr 26, 2016
I thought we handled this in #771
but I guess not?
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

Successfully merging this pull request may close these issues.

4 participants