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.

Sign up for GitHub

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

Jump to bottom

Add unique constraint to reader and readable #78

Merged
merged 3 commits into from
Aug 22, 2016
Merged

Add unique constraint to reader and readable #78

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1d70820
Make reader and readable unique
allenwq Aug 20, 2016
5cba714
Handle RecordNotUnique in .mark_array_as_read
allenwq Aug 20, 2016
d7e724f
Add specs to test when RecordNotUnique is raised
allenwq Aug 20, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/generators/unread/migration/templates/migration.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ def self.up
t.datetime :timestamp
end

add_index ReadMark, [:reader_id, :reader_type, :readable_type, :readable_id], name: 'read_marks_reader_readable_index'
add_index ReadMark, [:reader_id, :reader_type, :readable_type, :readable_id], name: 'read_marks_reader_readable_index', unique: true
end

def self.down
Expand Down
17 changes: 12 additions & 5 deletions lib/unread/readable.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,18 @@ def mark_array_as_read(array, reader)
if global_timestamp && global_timestamp >= timestamp
# The object is implicitly marked as read, so there is nothing to do
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!
# This transaction is needed, so that parent transaction won't rollback even there's an error.
ReadMark.transaction(requires_new: true) do
begin
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::RecordNotUnique
raise ActiveRecord::Rollback
Copy link
Owner

Choose a reason for hiding this comment

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

This line seems not to be required. A rollback is already made on this exception, or am I wrong?

Copy link
Contributor Author

@allenwq allenwq Aug 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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 )

Copy link
Owner

Choose a reason for hiding this comment

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

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::RecordNotUnique

No inner transaction needed, no thinking about savepoints, just add a rescue for the save! operation. What do you think?

Copy link
Contributor Author

@allenwq allenwq Aug 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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 ?

Copy link
Owner

Choose a reason for hiding this comment

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

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.

end
end
end
end
end
Expand Down
14 changes: 14 additions & 0 deletions spec/unread/readable_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,20 @@
expect(@email2.unread?(@reader)).to be_falsey
end

it "should mark the rest as read when the first record is not unique" do
Email.mark_as_read! [ @email1 ], for: @reader

allow(@email1).to receive_message_chain("read_marks.build").and_return(@email1.read_marks.build)
allow(@email1).to receive_message_chain("read_marks.where").and_return([])
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of mocking the implementation: What do you think of using two threads to demonstrate that it's really thread-safe now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think database level unique constraint guarantees that no duplicate records will be saved.

It's very hard to simulate a non-thread safe condition using threads because they are non-deterministic. Actually, the mocking achieves the same propose and it's deterministic.


expect do
Email.mark_as_read! [ @email1, @email2 ], for: @reader
end.to change(ReadMark, :count).by(1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ledermann How about this ? think it's more explicit.


expect(@email1.unread?(@reader)).to be_falsey
expect(@email2.unread?(@reader)).to be_falsey
end
Copy link
Owner

Choose a reason for hiding this comment

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

Please wrap the whole example into a expect { ... }.to change(ReadMark, :count).by(2). This makes sure that the example fails if the database index is not unique.

Copy link
Contributor Author

@allenwq allenwq Aug 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

not sure if nested expectations will work, but will try that.


it "should perform less queries if the objects are already read" do
Email.mark_as_read! :all, :for => @reader

Expand Down