mbleigh / acts-as-taggable-on

A tagging plugin for Rails applications that allows for custom tagging along dynamic contexts.
http://mbleigh.lighthouseapp.com/projects/10116-acts-as-taggable-on
MIT License
4.96k stars 1.19k forks source link

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

Open ayufan opened 2 years ago

ayufan commented 2 years ago

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 commented 2 years ago

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

 ...