Merge pull request #983 from unepwcmc/hotfix/1.14.1

Hotfix/1.14.1

CHANGELOG.md

@@ -1,3 +1,15 @@
+### 1.14.1
+
+**Rails 6 Upgrade**
+
+Hotfixes to solve issues mostly relating to the Rails 6 upgrade
+
+* Sanitizing some parameters.
+* Avoiding retries on jobs which was causing instability during the overnight
+  scripts.
+* Fixing an issue with storing timestamped dates which was affected by the
+  difference between BST and UTC.
+
 ### 1.14.0
 
 **Rails 6 Upgrade**

app/jobs/application_job.rb

@@ -1,2 +1,6 @@
 class ApplicationJob < ActiveJob::Base
+  # Some jobs are long-running, others are triggered frequently.
+  # Long-running jobs which fail should not be retried every few hours.
+  # Default to not retrying. Jobs that are safe to retry can override this.
+  sidekiq_options retry: false
 end

app/models/trade/inclusion_validation_rule.rb

@@ -130,7 +130,7 @@ def matching_record_to_matching_hash(mr)
       column_names.map do |column_name|
         column_value = mr.send(column_name)
         if column_value.nil?
-          [ column_name, column_value ]
+          [ column_name, "" ]
         else
           [ column_name, column_value.to_s ]
         end

config/application.rb

@@ -17,8 +17,13 @@ class Application < Rails::Application
     #
     # These settings can be overridden in specific environments using the files
     # in config/environments, which are processed later.
+
+    # Don't set the time zone - it causes issues when converting to columns of
+    # type TIMESTAMP WITHOUT TIME ZONE - during BST, dates without time parts
+    # first become midnight, then become 2300 the day before.
     #
-    config.time_zone = "London"
+    # config.time_zone = "London"
+
     # config.eager_load_paths << Rails.root.join("extras")
 
     # @see https://gist.github.com/maxivak/381f1e964923f1d469c8d39da8e2522f

config/deploy/production.rb

@@ -1,5 +1,5 @@
 set :stage, :production
-set :branch, "master"
+set :branch, ENV['CAP_BRANCH'] || 'master'
 
 server "sapi-production.linode.unep-wcmc.org", user: "wcmc", roles: %w{app web db}
 

lib/modules/material_doc_ids_retriever.rb

@@ -69,17 +69,27 @@ def self.locale_document(doc)
   end
 
   def self.ancestors_ids(tc_ids, taxon_name = nil, exact_match = nil)
+    # TODO: Raise an error if any of these are not integers instead of leaving
+    # it to the db to do this. That logic maybe belongs elsewhere?
+    # Consider using `SearchParamSanitiser.sanitise_integer_array` for this.
+    tc_ids_array = Array(tc_ids.is_a?(String) ? tc_ids.split(',') : tc_ids)
+    tc_ids_sql = tc_ids_array.map(&:to_i).filter{ |tc_id| tc_id > 0 }.join(', ')
+
+    # Don't bother running a query if we didn't get any taxon concept ids
+    return [] if tc_ids_sql.empty?
+
     res = ApplicationRecord.connection.execute(
       <<-SQL
       SELECT ancestor_taxon_concept_id
       FROM taxon_concepts_and_ancestors_mview
-      WHERE taxon_concept_id IN (#{tc_ids})
+      WHERE taxon_concept_id IN (#{tc_ids_sql})
       AND ancestor_taxon_concept_id IS NOT NULL
       ORDER BY
         #{order_case(exact_match, taxon_name)}
       tree_distance DESC, ancestor_taxon_concept_id;
       SQL
     )
+
     res.map(&:values).flatten.map(&:to_i).uniq
   end
 

lib/modules/sapi_module/stored_procedures.rb

@@ -1,9 +1,19 @@
 module SapiModule
   module StoredProcedures
 
+    ##
+    # This takes several hours to run:
+    #
+    # - The first hour is for all the views prior to trade_plus_complete view
+    # - The remainder of the time is exclusively spent on trade_plus_complete_view
+    #
+    # Runtime grows as the database grows, and in particular the trade plus
+    # dataset is growing quite a bit year on year.
+
     def self.rebuild
       ActiveRecord::Base.transaction do
-        [
+        # The names of the functions beginnning 'rebuild_' to be run in sequence
+        to_rebuild = [
           :taxonomy,
           :cites_accepted_flags,
           :listing_changes_mview,
@@ -24,9 +34,29 @@ def self.rebuild
           :trade_shipments_cites_suspensions_mview,
           :non_compliant_shipments_view,
           :trade_plus_complete_mview
-        ].each { |p|
+        ]
+
+        connection = ActiveRecord::Base.connection
+
+        to_lock = connection.execute(
+          # This is not great, because it relies on things being called mview
+          # when they're not matviews, it's the tables we're locking, matviews
+          # don't respond to LOCK TABLE.
+          "SELECT relname FROM pg_class WHERE relname LIKE '%_mview' AND relkind = 'r';"
+        ).to_a.map{ |row| row['relname'] }
+
+        to_lock.each { |relname|
+          # Lock tables in advance to prevent deadlocks forcing a rollback.
+          puts "Locking table: #{relname}"
+
+          # We need ACCESS EXCLUSIVE because this is used by DROP TABLE, and
+          # most of the rebuild_... functions are dropping and recreating the
+          # matviews.
+          connection.execute("LOCK TABLE #{relname} IN ACCESS EXCLUSIVE MODE")
+        }
+
+        to_rebuild.each { |p|
           puts "Procedure: #{p}"
-          connection = ActiveRecord::Base.connection
 
           # Within the current transaction, set work_mem to a higher-than-usual
           # value, so that matviews can be built more efficiently.