Skip to content

Reject ontology accessions written under a short source prefix (U:/M:/R:)#117

Closed
trishorts wants to merge 1 commit into
topdownproteomics:masterfrom
trishorts:validate/source-prefix-accession-form
Closed

Reject ontology accessions written under a short source prefix (U:/M:/R:)#117
trishorts wants to merge 1 commit into
topdownproteomics:masterfrom
trishorts:validate/source-prefix-accession-form

Conversation

@trishorts

Copy link
Copy Markdown
Contributor

Summary

[U:35], [M:00719], and [R:AA0581] are listed in the ProForma 2.0 specification as invalid counter-examples: an ontology accession must use the full prefix (UNIMOD:, MOD:, RESID:), whereas the short prefixes (U:, M:, R:) denote modification names. The parser currently accepts these as a name (e.g. a Unimod modification literally named "35"), so the three spec counter-examples round-trip instead of being rejected.

This PR rejects a name value that is written in the accession format of its ontology:

new ProFormaParser().ParseString("EM[U:35]EVEES[U:56]PEK");       // now throws ProFormaParseException
new ProFormaParser().ParseString("EM[M:00719]EVEESPEK");          // now throws
new ProFormaParser().ParseString("EM[R:AA0581]EVEESPEK");         // now throws

new ProFormaParser().ParseString("EM[U:Oxidation]EVEES[U:Phospho]PEK");   // still valid (real name)
new ProFormaParser().ParseString("EM[U:+15.9949]EVEESPEK");              // still valid (source-tagged mass)

Detection is deliberately narrow — Unimod/PSI-MOD: all-digits; RESID: AA + digits — matching the three documented counter-examples, and only applies to Name descriptors (masses and real names are untouched). The same idea could extend to X: (XLMOD) / G: (GNO) if desired.

🟨 This is a policy/judgment call — your decision

This is not a clear-cut bug — it's a question of where validation belongs. The parser is otherwise syntactic, and [U:35] is syntactically well-formed (a name "35"); its invalidity is semantic (no such name exists), which a downstream resolver/validator would also catch. We're happy to:

  • merge as-is (parser rejects accession-shaped values under short prefixes), or
  • scope it differently (e.g. behind an option, or limited to specific ontologies), or
  • close it if you'd rather keep this check in a validation layer rather than the parser.

Whatever you prefer. The implementation is small and self-contained either way.

Tests

  • RejectAccessionUnderShortSourcePrefix — the three spec counter-examples now throw.
  • AllowNamesAndMassesUnderShortSourcePrefix — real names and source-tagged masses still parse.
  • Full suite green: 235 passed, 7 skipped, 0 failed (net472 + net6.0).

🤖 Generated with Claude Code

https://claude.ai/code/session_01JNnpzBNwfnTbZxtZdtAzLG

A short source prefix (U:, M:, R:) denotes a modification name. An accession
written under it -- "U:35", "M:00719", "R:AA0581" -- is the full-prefix accession
(UNIMOD:/MOD:/RESID:) used with the wrong prefix, which ProForma 2.0 lists as
invalid; the parser previously accepted it as a (non-existent) name.

Reject a name value that is in the accession format of its ontology (Unimod and
PSI-MOD: all digits; RESID: AA followed by digits). Real names and source-tagged
masses under the short prefix are unaffected. Adds parser tests for the three
spec counter-examples and for the valid name/mass forms.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JNnpzBNwfnTbZxtZdtAzLG
@rfellers

Copy link
Copy Markdown
Member

I believe that there is a validation layer after the parsing layer that would catch these issues. For example, the parser would accept "EM[MOD:99999]" even though it isn't a real mod accession. So, as the PR says, "EM[U:35]" is technically valid syntax but is likely not what the user intended and would fail downstream. Your PR is likely a fair thing to add but is technically a validation concern. In your client code, how are you planning to handle cases like [MOD:99999]?

@trishorts trishorts closed this Jun 30, 2026
trishorts pushed a commit to trishorts/sdk that referenced this pull request Jun 30, 2026
The Build and Test job failed on all open PRs (topdownproteomics#116, topdownproteomics#117, topdownproteomics#118) because
the final Codecov step errored on upload (exit 4294967295) while
fail_ci_if_error was true, marking the whole job failed even though
restore, build, and tests all passed (235 passed, 0 failed).

- fail_ci_if_error: true -> false so a Codecov-side hiccup no longer
  fails the build
- fix coverage file path: coverage.net6.0.xml -> coverage.xml to match
  the Coverlet output (/p:CoverletOutput='../../coverage.xml')
- bump checkout/setup-dotnet/codecov-action v3 -> v4 to clear the
  Node 20 deprecation warnings

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants