Make a SpeciesRecord dataclass for species_map records

[adityasingh2400]

Fixes #1867

species_map was a list of loose 4-tuples, which is what @CodyCBakerPhD flagged in the #1866 review. That prerequisite landed on 2026-06-01, so this is now unblocked.

Each entry becomes a frozen SpeciesRecord dataclass with common_names, prefix, uri, and name. __post_init__ enforces the invariants that until now only test_species_map checked: common names and prefix lower-cased, URI an NCBITaxon PURL, and name formatted as {scientific name} - {GenBank common name}. A malformed entry now fails at import rather than only under pytest.

The matching logic moves onto the record as matches_name and matches_common_name, so extract_species reads as the two-pass lookup it already was. The name.partition(" - ") calls are replaced by scientific_name and genbank_common_name properties. The separator element that partition returned could never equal a stripped input, so behavior is unchanged.

Tested with 3 new tests (6 cases with parametrization), all carrying @pytest.mark.ai_generated per CLAUDE.md, plus test_species_map reworked to take records. Full dandi/tests/test_metadata.py run gives 142 passed, 1 xfailed, 8 xpassed.

Since this is a refactor, an import failure alone would be weak evidence, so the behavior change was checked directly against the base ref. On master a malformed entry is accepted silently. On this branch it is rejected with Common name 'Mouse' of http://example.com/not-ncbitaxon must be lower-cased.

AI assistance disclosure: this change was written with the help of Claude Code, and the added tests are marked ai_generated as CLAUDE.md asks. I reviewed and tested everything before submitting.

[codecov]

Codecov Report

❌ Patch coverage is 77.04918% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.91%. Comparing base (2b5f8ea) to head (a1c4230).

Files with missing lines	Patch %	Lines
dandi/metadata/util.py	61.11%	14 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1899      +/-   ##
==========================================
- Coverage   76.96%   76.91%   -0.05%     
==========================================
  Files          88       88              
  Lines       12882    12927      +45     
==========================================
+ Hits         9914     9943      +29     
- Misses       2968     2984      +16     

Flag	Coverage Δ
unittests	76.91% <77.04%> (-0.05%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[yarikoptic]

THANK YOU for the PR @adityasingh2400 ! Overall looks good and worth pursuing. With claude we seems have identified a number of concerns which I will post now. See (with your claude ;) ) the about to be posted review.

[yarikoptic-gitmate]

Nice refactor — this is the right shape for #1867, and moving the matching onto the record makes extract_species read as the two-pass lookup it always was. I checked it out and verified the behavior-preservation claim rather than taking it on faith:

• pytest dandi/tests/test_metadata.py → 143 passed, 1 skipped, 8 xpassed
• flake8 clean; black --check unchanged
• mypy: 5 errors, all missing-dateutil-stub errors in dandi/utils.py, dandi/tests/fixtures.py, and dandi/tests/test_metadata.py:31 — identical set on master, so nothing introduced
• Matching equivalence: I reimplemented the old partition-based predicate and diffed it against matches_name/matches_common_name over every name, name-half, common name and prefix in the map, plus edge inputs ("", " ", "-", " - ", case and whitespace variants) → 0 mismatches

Your reasoning about partition's separator element is right, and for a second reason worth stating: partition(" - ")[1] is always " - ", and lower_value is .strip()ed, so it can never match regardless of the data.

Requesting changes on one class of issue: __post_init__ is now the sole guardian of these invariants, but it accepts several malformed entries that are worse than the ones it rejects. Since the stated goal is "a malformed entry now fails at import rather than only under pytest," it's worth closing those before this lands.

Must fix

1. prefix="" and common_names=("",) are accepted, and either one breaks the whole table

SpeciesRecord(("mouse",), "", uri, "Mus musculus - House mouse")   # ACCEPTED
SpeciesRecord(("",), "mus", uri, "Mus musculus - House mouse")     # ACCEPTED

"" == "".lower() passes the lower-case check, and value.startswith("") is True for every input — so that record matches everything, and every other lookup starts failing with "Got multiple (N) species matched … Should not happen." Verified: an empty-prefix record returns True for matches_name("zebrafish").

This is a bigger hole than anything __post_init__ currently catches. Please reject empty prefix and empty entries in common_names.

2. common_names type isn't enforced, and the likely mistake is silent

Five of the thirteen entries are single-element tuples, so the realistic error is a dropped trailing comma:

SpeciesRecord(("mouse"), "mus", ...)   # common_names == "mouse", a str

That constructs, passes __post_init__ (iterating a lowercase string yields lowercase chars), and is hashable — no loud failure anywhere. I patched it in and ran it:

'm'      -> Mus musculus - House mouse
'e'      -> Mus musculus - House mouse
'mouse'  -> ERROR: Cannot interpret species field: mouse

Single letters resolve to mouse; the real common name stops resolving entirely. And test_species_map passes anyway, because chain(...) iterates the string's characters and every one of them now "matches."

The one thing that caught it was assert isinstance(record.common_names, tuple) in your new test_species_map_entries_are_records — so that assertion is genuinely load-bearing, please keep it. But it should be a guarantee rather than a spot-check: either validate the type in __post_init__, or normalize with object.__setattr__(self, "common_names", tuple(self.common_names)). Worth a case in test_species_record_rejects_malformed_entry too, which currently has no bad-common_names-type case.

3. The URI check is prefix-only

if not self.uri.startswith(NCBITAXON_URI_TEMPLATE.format("")):

All of these are accepted: NCBITaxon_, NCBITaxon_abc, NCBITaxon_9606/junk, NCBITaxon_9606extra. That matters because extract_species matches incoming URIs with NCBITaxon_([0-9]+), so a non-numeric entry constructs fine and is then silently unreachable via the URI path.

Suggest validating the taxon id is numeric — but please don't do it with a second hardcoded copy of the URL, which would defeat the point of NCBITAXON_URI_TEMPLATE. Cleanest is probably to store the numeric id on the record and derive uri from the template.

4. The comment on the hash assertion is inaccurate

# frozen dataclasses are hashable, which `extract_species` relies on
# indirectly when de-duplicating matches
assert hash(record) == hash(record)

extract_species de-duplicates list(set(value_matches)) where the elements are (uri, name) string tuples — records are never hashed, so the stated dependency doesn't exist.

On the assertion itself: it isn't reading a cached value (the generated __hash__ rebuilds and rehashes the field tuple on every call), but with all fields being str/tuple[str, ...]/None it can only ever fail by raising, which a bare hash(record) detects identically. And the hash isn't reproducible across runs — PYTHONHASHSEED randomizes str hashing, and nothing here pins it or needs it to be stable.

The eq/hash contract is worth locking in, so rather than dropping the line, suggest strengthening it to two distinct-but-equal instances:

copy = dataclasses.replace(record)
assert copy is not record
assert copy == record and hash(copy) == hash(record)

That version additionally catches eq=False or a hand-rolled identity __hash__, which the current form passes silently.

Should fix

5. matches_name / matches_common_name are public with an unenforced case contract

species_map[0].matches_name("Mus musculus")     # False
species_map[0].matches_common_name("Mouse")     # False

The refactor promoted an inline predicate that always received pre-normalized input into public API that doesn't normalize, with only docstring prose to warn callers. Either normalize inside the methods, or make them _matches_name/_matches_common_name.

6. No direct tests of the two methods the refactor exists to create

The new tests cover the name-half properties and the four rejection paths, but matches_name's prefix branch is exercised only transitively through extract_species. A direct test would also usefully document the surprising bit — matches_name("mushroom") is True for Mus musculus.

7. __post_init__ accepts degenerate names

" - Human" and "Human - " both pass. The first yields scientific_name == "", which makes matches_name("") return True — and extract_species guards value_orig != "" but not whitespace-only, so {"species": " "} reaches it. Not live with the current data, but the check is "separator present," not "both halves non-empty."

8. Docstring nit

matches_name's "or its prefix" reads as equality; it's startswith. The class docstring gets this right — worth matching. Also worth a one-line note on genbank_common_name that it relies on __post_init__ having validated the separator, since it would IndexError otherwise.

Optional

• Cross-record invariants are the ones that actually reach users. Nothing checks for duplicate uris or a prefix that matches another record. Adding prefix="ma" to Macaca mulatta, for example, breaks radiata and nemestrina (mulatta itself keeps resolving, since the conditions OR within one record). No collisions exist today — this is hardening, not a live bug — but a module-level check or an extra test would cover a class of mistake that per-record lower-casing doesn't.
• species_map is still a mutable list of frozen records; species_map: tuple[SpeciesRecord, ...] would match the intent.
• In test_species_map, assert key.lower() == key and the separator assertion can no longer fail — __post_init__ guarantees them at import. Worse, the first also passes for the bare-string case in #2, so it's misleading about what it protects.
• matches_common_name could just be value in self.common_names.

None of this changes the verdict — the refactor is behavior-preserving and the validation is a real improvement over what only test_species_map checked before. Mostly it's that __post_init__ should reject the entries that would do the most damage.

---

Generated by Claude Code