Repository navigation
Conversation
The label validator was the only caller of sanitize_target_sequence_label, which lived in lib/identifiers.py with the publication code. The validator now does the cleanup itself, as the sequence validator beside it does. The test compared the label with the helper's own output, so it passed whatever the helper did. It now asserts the cleaned label.
The create and update models declared sequenceType as a plain string, so clients couldn't see the accepted values. The schema now lists them, built from valid_sequence_types so the two can't drift. The validator still checks the value with its own message and accepts any case. The enum is on the input models only. "infer" is never stored, so the response models keep a plain string.
lib/identifiers.py served the shared identifier tables. Now that DOIs, raw reads and target gene identifiers are stored on the rows that use them, nearly all that's left is publication code, under a name that doesn't say so. Each piece moves, unchanged, to the domain it belongs to: - ExternalPublication and the fetch functions go to lib/external_publications.py, beside the Crossref and Rxiv clients. - Finding and storing publications goes to a new lib/publications.py. - target_gene_external_identifiers goes to lib/target_genes.py, and distinct_identifiers to lib/utils.py. Callers import from the new modules, and test_identifiers.py becomes test_publications.py.
Citations referenced a publication by its numeric id, so writing one meant reading the publication first. Under row-level security (#808) a publication is visible only through a citation the caller can see, so that read fails for a publication cited only by others' private records, and for a brand-new one. Score set, experiment and calibration citations now reference (db_name, identifier), a foreign key to a new unique key on publication_identifiers. Foreign key checks ignore row security. ensure_publication_keys resolves every list a record cites in one pass and stores new publications with ON CONFLICT DO NOTHING, which skips a row whether or not the caller can see it. Citations are built from the keys, so the api never reads a publication to cite it. The unused experiment_set_publication_identifiers table is dropped. A publication database failure now raises PublicationServiceError (or PublicationServiceTimeoutError) where the request goes out. One handler maps it to 502 or 504 with the `detail` the ui shows. This replaces per-route try/except blocks, which the calibration routes and publication search lacked, so those returned 500 when a publication database failed. Also fixes along the way: - A publication listed twice in one submission, such as a PMID and its url, is cited once instead of colliding on the primary key. - A partial score set update keeps its primary publication. It read the flag from a temporary attribute on the shared publication object, which publications loaded from the database don't have.
primary on score set and experiment citations was nullable, and some legacy rows hold NULL. Reads already treat NULL as secondary, and a record that lists one publication in both lists cites it once as primary, so NULL never meant anything else. The migration stores NULL as false and makes the column NOT NULL with a false default, so the column says what reads assume.
Two requests reached a ValueError in lib/score_calibrations.py and returned 500: - An admin deleting a primary calibration. Admins may delete any calibration, and the delete helper refuses primaries. The route now answers 400 and asks for a demotion first. Other users still get the 403 from the permission check. - A PUT without scoreSetUrn. The route treated it as optional, but the update replaces the calibration and the modify helper requires it. The route now answers 422, as the create route does.
Most calibration routes declared only 404, which the router already declares for every route, so OpenAPI left out the 400, 401, 403 and gateway errors they return. Each route now declares what it can return: access control errors where it needs a login or a permission, 400 where it rejects a request, and gateway errors where it reaches the publication or disease services. Behaviour doesn't change.
When NCBI failed, we told the user their PMID didn't exist. eutils raises EutilsRequestError for an error with a JSON body, such as a rate limit (429), and a global handler turned that into a 404. An error page it can't parse becomes EutilsNCBIError, which fetch_pubmed_article read as "no article". Either way a valid PMID looked missing, and the lookup stopped before trying the other publication databases. NCBI answers an unknown PMID with an empty article set, not an error. Identifiers are validated before any fetch, so every eutils error here means PubMed failed. Both now raise PublicationServiceError, which answers 502 and lets the lookup try the other databases. The EutilsRequestError handler had no other use and is removed.
The Crossref and Rxiv clients called requests.get with no timeout, so a database that accepted the connection and then stalled held the request open indefinitely, and the 504 for a publication database timeout could only fire on a failed connection. Both clients now wait 10 seconds. eutils, which fetches from PubMed, doesn't take a timeout.
bencap
added
app: backend
Coverage Report for CI Build 40575053571Warning No base build found for commit Coverage: 90.728%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
View models import PublicationAuthors from lib/external_publications.py, and they must import without the server extras. The publication service error helper imported the request logging context, which needs FastAPI and Starlette, so view model tests failed to collect in CI's core-dependencies job. The helper now logs without the request context. The exception handler already records the exception in that context, so the helper's own save_to_logging_context call was redundant.
bencap
marked this pull request as ready for review
October 30, 2026 23:41
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Citations referenced a publication by its numeric id, so writing one meant reading the publication first. Under row-level security (#808), a caller can only see a publication through a citation they can already see, so that read fails for a publication cited only by others' private records, and for a brand-new one. Score set, experiment and calibration citations now reference the publication's natural key,
(db_name, identifier), so the api never reads a publication to cite it.Stacked on #949. Review the last nine commits; GitHub retargets this to
release-2026.3.3once #949 merges.ensure_publication_keysresolves every list a record cites in one pass and stores new publications withON CONFLICT DO NOTHING. The citations reference the key through a foreign key, and foreign key checks ignore row security.PublicationServiceErrorwhere the request goes out, and one handler answers 504 or 502. This replaces per-routetry/exceptblocks, which calibration saves and publication search lacked, so those returned 500. PubMed rate limits and outages were reported as a missing PMID (404); they're now a 502. Crossref and bioRxiv requests time out after 10 seconds.lib/identifiers.pyis split by domain intolib/publications.py,lib/external_publications.py,lib/target_genes.pyandlib/utils.py. That commit moves code without changing it.scoreSetUrngets 422; both were 500s. Each route'sresponses=now lists what it actually returns.primaryis non-null. The target sequence label cleanup is inlined, and its test asserts the result.Migrations:
d0c23896b0c5adds a unique(db_name, identifier)topublication_identifiers, rewrites the three citation tables to reference it, and drops the emptyexperiment_set_publication_identifierstable, refusing if it has rows.e59e1cc49c9cstoresNULLcitationprimaryvalues asfalseand makes the columnNOT NULL.Both ran cleanly on a production-scale copy from 2026-10-16, with no duplicate keys.
API schema changes: the target sequence create and update models list the accepted
sequenceTypevalues. The calibration routes declare their error responses. Publication gateway errors answer withdetail, as the ui's record editors expect. VariantEffect/mavedb-ui#760 regeneratessrc/schema/openapi.d.ts.Closes #945
Tests
bin/checkand the PR test set (bin/t tests/ -n 6 -m "not network and not slow"): 5,508 passed, 22 skipped. Each commit passesbin/checkand its targeted tests on its own.e59e1cc49c9cdown and up again, on a scratch database.Checklist
release-<year>.<n>.0branch otherwise; never
main. Stacked on feat(db): store DOI, raw read and target gene identifiers on the rows that use them #949, which targetsrelease-2026.3.3mavedb-uiPR regeneratessrc/schema/openapi.d.ts: chore(schema): regenerate API types for publication natural-key citations mavedb-ui#760