feat(prepro): use submissionId for displayName if specimenCollectorSampleId cannot be parsed by maverbiest · Pull Request #6774 · loculus-project/loculus

maverbiest · 2026-06-29T07:44:17Z

resolves #6763

build_display_name now attempts to use the submissionId as the identifier field in displayName when specimenCollectorSampleId is present but cannot be parsed (see extended description in the issue).

Additionally, this PR extracts the identifier string parsing logic into its own function.

PR Checklist

All necessary documentation has been adapted.
The implemented feature is covered by appropriate, automated tests.
Any manual testing that has been done is documented (i.e. what exactly was tested?)

🚀 Preview: Add preview label to enable

maverbiest · 2026-06-29T09:09:41Z

@claude review

claude · 2026-06-29T09:09:57Z

Claude finished @maverbiest's task in 2m 37s —— View job

Code Review

Gather context
Read changed files
Provide review feedback

Overall this is a clean, well-structured PR. The feature works correctly and the extraction of parse_identifier_string into its own function is a good refactor. A few observations:

What's good

The two-step fallback at lines 1665–1667 is minimal and readable:

identifier = parse_identifier_string(collector_id, insdc_ingested, pattern)
if identifier is None:
    identifier = parse_identifier_string(submission_id, insdc_ingested, pattern)

parse_identifier_string is a pure function with no side effects, easy to test in isolation.
The docstring on build_display_name has been updated to accurately describe the three-level fallback (specimenCollectorSampleId → submissionId → ACCESSION_VERSION).
Test coverage captures the key scenarios: direct use, INSDC path, regex extraction, and the new fallback behaviour.

Issues / Suggestions

1. Warning message is misleading when both identifiers fail

processing_functions.py:1676–1690

failed_source = (
    collector_id
    if isinstance(collector_id, str) and collector_id
    else submission_id
)
warnings.append(... message=(
    f"identifier string '{failed_source}' could not be parsed,"
    " using ACCESSION_VERSION in displayName instead"
))

Now that submissionId is also tried before falling back to ACCESSION_VERSION, this warning only surfaces one of the two attempted values. If specimenCollectorSampleId is a non-empty string that fails parsing, the warning blames it alone — but submissionId was also tried (and also failed). A user seeing this warning wouldn't know to check their submissionId too.

A more accurate message would be:

message=(
    f"Neither specimenCollectorSampleId ('{collector_id}') nor submissionId ('{submission_id}') "
    "could be parsed, using ACCESSION_VERSION in displayName instead"
)

Or, if conciseness is preferred, at minimum note that the fallback was also attempted:

message=(
    f"specimenCollectorSampleId '{collector_id}' could not be parsed "
    "and submissionId was not usable as an identifier, "
    "using ACCESSION_VERSION in displayName instead"
)

Fix this →

2. Minor docstring typo in parse_identifier_string

processing_functions.py:2171

"""Return a IDENTIFIER string to use in the displayName...

Should be "an IDENTIFIER".

3. Test state mutation between sub-cases

test_metadata_processing_functions.py:1257

test_display_name_construction mutates input_data in place throughout the function (e.g. setting specimenCollectorSampleId and geoLocCountry at lines 1317, 1329, 1341, 1353). This is a pre-existing pattern in the file, but it means each sub-case depends on the accumulated state of previous ones. It makes the test harder to read in isolation and fragile if sub-cases are ever reordered. Consider defining a fresh input_data dict per sub-case, or at least resetting fields explicitly.

4. Missing explicit test for submissionId fallback with clean simple ID

The test at line 1341–1351 does cover the new fallback (when specimenCollectorSampleId = "hDENV1/myExtractedSample/2025" fails and submissionId = "mySample" is used). That's correct. But there's no test for the edge case where specimenCollectorSampleId has forbidden chars AND a regex_pattern is configured, but the regex fails extraction — and submissionId is a clean string that gets returned directly without regex. That path hits the not has_forbidden_char → return input branch in parse_identifier_string, which is subtly different from the case at line 1349 (where the collector_id had forbidden chars → regex tried → failed → fallback). Worth a targeted test if you want full branch coverage of parse_identifier_string.

Summary: One real correctness concern (the warning message only reflects one of two attempted sources), one typo, and two style/robustness notes. Nothing blocks the feature from working correctly.

specimenCollectorSampleId

anna-parker · 2026-06-29T15:13:14Z

-                    )
-                identifier = extract_result.datum
+        insdc_ingested = bool(args["is_insdc_ingest_group"])
+        pattern = str(regex_pattern) if regex_pattern is not None else None


this shouldnt be needed you should be able to just use regex_pattern - it is of type string| None

regex_pattern is of type ArgValue which is very broad:

loculus/preprocessing/nextclade/src/loculus_preprocessing/datatypes.py

Lines 17 to 19 in 36b77c9

ArgValue = (

list[str] | str | bool | int | float | None

) # Value of an argument passed to processing_functions

So we do need to narrow to str or None first

anna-parker · 2026-06-29T15:18:32Z

@@ -1658,47 +1661,35 @@ def build_display_name(  # noqa: C901
        def replace_identifier(values, replacement):


maybe we could move this helper function to where it is used

anna-parker · 2026-06-29T15:22:44Z

        collector_id = input_data.get("specimenCollectorSampleId", None)
        submission_id = input_data.get("submissionId", None)
        warnings: list[ProcessingAnnotation] = []
        if submission_id is None:


tbf we can actually probably drop this check, the build_display_name uses "ACCESSION_VERSION" as the ultimate fallback value - if anything we could throw an error if "ACCESSION_VERSION" isnt passed by this would be an internal preprocessing error which is handled.

corneliusroemer · 2026-06-29T16:34:05Z

+                            f"specimencollectorSampleId '{collector_id}' and submissionId"
+                            f" '{submission_id}' could not be parsed, using ACCESSION_VERSION"
+                            f" in displayName instead"


As this shows to users it should be clear for users what the problem is and how to fix it - direct submitters can then potentially fix it. Ideally:

why it didn't parse

what format is legal, maybe link to some docs

corneliusroemer · 2026-06-29T16:35:50Z

+    """
+    if not isinstance(input, str):
+        return None
+    has_forbidden_char = " " in input or "/" in input


I don't remember: Might this contain whitespace other than space or do we sanitize this in backend already? Things like tab, newline

good point! LAPIS allows us to download tsv with TSV-ESCAPED but in my understanding the backend doesnt do this!

submissionId goes through getValueAndValidateNoWhitespace in the backend, which should guard against all whitespace:

loculus/backend/src/main/kotlin/org/loculus/backend/utils/MetadataEntry.kt

Lines 99 to 104 in 36b77c9

if (fieldValue.any { it.isWhitespace() }) {

throw UnprocessableEntityException(

"Record #$recordNumber in the metadata file: the value for '$fieldName' contains whitespace: '$fieldValue'",

)

}

return fieldValue

But I don't think the specimenCollectorSampleId goes through this. Might be safer to check for all whitespace (although I haven't seen this in practice yet)

corneliusroemer · 2026-06-29T16:37:13Z

+    """
+    if not isinstance(input, str):
+        return None
+    has_forbidden_char = " " in input or "/" in input


Should we also forbid | ? Why only and /?

Sometimes | is used causing things like Argentina/Argentina|XYZ|2026/2026

Unsure specifically why we have an issue with . Our aim is not to avoid ever having redundancy, which I guess is why | is allowed, but yeah one can argue that the fact that this will mess up FASTA parsing is an argument to at least sanitise it downstream (and maybe same with / (and ) instead of banning it`).

we dont want to have " " in the displayName, but I guess that can always be replaced with "_" in a str.replace() step.

I think @maverbiest tried parsing "|" as well at some point, I cant recall why it wasnt added (maybe bad results), I also would have thought it should be included

Indeed we didn't want " " in the displayName. My recollection if that we only check for '/' because that's the separator we use in the displayName ourselves. We didn't include other separators because we couldn't decide on where to draw the line (|, -, _, ..., also what do we do if there's multpile types?)

corneliusroemer · 2026-06-29T16:39:48Z

+    has_forbidden_char = " " in input or "/" in input
+
+    if insdc_ingested:
+        # For INSDC ingested sequences: use the value as-is unless it contains a ' ' or '/'


Why don't we parse INSDC ingested? Was this found to be problematic?

I think the order of fields was just so all over the place that we decided to just not try to extract the submissionId using regex

This parsing was intended as a service to direct submitters so they wouldn't have to reformat their identifiers. E.g., if someone already formatted their identifiers to submit to a different DB that expects a certain format, we could try to parse out the relevant field to use

(also INSDC was a mess)

claude Bot added the preprocessing Issues related to the preprocessing component label Jun 29, 2026

anna-parker self-requested a review June 29, 2026 08:07

tombch self-requested a review June 29, 2026 08:08

maverbiest added 4 commits June 29, 2026 15:36

Extract identifier parsing logic, fall back to submissionId from

fe2d9b8

specimenCollectorSampleId

Add back re.compile check

e0ac8f0

Type, update warning message

6b8c61f

Fix tests

bb7ccc8

maverbiest force-pushed the display-name-fallback branch from 64d83cb to bb7ccc8 Compare June 29, 2026 13:36

anna-parker reviewed Jun 29, 2026

View reviewed changes

anna-parker mentioned this pull request Jun 29, 2026

feat(prepro): make tests more readable #6790

Open

3 tasks

corneliusroemer reviewed Jun 29, 2026

View reviewed changes

Add Anya's suggested test rework. Dont use submissionId for INSDC

87ac33e

	ArgValue = (
	list[str] \| str \| bool \| int \| float \| None
	) # Value of an argument passed to processing_functions

		@@ -1658,47 +1661,35 @@ def build_display_name( # noqa: C901
		def replace_identifier(values, replacement):

	if (fieldValue.any { it.isWhitespace() }) {
	throw UnprocessableEntityException(
	"Record #$recordNumber in the metadata file: the value for '$fieldName' contains whitespace: '$fieldValue'",
	)
	}
	return fieldValue

Uh oh!

Conversation

maverbiest commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Uh oh!

maverbiest commented Jun 29, 2026

Uh oh!

claude Bot commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review

What's good

Issues / Suggestions

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maverbiest Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corneliusroemer Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theosanderson Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

maverbiest commented Jun 29, 2026 •

edited

Loading

claude Bot commented Jun 29, 2026 •

edited

Loading

maverbiest Jun 30, 2026 •

edited

Loading

corneliusroemer Jun 29, 2026 •

edited

Loading

theosanderson Jun 29, 2026 •

edited

Loading