diff --git a/Gemfile.lock b/Gemfile.lock
index d151f2d4..7af689fd 100644
--- a/Gemfile.lock
+++ b/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)
diff --git a/app/controllers/names_controller.rb b/app/controllers/names_controller.rb
index 1115e4b6..d4a0188f 100644
--- a/app/controllers/names_controller.rb
+++ b/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
diff --git a/app/models/genome.rb b/app/models/genome.rb
index 9b8c0ce4..6a0d120a 100644
--- a/app/models/genome.rb
+++ b/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!
diff --git a/app/models/name.rb b/app/models/name.rb
index db8d9361..291a5ff4 100644
--- a/app/models/name.rb
+++ b/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?
diff --git a/lib/tasks/evaluate_genomes.rake b/lib/tasks/evaluate_genomes.rake
index add0c568..fa036d25 100644
--- a/lib/tasks/evaluate_genomes.rake
+++ b/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
diff --git a/test/controllers/names_controller_test.rb b/test/controllers/names_controller_test.rb
index 1863a1cd..a8a3aa9a 100644
--- a/test/controllers/names_controller_test.rb
+++ b/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
diff --git a/test/models/genome_test.rb b/test/models/genome_test.rb
index 867af62f..685e5118 100644
--- a/test/models/genome_test.rb
+++ b/test/models/genome_test.rb
@@ -67,7 +67,8 @@ class GenomeTest < ActiveSupport::TestCase
end
test 'unlink check agrees with link check on secondary biosample accessions' do
- one = sequencing_experiments(:one) # biosample_accession: 'MyString'
+ one = sequencing_experiments(:one)
+ one.update_column(:biosample_accession, 'SAMN00000001')
two = SequencingExperiment.create!(
sra_accession: 'SRXALT', biosample_accession_2: 'AltAccession'
)
@@ -79,10 +80,10 @@ class GenomeTest < ActiveSupport::TestCase
database: 'assembly',
accession: 'GCA_000000003.1',
source_database: 'biosample',
- source_accession: 'MyString',
+ source_accession: 'SAMN00000001',
source_json: {
retrieved_at: Time.now,
- samples: { MyString: { biosample_accessions: %w[MyString AltAccession] } }
+ samples: { SAMN00000001: { biosample_accessions: %w[SAMN00000001 AltAccession] } }
}.to_json
)
assert_includes genome.sequencing_experiments, one
@@ -119,4 +120,132 @@ class GenomeTest < ActiveSupport::TestCase
assert_not genome.can_edit?(nil)
assert genome.can_edit?(users(:contributor))
end
+
+ # --- Accession formats ---
+
+ def accession_errors(attrs)
+ genome = Genome.new(attrs)
+ genome.validate
+ genome.errors
+ end
+
+ test 'accepts valid assembly accessions, single or comma-separated' do
+ [
+ 'GCA_000175575', 'GCF_000175575.2',
+ 'GCA_030737935.1, GCA_030731815.1', 'GCA_030737935.1,GCF_030731815.1'
+ ].each do |acc|
+ errors = accession_errors(database: 'assembly', accession: acc)
+ assert_empty(errors[:accession], "expected #{acc.inspect} to be valid")
+ end
+ end
+
+ test 'rejects malformed assembly accessions' do
+ [
+ 'GCA_000175575.2 GCA_000175576.1', 'GCA_00017557', 'ASM1081187v1',
+ 'GCA_000175575.2,', 'GCA_000175575.2,,GCA_000175576.1', '3445352'
+ ].each do |acc|
+ errors = accession_errors(database: 'assembly', accession: acc)
+ assert_not_empty(errors[:accession], "expected #{acc.inspect} to be invalid")
+ end
+ end
+
+ test 'accepts valid nucleotide accessions, including WGS and RefSeq' do
+ [
+ 'U00096', 'CP036432.1', 'AUEK00000000', 'BAAIHG010000001',
+ 'NC_000913.3', 'NZ_CP012345.1', 'NZ_AUEK01000001',
+ 'CP036432.1, CP036433.1'
+ ].each do |acc|
+ errors = accession_errors(database: 'nuccore', accession: acc)
+ assert_empty(errors[:accession], "expected #{acc.inspect} to be valid")
+ end
+ end
+
+ test 'rejects non-nucleotide or space-separated nuccore accessions' do
+ [
+ 'CP036432 CP036433', 'SRR37301126', 'DRR905474', 'ERS3208922',
+ 'PRJEB100972', 'GWHHOSX00000000.1', 'not yet available', 'cp036432',
+ 'SAMN48198398', 'SAMD00093579', 'SAMN48697936, JBPYXL000000000'
+ ].each do |acc|
+ errors = accession_errors(database: 'nuccore', accession: acc)
+ assert_not_empty(errors[:accession], "expected #{acc.inspect} to be invalid")
+ end
+ end
+
+ test 'suggests the right database when an accession belongs to another' do
+ errors = accession_errors(database: 'nuccore', accession: 'GCA_039803745.1')
+ assert_match(/looks like an NCBI Assembly accession/, errors[:accession].first)
+
+ errors = accession_errors(database: 'assembly', accession: 'AUEK00000000')
+ assert_match(/looks like an INSDC Nucleotide accession/, errors[:accession].first)
+ end
+
+ test 'does not check the format of pending genomes' do
+ errors = accession_errors(database: 'pending', accession: 'anything goes here')
+ assert_empty(errors[:accession])
+ end
+
+ test 'strips leading and trailing whitespace from the accession' do
+ genome = Genome.new(database: 'assembly', accession: " GCA_000175575.2 \n")
+ genome.validate
+ assert_equal('GCA_000175575.2', genome.accession)
+ assert_empty(genome.errors[:accession])
+ end
+
+ test 'existing genomes with legacy accessions can still be updated' do
+ genome = genomes(:one)
+ genome.update_columns(database: 'nuccore', accession: 'GCA_039803745.1')
+
+ assert genome.update(auto_failed: 'Cannot find ncbi:nuccore:GCA_039803745.1')
+ assert_not genome.update(accession: 'GCA_039803745.2')
+ end
+
+ test 'accepts BioSample and SRA Experiment sources, comma-separated' do
+ {
+ biosample: ['SAMN10878311', 'SAMEA6956417, SAMEA6939414', 'SAMD00093579'],
+ sra: ['SRX1234567', 'ERX123456, DRX123456']
+ }.each do |db, list|
+ list.each do |acc|
+ errors = accession_errors(
+ database: 'pending', accession: 'x',
+ source_database: db.to_s, source_accession: acc
+ )
+ assert_empty(errors[:source_accession], "expected #{db}:#{acc} to be valid")
+ end
+ end
+ end
+
+ test 'rejects sources that are not BioSamples or SRA Experiments' do
+ {
+ biosample: ['ERS861224', 'SRS9415155', 'PRJNA384792', 'SAMN15855652 SAMN15855653'],
+ sra: ['SRR12620258', 'SRS18309842', 'SAMN10878311', 'SRP119423']
+ }.each do |db, list|
+ list.each do |acc|
+ errors = accession_errors(
+ database: 'pending', accession: 'x',
+ source_database: db.to_s, source_accession: acc
+ )
+ assert_not_empty(errors[:source_accession], "expected #{db}:#{acc} to be invalid")
+ end
+ end
+
+ errors = accession_errors(
+ database: 'pending', accession: 'x',
+ source_database: 'sra', source_accession: 'SAMN10878311'
+ )
+ assert_match(/looks like an INSDC BioSample accession/, errors[:source_accession].first)
+ end
+
+ test 'rejects unknown source databases' do
+ errors = accession_errors(
+ database: 'pending', accession: 'x',
+ source_database: 'Free text
', source_accession: 'SAMN10878311'
+ )
+ assert_not_empty(errors[:source_database])
+ end
+
+ test 'accession formats can be checked without full validation' do
+ genome = Genome.new(database: 'nuccore', accession: 'GCA_039803745.1')
+ genome.validate_accession_formats
+ assert_not_empty(genome.errors[:accession])
+ end
end
diff --git a/test/models/name_test.rb b/test/models/name_test.rb
index c590bd6f..b0de47a2 100644
--- a/test/models/name_test.rb
+++ b/test/models/name_test.rb
@@ -59,4 +59,28 @@ class NameTest < ActiveSupport::TestCase
'incertae sedis (Bacteria)', name.incertae_sedis_html
)
end
+
+ test 'reports invalid type genome accessions on the type entry field' do
+ name = names(:draft_by_contributor)
+ name.nomenclatural_type_type = 'nuccore'
+ name.nomenclatural_type_entry = 'GCA_039803745.1'
+
+ assert_not name.valid?
+ assert_match(
+ /looks like an NCBI Assembly accession/,
+ name.errors[:nomenclatural_type_entry].first
+ )
+ assert_empty name.errors[:nomenclatural_type_id]
+ assert_not Genome.exists?(database: 'nuccore', accession: 'GCA_039803745.1')
+ end
+
+ test 'accepts valid type genome accessions from the type entry field' do
+ name = names(:draft_by_contributor)
+ name.update!(
+ nomenclatural_type_type: 'assembly',
+ nomenclatural_type_entry: ' GCA_039803745.1 '
+ )
+
+ assert_equal 'GCA_039803745.1', name.reload.nomenclatural_type.accession
+ end
end
diff --git a/test/tasks/genomes_fix_accessions_test.rb b/test/tasks/genomes_fix_accessions_test.rb
new file mode 100644
index 00000000..f59e5d34
--- /dev/null
+++ b/test/tasks/genomes_fix_accessions_test.rb
@@ -0,0 +1,108 @@
+require 'test_helper'
+require 'rake'
+
+class GenomesFixAccessionsTest < ActiveSupport::TestCase
+ setup do
+ Rails.application.load_tasks unless Rake::Task.task_defined?('genomes:fix_accessions')
+ @task = Rake::Task['genomes:fix_accessions']
+
+ # Don't touch any real MiGA project when requeueing
+ Genome.class_eval do
+ alias_method :original_remove_miga!, :remove_miga!
+ define_method(:remove_miga!) { nil }
+ end
+
+ failed = ->(g) { "Cannot find ncbi:#{g[0]}:#{g[1]}" }
+ @genomes = {
+ requeue: ['assembly', 'GCA_000175575', failed],
+ to_asm: ['nuccore', 'GCA_039803745.1', failed],
+ to_nuc: ['assembly', 'AUEK00000000', nil],
+ conflict: ['nuccore', 'GCA_016751895.1', failed],
+ malformed: ['assembly', 'ASM1081187v1', failed],
+ bypassed: ['assembly', 'GCA_000175576.1', 'Genome processing bypassed: Low quality'],
+ orphan: ['nuccore', 'GCA_000175577.1', failed]
+ }.map do |key, (db, acc, fail)|
+ genome = Genome.new(
+ database: db, accession: acc,
+ auto_failed: fail.respond_to?(:call) ? fail.call([db, acc]) : fail,
+ auto_scheduled_at: 1.day.ago, auto_check: key == :to_nuc
+ )
+ genome.save(validate: false) # Legacy data predates the format checks
+ [key, genome]
+ end.to_h
+
+ # Correct twin of the conflicting genome, entered later by the user
+ Genome.new(database: 'assembly', accession: 'GCA_016751895.1')
+ .save(validate: false)
+
+ names = %i[
+ escherichia_coli bacillus_subtilis nanoclepta_minutus luteria_ianthellae
+ escherichia bacillus
+ ]
+ (@genomes.keys - [:orphan]).zip(names).each do |key, name|
+ names(name).update_columns(
+ nomenclatural_type_type: 'Genome',
+ nomenclatural_type_id: @genomes[key].id
+ )
+ end
+ end
+
+ teardown do
+ @task.reenable
+ Genome.class_eval do
+ alias_method :remove_miga!, :original_remove_miga!
+ remove_method :original_remove_miga!
+ end
+ ENV.delete('APPLY')
+ end
+
+ def run_task(apply: false)
+ ENV['APPLY'] = '1' if apply
+ _out, err = capture_io { @task.invoke }
+ @genomes.each_value(&:reload)
+ err
+ end
+
+ test 'dry run reports changes without applying them' do
+ err = run_task
+
+ assert_match(/dry run/, err)
+ assert_match(/#{@genomes[:to_asm].miga_name} .*nuccore:GCA_039803745.1 -> assembly \(requeued\)/, err)
+ assert_match(/Would do: 2 moved, 2 requeued, 1 conflicts, 1 malformed/, err)
+ assert_equal 'nuccore', @genomes[:to_asm].database
+ assert_not_nil @genomes[:requeue].auto_failed
+ end
+
+ test 'applies moves and requeues failed downloads' do
+ err = run_task(apply: true)
+
+ assert_match(/Done: 2 moved, 2 requeued, 1 conflicts, 1 malformed/, err)
+
+ # Valid accession, failed download: requeued
+ assert_nil @genomes[:requeue].auto_failed
+ assert_nil @genomes[:requeue].auto_scheduled_at
+
+ # Wrong database, failed download: moved and requeued
+ assert_equal 'assembly', @genomes[:to_asm].database
+ assert_nil @genomes[:to_asm].auto_failed
+
+ # Wrong database, downloaded fine: moved, MiGA results kept
+ assert_equal 'nuccore', @genomes[:to_nuc].database
+ assert @genomes[:to_nuc].auto_check
+ assert_not_nil @genomes[:to_nuc].auto_scheduled_at
+ end
+
+ test 'leaves conflicts, malformed, bypassed, and orphan genomes alone' do
+ err = run_task(apply: true)
+
+ assert_match(/\? .*nuccore:GCA_016751895.1 also exists as assembly/, err)
+ assert_match(/\? .*assembly:ASM1081187v1 is malformed/, err)
+ %i[conflict malformed bypassed orphan].each do |key|
+ genome = @genomes[key]
+ assert_not_nil genome.auto_failed, "#{key} should not be requeued"
+ end
+ assert_equal 'nuccore', @genomes[:conflict].database
+ assert_equal 'nuccore', @genomes[:orphan].database
+ assert_no_match(/#{@genomes[:orphan].miga_name}\b/, err)
+ end
+end