Feature/validate accessions

Gemfile.lock

@@ -190,7 +190,7 @@ GEM
     marcel (1.2.1)
     matrix (0.4.3)
     method_source (1.1.0)
-    miga-base (1.4.3.1)
+    miga-base (1.4.4.0)
       daemons (~> 1.3)
       json (~> 2)
       net-ftp
@@ -411,10 +411,10 @@ GEM
       actionpack (>= 6.1)
       activesupport (>= 6.1)
       sprockets (>= 3.0.0)
-    sqlite3 (2.10.0)
+    sqlite3 (2.10.2)
       mini_portile2 (~> 2.8.0)
-    sqlite3 (2.10.0-x86_64-darwin)
-    sqlite3 (2.10.0-x86_64-linux-gnu)
+    sqlite3 (2.10.2-x86_64-darwin)
+    sqlite3 (2.10.2-x86_64-linux-gnu)
     strain-code (0.4.4)
     thor (1.5.0)
     tilt (2.9.0)

app/controllers/names_controller.rb

@@ -334,7 +334,17 @@ def update
         format.html { redirect_to(params[:return_to] || @name, notice: 'Name was successfully updated') }
         format.json { render(:show, status: :ok, location: @name) }
       else
-        format.html { render(name_params[:name] ? :edit : :edit_etymology) }
+        format.html do
+          render(
+            if name_params[:name]
+              :edit
+            elsif name_params.key?(:nomenclatural_type_type)
+              :edit_type
+            else
+              :edit_etymology
+            end
+          )
+        end
         format.json { render(json: @name.errors, status: :unprocessable_entity) }
       end
     end

app/models/genome.rb

@@ -7,6 +7,7 @@ class Genome < ApplicationRecord
   has_many(:genome_sequencing_experiments, dependent: :destroy)
   has_many(:sequencing_experiments, through: :genome_sequencing_experiments)
 
+  before_validation(:standardize_accession)
   before_validation(:standardize_source)
   # after_commit (not after_save): this runs the external metadata refresh
   # synchronously (see monitor_source_changes), so it must not happen inside
@@ -25,6 +26,35 @@ class Genome < ApplicationRecord
   validates(:source_accession, presence: true, if: :source?)
   validates(:source_database, presence: true, if: :source?)
   validates(:accession, uniqueness: { scope: :database })
+  validates(
+    :source_database,
+    inclusion: { in: ->(_) { Genome.source_databases.keys.map(&:to_s) } },
+    allow_blank: true, if: :will_save_change_to_source_database?
+  )
+  validate(:validate_accession_formats)
+
+  ##
+  # Expected accession format per (source) database: +pattern+ matches a
+  # single accession, and lists must be comma-separated. Databases without an
+  # entry (e.g., +pending+) are not checked
+  ACCESSION_FORMATS = {
+    assembly: { pattern: /GC[AF]_\d{9}(?:\.\d+)?/, example: 'GCA_000175575.2' },
+    # INSDC (1, 2, 4, or 6 letters) or RefSeq (two letters and underscore)
+    # nucleotide records. Three-letter prefixes are excluded: in INSDC those
+    # are proteins, and SRA runs or samples (e.g., SRR, ERS) are not sequences.
+    # BioSamples (SAMN, SAMD) are excluded explicitly, since they otherwise
+    # look like four-letter WGS accessions (e.g., SAMN48198398, AUEK00000000)
+    nuccore: {
+      pattern: /
+        (?!SAM[NED])
+        (?:[A-Z]{2}_(?:[A-Z]{2}|[A-Z]{4}|[A-Z]{6})?|[A-Z]{1,2}|[A-Z]{4}|[A-Z]{6})
+        \d{5,}(?:\.\d+)?
+      /x,
+      example: 'CP036432.1'
+    },
+    biosample: { pattern: /SAM(?:N|EA|D)\d+/, example: 'SAMN10878311' },
+    sra: { pattern: /[SED]RX\d+/, example: 'SRX1234567' }
+  }.freeze
 
   has_rich_text(:submitter_comments)
 
@@ -82,6 +112,14 @@ def source_databases_opt
       source_databases.map { |k, v| [v[:name], k] }
     end
 
+    ##
+    # Regexp matching a full (comma-separated) list of accessions for the
+    # (source) database +db+, or +nil+ if the database has no expected format
+    def accession_list_regexp(db)
+      re = ACCESSION_FORMATS.dig(db.to_s.to_sym, :pattern) or return
+      /\A#{re}(?: *, *#{re})*\z/
+    end
+
     def required
       %i[
         kind? source_database? source_accession?
@@ -173,6 +211,25 @@ def source_accessions
     source_accession.try(:split, /, */)
   end
 
+  ##
+  # Check that the accession and source accession (if they are changing) have
+  # the expected format for their databases. Runs as a validation, but it can
+  # also be called directly on records that are not validated as a whole
+  def validate_accession_formats
+    if will_save_change_to_accession? || will_save_change_to_database?
+      check_accession_format(:accession, database, self.class.db_names)
+    end
+
+    if source_accession.present? &&
+         (will_save_change_to_source_accession? ||
+          will_save_change_to_source_database?)
+      check_accession_format(
+        :source_accession, source_database,
+        self.class.source_databases.transform_values { |v| v[:name] }
+      )
+    end
+  end
+
   def rrnas_or_trnas?
     number_of_16s_any.present? ||
       number_of_23s_any.present? ||
@@ -454,6 +511,29 @@ def insdc_and_refseq_links(ncbi_url, acc)
     { NCBI: ncbi_url, EMBL: 'https://www.ebi.ac.uk/ena/browser/view/%s' % acc }
   end
 
+  def standardize_accession
+    self.accession = accession.strip if accession.present?
+  end
+
+  ##
+  # Add an error to +attr+ unless its value matches the expected format for
+  # +db+, with a hint if it matches another of the databases in +names+
+  # (a Hash of database keys to display names)
+  def check_accession_format(attr, db, names)
+    re = self.class.accession_list_regexp(db) or return
+    value = send(attr).to_s
+    return if value =~ re
+
+    msg = "is not a valid #{names[db.to_s.to_sym]} accession " \
+          "(e.g., #{ACCESSION_FORMATS[db.to_s.to_sym][:example]}); " \
+          'separate multiple accessions with commas'
+    other = names.keys.find do |k|
+      k.to_s != db.to_s && value =~ self.class.accession_list_regexp(k)
+    end
+    msg += "; it looks like an #{names[other]} accession instead" if other
+    errors.add(attr, :invalid_format, message: msg)
+  end
+
   def standardize_source
     if source_accession
       self.source_accession.strip!

app/models/name.rb

@@ -148,6 +148,7 @@ class Name < ApplicationRecord
     :nomenclatural_type_id,
     presence: { if: :nomenclatural_type_type? }
   )
+  validate(:nomenclatural_type_genome_is_valid)
 
   include HasObservers
   include HasExternalResources
@@ -1489,6 +1490,21 @@ def observe_nomenclatural_type_entry
     self.nomenclatural_type = nomenclatural_type_from_entry
   end
 
+  ##
+  # When the type genome could not be created from the entered accession
+  # (e.g., the accession doesn't match the selected database), report the
+  # genome's own errors on the entry field instead of a missing type
+  def nomenclatural_type_genome_is_valid
+    genome = nomenclatural_type
+    return unless genome.is_a?(Genome) && genome.new_record?
+    return if genome.valid?
+
+    errors.delete(:nomenclatural_type_id)
+    genome.errors.each do |error|
+      errors.add(:nomenclatural_type_entry, error.message)
+    end
+  end
+
   def observe_genome_strain
     return unless type_genome.present?
 

lib/tasks/evaluate_genomes.rake

@@ -133,4 +133,65 @@ namespace :genomes do
       genome.update(new_data.merge(auto_check: true))
     end
   end
+
+  desc 'Move genome accessions stored under the wrong database and requeue ' \
+       'failed downloads (dry run unless APPLY=1)'
+  task fix_accessions: :environment do
+    apply = ENV['APPLY'] == '1'
+    $stderr.puts(
+      apply ? 'Fixing genome accessions' :
+              'Fixing genome accessions (dry run, set APPLY=1 to apply)'
+    )
+    counts = Hash.new(0)
+    review = []
+
+    Genome.where(database: %w[assembly nuccore]).find_each do |genome|
+      # Orphan genomes (no linked names) are purged separately
+      next if genome.names.empty?
+
+      db = genome.database
+      other = db == 'assembly' ? 'nuccore' : 'assembly'
+      label = "#{genome.miga_name} (#{genome.names.first.name})"
+      failed = genome.auto_failed.to_s.start_with?('Cannot find ncbi:')
+      move = nil
+
+      if genome.accession =~ Genome.accession_list_regexp(db)
+        next unless failed
+      elsif genome.accession =~ Genome.accession_list_regexp(other)
+        twin = Genome.find_by(database: other, accession: genome.accession)
+        if twin
+          review << "#{label}: #{db}:#{genome.accession} also exists as " \
+                    "#{other} in #{twin.miga_name}"
+          counts[:conflict] += 1
+          next
+        end
+        move = other
+      else
+        review << "#{label}: #{db}:#{genome.accession} is malformed"
+        counts[:malformed] += 1
+        next
+      end
+
+      $stderr.puts "o #{label}: #{db}:#{genome.accession}" \
+                   "#{" -> #{move}" if move}#{' (requeued)' if failed}"
+      counts[:moved] += 1 if move
+      counts[:requeued] += 1 if failed
+      next unless apply
+
+      if move && !genome.update(database: move)
+        review << "#{label}: could not move to #{move}: " \
+                  "#{genome.errors.full_messages.to_sentence}"
+        next
+      end
+      genome.recalculate_miga! if failed
+    end
+
+    unless review.empty?
+      $stderr.puts '', 'Needs manual review:'
+      review.each { |i| $stderr.puts "? #{i}" }
+    end
+    $stderr.puts '', "#{apply ? 'Done' : 'Would do'}: " \
+                 "#{counts[:moved]} moved, #{counts[:requeued]} requeued, " \
+                 "#{counts[:conflict]} conflicts, #{counts[:malformed]} malformed"
+  end
 end

test/controllers/names_controller_test.rb

@@ -150,4 +150,23 @@ class NamesControllerTest < ActionDispatch::IntegrationTest
     def autocomplete_values
       JSON.parse(response.body).pluck('value')
     end
+
+  test 'shows genome accession errors on the type form' do
+    name = names(:draft_by_contributor)
+    name.update_column(:rank, 'species')
+
+    assert_no_difference('Genome.count') do
+      patch name_url(name), params: {
+        name: {
+          nomenclatural_type_type: 'nuccore',
+          nomenclatural_type_entry: 'GCA_039803745.1'
+        }
+      }
+    end
+
+    assert_response :success
+    assert_select 'h1', /Type of/
+    assert_includes @response.body, 'looks like an NCBI Assembly accession'
+    assert_nil name.reload.nomenclatural_type
+  end
 end