Merge pull request #131 from af83/5024-prevent-duplicate-referentials-from-being-created-during-parallel-db-transactions--rb201711271659

5024 prevent duplicate referentials from being created during parallel db transactions  rb201711271659

11 files changed, 268 insertions, 49 deletions

diff --git a/app/controllers/referentials_controller.rb b/app/controllers/referentials_controller.rb
index 6f398b7f5..83e3bc56a 100644
--- a/app/controllers/referentials_controller.rb
+++ b/app/controllers/referentials_controller.rb
@@ -8,7 +8,7 @@ class ReferentialsController < ChouetteController
 
   def new
     new! do
-      build_referenial
+      build_referential
     end
   end
 
@@ -137,7 +137,7 @@ class ReferentialsController < ChouetteController
     super
   end
 
-  def build_referenial
+  def build_referential
     if params[:from]
       source_referential = Referential.find(params[:from])
       @referential = Referential.new_from(source_referential, current_functional_scope)
diff --git a/app/errors/table_lock_timeout_error.rb b/app/errors/table_lock_timeout_error.rb
new file mode 100644
index 000000000..102f3a4a0
--- /dev/null
+++ b/app/errors/table_lock_timeout_error.rb
@@ -0,0 +1 @@
+class TableLockTimeoutError < ActiveRecord::StatementInvalid; end
diff --git a/app/models/referential.rb b/app/models/referential.rb
index 8db009ebd..1cdda9e6a 100644
--- a/app/models/referential.rb
+++ b/app/models/referential.rb
@@ -13,17 +13,17 @@ class Referential < ActiveRecord::Base
 
   validates_uniqueness_of :slug
 
-  validates_format_of :slug, :with => %r{\A[a-z][0-9a-z_]+\Z}
-  validates_format_of :prefix, :with => %r{\A[0-9a-zA-Z_]+\Z}
-  validates_format_of :upper_corner, :with => %r{\A-?[0-9]+\.?[0-9]*\,-?[0-9]+\.?[0-9]*\Z}
-  validates_format_of :lower_corner, :with => %r{\A-?[0-9]+\.?[0-9]*\,-?[0-9]+\.?[0-9]*\Z}
+  validates_format_of :slug, with: %r{\A[a-z][0-9a-z_]+\Z}
+  validates_format_of :prefix, with: %r{\A[0-9a-zA-Z_]+\Z}
+  validates_format_of :upper_corner, with: %r{\A-?[0-9]+\.?[0-9]*\,-?[0-9]+\.?[0-9]*\Z}
+  validates_format_of :lower_corner, with: %r{\A-?[0-9]+\.?[0-9]*\,-?[0-9]+\.?[0-9]*\Z}
   validate :slug_excluded_values
 
   attr_accessor :upper_corner
   attr_accessor :lower_corner
 
   has_one :user
-  has_many :api_keys, :class_name => 'Api::V1::ApiKey', :dependent => :destroy
+  has_many :api_keys, class_name: 'Api::V1::ApiKey', dependent: :destroy
 
   belongs_to :organisation
   validates_presence_of :organisation
@@ -62,6 +62,18 @@ class Referential < ActiveRecord::Base
   scope :order_by_validity_period, ->(dir) { joins(:metadatas).order("unnest(periodes) #{dir}") }
   scope :order_by_lines, ->(dir) { joins(:metadatas).group("referentials.id").order("sum(array_length(referential_metadata.line_ids,1)) #{dir}") }
 
+  def save_with_table_lock_timeout(options = {})
+    save_without_table_lock_timeout(options)
+  rescue ActiveRecord::StatementInvalid => e
+    if e.message.include?('PG::LockNotAvailable')
+      raise TableLockTimeoutError.new(e)
+    else
+      raise
+    end
+  end
+
+  alias_method_chain :save, :table_lock_timeout
+
   def lines
     if metadatas.blank?
       workbench ? workbench.lines : associated_lines
@@ -79,7 +91,7 @@ class Referential < ActiveRecord::Base
         errors.add(:slug,I18n.t("referentials.errors.public_excluded"))
       end
       if slug == self.class.connection_config[:username]
-        errors.add(:slug,I18n.t("referentials.errors.user_excluded", :user => slug))
+        errors.add(:slug,I18n.t("referentials.errors.user_excluded", user: slug))
       end
     end
   end
@@ -146,7 +158,7 @@ class Referential < ActiveRecord::Base
   end
 
   def self.new_from(from, functional_scope)
-     Referential.new(
+    Referential.new(
       name: I18n.t("activerecord.copy", name: from.name),
       slug: "#{from.slug}_clone",
       prefix: from.prefix,
@@ -196,9 +208,15 @@ class Referential < ActiveRecord::Base
     projection_type || ""
   end
 
-  before_validation :assign_line_and_stop_area_referential, :on => :create, if: :workbench
-  before_validation :assign_slug, :on => :create
-  before_validation :assign_prefix, :on => :create
+  before_validation :assign_line_and_stop_area_referential, on: :create, if: :workbench
+  before_validation :assign_slug, on: :create
+  before_validation :assign_prefix, on: :create
+
+  # Lock the `referentials` table to prevent duplicate referentials from being
+  # created simultaneously in separate transactions. This must be the last hook
+  # to minimise the duration of the lock.
+  before_save :lock_table, on: [:create, :update]
+
   before_create :create_schema
   after_create :clone_schema, if: :created_from
 
@@ -293,7 +311,11 @@ class Referential < ActiveRecord::Base
 
   def create_schema
     unless created_from
-      Apartment::Tenant.create slug
+      report = Benchmark.measure do
+        Apartment::Tenant.create slug
+      end
+
+      Rails.logger.info("Schema create benchmark: '#{slug}'\t#{report}")
       Rails.logger.error( "Schema migrations count for Referential #{slug} " + Referential.connection.select_value("select count(*) from #{slug}.schema_migrations;").to_s )
     end
   end
@@ -378,4 +400,13 @@ class Referential < ActiveRecord::Base
     not metadatas_overlap?
   end
 
+  private
+
+  def lock_table
+    # No explicit unlock is needed as it will be released at the end of the
+    # transaction.
+    ActiveRecord::Base.connection.execute(
+      'LOCK referentials IN ACCESS EXCLUSIVE MODE'
+    )
+  end
 end
diff --git a/spec/features/lines_spec.rb b/spec/features/lines_spec.rb
index f142b7da9..f176e34fe 100644
--- a/spec/features/lines_spec.rb
+++ b/spec/features/lines_spec.rb
@@ -61,22 +61,22 @@ describe "Lines", type: :feature do
     #   it "creates line and return to show" do
     #     visit line_referential_lines_path(line_referential)
     #     click_link "Ajouter une ligne"
-    #     fill_in "line_name", :with => "Line 1"
-    #     fill_in "Numéro d'enregistrement", :with => "1"
-    #     fill_in "Identifiant Neptune", :with => "chouette:test:Line:999"
+    #     fill_in "line_name", with: "Line 1"
+    #     fill_in "Numéro d'enregistrement", with: "1"
+    #     fill_in "Identifiant Neptune", with: "chouette:test:Line:999"
     #     click_button("Créer ligne")
     #     expect(page).to have_content("Line 1")
     #   end
     # end
 
     # Fixme #1780
-    # describe "new with group of line", :js => true do
+    # describe "new with group of line", truncation: true do
     #   it "creates line and return to show" do
     #     visit new_line_referential_line_path(line_referential)
-    #     fill_in "line_name", :with => "Line 1"
-    #     fill_in "Numéro d'enregistrement", :with => "1"
-    #     fill_in "Identifiant Neptune", :with => "test:Line:999"
-    #     fill_in_token_input('line_group_of_line_tokens', :with => "#{group_of_line.name}")
+    #     fill_in "line_name", with: "Line 1"
+    #     fill_in "Numéro d'enregistrement", with: "1"
+    #     fill_in "Identifiant Neptune", with: "test:Line:999"
+    #     fill_in_token_input('line_group_of_line_tokens', with: "#{group_of_line.name}")
     #     find_button("Créer ligne").trigger("click")
     #     expect(page).to have_text("Line 1")
     #     expect(page).to have_text("#{group_of_line.name}")
@@ -88,8 +88,8 @@ describe "Lines", type: :feature do
     #   it "edit line" do
     #     visit line_referential_line_path(line_referential, subject)
     #     click_link "Editer cette ligne"
-    #     fill_in "line_name", :with => "Line Modified"
-    #     fill_in "Numéro d'enregistrement", :with => "test-1"
+    #     fill_in "line_name", with: "Line Modified"
+    #     fill_in "Numéro d'enregistrement", with: "test-1"
     #     click_button("Editer ligne")
     #     expect(page).to have_content("Line Modified")
     #   end
diff --git a/spec/features/networks_spec.rb b/spec/features/networks_spec.rb
index 1a822e7c9..8586b2a16 100644
--- a/spec/features/networks_spec.rb
+++ b/spec/features/networks_spec.rb
@@ -1,7 +1,7 @@
 # -*- coding: utf-8 -*-
 require 'spec_helper'
 
-describe "Networks", :type => :feature do
+describe "Networks", type: :feature do
   login_user
 
   let(:line_referential) { create :line_referential, member: @user.organisation }
@@ -54,9 +54,9 @@ describe "Networks", :type => :feature do
   #     # allow(subject).to receive(:stop_areas).and_return(Array.new(2) { create(:stop_area) })
   #     visit line_referential_networks_path(line_referential)
   #     click_link "Ajouter un réseau"
-  #     fill_in "network_name", :with => "Network 1"
-  #     fill_in "Numéro d'enregistrement", :with => "test-1"
-  #     fill_in "Identifiant Neptune", :with => "chouette:test:GroupOfLine:1"
+  #     fill_in "network_name", with: "Network 1"
+  #     fill_in "Numéro d'enregistrement", with: "test-1"
+  #     fill_in "Identifiant Neptune", with: "chouette:test:GroupOfLine:1"
   #     click_button("Créer réseau")
   #     expect(page).to have_content("Network 1")
   #   end
@@ -67,14 +67,14 @@ describe "Networks", :type => :feature do
   #     # allow(subject).to receive(:stop_areas).and_return(Array.new(2) { create(:stop_area) })
   #     visit line_referential_network_path(line_referential, subject)
   #     click_link "Editer ce réseau"
-  #     fill_in "network_name", :with => "Network Modified"
-  #     fill_in "Numéro d'enregistrement", :with => "test-1"
+  #     fill_in "network_name", with: "Network Modified"
+  #     fill_in "Numéro d'enregistrement", with: "test-1"
   #     click_button("Editer réseau")
   #     expect(page).to have_content("Network Modified")
   #   end
   # end
 
-  # describe "delete", :js => true do
+  # describe "delete", truncation: true do
   #   it "delete network and return to the list" do
   #     subject.stub(:stop_areas).and_return(Array.new(2) { create(:stop_area) })
   #     visit line_referential_network_path(line_referential, subject)
diff --git a/spec/features/users/user_delete_spec.rb b/spec/features/users/user_delete_spec.rb
index 48f4e35d1..8b2ffcbe5 100644
--- a/spec/features/users/user_delete_spec.rb
+++ b/spec/features/users/user_delete_spec.rb
@@ -7,7 +7,7 @@ Warden.test_mode!
 #   As a user
 #   I want to delete my user profile
 #   So I can close my account
-feature 'User delete', :devise, :js do
+feature 'User delete', :devise, :truncation do
 
   after(:each) do
     Warden.test_reset!
diff --git a/spec/models/referential/referential_lock_during_creation_spec.rb b/spec/models/referential/referential_lock_during_creation_spec.rb
new file mode 100644
index 000000000..717a96136
--- /dev/null
+++ b/spec/models/referential/referential_lock_during_creation_spec.rb
@@ -0,0 +1,198 @@
+RSpec.describe Referential, type: :model do
+  let (:workbench) { create(:workbench) }
+
+  def with_a_mutual_lock timeout: false
+    @with_a_mutual_lock = true
+    yield
+    thread_1 = Thread.new do
+      ActiveRecord::Base.transaction do
+        # seize LOCK
+        @locking_thread_content.try :call
+        sleep 10
+        # release LOCK
+      end
+    end
+
+    thread_2 = Thread.new do
+      sleep 5
+      ActiveRecord::Base.transaction do
+        if timeout
+          ActiveRecord::Base.connection.execute "SET lock_timeout = '1s'"
+        end
+        # waits for LOCK, (because of sleep 5)
+        @waiting_thread_content.try :call
+        # when lock was eventually obtained validation failed
+      end
+    end
+
+    thread_1.join
+    if timeout
+      expect do
+        thread_2.join
+      end.to raise_error(TableLockTimeoutError)
+    else
+      thread_2.join
+    end
+    @locking_thread_content = nil
+    @waiting_thread_content = nil
+    @with_a_mutual_lock = false
+  end
+
+  def locking_thread
+    raise "this method is intended to be called inside a `with_a_mutual_lock`" unless @with_a_mutual_lock
+    @locking_thread_content = yield
+  end
+
+  def waiting_thread
+    @waiting_thread_content = yield
+  end
+
+  context "when two identical Referentials are created, only one is saved" do
+    it "works synchronously" do
+      referential_1 = build(
+        :referential,
+        workbench: workbench,
+        organisation: workbench.organisation
+      )
+      referential_2 = referential_1.dup
+      referential_2.slug = "#{referential_1.slug}_different"
+
+      metadata_1 = build(:referential_metadata, referential: nil)
+      metadata_2 = metadata_1.dup
+
+      referential_1.metadatas << metadata_1
+      referential_2.metadatas << metadata_2
+
+      referential_1.save
+      referential_2.save
+
+      expect(referential_1).to be_persisted
+      expect(referential_2).not_to be_persisted
+    end
+
+    context truncation: true do
+      it "works asynchronously" do
+        skip('The truncation strategy breaks all subsequent tests (See #5295)')
+
+        begin
+          referential_1 = build(
+            :referential,
+            workbench: workbench,
+            organisation: workbench.organisation
+          )
+          referential_2 = referential_1.dup
+          referential_2.slug = "#{referential_1.slug}_different"
+          referential_3 = nil
+
+          metadata_1 = build(:referential_metadata, referential: nil)
+          metadata_2 = metadata_1.dup
+
+          referential_1.metadatas << metadata_1
+          referential_2.metadatas << metadata_2
+
+          with_a_mutual_lock do
+            locking_thread do
+              referential_1.save
+            end
+            waiting_thread do
+              referential_2.save
+              referential_3 = create(:referential)
+            end
+          end
+
+          expect(referential_1).to be_persisted
+          expect(referential_2).not_to be_persisted
+          expect(referential_3).to be_persisted
+        ensure
+          Apartment::Tenant.drop(referential_1.slug) if referential_1.persisted?
+          Apartment::Tenant.drop(referential_2.slug) if referential_2.persisted?
+
+          if referential_3.try(:persisted?)
+            Apartment::Tenant.drop(referential_3.slug)
+          end
+        end
+      end
+
+      it "works asynchronously when one is updated" do
+        skip('The truncation strategy breaks all subsequent tests (See #5295)')
+
+        begin
+          referential_1 = nil
+          referential_2 = nil
+
+          ActiveRecord::Base.transaction do
+            referential_1 = create(
+              :referential,
+              workbench: workbench,
+              organisation: workbench.organisation
+            )
+            referential_2 = referential_1.dup
+            referential_2.name = 'Another'
+            referential_2.slug = "#{referential_1.slug}_different"
+            referential_2.save!
+          end
+
+          metadata_2 = build(:referential_metadata, referential: nil)
+          metadata_1 = metadata_2.dup
+          with_a_mutual_lock do
+            locking_thread do
+              referential_1.metadatas_attributes = [metadata_1.attributes]
+              referential_1.save
+            end
+            waiting_thread do
+              referential_2.metadatas_attributes = [metadata_2.attributes]
+              referential_2.save
+            end
+          end
+
+          expect(referential_1).to be_valid
+          expect(referential_2).not_to be_valid
+        ensure
+          Apartment::Tenant.drop(referential_1.slug) if referential_1.persisted?
+          Apartment::Tenant.drop(referential_2.slug) if referential_2.persisted?
+        end
+      end
+    end
+  end
+
+  context "when two Referentials are created at the same time", truncation: true do
+    it "raises an error when the DB lock timeout is reached" do
+      skip('The truncation strategy breaks all subsequent tests (See #5295)')
+
+      begin
+        referential_1 = build(
+          :referential,
+          workbench: workbench,
+          organisation: workbench.organisation
+        )
+        referential_2 = referential_1.dup
+        referential_2.slug = "#{referential_1.slug}_different"
+        referential_3 = nil
+
+        metadata_1 = build(:referential_metadata, referential: nil)
+        metadata_2 = metadata_1.dup
+
+        referential_1.metadatas << metadata_1
+        referential_2.metadatas << metadata_2
+        with_a_mutual_lock timeout: true do
+          locking_thread do
+            referential_1.save
+          end
+          waiting_thread do
+            referential_2.save
+            referential_3 = create(:referential)
+          end
+        end
+
+        expect(referential_1).to be_persisted
+      ensure
+        Apartment::Tenant.drop(referential_1.slug) if referential_1.persisted?
+        Apartment::Tenant.drop(referential_2.slug) if referential_2.persisted?
+
+        if referential_3.try(:persisted?)
+          Apartment::Tenant.drop(referential_3.slug)
+        end
+      end
+    end
+  end
+end
diff --git a/spec/models/referential_spec.rb b/spec/models/referential_spec.rb
index d0b1d6447..7816e7232 100644
--- a/spec/models/referential_spec.rb
+++ b/spec/models/referential_spec.rb
@@ -1,13 +1,6 @@
-require 'spec_helper'
-
 describe Referential, :type => :model do
   let(:ref) { create :workbench_referential, metadatas: [create(:referential_metadata)] }
 
-  # it "create a rule_parameter_set" do
-  #   referential = create(:referential)
-  #   expect(referential.rule_parameter_sets.size).to eq(1)
-  # end
-
   it { should have_many(:metadatas) }
   it { should belong_to(:workbench) }
   it { should belong_to(:referential_suite) }
@@ -131,4 +124,5 @@ describe Referential, :type => :model do
       end
     end
   end
+
 end
diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb
index 47578405e..9eb628d95 100644
--- a/spec/rails_helper.rb
+++ b/spec/rails_helper.rb
@@ -32,11 +32,6 @@ RSpec.configure do |config|
   # Remove this line if you're not using ActiveRecord or ActiveRecord fixtures
   config.fixture_path = "#{::Rails.root}/spec/fixtures"
 
-  # If you're not using ActiveRecord, or you'd prefer not to run each of your
-  # examples within a transaction, remove the following line or assign false
-  # instead of true.
-  config.use_transactional_fixtures = true
-
   # RSpec Rails can automatically mix in different behaviours to your tests
   # based on their file location, for example enabling you to call `get` and
   # `post` in specs under `spec/controllers`.
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 9679952df..2d13d3802 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -64,11 +64,11 @@ RSpec.configure do |config|
   Capybara.javascript_driver = :poltergeist
   # :meta tests can be run seperately in case of doubt about the tests themselves
   # they serve mainly as an explanataion of complicated tests (as e.g. PG information_schema introspection)
-  config.filter_run_excluding :meta => true
-  config.filter_run_excluding :js   => true
-  config.filter_run           :wip  => true
+  config.filter_run_excluding meta: true
+  config.filter_run_excluding truncation: true
+  config.filter_run_excluding wip: true
   config.run_all_when_everything_filtered = true
-  config.include TokenInputHelper, :type => :feature
+  config.include TokenInputHelper, type: :feature
 
   # ## Mock Framework
   #
@@ -93,7 +93,7 @@ RSpec.configure do |config|
   # You can disable this behaviour by removing the line below, and instead
   # explicitly tag your specs with their type, e.g.:
   #
-  #     RSpec.describe UsersController, :type => :controller do
+  #     RSpec.describe UsersController, type: :controller do
   #       # ...
   #     end
   #
diff --git a/spec/support/referential.rb b/spec/support/referential.rb
index 7f56e5143..497ff47a8 100644
--- a/spec/support/referential.rb
+++ b/spec/support/referential.rb
@@ -77,7 +77,7 @@ RSpec.configure do |config|
     first_referential.switch
   end
 
-  config.before(:each, :js => true) do
+  config.before(:each, truncation: true) do
     DatabaseCleaner.strategy = :truncation, { except: %w[spatial_ref_sys] }
   end