diff options
| -rw-r--r-- | app/assets/stylesheets/modules/import_messages.sass | 37 | ||||
| -rw-r--r-- | app/controllers/referentials_controller.rb | 4 | ||||
| -rw-r--r-- | app/errors/table_lock_timeout_error.rb | 1 | ||||
| -rw-r--r-- | app/helpers/imports_helper.rb | 14 | ||||
| -rw-r--r-- | app/models/referential.rb | 53 | ||||
| -rw-r--r-- | app/views/imports/_import_messages.html.slim | 11 | ||||
| -rw-r--r-- | spec/features/lines_spec.rb | 20 | ||||
| -rw-r--r-- | spec/features/networks_spec.rb | 14 | ||||
| -rw-r--r-- | spec/features/users/user_delete_spec.rb | 2 | ||||
| -rw-r--r-- | spec/models/referential/referential_lock_during_creation_spec.rb | 198 | ||||
| -rw-r--r-- | spec/models/referential_spec.rb | 8 | ||||
| -rw-r--r-- | spec/rails_helper.rb | 5 | ||||
| -rw-r--r-- | spec/spec_helper.rb | 10 | ||||
| -rw-r--r-- | spec/support/referential.rb | 2 | ||||
| -rw-r--r-- | spec/views/imports/show.html.slim_spec.rb | 5 |
15 files changed, 288 insertions, 96 deletions
diff --git a/app/assets/stylesheets/modules/import_messages.sass b/app/assets/stylesheets/modules/import_messages.sass index 6a088dc06..cde903b00 100644 --- a/app/assets/stylesheets/modules/import_messages.sass +++ b/app/assets/stylesheets/modules/import_messages.sass @@ -4,40 +4,5 @@ h1 padding-bottom: 20px - -.import_message-list +ul.list-unstyled padding-bottom: 20px - - .import_messages-head - display: block - font-size: 1.8rem - font-weight: 700 - border-bottom: 2px solid #4b4b4b - padding: 5px 15px 6px 15px - - dl - dd, dt - display: inline-block - letter-spacing: normal - word-spacing: normal - text-rendering: auto - vertical-align: top - padding: 5px 15px 6px 15px - - dt - position: relative - width: 7% - font-weight: 700 - - &:before - content: "" - display: block - position: absolute - z-index: 1 - top: 0 - left: 0 - width: 250% - border-bottom: 1px solid rgba(164, 164, 164, 0.5) - - dd - width: 93%
\ No newline at end of file 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/helpers/imports_helper.rb b/app/helpers/imports_helper.rb index 1c4549e50..140660153 100644 --- a/app/helpers/imports_helper.rb +++ b/app/helpers/imports_helper.rb @@ -15,6 +15,20 @@ module ImportsHelper end end + # Compliance check set messages + def bootstrap_class_for_message_criticity message_criticity + case message_criticity + when "error" + "alert alert-danger" + when "warning" + "alert alert-warning" + when "info" + "alert alert-info" + else + message_criticity.to_s + end + end + ############################## # TO CLEAN!!! ############################## 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/app/views/imports/_import_messages.html.slim b/app/views/imports/_import_messages.html.slim index a10dce065..af10b23e5 100644 --- a/app/views/imports/_import_messages.html.slim +++ b/app/views/imports/_import_messages.html.slim @@ -1,11 +1,8 @@ - if import_messages.any? - .import_message-list - .import_messages-head Messages - dl - - import_messages.each do | import_message | - dt.criticity - = import_message.criticity - dd + ul.list-unstyled.import_message-list + - import_messages.each do | import_message | + li + span(class="#{bootstrap_class_for_message_criticity import_message.criticity}") = t( ['import_messages', 'compliance_check_messages', import_message.message_key].join('.'), import_message.message_attributes.symbolize_keys) 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 diff --git a/spec/views/imports/show.html.slim_spec.rb b/spec/views/imports/show.html.slim_spec.rb index 7a046d1a2..faf473758 100644 --- a/spec/views/imports/show.html.slim_spec.rb +++ b/spec/views/imports/show.html.slim_spec.rb @@ -22,10 +22,7 @@ RSpec.describe '/imports/show', type: :view do messages.each do | message | # require 'htmlbeautifier' # b = HtmlBeautifier.beautify(rendered, indent: ' ') - expect(rendered).to have_selector('.import_message-list dl dt.criticity') do - with_text message.criticity - end - expect(rendered).to have_selector('.import_message-list dl dd') do + expect(rendered).to have_selector('.import_message-list li') do with_text rendered_message( message ) end end |