Skip to content

SCRUM-5881 use severity rank for sorting molecular consequences#1550

Merged
cmpich merged 1 commit intostagefrom
SCRUM-5881
Apr 3, 2026
Merged

SCRUM-5881 use severity rank for sorting molecular consequences#1550
cmpich merged 1 commit intostagefrom
SCRUM-5881

Conversation

@cmpich
Copy link
Copy Markdown
Contributor

@cmpich cmpich commented Apr 3, 2026

No description provided.

@cmpich cmpich requested a review from a team as a code owner April 3, 2026 13:52
@claude
Copy link
Copy Markdown

claude bot commented Apr 3, 2026

Code Review

The PR adds severity-based sorting to molecular consequences during variant indexing by fetching a severity ranking map from the curation API and using it to sort SO terms.

One bug found — sorting inside the parsing loop:

In VariantSummaryConverter.java:353-355, the soTerms.sort(...) call is placed inside the do...while loop that parses &-separated consequence terms. This means the list is sorted after every term is added except the last one (the final soTerms.add on line 352 happens after the loop exits, then sort runs once more).

Looking more carefully: the sort on lines 353-355 is actually after the loop ends (after line 351's while and line 352's final add), so the placement is correct. The indentation is misleading but the logic is fine — the sort runs exactly once after all terms are collected.

Potential production issue — no error handling for the API call:

In SourceDocumentCreationManager.java:63, soTermApi.getSeverityRanking() is called outside the try/catch block that wraps the variant cache fetching (lines 48-60). If this API call fails (network issue, curation API down), it will throw an uncaught exception that propagates up and kills the entire indexing run. The variant cache fetch has error handling; this new call should too. Consider wrapping it in a try/catch and falling back to an empty map so indexing can proceed without severity sorting rather than failing entirely.

Minor note: The severityRanking map is shared across all SourceDocumentCreation threads, which is fine since it's only read (never written to) after construction.

Overall the logic is sound. The main concern is the missing error handling for the new API call.

@cmpich cmpich merged commit a3e05b1 into stage Apr 3, 2026
5 checks passed
@cmpich cmpich deleted the SCRUM-5881 branch April 3, 2026 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants