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

self.find_or_create_all_with_like_by_name does not respect many databases nor nested transactions #1068

Open
ayufan opened this issue Dec 13, 2021 · 1 comment

Comments

@ayufan
Copy link

ayufan commented Dec 13, 2021

The self.find_or_create_all_with_like_by_name seems to implement something that resembles concurrency safe creation of records: if many records are created concurrently and one of them ends-up being unique transaction is being reversed.

This implementation is sketchy as it:

  1. Does not adhere to materialised nested transactions that might use savepoints breaking transaction boundaries
  2. Does not adhere to the connection used by the model

1. Materialised nested transactions

The code can run in many level of transactions which will make ROLLBACK to revert the top-most which is incorrect beaviour.

transaction(joinable: false) do # top-level
  transaction(joinable: false) do # savepoint
    ActsAsTaggableOn::Tag.find_or_create_all_with_like_by_name('AWESOME')
    # the ROLLBACK will revert to top-level and break `savepoint`
  end
end

2. Does not adhere to the connection used by the model

The Rails 6 allows to support many databases, and allows to instruct indivdual models to use a different database. The usage of ActiveRecord::Base. is then deemed no longer safe as it might be pointing to wrong database, and rollback might be executed on a wrong database.

class MyModel
  connects_to database: { reading: :another_db, writing: :another_db }
end

ActsAsTaggableOn::Tag.connection_specification_name = MyModel.connection_specification_name

The ActsAsTaggableOn::Tag.connection will be different than ActiveRecord::Base.connection

Ref.: https://guides.rubyonrails.org/active_record_multiple_databases.html

@ayufan
Copy link
Author

ayufan commented Dec 13, 2021

Likely the proper way to implement this would be somewhere along those lines.

...

      list.map do |tag_name|
        comparable_tag_name = comparable_name(tag_name)

        begin
          tries ||= 3
          existing_tag = existing_tags.find { |tag| comparable_name(tag.name) == comparable_tag_name }
          next existing_tag if existing_tag

          transaction { create(name: tag_name) }
        rescue ActiveRecord::RecordNotUnique
          if (tries -= 1).positive?
            existing_tags = named_any(list)
            retry
          end

          raise DuplicateTagError.new("'#{tag_name}' has already been taken")
        end
      end
    end

 ...

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

No branches or pull requests

1 participant