westonganger / active_snapshot

Simplified snapshots and restoration for ActiveRecord models and associations with a transparent white-box implementation
MIT License
103 stars 16 forks source link

Add restore_first option to has_snapshot_children to fix foreign keys #33

Closed AmShaegar13 closed 1 year ago

AmShaegar13 commented 1 year ago

Thanks for this gem. Much simpler to use than paper_trail, especially when you want to restore multiple items at once. I've got a suggestion that would be necessary to make it work for us. Maybe you want to include it.

When using foreign keys in the database you cannot always restore the item that created the snapshot first. For example: We have an Order which has_many :order_items where in turn each belongs_to :order. When I destroy the last OrderItem the Order is destroyed with it. When restoring the OrderItem validations are skipped. However, foreign keys will still prevent restore.

This PR adds the new option restore_first. When set to true, the corresponding records will be the first snapshot_items to be saved and thus later restored.

Let me know what you think!

westonganger commented 1 year ago

Interesting approach I like the simplicity, however I think it has some issues/assumptions

This doesnt actually do anything to determine the order of restore, it just reorders the snapshot item creation so that when it comes time to restore they are already in order by nature of the id/pk.

In light of that, rather than even offer a restore_first option I would propose that the user simply put the children in order as necessary. We can add some documentation for this approach.

In order to solve this properly then we would need to re-order the snapshot items at restore time according to the current has_snapshot_children definition ordering.

AmShaegar13 commented 1 year ago

I admit, that this would be a much cleaner approach. I assumed that order of creation would be sufficient, yes. Everything with restore_first will be created before the actual item. Also, all snapshot children should be created in order of their appearance in the configuration. This results in the correct restore order when you use common id for primary key.

However, I can see, why you are thinking about a cleaner way. Sadly, I don't have time to do a different approach at the moment. ☹️ Maybe you want to take this as a starting point. But I don't blame you if you won't. I know that this feature solves a really special problem not everyone will run into.

westonganger commented 1 year ago

I created PR #37 to explore the potential of the alternative approach I suggested. Seems there is a trade-off between the 2 approaches.

Pros of this approach:

Cons of this approach

westonganger commented 1 year ago

Closing as this is just for example. This gem will not support foreign key constraints it adds too much complication.