Add unique constraint to reader and readable #78
Conversation
|
@ledermann Let me know your comments, thanks ! |
|
@ledermann Tests added, sorry I have been testing travis here and so got spammed by coveralls. |
| rm.timestamp = timestamp | ||
| rm.save! | ||
| rescue ActiveRecord::RecordNotUnique | ||
| raise ActiveRecord::Rollback |
There was a problem hiding this comment.
This line seems not to be required. A rollback is already made on this exception, or am I wrong?
There was a problem hiding this comment.
Initially, the code here was next, but it did not work and I debugged for quite long time. I realised that most of the databases do not support nested transactions, and AR uses savepoints to mitigate that, this Rollback is needed in order to let AR rollback to the savepoint. ( Check here http://api.rubyonrails.org/classes/ActiveRecord/Transactions/ClassMethods.html )
There was a problem hiding this comment.
In my tests with SQLite and MySQL removing the raise ActiveRecord::Rollback works fine. IMHO there is no need for a rollback here, because there is nothing to rollback. A Rollback is required if you want to undo a successful write operation, but we don't have one here, because the only write operation has already failed.
Some further investigation leads me to the following simplification of your change:
else
rm = obj.read_marks.where(:reader_id => reader.id, :reader_type => reader.class.base_class.name).first || obj.read_marks.build
rm.reader_id = reader.id
rm.reader_type = reader.class.base_class.name
rm.timestamp = timestamp
rm.save! rescue ActiveRecord::RecordNotUniqueNo inner transaction needed, no thinking about savepoints, just add a rescue for the save! operation. What do you think?
There was a problem hiding this comment.
Yah, MySQL works, but Postgres won't work. ( see Nested Transactions section in the link which I posted earlier ).
The solution you suggested will rollback the entire transaction, which means all elements in that array will be rollback if one of them is not unique. Is that what we want ?
There was a problem hiding this comment.
You are right, Postgres needs some special handling, because of the error message "current transaction is aborted, commands ignored until end of transaction block". The inner transaction with the additional rollback both seems to be needed to make it compatible with Postgres. For SQLite and MySQL it's not needed.
|
Thank you very much for contribution, @allenwq! I'm very interested to improve the gem to fix edge cases like the one you described. Before merging, please see me comments above. |
|
@ledermann Thanks for the comments ! |
|
|
||
| expect do | ||
| Email.mark_as_read! [ @email1, @email2 ], for: @reader | ||
| end.to change(ReadMark, :count).by(1) |
There was a problem hiding this comment.
@ledermann How about this ? think it's more explicit.
1 similar comment
|
Thanks for your great work, @allenwq! |
|
@ledermann My pleasure, thanks for comments and writing this awesome gem ! |
The readable and reader is not unique before, and
mark_array_as_readis not thread safe.Our production app has lots of duplicates generated under high load. And this further result in duplicated records returned in
.with_readmarks_for(user).This PR adds a unique constraint to the table and handle the error.
Still working on a test for this.