Added transaction support for restoring associations #44

Closed
wants to merge 21 commits into
from

Conversation

Projects
None yet
10 participants

tderks commented Mar 21, 2011

I extended the gem to add transaction support for restoring associations. It stores a transaction id for every version change done inside of a transaction. (transaction id is version id of first change). It works pretty well for me, but haven't tested it with complex scenerios yet.

You can list other versions in same transaction by
version.transact

When you want to restore model with associations (eg other changes made in the same transaction) do

version.rollback

It's like impossible to put all associations in memory with correct relations. Therefor the rollback function, that undoes the initial transaction in reverse order. The rollback function uses save! to throw an exception on invalid validations. It should however pass validations if you never have failing validations on models in your database. As a last resort you could also skip validations by changing to save(false). But i don't want to remove validation if it doesn't cause any problems

You can also do a virtual transaction by:
PaperTrail.start_transaction
#make changes
PaperTrail.stop_transaction

tderks added some commits Mar 21, 2011

@tderks tderks Added support for transactions.
This should solve all the problems with restoring associations.
870a1f7
@tderks tderks Changed rollback to be a real transaction
Changed rollback save to save!, will raise exception when validation fails
3688c3b
Owner

airblade commented Mar 22, 2011

Thanks for this. I was away last week on holiday and am now pretty busy catching up, so I hope to look at this properly in a day or two.

tderks commented Mar 22, 2011

I played with it some more, but having transaction support doesn't solve all of the issues with associations.
For example this case
@widget.version_at(2.days.ago).rollback
This won't restore the widget object with his associations like they were 2 days ago, but only roll back the changes made that update. Therefor having transaction support is very nice for rolling back deletes dependant destroys and simultanious creates/updates, but can't really go back in time.

I put some more thought into this and i think i have a solution.
An additional table needs to be created:
version_id|foreign_key_name|foreign_id
For all belong_to associations of a model a record is inserted into this database on every create/update/delete
When we want to restore has_many or has_one associations of a model to a specific time, we can lookup the last change before that time for every child id.

Like i said before, it's very hard to build all the associations in memory, especially when you get multiple levels of associations. It's alot easier by using a restore method, which reverts the object with his has_one/has_many relations to a specific time directly.
That way you can save the parent into the database before creating the childs

Shouldn't be too hard implementing this, i will see if i can submit a patch next week.

Owner

airblade commented Mar 28, 2011

I agree with your logic, and also that the problem is harder than it looks ;)

I'll be interested to see what you come up with...

tderks commented Mar 31, 2011

I finally got around to implementing my idea.
I even got it to work using reify, so you can reify your object with all childs and save after!

When restoring the assocations, the transaction code i already added came in handy.
To get the current childs it looks for the first version after version_at OR with same transaction id.

I didn't add any tests yet and only tested it with 1-level of associations.
Maybe it also makes more sense to turn association restore off by default.
Let me know what you think.

tderks closed this Mar 31, 2011

tderks reopened this Mar 31, 2011

Owner

airblade commented Mar 31, 2011

This looks really promising. I think introducing a new model (VersionAssociation) was the breakthrough we needed.

I agree it makes sense to turn off association restoration by default at this stage.

This'll need some solid tests...

@egoh what's the status of your implementation?

tderks added some commits Apr 8, 2011

@tderks tderks Fixed reify_has_manys query
Added delete support (add :autosave=>true to association in model)
32b08e8
@tderks tderks Turned off has_one and has_many association restore by default 93029c7
@tderks tderks Adjusted reify_has_one query
Only mark for destruction if child still exists
a16dfd5

tderks commented Apr 8, 2011

The querys i used for fetching the first version for every child was wrong. Group by returns arbitrary results when not using group_by aggregate functions on columns. Therefor i switched to a subquery with fetches the minimum version id's for each child.

To delete childs from the model that did not exist at the time of the version, add :autosave => true to the association in the model

I also turned off association restore by default use has_one and has_many options to turn this on
@version.reify(:has_one=>true,:has_many=>true)

I wanted to write some tests today, but i can't get the tests inside the gem to run.
Do i need to turn on a flag somewhere to run the tests of used gems?

@saimonmoore: i think i just fixed the major bugs. I tested most common cases by hand, but there are so many combinations that it's easy to overlook something. You can try it out and see if it gives expected results, but wouldn't use it in production yet before testing is added.

Owner

airblade commented Apr 8, 2011

To test: bundle exec rake test

Might be worth clearing out the sqlite3 databases in /test/dummy/db first.

tderks commented Apr 8, 2011

Already saw that comment in the readme, but it doesn't run anything.

I have the gem installed like this:
gem 'paper_trail', :path => "/path"

Do i need to add anything to the general rake file?

Owner

airblade commented Apr 8, 2011

It sounds like you're trying to test paper_trail within your app. I test PaperTrail independently of any app by changing into its root directory and running that command.

The dummy directory inside test is a full blown Rails 3 app which PaperTrail uses when running its tests.

@egoh what's the status of your proposed patch? Is it stable enough to test with a small application?

tderks commented Apr 26, 2011

Yes it's stable enough to test in a small application.
I didn't create a seperate migration file to add the transaction column, so you need to reset the version table or add a transaction id column yourself.

Still haven't got around to writing tests, the weather here is way too good lately..

tderks closed this Apr 26, 2011

tderks reopened this Apr 26, 2011

Owner

airblade commented Apr 27, 2011

Haha, the weather's been great here too. You have to make the most of it!

agustinv commented May 5, 2011

EgoH,

Kudos and thanks for taken this on. Its a much needed addition.

A question I have after reading a little over your implementation is how is it going to work with multiple versions tables.

Also it seems that you might want to call after_rollback to reset the transaction id, in case a save or destroy fails.

tderks commented May 6, 2011

@augustinV
I have added the after_rollback transaction id reset, thanx!

The reason the implementation isn't working with multiple version tables is that the feature didn't exist at the time i made the fork! I already wrapped up some code to make the class_name of the version_association configurable, just like it's implemented for the version table. Once i get to test my changes i'll commit the changes.

I'm working on an app that uses a homemade temporal-persistence lib.
We can/need to ask questions like :

c = User.find(17).at(7.days.ago).contracts
salaries_1_week_ago = c.collect(&:submitter).collect(&salary)

This requires retrieving historical data through many levels of relations.

Question :
Could [paper_trail + this new transaction support] go in this direction? It looks like I'd just need to add validity dates, to find the version of the top object - the user - active at the specified time.

Am I missing something?

Owner

airblade commented May 26, 2011

You can already get the version of an object at a particular time with version_at. For example:

User.find(17).version_at(7.days.ago)

The tricky part is restoring the associations as they were at that time. If this issue/pull request's code works out, you'll be able to do that too.

It sounds like you've been thinking about how to restore historic associations. Do you have any working code for this?

It sounds like you've been thinking about how to restore historic associations. Do you have any working code for this?

I'm afraid not: the design of the lib we use is way too specific. It was written some time ago - not by me - and is not active_record compatible : everything (and I mean EVERYTHING: 70+ models) is stored in 3 tables : "models", "relations", and "properties". Each record in "properties" represent 1 attribute in 1 object => a User has 50 attributes -> 50 records in "properties" to represent this user attributes + 1 record in 'models', to represent this User record.

It does the job (the "temporal DB" aspect), but the performance is "not that great", as you could expect from this brief design description.

That's why I'm looking for more standard/off-the-shelf solutions.

Owner

airblade commented May 26, 2011

I saw a database like that once. The design appeals to theoreticians because it's so general, but to those of us trying to use the wretched thing it's so general it's very hard to work with.

EgoH,

I just checked out your fork and tried to run your CreateVersionAssociations and got

Index name 'index_version_associations_on_foreign_key_name_and_foreign_key_id' on table 'version_associations' is too long; the limit is 64 characters

This was running against MySQL 5.1. Obviously, this is easily fixed but I thought I'd mention it.

EgoH,

Further to the db index comment, I've just plugged your version of Paper Trail into my application. It is just awesome. Great job!

airblade, can this pull request be merged to upstream? I'm looking to use papertrail in my app, and this functionality is a requirement for my case (the ability to restore associations).

Owner

airblade commented Aug 1, 2011

I want to look through the code properly before I merge it. I know it's been outstanding for a long time but I can't look at it right now -- I've just moved house and am swamped with unpacking, admin, etc.

Since you need it for your app, I suggest you use the fork until I get myself organised again.

big +1 for this, and happy to write tests/test it out in my app if would be useful.

I tried using this in my app, but couldn't quite get it working. Ultimately what I want is to be able to do this: https://gist.github.com/1175753

Is this what this issue is heading towards? Could you post some example code?

@cambridgemike cambridgemike commented on the diff Sep 2, 2011

lib/paper_trail/version.rb
@@ -137,22 +158,52 @@ class Version < ActiveRecord::Base
# The `lookback` sets how many seconds before the model's change we go.
def reify_has_ones(model, lookback)
@cambridgemike

cambridgemike Sep 2, 2011

I think you want to change lookback to "options"

@cambridgemike

cambridgemike Sep 2, 2011

I was running the tests against this pull request and got the error:

NameError: undefined local variable or method `options' for #Version:0x10f99c978, at line 165

So it looks like this method is expecting an options hash to be passed in, and not a loopback. This seems to be confirmed at line 80, where 'options' is passed as the second argument.

@airblade

airblade Sep 3, 2011

Owner

Oh, OK, thanks. I'm running out the door on holiday now so I'll fix this when I get back...

batter added the Not a bug label Apr 17, 2014

Collaborator

batter commented Dec 28, 2014

Closed by #439, which looks like it was heavily influenced by the work done for this PR.. cheers!

batter closed this Dec 28, 2014

batter referenced this pull request Dec 28, 2014

Closed

Transaction #90

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment