From dc797b76be26faf9009aba299661253e2b220cce Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Thu, 22 Oct 2026 11:41:58 -0700 Subject: [PATCH 01/10] test: stop the target gene statistics test mutating shared constants The target gene identifier statistics test updated the first target of TEST_MINIMAL_ACC_SCORESET and TEST_MINIMAL_SEQ_SCORESET in place, so any score set test that ran later in the same process created its score set with three external identifiers and failed its response comparison. Full-suite runs collect test_score_set.py before test_statistics.py, so CI never ran a score set test after the mutation. Listing the files in another order, or a different xdist split, did. The test now deep-copies the target and passes it as a targetGenes update. --- tests/routers/test_statistics.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/tests/routers/test_statistics.py b/tests/routers/test_statistics.py index 6618eaf7..1d0dab10 100644 --- a/tests/routers/test_statistics.py +++ b/tests/routers/test_statistics.py @@ -1,5 +1,7 @@ # ruff: noqa: E402 +from copy import deepcopy + import pytest from humps import camelize from unittest.mock import patch @@ -241,14 +243,16 @@ def test_target_gene_field_statistics_seq(client, field, desired_field_value, se # Target gene identifier based statistics behave similarly enough to parametrize. @pytest.mark.parametrize("field_value", TARGET_GENE_IDENTIFIER_FIELDS) @pytest.mark.parametrize( - "target", (TEST_MINIMAL_ACC_SCORESET["targetGenes"][0], TEST_MINIMAL_SEQ_SCORESET["targetGenes"][0]) + "score_set_template", (TEST_MINIMAL_ACC_SCORESET, TEST_MINIMAL_SEQ_SCORESET), ids=["accession", "sequence"] ) -def test_target_gene_identifier_statistiscs( - session, data_provider, client, setup_router_db, data_files, field_value, target +def test_target_gene_identifier_statistics_published_score_set_counts_its_identifiers( + session, data_provider, client, setup_router_db, data_files, field_value, score_set_template ): """Test target gene identifier statistics endpoint for published score sets.""" - record_update = {"externalIdentifiers": [v for v in EXTERNAL_IDENTIFIERS.values()]} - target.update(record_update) + # Copy the target rather than updating the shared constant, which later tests in this process would inherit. + target = deepcopy(score_set_template["targetGenes"][0]) + target["externalIdentifiers"] = list(EXTERNAL_IDENTIFIERS.values()) + score_set_update = {"targetGenes": [target]} # Create whichever score set target type is being tested. experiment = create_experiment(client) @@ -256,13 +260,13 @@ def test_target_gene_identifier_statistiscs( with patch.object( cdot.hgvs.dataproviders.RESTDataProvider, "_get_transcript", return_value=TEST_NT_CDOT_TRANSCRIPT ): - unpublished_score_set = create_acc_score_set(client, experiment["urn"]) + unpublished_score_set = create_acc_score_set(client, experiment["urn"], score_set_update) unpublished_score_set = mock_worker_variant_insertion( client, session, data_provider, unpublished_score_set, data_files / "scores_acc.csv" ) elif "targetSequence" in target: - unpublished_score_set = create_seq_score_set(client, experiment["urn"]) + unpublished_score_set = create_seq_score_set(client, experiment["urn"], score_set_update) unpublished_score_set = mock_worker_variant_insertion( client, session, data_provider, unpublished_score_set, data_files / "scores.csv" ) From 75ae7e24fbf60d9f9f38660f4521b00d4ed5d544 Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Thu, 22 Oct 2026 16:35:43 -0700 Subject: [PATCH 02/10] refactor(models): move ValidTime into models/mixins Model mixins lived in db/mixins.py, and the external identifier base for #944 needs a home too. Splitting mixins between db/ and models/ by whether they're MaveDB-specific would leave two places named mixins and a judgment call for each new one. Every model mixin now lives in models/mixins/.py, and db/ keeps what every table builds on: Base, MaterializedView and sessions. The database rule says so, and now loads for db/ too. ValidTime's code is unchanged apart from its module docstring. --- .claude/rules/database.md | 2 ++ src/mavedb/models/clinvar_allele_link.py | 2 +- src/mavedb/models/gnomad_allele_link.py | 2 +- src/mavedb/models/mapping_record.py | 2 +- src/mavedb/models/mapping_record_allele.py | 2 +- src/mavedb/models/mixins/__init__.py | 3 +++ src/mavedb/{db/mixins.py => models/mixins/valid_time.py} | 2 +- src/mavedb/models/projection_group.py | 2 +- src/mavedb/models/vep_allele_consequence.py | 2 +- tests/models/mixins/__init__.py | 0 tests/{db/test_mixins.py => models/mixins/test_valid_time.py} | 2 +- 11 files changed, 13 insertions(+), 8 deletions(-) create mode 100644 src/mavedb/models/mixins/__init__.py rename src/mavedb/{db/mixins.py => models/mixins/valid_time.py} (99%) create mode 100644 tests/models/mixins/__init__.py rename tests/{db/test_mixins.py => models/mixins/test_valid_time.py} (99%) diff --git a/.claude/rules/database.md b/.claude/rules/database.md index 74f29ab8..db630809 100644 --- a/.claude/rules/database.md +++ b/.claude/rules/database.md @@ -1,5 +1,6 @@ --- paths: + - "src/mavedb/db/**" - "src/mavedb/models/**" - "src/mavedb/lib/**" - "src/mavedb/routers/**" @@ -12,3 +13,4 @@ paths: - Write new queries in SQLAlchemy 2.0 `select()` style. Older code still uses `db.query()`; don't copy it. - ValidTime tables never return only live rows by default. Filter with `Model.current` or `Model.live_at(...)`. - Replace ValidTime rows with `supersede_with` or `supersede_live_where`, never a separate retire-then-insert, so the handoff has no gap. +- Mixins shared by models live in `models/mixins/.py`, such as `ValidTime`. `db/` holds what every table builds on: `Base`, `MaterializedView` and sessions. Each top-level `models/` module defines a table. diff --git a/src/mavedb/models/clinvar_allele_link.py b/src/mavedb/models/clinvar_allele_link.py index 941ee2f2..97c859e1 100644 --- a/src/mavedb/models/clinvar_allele_link.py +++ b/src/mavedb/models/clinvar_allele_link.py @@ -4,7 +4,7 @@ from sqlalchemy.orm import Mapped, relationship from mavedb.db.base import Base -from mavedb.db.mixins import ValidTime +from mavedb.models.mixins.valid_time import ValidTime if TYPE_CHECKING: from .allele import Allele diff --git a/src/mavedb/models/gnomad_allele_link.py b/src/mavedb/models/gnomad_allele_link.py index 75294ba6..12f01c51 100644 --- a/src/mavedb/models/gnomad_allele_link.py +++ b/src/mavedb/models/gnomad_allele_link.py @@ -4,7 +4,7 @@ from sqlalchemy.orm import Mapped, relationship from mavedb.db.base import Base -from mavedb.db.mixins import ValidTime +from mavedb.models.mixins.valid_time import ValidTime if TYPE_CHECKING: from .allele import Allele diff --git a/src/mavedb/models/mapping_record.py b/src/mavedb/models/mapping_record.py index d0838ab2..1fa3cf7e 100644 --- a/src/mavedb/models/mapping_record.py +++ b/src/mavedb/models/mapping_record.py @@ -7,7 +7,7 @@ from sqlalchemy.orm import Mapped, relationship from mavedb.db.base import Base -from mavedb.db.mixins import ValidTime +from mavedb.models.mixins.valid_time import ValidTime from mavedb.lib.hgvs import extract_accession from mavedb.models.enums.sequence_level import SequenceLevel diff --git a/src/mavedb/models/mapping_record_allele.py b/src/mavedb/models/mapping_record_allele.py index b1982306..b5c89024 100644 --- a/src/mavedb/models/mapping_record_allele.py +++ b/src/mavedb/models/mapping_record_allele.py @@ -4,7 +4,7 @@ from sqlalchemy.orm import Mapped, relationship from mavedb.db.base import Base -from mavedb.db.mixins import ValidTime +from mavedb.models.mixins.valid_time import ValidTime if TYPE_CHECKING: from .allele import Allele diff --git a/src/mavedb/models/mixins/__init__.py b/src/mavedb/models/mixins/__init__.py new file mode 100644 index 00000000..c9c215b8 --- /dev/null +++ b/src/mavedb/models/mixins/__init__.py @@ -0,0 +1,3 @@ +""" +Mixins shared by models. +""" diff --git a/src/mavedb/db/mixins.py b/src/mavedb/models/mixins/valid_time.py similarity index 99% rename from src/mavedb/db/mixins.py rename to src/mavedb/models/mixins/valid_time.py index c9b17f4b..57809040 100644 --- a/src/mavedb/db/mixins.py +++ b/src/mavedb/models/mixins/valid_time.py @@ -1,4 +1,4 @@ -"""Reusable declarative mixins.""" +"""Valid-time versioning for rows that change over time.""" from datetime import datetime from typing import ClassVar, Optional, Sequence, TypeVar diff --git a/src/mavedb/models/projection_group.py b/src/mavedb/models/projection_group.py index 89e2a93c..9abf4228 100644 --- a/src/mavedb/models/projection_group.py +++ b/src/mavedb/models/projection_group.py @@ -4,7 +4,7 @@ from sqlalchemy.orm import Mapped, relationship from mavedb.db.base import Base -from mavedb.db.mixins import ValidTime +from mavedb.models.mixins.valid_time import ValidTime if TYPE_CHECKING: from .mapping_record import MappingRecord diff --git a/src/mavedb/models/vep_allele_consequence.py b/src/mavedb/models/vep_allele_consequence.py index 8d6eaeac..aac1aa0f 100644 --- a/src/mavedb/models/vep_allele_consequence.py +++ b/src/mavedb/models/vep_allele_consequence.py @@ -6,7 +6,7 @@ from sqlalchemy.orm import Mapped, relationship from mavedb.db.base import Base -from mavedb.db.mixins import ValidTime +from mavedb.models.mixins.valid_time import ValidTime from mavedb.models.enums.vep import VepConsequenceSource if TYPE_CHECKING: diff --git a/tests/models/mixins/__init__.py b/tests/models/mixins/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/db/test_mixins.py b/tests/models/mixins/test_valid_time.py similarity index 99% rename from tests/db/test_mixins.py rename to tests/models/mixins/test_valid_time.py index cd7ef373..aa687680 100644 --- a/tests/db/test_mixins.py +++ b/tests/models/mixins/test_valid_time.py @@ -21,7 +21,7 @@ from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import aliased, declarative_base, relationship -from mavedb.db.mixins import ValidTime +from mavedb.models.mixins.valid_time import ValidTime VtBase = declarative_base() From 3aa143fd2cd2b35a0fde493c7912107c66b58798 Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Fri, 23 Oct 2026 10:07:55 -0700 Subject: [PATCH 03/10] feat(permissions): add score set, experiment and experiment set viewers Identifier search fans out from identifiers to the records that cite them, so it needs a request-scoped read check per record type. Each viewer delegates to its module's has_permission and memoizes the result, as the existing viewers do. --- src/mavedb/lib/permissions/experiment.py | 11 ++++++++++ src/mavedb/lib/permissions/experiment_set.py | 11 ++++++++++ src/mavedb/lib/permissions/score_set.py | 11 ++++++++++ tests/lib/permissions/test_experiment.py | 23 ++++++++++++++++++++ tests/lib/permissions/test_experiment_set.py | 23 ++++++++++++++++++++ tests/lib/permissions/test_score_set.py | 23 ++++++++++++++++++++ 6 files changed, 102 insertions(+) diff --git a/src/mavedb/lib/permissions/experiment.py b/src/mavedb/lib/permissions/experiment.py index 2c7e6bdb..3ca8c8b2 100644 --- a/src/mavedb/lib/permissions/experiment.py +++ b/src/mavedb/lib/permissions/experiment.py @@ -1,9 +1,11 @@ +from dataclasses import dataclass from typing import Optional from mavedb.lib.logging.context import save_to_logging_context from mavedb.lib.permissions.actions import Action from mavedb.lib.permissions.models import PermissionResponse from mavedb.lib.permissions.utils import deny_action_for_entity, roles_permitted +from mavedb.lib.permissions.viewer import Viewer from mavedb.lib.types.authentication import UserData from mavedb.models.enums.user_role import UserRole from mavedb.models.experiment import Experiment @@ -71,6 +73,15 @@ def has_permission(user_data: Optional[UserData], entity: Experiment, action: Ac ) +@dataclass(frozen=True) +class ExperimentViewer(Viewer[Experiment]): + """Which experiments a caller may read, for reads that reach an experiment through something else.""" + + @staticmethod + def _has_permission(user_data: Optional[UserData], entity: Experiment, action: Action) -> PermissionResponse: + return has_permission(user_data, entity, action) + + def _handle_read_action( user_data: Optional[UserData], entity: Experiment, diff --git a/src/mavedb/lib/permissions/experiment_set.py b/src/mavedb/lib/permissions/experiment_set.py index d8cbecc3..a0b643ed 100644 --- a/src/mavedb/lib/permissions/experiment_set.py +++ b/src/mavedb/lib/permissions/experiment_set.py @@ -1,9 +1,11 @@ +from dataclasses import dataclass from typing import Optional from mavedb.lib.logging.context import save_to_logging_context from mavedb.lib.permissions.actions import Action from mavedb.lib.permissions.models import PermissionResponse from mavedb.lib.permissions.utils import deny_action_for_entity, roles_permitted +from mavedb.lib.permissions.viewer import Viewer from mavedb.lib.types.authentication import UserData from mavedb.models.enums.user_role import UserRole from mavedb.models.experiment_set import ExperimentSet @@ -72,6 +74,15 @@ def has_permission(user_data: Optional[UserData], entity: ExperimentSet, action: ) +@dataclass(frozen=True) +class ExperimentSetViewer(Viewer[ExperimentSet]): + """Which experiment sets a caller may read, for reads that reach an experiment set through something else.""" + + @staticmethod + def _has_permission(user_data: Optional[UserData], entity: ExperimentSet, action: Action) -> PermissionResponse: + return has_permission(user_data, entity, action) + + def _handle_read_action( user_data: Optional[UserData], entity: ExperimentSet, diff --git a/src/mavedb/lib/permissions/score_set.py b/src/mavedb/lib/permissions/score_set.py index e54b6b87..38b32ca8 100644 --- a/src/mavedb/lib/permissions/score_set.py +++ b/src/mavedb/lib/permissions/score_set.py @@ -1,9 +1,11 @@ +from dataclasses import dataclass from typing import Optional from mavedb.lib.logging.context import save_to_logging_context from mavedb.lib.permissions.actions import Action from mavedb.lib.permissions.models import PermissionResponse from mavedb.lib.permissions.utils import deny_action_for_entity, roles_permitted +from mavedb.lib.permissions.viewer import Viewer from mavedb.lib.types.authentication import UserData from mavedb.models.enums.user_role import UserRole from mavedb.models.score_set import ScoreSet @@ -73,6 +75,15 @@ def has_permission(user_data: Optional[UserData], entity: ScoreSet, action: Acti ) +@dataclass(frozen=True) +class ScoreSetViewer(Viewer[ScoreSet]): + """Which score sets a caller may read, for reads that reach a score set through something else.""" + + @staticmethod + def _has_permission(user_data: Optional[UserData], entity: ScoreSet, action: Action) -> PermissionResponse: + return has_permission(user_data, entity, action) + + def _handle_read_action( user_data: Optional[UserData], entity: ScoreSet, diff --git a/tests/lib/permissions/test_experiment.py b/tests/lib/permissions/test_experiment.py index d1d11b2b..8becca56 100644 --- a/tests/lib/permissions/test_experiment.py +++ b/tests/lib/permissions/test_experiment.py @@ -11,6 +11,7 @@ from mavedb.lib.permissions.actions import Action from mavedb.lib.permissions.experiment import ( + ExperimentViewer, _handle_add_score_set_action, _handle_delete_action, _handle_read_action, @@ -287,3 +288,25 @@ def test_handle_add_score_set_action(self, test_case: PermissionTest, entity_hel assert result.permitted == test_case.should_be_permitted if not test_case.should_be_permitted and test_case.expected_code: assert result.http_code == test_case.expected_code + + +class TestExperimentViewer: + """Test that the viewer applies experiment READ rules. The caching and fail-closed behaviour it inherits are + covered by test_viewer.py.""" + + @pytest.mark.parametrize( + "user_type, entity_state, expected", + [ + ("anonymous", "private", False), + ("other_user", "private", False), + ("owner", "private", True), + ("anonymous", "published", True), + ], + ids=["anonymous-private", "other-user-private", "owner-private", "anonymous-published"], + ) + def test_may_read_follows_experiment_read_rules( + self, entity_helper: EntityTestHelper, user_type: str, entity_state: str, expected: bool + ) -> None: + viewer = ExperimentViewer(entity_helper.create_user_data(user_type)) + + assert viewer.may_read(entity_helper.create_experiment(entity_state)) is expected diff --git a/tests/lib/permissions/test_experiment_set.py b/tests/lib/permissions/test_experiment_set.py index b47d08e1..467e1fbf 100644 --- a/tests/lib/permissions/test_experiment_set.py +++ b/tests/lib/permissions/test_experiment_set.py @@ -11,6 +11,7 @@ from mavedb.lib.permissions.actions import Action from mavedb.lib.permissions.experiment_set import ( + ExperimentSetViewer, _handle_add_experiment_action, _handle_delete_action, _handle_read_action, @@ -293,3 +294,25 @@ def test_handle_add_experiment_action(self, test_case: PermissionTest, entity_he assert result.permitted == test_case.should_be_permitted if not test_case.should_be_permitted and test_case.expected_code: assert result.http_code == test_case.expected_code + + +class TestExperimentSetViewer: + """Test that the viewer applies experiment set READ rules. The caching and fail-closed behaviour it inherits are + covered by test_viewer.py.""" + + @pytest.mark.parametrize( + "user_type, entity_state, expected", + [ + ("anonymous", "private", False), + ("other_user", "private", False), + ("owner", "private", True), + ("anonymous", "published", True), + ], + ids=["anonymous-private", "other-user-private", "owner-private", "anonymous-published"], + ) + def test_may_read_follows_experiment_set_read_rules( + self, entity_helper: EntityTestHelper, user_type: str, entity_state: str, expected: bool + ) -> None: + viewer = ExperimentSetViewer(entity_helper.create_user_data(user_type)) + + assert viewer.may_read(entity_helper.create_experiment_set(entity_state)) is expected diff --git a/tests/lib/permissions/test_score_set.py b/tests/lib/permissions/test_score_set.py index fd004b90..f0dfa9d9 100644 --- a/tests/lib/permissions/test_score_set.py +++ b/tests/lib/permissions/test_score_set.py @@ -11,6 +11,7 @@ from mavedb.lib.permissions.actions import Action from mavedb.lib.permissions.score_set import ( + ScoreSetViewer, _handle_add_calibration_action, _handle_delete_action, _handle_publish_action, @@ -385,3 +386,25 @@ def test_handle_add_calibration_action(self, test_case: PermissionTest, entity_h assert result.permitted == test_case.should_be_permitted if not test_case.should_be_permitted and test_case.expected_code: assert result.http_code == test_case.expected_code + + +class TestScoreSetViewer: + """Test that the viewer applies score set READ rules. The caching and fail-closed behaviour it inherits are + covered by test_viewer.py.""" + + @pytest.mark.parametrize( + "user_type, entity_state, expected", + [ + ("anonymous", "private", False), + ("other_user", "private", False), + ("owner", "private", True), + ("anonymous", "published", True), + ], + ids=["anonymous-private", "other-user-private", "owner-private", "anonymous-published"], + ) + def test_may_read_follows_score_set_read_rules( + self, entity_helper: EntityTestHelper, user_type: str, entity_state: str, expected: bool + ) -> None: + viewer = ScoreSetViewer(entity_helper.create_user_data(user_type)) + + assert viewer.may_read(entity_helper.create_score_set(entity_state)) is expected From ceb3c4d563cadeb6d5078e6190591a232257db28 Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Fri, 23 Oct 2026 10:08:12 -0700 Subject: [PATCH 04/10] feat(db): store DOI, raw read and target gene identifiers on the rows that use them DOI, raw read and target gene identifiers each lived in a shared table that records linked to by id, though every column but the identifier was constant, null or derivable from it. Each identifier now lives on the row that cites it, so under row-level security (#808) it is an owned row, visible exactly when its record is. The six DOI and raw read link tables hold the identifier, keyed by (_id, identifier). The three offset tables merge into target_gene_external_identifiers, keyed by (target_gene_id, db_name). Writes build link rows from the submitted strings, replacing the find-or-create helpers. Links are computed on serialization from lib/external_identifier_urls.py: raw read links follow the accession type (#445) and gene identifier links, always null before, are filled. DOIs are validated by a reusable Doi field type and stored bare and upper-cased, the registry's form, so a record can't cite one DOI twice. Identifier search returns only identifiers on records the caller can read. Publications keep their shared table (#945), since their rows hold fetched metadata. BREAKING CHANGE: DoiIdentifier and RawReadIdentifier lose id, ExternalGeneIdentifier loses referenceHtml, and DOIs come back upper-cased. --- ...4a88870_store_identifiers_on_owner_rows.py | 192 +++++++++++++++ src/mavedb/lib/external_identifier_urls.py | 47 ++++ src/mavedb/lib/identifiers.py | 147 +++++------ src/mavedb/lib/score_sets.py | 39 +-- src/mavedb/lib/validation/transform.py | 9 - src/mavedb/models/__init__.py | 7 +- src/mavedb/models/doi_identifier.py | 44 +++- src/mavedb/models/ensembl_identifier.py | 17 -- src/mavedb/models/ensembl_offset.py | 19 -- src/mavedb/models/experiment.py | 32 +-- src/mavedb/models/experiment_set.py | 28 +-- .../models/mixins/external_identifier.py | 14 ++ src/mavedb/models/raw_read_identifier.py | 43 +++- src/mavedb/models/refseq_identifier.py | 18 -- src/mavedb/models/refseq_offset.py | 16 -- src/mavedb/models/score_set.py | 28 +-- src/mavedb/models/target_gene.py | 12 +- .../models/target_gene_external_identifier.py | 37 +++ src/mavedb/models/uniprot_identifier.py | 18 -- src/mavedb/models/uniprot_offset.py | 19 -- src/mavedb/routers/doi_identifiers.py | 61 +++-- src/mavedb/routers/experiments.py | 43 ++-- src/mavedb/routers/genes.py | 7 +- src/mavedb/routers/raw_read_identifiers.py | 65 +++-- src/mavedb/routers/score_sets.py | 39 +-- src/mavedb/routers/statistics.py | 129 ++++------ src/mavedb/routers/target_gene_identifiers.py | 53 ++-- src/mavedb/view_models/components/doi.py | 30 +++ src/mavedb/view_models/doi_identifier.py | 13 +- .../view_models/external_gene_identifier.py | 1 - .../external_gene_identifier_offset.py | 17 +- src/mavedb/view_models/raw_read_identifier.py | 1 - src/mavedb/view_models/target_gene.py | 16 +- tests/helpers/constants.py | 2 - tests/lib/test_external_identifier_urls.py | 40 +++ tests/routers/test_record_identifiers.py | 233 ++++++++++++++++++ tests/view_models/test_doi_identifier.py | 14 ++ tests/view_models/test_score_set.py | 10 +- tests/view_models/test_target_gene.py | 75 +++--- 39 files changed, 1030 insertions(+), 605 deletions(-) create mode 100644 alembic/versions/2dfdc4a88870_store_identifiers_on_owner_rows.py create mode 100644 src/mavedb/lib/external_identifier_urls.py delete mode 100644 src/mavedb/models/ensembl_identifier.py delete mode 100644 src/mavedb/models/ensembl_offset.py create mode 100644 src/mavedb/models/mixins/external_identifier.py delete mode 100644 src/mavedb/models/refseq_identifier.py delete mode 100644 src/mavedb/models/refseq_offset.py create mode 100644 src/mavedb/models/target_gene_external_identifier.py delete mode 100644 src/mavedb/models/uniprot_identifier.py delete mode 100644 src/mavedb/models/uniprot_offset.py create mode 100644 src/mavedb/view_models/components/doi.py create mode 100644 tests/lib/test_external_identifier_urls.py create mode 100644 tests/routers/test_record_identifiers.py diff --git a/alembic/versions/2dfdc4a88870_store_identifiers_on_owner_rows.py b/alembic/versions/2dfdc4a88870_store_identifiers_on_owner_rows.py new file mode 100644 index 00000000..ccd080f8 --- /dev/null +++ b/alembic/versions/2dfdc4a88870_store_identifiers_on_owner_rows.py @@ -0,0 +1,192 @@ +"""store DOI, raw read and target gene identifiers on the rows that use them + +Revision ID: 2dfdc4a88870 +Revises: 6987f253e02a +Create Date: 2026-10-21 + +DOI, raw read (SRA), Ensembl, RefSeq and UniProt identifiers each lived in a shared table that records linked +to by id. Every column but the identifier was a constant, always null, or derivable from it, so the identifier +moves onto the row that uses it (#944): + +- The six DOI and raw read link tables hold the identifier string, keyed by ``(_id, identifier)``. + DOIs are upper-cased on the way, the form the DOI registry stores, since they're case-insensitive and a record + shouldn't cite one twice. +- ``ensembl_offsets``, ``refseq_offsets`` and ``uniprot_offsets`` merge into + ``target_gene_external_identifiers``, keyed by ``(target_gene_id, db_name)``: a target gene has at most one + identifier per database. +- The five shared identifier tables are dropped, with any rows no record linked to. Urls are computed when + serializing. + +Under row-level security (#808), these become owned rows, visible exactly when their record is. + +The tables are small (a few thousand rows in all), so the rewrites hold their locks briefly. + +The downgrade rebuilds the shared tables from the link rows, so the unlinked rows don't come back. Ids are +reassigned, the dates are the day of the downgrade, and urls are rebuilt the way the api used to build them, so +gene identifier urls come back null. +""" + +from alembic import op + +# revision identifiers, used by Alembic. +revision = "2dfdc4a88870" +down_revision = "6987f253e02a" +branch_labels = None +depends_on = None + +# (link table, parent column, shared table, shared id column) +DOI_LINKS = [ + ("scoreset_doi_identifiers", "scoreset_id", "doi_identifiers", "doi_identifier_id"), + ("experiment_doi_identifiers", "experiment_id", "doi_identifiers", "doi_identifier_id"), + ("experiment_set_doi_identifiers", "experiment_set_id", "doi_identifiers", "doi_identifier_id"), +] +RAW_READ_LINKS = [ + ("scoreset_sra_identifiers", "scoreset_id", "sra_identifiers", "sra_identifier_id"), + ("experiment_sra_identifiers", "experiment_id", "sra_identifiers", "sra_identifier_id"), + ("experiment_set_sra_identifiers", "experiment_set_id", "sra_identifiers", "sra_identifier_id"), +] + +# (db_name, offset table, identifier table, whether the identifier table has reference_html) +GENE_IDENTIFIERS = [ + ("Ensembl", "ensembl_offsets", "ensembl_identifiers", False), + ("RefSeq", "refseq_offsets", "refseq_identifiers", True), + ("UniProt", "uniprot_offsets", "uniprot_identifiers", True), +] + + +def _fold_link_table(link: str, parent: str, shared: str, shared_id: str, normalize: str) -> None: + op.execute(f"ALTER TABLE {link} ADD COLUMN identifier character varying") + op.execute( + f"UPDATE {link} l SET identifier = {normalize.format('s.identifier')} FROM {shared} s WHERE s.id = l.{shared_id}" + ) + + # Two shared rows differing only in case would collide once upper-cased. There are none today. + op.execute( + f"DELETE FROM {link} a USING {link} b " + f"WHERE a.{parent} = b.{parent} AND a.identifier = b.identifier AND a.ctid > b.ctid" + ) + + op.execute(f"ALTER TABLE {link} DROP CONSTRAINT {link}_pkey") + op.execute(f"ALTER TABLE {link} DROP COLUMN {shared_id}") + op.execute(f"ALTER TABLE {link} ALTER COLUMN identifier SET NOT NULL") + op.execute(f"ALTER TABLE {link} ADD CONSTRAINT {link}_pkey PRIMARY KEY ({parent}, identifier)") + + +def _create_shared_table(shared: str, with_reference_html: bool = False) -> None: + reference_html = "reference_html character varying," if with_reference_html else "" + op.execute( + f""" + CREATE TABLE {shared} ( + id serial NOT NULL, + identifier character varying NOT NULL, + db_name character varying NOT NULL, + db_version character varying, + url character varying, + {reference_html} + creation_date date NOT NULL, + modification_date date NOT NULL, + CONSTRAINT {shared}_pkey PRIMARY KEY (id) + ) + """ + ) + + +def _unfold_link_table(link: str, parent: str, shared: str, shared_id: str) -> None: + op.execute(f"ALTER TABLE {link} ADD COLUMN {shared_id} integer") + op.execute(f"UPDATE {link} l SET {shared_id} = s.id FROM {shared} s WHERE s.identifier = l.identifier") + op.execute(f"ALTER TABLE {link} DROP CONSTRAINT {link}_pkey") + op.execute(f"ALTER TABLE {link} DROP COLUMN identifier") + op.execute(f"ALTER TABLE {link} ALTER COLUMN {shared_id} SET NOT NULL") + op.execute(f"ALTER TABLE {link} ADD CONSTRAINT {link}_pkey PRIMARY KEY ({parent}, {shared_id})") + op.execute( + f"ALTER TABLE {link} ADD CONSTRAINT {link}_{shared_id}_fkey FOREIGN KEY ({shared_id}) REFERENCES {shared}(id)" + ) + + +def upgrade() -> None: + for link, parent, shared, shared_id in DOI_LINKS: + _fold_link_table(link, parent, shared, shared_id, normalize="upper({})") + for link, parent, shared, shared_id in RAW_READ_LINKS: + _fold_link_table(link, parent, shared, shared_id, normalize="{}") + + op.execute( + """ + CREATE TABLE target_gene_external_identifiers ( + target_gene_id integer NOT NULL, + db_name character varying NOT NULL, + identifier character varying NOT NULL, + "offset" integer NOT NULL, + CONSTRAINT target_gene_external_identifiers_pkey PRIMARY KEY (target_gene_id, db_name), + CONSTRAINT target_gene_external_identifiers_target_gene_id_fkey + FOREIGN KEY (target_gene_id) REFERENCES target_genes(id) ON DELETE CASCADE, + CONSTRAINT ck_target_gene_external_identifiers_db_name_valid + CHECK (db_name IN ('Ensembl', 'RefSeq', 'UniProt')) + ) + """ + ) + for db_name, offsets, identifiers, _ in GENE_IDENTIFIERS: + op.execute( + f""" + INSERT INTO target_gene_external_identifiers (target_gene_id, db_name, identifier, "offset") + SELECT o.target_gene_id, '{db_name}', i.identifier, o."offset" + FROM {offsets} o JOIN {identifiers} i ON i.id = o.identifier_id + """ + ) + op.execute(f"DROP TABLE {offsets}") + op.execute(f"DROP TABLE {identifiers}") + + op.execute("DROP TABLE doi_identifiers") + op.execute("DROP TABLE sra_identifiers") + + +def downgrade() -> None: + for shared, links, db_name, url_prefix in [ + ("doi_identifiers", DOI_LINKS, "DOI", "https://doi.org/"), + ("sra_identifiers", RAW_READ_LINKS, "SRA", "http://www.ebi.ac.uk/ena/data/view/"), + ]: + _create_shared_table(shared) + every_identifier = " UNION ".join(f"SELECT identifier FROM {link}" for link, *_ in links) + op.execute( + f""" + INSERT INTO {shared} (identifier, db_name, url, creation_date, modification_date) + SELECT identifier, '{db_name}', '{url_prefix}' || identifier, current_date, current_date + FROM ({every_identifier}) AS every_identifier + ORDER BY identifier + """ + ) + for link, parent, _, shared_id in links: + _unfold_link_table(link, parent, shared, shared_id) + + for db_name, offsets, identifiers, with_reference_html in GENE_IDENTIFIERS: + _create_shared_table(identifiers, with_reference_html) + op.execute( + f""" + INSERT INTO {identifiers} (identifier, db_name, creation_date, modification_date) + SELECT DISTINCT identifier, db_name, current_date, current_date + FROM target_gene_external_identifiers WHERE db_name = '{db_name}' + ORDER BY identifier + """ + ) + op.execute( + f""" + CREATE TABLE {offsets} ( + identifier_id integer NOT NULL, + target_gene_id integer NOT NULL, + "offset" integer NOT NULL, + CONSTRAINT {offsets}_pkey PRIMARY KEY (identifier_id, target_gene_id), + CONSTRAINT {offsets}_identifier_id_fkey FOREIGN KEY (identifier_id) REFERENCES {identifiers}(id), + CONSTRAINT {offsets}_target_gene_id_fkey FOREIGN KEY (target_gene_id) REFERENCES target_genes(id) + ) + """ + ) + op.execute( + f""" + INSERT INTO {offsets} (identifier_id, target_gene_id, "offset") + SELECT i.id, t.target_gene_id, t."offset" + FROM target_gene_external_identifiers t + JOIN {identifiers} i ON i.identifier = t.identifier + WHERE t.db_name = '{db_name}' + """ + ) + + op.execute("DROP TABLE target_gene_external_identifiers") diff --git a/src/mavedb/lib/external_identifier_urls.py b/src/mavedb/lib/external_identifier_urls.py new file mode 100644 index 00000000..8a9411ab --- /dev/null +++ b/src/mavedb/lib/external_identifier_urls.py @@ -0,0 +1,47 @@ +"""Links to the external databases that records cite through :class:`ExternalIdentifier` models. + +MaveDB has no other code for these databases, so their link rules live together here. A source MaveDB also +talks to, such as gnomAD or ClinGen, keeps its link with that code. This module imports nothing from mavedb, so +models can use it. +""" + +import re + +import idutils + +# NCBI keeps RefSeq proteins (these prefixes) in its `protein` database and everything else in `nuccore`, its +# nucleotide database. A link to the wrong one finds nothing. +REFSEQ_PROTEIN_ACCESSION = re.compile(r"^(AP|NP|WP|XP|YP)_") + + +def doi_url(identifier: str) -> str: + """The resolver link for a DOI.""" + return f"https://doi.org/{identifier}" + + +def raw_read_url(identifier: str) -> str: + """The page for a raw read accession, chosen by its accession type (#445).""" + if idutils.is_bioproject(identifier): + return f"https://www.ncbi.nlm.nih.gov/bioproject/{identifier}" + if idutils.is_geo(identifier): + return f"https://www.ncbi.nlm.nih.gov/geo/query/acc.cgi?acc={identifier}" + if idutils.is_arrayexpress_experiment(identifier): + return f"https://www.ebi.ac.uk/biostudies/arrayexpress/studies/{identifier}" + if idutils.is_arrayexpress_array(identifier): + return f"https://www.ebi.ac.uk/biostudies/arrayexpress/arrays/{identifier}" + + # SRA accessions (SRR, ERR, DRR and their study and experiment forms) are shared across the INSDC archives, + # and ENA's browser resolves all of them. It's also where every raw read url pointed before #445, so + # accessions that match none of the types above keep working. + return f"https://www.ebi.ac.uk/ena/browser/view/{identifier}" + + +def external_gene_identifier_url(db_name: str, identifier: str) -> str: + """The page for a target gene's identifier in Ensembl, RefSeq or UniProt.""" + if db_name == "Ensembl": + return f"https://www.ensembl.org/id/{identifier}" + if db_name == "UniProt": + return f"https://www.uniprot.org/uniprotkb/{identifier}" + + database = "protein" if REFSEQ_PROTEIN_ACCESSION.match(identifier) else "nuccore" + return f"https://www.ncbi.nlm.nih.gov/{database}/{identifier}" diff --git a/src/mavedb/lib/identifiers.py b/src/mavedb/lib/identifiers.py index 7855b6af..f6c97cf1 100644 --- a/src/mavedb/lib/identifiers.py +++ b/src/mavedb/lib/identifiers.py @@ -1,7 +1,7 @@ import json import logging import os -from typing import Mapping, Optional, Union +from typing import Any, Iterable, Mapping, Optional, Sequence, Union import eutils # type: ignore import requests @@ -9,41 +9,22 @@ from eutils._internal.xmlfacades.pubmedarticle import PubmedArticle # type: ignore from eutils._internal.xmlfacades.pubmedarticleset import PubmedArticleSet # type: ignore from idutils import is_doi, normalize_doi -from sqlalchemy import select +from sqlalchemy import Select, func, select from sqlalchemy.orm import Session from mavedb.lib.exceptions import AmbiguousIdentifierError, NonexistentIdentifierError from mavedb.lib.external_publications import Crossref, CrossrefWork, PublicationAuthors, Rxiv, RxivContentDetail +from mavedb.lib.permissions.principal import Principal +from mavedb.lib.permissions.viewer import Viewer from mavedb.lib.validation.publication import identifier_valid_for, infer_identifier_from_url, validate_db_name -from mavedb.models.doi_identifier import DoiIdentifier -from mavedb.models.ensembl_identifier import EnsemblIdentifier -from mavedb.models.ensembl_offset import EnsemblOffset +from mavedb.models.mixins.external_identifier import ExternalIdentifier from mavedb.models.publication_identifier import PublicationIdentifier -from mavedb.models.raw_read_identifier import RawReadIdentifier -from mavedb.models.refseq_identifier import RefseqIdentifier -from mavedb.models.refseq_offset import RefseqOffset -from mavedb.models.target_gene import TargetGene -from mavedb.models.uniprot_identifier import UniprotIdentifier -from mavedb.models.uniprot_offset import UniprotOffset +from mavedb.models.target_gene_external_identifier import TargetGeneExternalIdentifier +from mavedb.view_models.external_gene_identifier_offset import ExternalGeneIdentifierOffsetCreate logger = logging.getLogger(__name__) -# XXX these classes all have an "identifier" attribute but there's no superclass -# to unify them ... - -EXTERNAL_GENE_IDENTIFIER_CLASSES = { - "Ensembl": EnsemblIdentifier, - "RefSeq": RefseqIdentifier, - "UniProt": UniprotIdentifier, -} - -EXTERNAL_GENE_IDENTIFIER_OFFSET_CLASSES = {"Ensembl": EnsemblOffset, "RefSeq": RefseqOffset, "UniProt": UniprotOffset} - -EXTERNAL_GENE_IDENTIFIER_OFFSET_ATTRIBUTES = { - "Ensembl": "ensembl_offset", - "RefSeq": "refseq_offset", - "UniProt": "uniprot_offset", -} +IDENTIFIER_SEARCH_LIMIT = 50 class ExternalPublication: @@ -174,20 +155,6 @@ def reference_html(self) -> str: ) -async def find_or_create_doi_identifier(db: Session, identifier: str): - """ - Find an existing DOI identifier record with the specified identifier string, or create a new one. - - :param db: An active database session - :param identifier: A valid DOI identifier - :return: An existing DoiIdentifier containing the specified identifier string, or a new, unsaved DoiIdentifier - """ - doi_identifier = db.query(DoiIdentifier).filter(DoiIdentifier.identifier == identifier).one_or_none() - if not doi_identifier: - doi_identifier = DoiIdentifier(identifier=identifier, db_name="DOI", url=f"https://doi.org/{identifier}") - return doi_identifier - - async def fetch_pubmed_article(identifier: str) -> Optional[PubmedArticle]: """ Fetch an existing PubMed article from NCBI @@ -408,55 +375,69 @@ async def find_or_create_publication_identifier( return create_generic_article(matched_article) -async def find_or_create_raw_read_identifier(db: Session, identifier: str): - raw_read_identifier = db.query(RawReadIdentifier).filter(RawReadIdentifier.identifier == identifier).one_or_none() - if not raw_read_identifier: - raw_read_identifier = RawReadIdentifier( - identifier=identifier, db_name="SRA", url=f"http://www.ebi.ac.uk/ena/data/view/{identifier}" - ) - return raw_read_identifier - - -async def find_or_create_external_gene_identifier(db: Session, db_name: str, identifier: str): - """ - Find an existing gene identifier record with the specified gene database name and identifier string, or create a new - one. +def distinct_identifiers(identifiers: Iterable[str]) -> list[str]: + """The distinct identifier strings in a submitted list, in the order given. - :param db: An active database session - :param identifier: A valid identifier - :return: An existing EnsemblIdentifier, RefseqIdentifier, or UniprotIdentifier containing the specified identifier - string, or a new, unsaved instance of one of these classes + A record stores each identifier once, so a repeat in the submission would collide with itself. """ + return list(dict.fromkeys(identifiers)) - # TODO Handle key errors. - identifier_class = EXTERNAL_GENE_IDENTIFIER_CLASSES[db_name] - assert hasattr(identifier_class, "identifier") - external_gene_identifier = ( - db.query(identifier_class).filter(identifier_class.identifier == identifier).one_or_none() - ) +def target_gene_external_identifiers( + external_identifiers: Iterable[ExternalGeneIdentifierOffsetCreate], +) -> list[TargetGeneExternalIdentifier]: + """Rows for a target gene's submitted external identifiers. - if not external_gene_identifier: - external_gene_identifier = identifier_class( - identifier=identifier, + A target gene holds one identifier per database. When a submission names a database twice, the last one + wins, as it did when each database had its own offset table. + """ + by_db_name = { + external_identifier.identifier.db_name: external_identifier for external_identifier in external_identifiers + } + return [ + TargetGeneExternalIdentifier( db_name=db_name, - # TODO Set URL from identifier - # url=f'https://doi.org/{identifier}' + identifier=external_identifier.identifier.identifier, + offset=external_identifier.offset, ) - return external_gene_identifier - - -async def create_external_gene_identifier_offset( - db: Session, target_gene: TargetGene, db_name: str, identifier: str, offset: int -): - external_gene_identifier = await find_or_create_external_gene_identifier(db, db_name, identifier) - - # TODO Handle key errors. - offset_class = EXTERNAL_GENE_IDENTIFIER_OFFSET_CLASSES[external_gene_identifier.db_name] - external_gene_identifier_offset = offset_class(identifier=external_gene_identifier, offset=offset) - - identifier_offset_attribute = EXTERNAL_GENE_IDENTIFIER_OFFSET_ATTRIBUTES[external_gene_identifier.db_name] - setattr(target_gene, identifier_offset_attribute, external_gene_identifier_offset) + for db_name, external_identifier in by_db_name.items() + ] + + +def search_identifiers_on_readable_records( + db: Session, + principal: Principal, + text: str, + sources: Sequence[tuple[type[ExternalIdentifier], type[Viewer[Any]], Select]], +) -> list[ExternalIdentifier]: + """Find identifiers containing ``text`` that are attached to records the caller may read. + + Identifiers are stored on the rows that link them to their records, so one identifier can appear on many + rows. This returns one row per distinct identifier, sorted by identifier. + + Args: + db: An active database session. + principal: The caller. + text: The text to search for, matched case-insensitively anywhere in the identifier. + sources: For each table to search, its link model, the viewer for the record that governs each row's + visibility, and a query selecting ``(link row, record)``. + + Returns: + Up to ``IDENTIFIER_SEARCH_LIMIT`` link rows, one per distinct identifier. + """ + # Read access to a record is decided in Python, so the filter runs before the limit. These tables hold a + # few hundred rows. Once row-level security enforces reads (#808), the database does this filtering. + found: dict[str, ExternalIdentifier] = {} + for link, viewer_class, query in sources: + viewer = principal.viewer_for(viewer_class) + rows = db.execute(query.where(func.lower(link.identifier).contains(text.lower()))).all() + for link_row, record in rows: + if link_row.identifier in found: + continue + if viewer.may_read(record): + found[link_row.identifier] = link_row + + return [found[identifier] for identifier in sorted(found)][:IDENTIFIER_SEARCH_LIMIT] def sanitize_target_sequence_label(label: str): diff --git a/src/mavedb/lib/score_sets.py b/src/mavedb/lib/score_sets.py index 64b6dd03..67714de8 100644 --- a/src/mavedb/lib/score_sets.py +++ b/src/mavedb/lib/score_sets.py @@ -27,9 +27,7 @@ from mavedb.models.allele import Allele from mavedb.models.contributor import Contributor from mavedb.models.controlled_keyword import ControlledKeyword -from mavedb.models.doi_identifier import DoiIdentifier -from mavedb.models.ensembl_identifier import EnsemblIdentifier -from mavedb.models.ensembl_offset import EnsemblOffset +from mavedb.models.doi_identifier import ScoreSetDoiIdentifier from mavedb.models.experiment import Experiment from mavedb.models.experiment_controlled_keyword import ExperimentControlledKeywordAssociation from mavedb.models.experiment_publication_identifier import ExperimentPublicationIdentifierAssociation @@ -37,18 +35,15 @@ from mavedb.models.mapping_record import MappingRecord from mavedb.models.mapping_record_allele import MappingRecordAllele from mavedb.models.publication_identifier import PublicationIdentifier -from mavedb.models.refseq_identifier import RefseqIdentifier -from mavedb.models.refseq_offset import RefseqOffset from mavedb.models.score_set import ScoreSet from mavedb.models.score_set_publication_identifier import ( ScoreSetPublicationIdentifierAssociation, ) from mavedb.models.target_accession import TargetAccession from mavedb.models.target_gene import TargetGene +from mavedb.models.target_gene_external_identifier import TargetGeneExternalIdentifier from mavedb.models.target_sequence import TargetSequence from mavedb.models.taxonomy import Taxonomy -from mavedb.models.uniprot_identifier import UniprotIdentifier -from mavedb.models.uniprot_offset import UniprotOffset from mavedb.models.user import User from mavedb.models.variant import Variant from mavedb.view_models.search import ControlledKeywordFilterOption, ScoreSetsSearch @@ -141,26 +136,10 @@ def build_search_score_sets_query_filter( f"""$[*].name ? (@ like_regex "{lower_search_text}" flag "i")""", ) ), - ScoreSet.doi_identifiers.any(func.lower(DoiIdentifier.identifier).icontains(lower_search_text)), + ScoreSet.doi_identifiers.any(func.lower(ScoreSetDoiIdentifier.identifier).icontains(lower_search_text)), ScoreSet.target_genes.any( - TargetGene.uniprot_offset.has( - UniprotOffset.identifier.has( - func.lower(UniprotIdentifier.identifier).icontains(lower_search_text) - ) - ) - ), - ScoreSet.target_genes.any( - TargetGene.refseq_offset.has( - RefseqOffset.identifier.has( - func.lower(RefseqIdentifier.identifier).icontains(lower_search_text) - ) - ) - ), - ScoreSet.target_genes.any( - TargetGene.ensembl_offset.has( - EnsemblOffset.identifier.has( - func.lower(EnsemblIdentifier.identifier).icontains(lower_search_text) - ) + TargetGene.external_identifiers.any( + func.lower(TargetGeneExternalIdentifier.identifier).icontains(lower_search_text) ) ), ) @@ -262,9 +241,7 @@ def search_score_sets(db: Session, owner_or_contributor: Optional[User], search: ScoreSetPublicationIdentifierAssociation.publication ), joinedload(ScoreSet.target_genes).options( - joinedload(TargetGene.ensembl_offset).joinedload(EnsemblOffset.identifier), - joinedload(TargetGene.refseq_offset).joinedload(RefseqOffset.identifier), - joinedload(TargetGene.uniprot_offset).joinedload(UniprotOffset.identifier), + joinedload(TargetGene.external_identifiers), joinedload(TargetGene.target_sequence).joinedload(TargetSequence.taxonomy), joinedload(TargetGene.target_accession), ), @@ -276,9 +253,7 @@ def search_score_sets(db: Session, owner_or_contributor: Optional[User], search: ScoreSetPublicationIdentifierAssociation.publication ), selectinload(ScoreSet.target_genes).options( - joinedload(TargetGene.ensembl_offset).joinedload(EnsemblOffset.identifier), - joinedload(TargetGene.refseq_offset).joinedload(RefseqOffset.identifier), - joinedload(TargetGene.uniprot_offset).joinedload(UniprotOffset.identifier), + joinedload(TargetGene.external_identifiers), joinedload(TargetGene.target_sequence).joinedload(TargetSequence.taxonomy), joinedload(TargetGene.target_accession), ), diff --git a/src/mavedb/lib/validation/transform.py b/src/mavedb/lib/validation/transform.py index 32c2dff1..c7f4b594 100644 --- a/src/mavedb/lib/validation/transform.py +++ b/src/mavedb/lib/validation/transform.py @@ -18,9 +18,7 @@ from mavedb.models.score_calibration_publication_identifier import ScoreCalibrationPublicationIdentifierAssociation from mavedb.models.score_set import ScoreSet from mavedb.models.score_set_publication_identifier import ScoreSetPublicationIdentifierAssociation -from mavedb.models.target_gene import TargetGene from mavedb.models.user import User -from mavedb.view_models.external_gene_identifier_offset import ExternalGeneIdentifierOffset from mavedb.view_models.publication_identifier import PublicationIdentifier @@ -133,13 +131,6 @@ def transform_score_calibration_publication_identifiers( return transformed_publication_identifiers -def transform_external_identifier_offsets_to_list(data: TargetGene) -> list[ExternalGeneIdentifierOffset]: - ensembl_offset = data.ensembl_offset - refseq_offset = data.refseq_offset - uniprot_offset = data.uniprot_offset - return list(filter(lambda x: x is not None, [ensembl_offset, refseq_offset, uniprot_offset])) # type:ignore - - # We're unable to create a dynamically typed TypedDict which references enum keys and enforces totality of the enum, so this # is simply typed as dict[str, User] to avoid coupling and the need to rewrite such a TypedDict any time the enum changes. # See https://stackoverflow.com/a/72083153 for details about why this isn't possible. diff --git a/src/mavedb/models/__init__.py b/src/mavedb/models/__init__.py index 534ff0ec..c9c18737 100644 --- a/src/mavedb/models/__init__.py +++ b/src/mavedb/models/__init__.py @@ -8,8 +8,6 @@ "clinvar_allele_link", "controlled_keyword", "doi_identifier", - "ensembl_identifier", - "ensembl_offset", "experiment", "experiment_set", "genome_identifier", @@ -29,8 +27,6 @@ "published_variant", "variant_annotation_view", "raw_read_identifier", - "refseq_identifier", - "refseq_offset", "role", "score_calibration_functional_classification_variant_association", "score_calibration_functional_classification", @@ -38,11 +34,10 @@ "score_calibration", "score_set", "target_gene", + "target_gene_external_identifier", "target_gene_mapping", "target_sequence", "taxonomy", - "uniprot_identifier", - "uniprot_offset", "urn_redirect", "user", "variant_annotation_status", diff --git a/src/mavedb/models/doi_identifier.py b/src/mavedb/models/doi_identifier.py index 9cf75587..0b27f917 100644 --- a/src/mavedb/models/doi_identifier.py +++ b/src/mavedb/models/doi_identifier.py @@ -1,17 +1,37 @@ -from datetime import date - -from sqlalchemy import Column, Date, Integer, String +from sqlalchemy import Column, ForeignKey, Integer, String from mavedb.db.base import Base +from mavedb.lib.external_identifier_urls import doi_url +from mavedb.models.mixins.external_identifier import ExternalIdentifier + + +class DoiIdentifierLink(ExternalIdentifier): + """A DOI cited by one record. DOIs are case-insensitive, so the identifier is stored upper-cased, as the DOI + registry stores it.""" + + db_name = "DOI" + + @property + def url(self) -> str: + return doi_url(self.identifier) + + +class ScoreSetDoiIdentifier(DoiIdentifierLink, Base): + __tablename__ = "scoreset_doi_identifiers" + + score_set_id = Column("scoreset_id", Integer, ForeignKey("scoresets.id"), primary_key=True) + identifier = Column(String, primary_key=True) + + +class ExperimentDoiIdentifier(DoiIdentifierLink, Base): + __tablename__ = "experiment_doi_identifiers" + + experiment_id = Column(Integer, ForeignKey("experiments.id"), primary_key=True) + identifier = Column(String, primary_key=True) -class DoiIdentifier(Base): - __tablename__ = "doi_identifiers" +class ExperimentSetDoiIdentifier(DoiIdentifierLink, Base): + __tablename__ = "experiment_set_doi_identifiers" - id = Column(Integer, primary_key=True) - identifier = Column(String, nullable=False) - db_name = Column(String, nullable=False) - db_version = Column(String, nullable=True) - url = Column(String, nullable=True) - creation_date = Column(Date, nullable=False, default=date.today) - modification_date = Column(Date, nullable=False, default=date.today, onupdate=date.today) + experiment_set_id = Column(Integer, ForeignKey("experiment_sets.id"), primary_key=True) + identifier = Column(String, primary_key=True) diff --git a/src/mavedb/models/ensembl_identifier.py b/src/mavedb/models/ensembl_identifier.py deleted file mode 100644 index 95189b07..00000000 --- a/src/mavedb/models/ensembl_identifier.py +++ /dev/null @@ -1,17 +0,0 @@ -from datetime import date - -from sqlalchemy import Column, Date, Integer, String - -from mavedb.db.base import Base - - -class EnsemblIdentifier(Base): - __tablename__ = "ensembl_identifiers" - - id = Column(Integer, primary_key=True) - identifier = Column(String, nullable=False) - db_name = Column(String, nullable=False) - db_version = Column(String, nullable=True) - url = Column(String, nullable=True) - creation_date = Column(Date, nullable=False, default=date.today) - modification_date = Column(Date, nullable=False, default=date.today, onupdate=date.today) diff --git a/src/mavedb/models/ensembl_offset.py b/src/mavedb/models/ensembl_offset.py deleted file mode 100644 index 28bb09a4..00000000 --- a/src/mavedb/models/ensembl_offset.py +++ /dev/null @@ -1,19 +0,0 @@ -from sqlalchemy import Column, ForeignKey, Integer -from sqlalchemy.orm import Mapped, backref, relationship - -from mavedb.db.base import Base -from mavedb.models.ensembl_identifier import EnsemblIdentifier -from mavedb.models.target_gene import TargetGene - - -class EnsemblOffset(Base): - __tablename__ = "ensembl_offsets" - - identifier_id = Column(Integer, ForeignKey("ensembl_identifiers.id"), nullable=False, primary_key=True) - identifier: Mapped[EnsemblIdentifier] = relationship( - "EnsemblIdentifier", backref=backref("target_gene_offsets", uselist=True) - ) - target_gene_id = Column(Integer, ForeignKey("target_genes.id"), nullable=False, primary_key=True) - target_gene: Mapped[TargetGene] = relationship(back_populates="ensembl_offset") - - offset = Column(Integer, nullable=False) diff --git a/src/mavedb/models/experiment.py b/src/mavedb/models/experiment.py index eb8bb2cf..48ddebf3 100644 --- a/src/mavedb/models/experiment.py +++ b/src/mavedb/models/experiment.py @@ -12,7 +12,7 @@ from mavedb.lib.temp_urns import generate_temp_urn from mavedb.models.contributor import Contributor from mavedb.models.controlled_keyword import ControlledKeyword -from mavedb.models.doi_identifier import DoiIdentifier +from mavedb.models.doi_identifier import ExperimentDoiIdentifier from mavedb.models.experiment_controlled_keyword import ( ExperimentControlledKeywordAssociation, ) @@ -22,7 +22,7 @@ from mavedb.models.experiment_set import ExperimentSet from mavedb.models.legacy_keyword import LegacyKeyword from mavedb.models.publication_identifier import PublicationIdentifier -from mavedb.models.raw_read_identifier import RawReadIdentifier +from mavedb.models.raw_read_identifier import ExperimentRawReadIdentifier from mavedb.models.user import User if TYPE_CHECKING: @@ -36,13 +36,6 @@ Column("contributor_id", ForeignKey("contributors.id"), primary_key=True), ) -experiments_doi_identifiers_association_table = Table( - "experiment_doi_identifiers", - Base.metadata, - Column("experiment_id", ForeignKey("experiments.id"), primary_key=True), - Column("doi_identifier_id", ForeignKey("doi_identifiers.id"), primary_key=True), -) - experiments_legacy_keywords_association_table = Table( "experiment_keywords", Base.metadata, @@ -50,14 +43,6 @@ Column("keyword_id", ForeignKey("keywords.id"), primary_key=True), ) -# experiments_sra_identifiers_association_table = Table( -experiments_raw_read_identifiers_association_table = Table( - "experiment_sra_identifiers", - Base.metadata, - Column("experiment_id", ForeignKey("experiments.id"), primary_key=True), - Column("sra_identifier_id", ForeignKey("sra_identifiers.id"), primary_key=True), -) - class Experiment(Base): __tablename__ = "experiments" @@ -117,10 +102,8 @@ class Experiment(Base): secondary=experiments_legacy_keywords_association_table, backref="experiments", ) - doi_identifiers: Mapped[list[DoiIdentifier]] = relationship( - "DoiIdentifier", - secondary=experiments_doi_identifiers_association_table, - backref="experiments", + doi_identifiers: Mapped[list[ExperimentDoiIdentifier]] = relationship( + cascade="all, delete-orphan", order_by=ExperimentDoiIdentifier.identifier ) publication_identifier_associations: Mapped[list[ExperimentPublicationIdentifierAssociation]] = relationship( "ExperimentPublicationIdentifierAssociation", @@ -133,11 +116,8 @@ class Experiment(Base): creator=lambda p: ExperimentPublicationIdentifierAssociation(publication=p, primary=p.primary), ) - # sra_identifiers = relationship('SraIdentifier', secondary=experiments_sra_identifiers_association_table, backref='experiments') - raw_read_identifiers: Mapped[list[RawReadIdentifier]] = relationship( - "RawReadIdentifier", - secondary=experiments_raw_read_identifiers_association_table, - backref="experiments", + raw_read_identifiers: Mapped[list[ExperimentRawReadIdentifier]] = relationship( + cascade="all, delete-orphan", order_by=ExperimentRawReadIdentifier.identifier ) # Unfortunately, we can't use association_proxy here, because in spite of what the documentation seems to imply, it diff --git a/src/mavedb/models/experiment_set.py b/src/mavedb/models/experiment_set.py index a413cf04..f540d7c2 100644 --- a/src/mavedb/models/experiment_set.py +++ b/src/mavedb/models/experiment_set.py @@ -10,10 +10,10 @@ from mavedb.lib.temp_urns import generate_temp_urn from .contributor import Contributor -from .doi_identifier import DoiIdentifier +from .doi_identifier import ExperimentSetDoiIdentifier from .legacy_keyword import LegacyKeyword from .publication_identifier import PublicationIdentifier -from .raw_read_identifier import RawReadIdentifier +from .raw_read_identifier import ExperimentSetRawReadIdentifier from .user import User if TYPE_CHECKING: @@ -27,13 +27,6 @@ ) -experiment_sets_doi_identifiers_association_table = Table( - "experiment_set_doi_identifiers", - Base.metadata, - Column("experiment_set_id", ForeignKey("experiment_sets.id"), primary_key=True), - Column("doi_identifier_id", ForeignKey("doi_identifiers.id"), primary_key=True), -) - experiment_sets_legacy_keywords_association_table = Table( "experiment_set_keywords", Base.metadata, @@ -48,14 +41,6 @@ Column("publication_identifier_id", ForeignKey("publication_identifiers.id"), primary_key=True), ) -# experiment_sets_sra_identifiers_association_table = Table( -experiment_sets_raw_read_identifiers_association_table = Table( - "experiment_set_sra_identifiers", - Base.metadata, - Column("experiment_set_id", ForeignKey("experiment_sets.id"), primary_key=True), - Column("sra_identifier_id", ForeignKey("sra_identifiers.id"), primary_key=True), -) - class ExperimentSet(Base): __tablename__ = "experiment_sets" @@ -89,15 +74,14 @@ class ExperimentSet(Base): contributors: Mapped[list["Contributor"]] = relationship( "Contributor", secondary=experiment_sets_contributors_association_table, backref="experiment_sets" ) - doi_identifiers: Mapped[list[DoiIdentifier]] = relationship( - "DoiIdentifier", secondary=experiment_sets_doi_identifiers_association_table, backref="experiment_sets" + doi_identifiers: Mapped[list[ExperimentSetDoiIdentifier]] = relationship( + cascade="all, delete-orphan", order_by=ExperimentSetDoiIdentifier.identifier ) publication_identifiers: Mapped[list[PublicationIdentifier]] = relationship( "PublicationIdentifier", secondary=experiment_sets_publication_identifiers_association_table, backref="experiment_sets", ) - # sra_identifiers = relationship('SraIdentifier', secondary=experiment_sets_sra_identifiers_association_table, backref='experiment_sets') - raw_read_identifiers: Mapped[list[RawReadIdentifier]] = relationship( - "RawReadIdentifier", secondary=experiment_sets_raw_read_identifiers_association_table, backref="experiment_sets" + raw_read_identifiers: Mapped[list[ExperimentSetRawReadIdentifier]] = relationship( + cascade="all, delete-orphan", order_by=ExperimentSetRawReadIdentifier.identifier ) diff --git a/src/mavedb/models/mixins/external_identifier.py b/src/mavedb/models/mixins/external_identifier.py new file mode 100644 index 00000000..2f67c2ed --- /dev/null +++ b/src/mavedb/models/mixins/external_identifier.py @@ -0,0 +1,14 @@ +class ExternalIdentifier: + """An identifier in an external database that a record cites, and where it resolves. + + Subclasses store ``identifier`` on the citing record's row, supply ``db_name`` as a constant or a column, and + compute ``url`` from them. Annotation sources (ClinVar, gnomAD) aren't citations: their rows hold fetched, + versioned data keyed by ``db_identifier``, so they don't use this. + """ + + identifier: str + db_name: str + + @property + def url(self) -> str: + raise NotImplementedError diff --git a/src/mavedb/models/raw_read_identifier.py b/src/mavedb/models/raw_read_identifier.py index 3bc2a776..9303fac8 100644 --- a/src/mavedb/models/raw_read_identifier.py +++ b/src/mavedb/models/raw_read_identifier.py @@ -1,17 +1,36 @@ -from datetime import date - -from sqlalchemy import Column, Date, Integer, String +from sqlalchemy import Column, ForeignKey, Integer, String from mavedb.db.base import Base +from mavedb.lib.external_identifier_urls import raw_read_url +from mavedb.models.mixins.external_identifier import ExternalIdentifier + + +class RawReadIdentifierLink(ExternalIdentifier): + """A raw read accession (SRA, BioProject, GEO or ArrayExpress) cited by one record.""" + + db_name = "SRA" + + @property + def url(self) -> str: + return raw_read_url(self.identifier) + + +class ScoreSetRawReadIdentifier(RawReadIdentifierLink, Base): + __tablename__ = "scoreset_sra_identifiers" + + score_set_id = Column("scoreset_id", Integer, ForeignKey("scoresets.id"), primary_key=True) + identifier = Column(String, primary_key=True) + + +class ExperimentRawReadIdentifier(RawReadIdentifierLink, Base): + __tablename__ = "experiment_sra_identifiers" + + experiment_id = Column(Integer, ForeignKey("experiments.id"), primary_key=True) + identifier = Column(String, primary_key=True) -class RawReadIdentifier(Base): - __tablename__ = "sra_identifiers" +class ExperimentSetRawReadIdentifier(RawReadIdentifierLink, Base): + __tablename__ = "experiment_set_sra_identifiers" - id = Column(Integer, primary_key=True) - identifier = Column(String, nullable=False) - db_name = Column(String, nullable=False) - db_version = Column(String, nullable=True) - url = Column(String, nullable=True) - creation_date = Column(Date, nullable=False, default=date.today) - modification_date = Column(Date, nullable=False, default=date.today, onupdate=date.today) + experiment_set_id = Column(Integer, ForeignKey("experiment_sets.id"), primary_key=True) + identifier = Column(String, primary_key=True) diff --git a/src/mavedb/models/refseq_identifier.py b/src/mavedb/models/refseq_identifier.py deleted file mode 100644 index e01d14c0..00000000 --- a/src/mavedb/models/refseq_identifier.py +++ /dev/null @@ -1,18 +0,0 @@ -from datetime import date - -from sqlalchemy import Column, Date, Integer, String - -from mavedb.db.base import Base - - -class RefseqIdentifier(Base): - __tablename__ = "refseq_identifiers" - - id = Column(Integer, primary_key=True) - identifier = Column(String, nullable=False) - db_name = Column(String, nullable=False) - db_version = Column(String, nullable=True) - url = Column(String, nullable=True) - reference_html = Column(String, nullable=True) - creation_date = Column(Date, nullable=False, default=date.today) - modification_date = Column(Date, nullable=False, default=date.today, onupdate=date.today) diff --git a/src/mavedb/models/refseq_offset.py b/src/mavedb/models/refseq_offset.py deleted file mode 100644 index ba26b359..00000000 --- a/src/mavedb/models/refseq_offset.py +++ /dev/null @@ -1,16 +0,0 @@ -from sqlalchemy import Column, ForeignKey, Integer -from sqlalchemy.orm import Mapped, backref, relationship - -from mavedb.db.base import Base -from mavedb.models.refseq_identifier import RefseqIdentifier -from mavedb.models.target_gene import TargetGene - - -class RefseqOffset(Base): - __tablename__ = "refseq_offsets" - - identifier_id = Column(Integer, ForeignKey("refseq_identifiers.id"), nullable=False, primary_key=True) - identifier: Mapped[RefseqIdentifier] = relationship(backref=backref("target_gene_offsets", uselist=True)) - target_gene_id = Column(Integer, ForeignKey("target_genes.id"), nullable=False, primary_key=True) - target_gene: Mapped[TargetGene] = relationship(back_populates="refseq_offset", single_parent=True) - offset = Column(Integer, nullable=False) diff --git a/src/mavedb/models/score_set.py b/src/mavedb/models/score_set.py index 1c71bf6b..d4b8a1b9 100644 --- a/src/mavedb/models/score_set.py +++ b/src/mavedb/models/score_set.py @@ -10,7 +10,7 @@ import mavedb.models.score_set_publication_identifier from mavedb.db.base import Base from mavedb.models.contributor import Contributor -from mavedb.models.doi_identifier import DoiIdentifier +from mavedb.models.doi_identifier import ScoreSetDoiIdentifier from mavedb.models.enums.mapping_state import MappingState from mavedb.models.enums.processing_state import ProcessingState from mavedb.models.experiment import Experiment @@ -25,7 +25,6 @@ from mavedb.models.target_gene import TargetGene from mavedb.models.variant import Variant -# from .raw_read_identifier import SraIdentifier from mavedb.lib.temp_urns import generate_temp_urn # TODO Reformat code without removing dependencies whose use is not detected. @@ -38,14 +37,6 @@ ) -score_sets_doi_identifiers_association_table = Table( - "scoreset_doi_identifiers", - Base.metadata, - Column("scoreset_id", ForeignKey("scoresets.id"), primary_key=True), - Column("doi_identifier_id", ForeignKey("doi_identifiers.id"), primary_key=True), -) - - score_sets_legacy_keywords_association_table = Table( "scoreset_keywords", Base.metadata, @@ -62,15 +53,6 @@ ) -# score_sets_sra_identifiers_association_table = Table( -score_sets_raw_read_identifiers_association_table = Table( - "scoreset_sra_identifiers", - Base.metadata, - Column("scoreset_id", ForeignKey("scoresets.id"), primary_key=True), - Column("sra_identifier_id", ForeignKey("sra_identifiers.id"), primary_key=True), -) - - class ScoreSet(Base): __tablename__ = "scoresets" @@ -161,10 +143,8 @@ class ScoreSet(Base): secondary=score_sets_contributors_association_table, backref="score_sets", ) - doi_identifiers: Mapped[list["DoiIdentifier"]] = relationship( - "DoiIdentifier", - secondary=score_sets_doi_identifiers_association_table, - backref="score_sets", + doi_identifiers: Mapped[list[ScoreSetDoiIdentifier]] = relationship( + cascade="all, delete-orphan", order_by=ScoreSetDoiIdentifier.identifier ) publication_identifier_associations: Mapped[ list[mavedb.models.score_set_publication_identifier.ScoreSetPublicationIdentifierAssociation] @@ -180,8 +160,6 @@ class ScoreSet(Base): publication=p, primary=p.primary ), ) - # sra_identifiers = relationship('SraIdentifier', secondary=score_sets_sra_identifiers_association_table, backref='score_sets') - # raw_read_identifiers = relationship('RawReadIdentifier', secondary=score_sets_raw_read_identifiers_association_table,backref='score_sets') meta_analyzes_score_sets: Mapped[list["ScoreSet"]] = relationship( "ScoreSet", secondary=score_sets_meta_analysis_score_sets_association_table, diff --git a/src/mavedb/models/target_gene.py b/src/mavedb/models/target_gene.py index 5419c347..36c399f9 100644 --- a/src/mavedb/models/target_gene.py +++ b/src/mavedb/models/target_gene.py @@ -12,10 +12,8 @@ from mavedb.models.target_sequence import TargetSequence if TYPE_CHECKING: - from mavedb.models.ensembl_offset import EnsemblOffset - from mavedb.models.refseq_offset import RefseqOffset + from mavedb.models.target_gene_external_identifier import TargetGeneExternalIdentifier from mavedb.models.target_gene_mapping import TargetGeneMapping - from mavedb.models.uniprot_offset import UniprotOffset # TODO Reformat code without removing dependencies whose use is not detected. @@ -54,9 +52,11 @@ class TargetGene(Base): creation_date = Column(Date, nullable=False, default=date.today) modification_date = Column(Date, nullable=False, default=date.today, onupdate=date.today) - ensembl_offset: Mapped["EnsemblOffset"] = relationship(back_populates="target_gene", cascade="all, delete-orphan") - refseq_offset: Mapped["RefseqOffset"] = relationship(back_populates="target_gene", cascade="all, delete-orphan") - uniprot_offset: Mapped["UniprotOffset"] = relationship(back_populates="target_gene", cascade="all, delete-orphan") + external_identifiers: Mapped[list["TargetGeneExternalIdentifier"]] = relationship( + back_populates="target_gene", + cascade="all, delete-orphan", + order_by="TargetGeneExternalIdentifier.db_name", + ) target_gene_mappings: Mapped[list["TargetGeneMapping"]] = relationship( "TargetGeneMapping", diff --git a/src/mavedb/models/target_gene_external_identifier.py b/src/mavedb/models/target_gene_external_identifier.py new file mode 100644 index 00000000..5f0c3649 --- /dev/null +++ b/src/mavedb/models/target_gene_external_identifier.py @@ -0,0 +1,37 @@ +from typing import TYPE_CHECKING + +from sqlalchemy import CheckConstraint, Column, ForeignKey, Integer, String +from sqlalchemy.orm import Mapped, relationship + +from mavedb.db.base import Base +from mavedb.lib.external_identifier_urls import external_gene_identifier_url +from mavedb.lib.validation.constants.identifier import valid_dbnames +from mavedb.models.mixins.external_identifier import ExternalIdentifier + +if TYPE_CHECKING: + from mavedb.models.target_gene import TargetGene + + +class TargetGeneExternalIdentifier(ExternalIdentifier, Base): + """A target gene's identifier in one external database, and the offset of the target within it. + + A target gene has at most one identifier per database. + """ + + __tablename__ = "target_gene_external_identifiers" + __table_args__ = ( + CheckConstraint( + "db_name IN ({})".format(", ".join(f"'{db_name}'" for db_name in valid_dbnames)), + name="ck_target_gene_external_identifiers_db_name_valid", + ), + ) + + target_gene_id = Column(Integer, ForeignKey("target_genes.id", ondelete="CASCADE"), primary_key=True) + target_gene: Mapped["TargetGene"] = relationship(back_populates="external_identifiers") + db_name: Mapped[str] = Column(String, primary_key=True) + identifier: Mapped[str] = Column(String, nullable=False) + offset: Mapped[int] = Column(Integer, nullable=False) + + @property + def url(self) -> str: + return external_gene_identifier_url(self.db_name, self.identifier) diff --git a/src/mavedb/models/uniprot_identifier.py b/src/mavedb/models/uniprot_identifier.py deleted file mode 100644 index f44f334e..00000000 --- a/src/mavedb/models/uniprot_identifier.py +++ /dev/null @@ -1,18 +0,0 @@ -from datetime import date - -from sqlalchemy import Column, Date, Integer, String - -from mavedb.db.base import Base - - -class UniprotIdentifier(Base): - __tablename__ = "uniprot_identifiers" - - id = Column(Integer, primary_key=True) - identifier = Column(String, nullable=False) - db_name = Column(String, nullable=False) - db_version = Column(String, nullable=True) - url = Column(String, nullable=True) - reference_html = Column(String, nullable=True) - creation_date = Column(Date, nullable=False, default=date.today) - modification_date = Column(Date, nullable=False, default=date.today, onupdate=date.today) diff --git a/src/mavedb/models/uniprot_offset.py b/src/mavedb/models/uniprot_offset.py deleted file mode 100644 index d97fdb2c..00000000 --- a/src/mavedb/models/uniprot_offset.py +++ /dev/null @@ -1,19 +0,0 @@ -from sqlalchemy import Column, ForeignKey, Integer -from sqlalchemy.orm import Mapped, backref, relationship - -from mavedb.db.base import Base -from mavedb.models.target_gene import TargetGene -from mavedb.models.uniprot_identifier import UniprotIdentifier - - -class UniprotOffset(Base): - __tablename__ = "uniprot_offsets" - - identifier_id = Column(Integer, ForeignKey("uniprot_identifiers.id"), nullable=False, primary_key=True) - identifier: Mapped[UniprotIdentifier] = relationship( - "UniprotIdentifier", backref=backref("target_gene_offsets", uselist=True) - ) - target_gene_id = Column(Integer, ForeignKey("target_genes.id"), nullable=False, primary_key=True) - target_gene: Mapped[TargetGene] = relationship(back_populates="uniprot_offset", single_parent=True) - - offset = Column(Integer, nullable=False) diff --git a/src/mavedb/routers/doi_identifiers.py b/src/mavedb/routers/doi_identifiers.py index a13bb804..53946244 100644 --- a/src/mavedb/routers/doi_identifiers.py +++ b/src/mavedb/routers/doi_identifiers.py @@ -1,11 +1,20 @@ from typing import Any, List from fastapi import APIRouter, Depends, HTTPException -from sqlalchemy import func +from sqlalchemy import select from sqlalchemy.orm import Session from mavedb import deps -from mavedb.models.doi_identifier import DoiIdentifier +from mavedb.lib.authorization import get_principal +from mavedb.lib.identifiers import search_identifiers_on_readable_records +from mavedb.lib.permissions.principal import Principal +from mavedb.lib.permissions.experiment_set import ExperimentSetViewer +from mavedb.lib.permissions.experiment import ExperimentViewer +from mavedb.lib.permissions.score_set import ScoreSetViewer +from mavedb.models.doi_identifier import ExperimentDoiIdentifier, ExperimentSetDoiIdentifier, ScoreSetDoiIdentifier +from mavedb.models.experiment import Experiment +from mavedb.models.experiment_set import ExperimentSet +from mavedb.models.score_set import ScoreSet from mavedb.routers.shared import BASE_400_RESPONSE, PUBLIC_ERROR_RESPONSES, ROUTER_BASE_PREFIX from mavedb.view_models import doi_identifier from mavedb.view_models.search import TextSearch @@ -31,20 +40,42 @@ responses={**BASE_400_RESPONSE}, summary="Search DOI identifiers", ) -def search_doi_identifiers(search: TextSearch, db: Session = Depends(deps.get_db)) -> Any: +def search_doi_identifiers( + search: TextSearch, + db: Session = Depends(deps.get_db), + principal: Principal = Depends(get_principal), +) -> Any: """ - Search DOI identifiers based on the provided text. + Search the DOI identifiers cited by records you can read. """ - - query = db.query(DoiIdentifier) - - if search.text and len(search.text.strip()) > 0: - lower_search_text = search.text.strip().lower() - query = query.filter(func.lower(DoiIdentifier.identifier).contains(lower_search_text)) - else: + if not search.text or not search.text.strip(): raise HTTPException(status_code=400, detail="Search text is required") - items = query.order_by(DoiIdentifier.identifier).limit(50).all() - if not items: - items = [] - return items + return search_identifiers_on_readable_records( + db, + principal, + search.text.strip(), + [ + ( + ScoreSetDoiIdentifier, + ScoreSetViewer, + select(ScoreSetDoiIdentifier, ScoreSet).join( + ScoreSet, ScoreSet.id == ScoreSetDoiIdentifier.score_set_id + ), + ), + ( + ExperimentDoiIdentifier, + ExperimentViewer, + select(ExperimentDoiIdentifier, Experiment).join( + Experiment, Experiment.id == ExperimentDoiIdentifier.experiment_id + ), + ), + ( + ExperimentSetDoiIdentifier, + ExperimentSetViewer, + select(ExperimentSetDoiIdentifier, ExperimentSet).join( + ExperimentSet, ExperimentSet.id == ExperimentSetDoiIdentifier.experiment_set_id + ), + ), + ], + ) diff --git a/src/mavedb/routers/experiments.py b/src/mavedb/routers/experiments.py index a58d127b..4108757c 100644 --- a/src/mavedb/routers/experiments.py +++ b/src/mavedb/routers/experiments.py @@ -16,11 +16,7 @@ from mavedb.lib.collections import readable_official_collections from mavedb.lib.experiments import enrich_experiment_with_num_score_sets from mavedb.lib.experiments import search_experiments as _search_experiments -from mavedb.lib.identifiers import ( - find_or_create_doi_identifier, - find_or_create_publication_identifier, - find_or_create_raw_read_identifier, -) +from mavedb.lib.identifiers import distinct_identifiers, find_or_create_publication_identifier from mavedb.lib.keywords import search_keyword from mavedb.lib.logging import LoggedRoute from mavedb.lib.logging.context import logging_context, save_to_logging_context @@ -32,9 +28,11 @@ from mavedb.lib.validation.exceptions import ValidationError from mavedb.lib.validation.keywords import validate_keyword_list from mavedb.models.contributor import Contributor +from mavedb.models.doi_identifier import ExperimentDoiIdentifier from mavedb.models.experiment import Experiment from mavedb.models.experiment_controlled_keyword import ExperimentControlledKeywordAssociation from mavedb.models.experiment_set import ExperimentSet +from mavedb.models.raw_read_identifier import ExperimentRawReadIdentifier from mavedb.models.score_set import ScoreSet from mavedb.routers.shared import ( ACCESS_CONTROL_ERROR_RESPONSES, @@ -323,15 +321,16 @@ async def create_experiment( logger.error(msg="Could not find ORCID user with the provided user ID.", extra=logging_context()) raise HTTPException(status_code=404, detail=str(e)) + doi_identifiers = [ + ExperimentDoiIdentifier(identifier=identifier) + for identifier in distinct_identifiers(i.identifier for i in item_create.doi_identifiers or []) + ] + raw_read_identifiers = [ + ExperimentRawReadIdentifier(identifier=identifier) + for identifier in distinct_identifiers(i.identifier for i in item_create.raw_read_identifiers or []) + ] + try: - doi_identifiers = [ - await find_or_create_doi_identifier(db, identifier.identifier) - for identifier in item_create.doi_identifiers or [] - ] - raw_read_identifiers = [ - await find_or_create_raw_read_identifier(db, identifier.identifier) - for identifier in item_create.raw_read_identifiers or [] - ] primary_publication_identifiers = [ await find_or_create_publication_identifier(db, identifier.identifier, identifier.db_name) for identifier in item_create.primary_publication_identifiers or [] @@ -474,16 +473,16 @@ async def update_experiment( logger.error(msg="Could not find ORCID user with the provided user ID.", extra=logging_context()) raise HTTPException(status_code=404, detail=str(e)) - try: - doi_identifiers = [ - await find_or_create_doi_identifier(db, identifier.identifier) - for identifier in item_update.doi_identifiers or [] - ] - raw_read_identifiers = [ - await find_or_create_raw_read_identifier(db, identifier.identifier) - for identifier in item_update.raw_read_identifiers or [] - ] + doi_identifiers = [ + ExperimentDoiIdentifier(identifier=identifier) + for identifier in distinct_identifiers(i.identifier for i in item_update.doi_identifiers or []) + ] + raw_read_identifiers = [ + ExperimentRawReadIdentifier(identifier=identifier) + for identifier in distinct_identifiers(i.identifier for i in item_update.raw_read_identifiers or []) + ] + try: primary_publication_identifiers = [ await find_or_create_publication_identifier(db, identifier.identifier, identifier.db_name) for identifier in item_update.primary_publication_identifiers or [] diff --git a/src/mavedb/routers/genes.py b/src/mavedb/routers/genes.py index 5e6b10a0..c3c88e4f 100644 --- a/src/mavedb/routers/genes.py +++ b/src/mavedb/routers/genes.py @@ -11,16 +11,13 @@ from mavedb.lib.hgnc.client import fetch_gene_info from mavedb.lib.logging import LoggedRoute from mavedb.lib.logging.context import logging_context, save_to_logging_context -from mavedb.models.ensembl_offset import EnsemblOffset from mavedb.models.experiment import Experiment from mavedb.models.experiment_controlled_keyword import ExperimentControlledKeywordAssociation from mavedb.models.experiment_publication_identifier import ExperimentPublicationIdentifierAssociation -from mavedb.models.refseq_offset import RefseqOffset from mavedb.models.score_set import ScoreSet from mavedb.models.score_set_publication_identifier import ScoreSetPublicationIdentifierAssociation from mavedb.models.target_gene import TargetGene from mavedb.models.target_sequence import TargetSequence -from mavedb.models.uniprot_offset import UniprotOffset from mavedb.routers.shared import GATEWAY_ERROR_RESPONSES, PUBLIC_ERROR_RESPONSES, ROUTER_BASE_PREFIX from mavedb.view_models.gene import GeneResponse from mavedb.view_models.score_set import ShortScoreSet @@ -89,9 +86,7 @@ def _score_set_load_options(): ScoreSetPublicationIdentifierAssociation.publication ), selectinload(ScoreSet.target_genes).options( - joinedload(TargetGene.ensembl_offset).joinedload(EnsemblOffset.identifier), - joinedload(TargetGene.refseq_offset).joinedload(RefseqOffset.identifier), - joinedload(TargetGene.uniprot_offset).joinedload(UniprotOffset.identifier), + joinedload(TargetGene.external_identifiers), joinedload(TargetGene.target_sequence).joinedload(TargetSequence.taxonomy), joinedload(TargetGene.target_accession), ), diff --git a/src/mavedb/routers/raw_read_identifiers.py b/src/mavedb/routers/raw_read_identifiers.py index 90ff207b..075b94a4 100644 --- a/src/mavedb/routers/raw_read_identifiers.py +++ b/src/mavedb/routers/raw_read_identifiers.py @@ -1,11 +1,24 @@ from typing import Any, List from fastapi import APIRouter, Depends, HTTPException -from sqlalchemy import func +from sqlalchemy import select from sqlalchemy.orm import Session from mavedb import deps -from mavedb.models.raw_read_identifier import RawReadIdentifier +from mavedb.lib.authorization import get_principal +from mavedb.lib.identifiers import search_identifiers_on_readable_records +from mavedb.lib.permissions.principal import Principal +from mavedb.lib.permissions.experiment_set import ExperimentSetViewer +from mavedb.lib.permissions.experiment import ExperimentViewer +from mavedb.lib.permissions.score_set import ScoreSetViewer +from mavedb.models.experiment import Experiment +from mavedb.models.experiment_set import ExperimentSet +from mavedb.models.raw_read_identifier import ( + ExperimentRawReadIdentifier, + ExperimentSetRawReadIdentifier, + ScoreSetRawReadIdentifier, +) +from mavedb.models.score_set import ScoreSet from mavedb.routers.shared import BASE_400_RESPONSE, PUBLIC_ERROR_RESPONSES, ROUTER_BASE_PREFIX from mavedb.view_models import raw_read_identifier from mavedb.view_models.search import TextSearch @@ -31,20 +44,42 @@ responses={**BASE_400_RESPONSE}, summary="Search Raw Read identifiers", ) -def search_raw_read_identifiers(search: TextSearch, db: Session = Depends(deps.get_db)) -> Any: +def search_raw_read_identifiers( + search: TextSearch, + db: Session = Depends(deps.get_db), + principal: Principal = Depends(get_principal), +) -> Any: """ - Search Raw Read identifiers. + Search the Raw Read identifiers cited by records you can read. """ - - query = db.query(RawReadIdentifier) - - if search.text and len(search.text.strip()) > 0: - lower_search_text = search.text.strip().lower() - query = query.filter(func.lower(RawReadIdentifier.identifier).contains(lower_search_text)) - else: + if not search.text or not search.text.strip(): raise HTTPException(status_code=400, detail="Search text is required") - items = query.order_by(RawReadIdentifier.identifier).limit(50).all() - if not items: - items = [] - return items + return search_identifiers_on_readable_records( + db, + principal, + search.text.strip(), + [ + ( + ScoreSetRawReadIdentifier, + ScoreSetViewer, + select(ScoreSetRawReadIdentifier, ScoreSet).join( + ScoreSet, ScoreSet.id == ScoreSetRawReadIdentifier.score_set_id + ), + ), + ( + ExperimentRawReadIdentifier, + ExperimentViewer, + select(ExperimentRawReadIdentifier, Experiment).join( + Experiment, Experiment.id == ExperimentRawReadIdentifier.experiment_id + ), + ), + ( + ExperimentSetRawReadIdentifier, + ExperimentSetViewer, + select(ExperimentSetRawReadIdentifier, ExperimentSet).join( + ExperimentSet, ExperimentSet.id == ExperimentSetRawReadIdentifier.experiment_set_id + ), + ), + ], + ) diff --git a/src/mavedb/routers/score_sets.py b/src/mavedb/routers/score_sets.py index 01ed631d..e623fe79 100644 --- a/src/mavedb/routers/score_sets.py +++ b/src/mavedb/routers/score_sets.py @@ -57,9 +57,9 @@ from mavedb.lib.experiments import enrich_experiment_with_num_score_sets from mavedb.lib.gnomad import get_gnomad_variants_with_variant_urns from mavedb.lib.identifiers import ( - create_external_gene_identifier_offset, - find_or_create_doi_identifier, + distinct_identifiers, find_or_create_publication_identifier, + target_gene_external_identifiers, ) from mavedb.lib.logging import LoggedRoute from mavedb.lib.logging.context import logging_context, save_to_logging_context @@ -95,6 +95,7 @@ from mavedb.lib.variant_detail import load_variant_detail_inputs from mavedb.lib.workflow.kickoff import enqueue_pipeline_for_score_set from mavedb.models.contributor import Contributor +from mavedb.models.doi_identifier import ScoreSetDoiIdentifier from mavedb.models.enums.processing_state import ProcessingState from mavedb.models.experiment import Experiment from mavedb.models.license import License @@ -299,7 +300,8 @@ async def score_set_update( DoiIdentifierCreate(**identifier) for identifier in item_update_dict.get("doi_identifiers") or [] ] item.doi_identifiers = [ - await find_or_create_doi_identifier(db, identifier.identifier) for identifier in doi_identifiers_list + ScoreSetDoiIdentifier(identifier=identifier) + for identifier in distinct_identifiers(i.identifier for i in doi_identifiers_list) ] if any(key in item_update_dict for key in ["primary_publication_identifiers", "secondary_publication_identifiers"]): @@ -477,16 +479,7 @@ async def score_set_update( logger.info(msg="Failed to create score set; Could not infer target type.", extra=logging_context()) raise ValueError("One of either `target_accession` or `target_gene` should be present") - for external_gene_identifier_offset_create in gene.external_identifiers: - offset = external_gene_identifier_offset_create.offset - identifier_create = external_gene_identifier_offset_create.identifier - await create_external_gene_identifier_offset( - db, - target_gene, - identifier_create.db_name, - identifier_create.identifier, - offset, - ) + target_gene.external_identifiers = target_gene_external_identifiers(gene.external_identifiers) targets.append(target_gene) @@ -2115,11 +2108,12 @@ async def create_score_set( logger.error(msg="Could not find ORCID user with the provided user ID.", extra=logging_context()) raise HTTPException(status_code=404, detail=str(e)) + doi_identifiers = [ + ScoreSetDoiIdentifier(identifier=identifier) + for identifier in distinct_identifiers(i.identifier for i in item_create.doi_identifiers or []) + ] + try: - doi_identifiers = [ - await find_or_create_doi_identifier(db, identifier.identifier) - for identifier in item_create.doi_identifiers or [] - ] primary_publication_identifiers = [ await find_or_create_publication_identifier(db, identifier.identifier, identifier.db_name) for identifier in item_create.primary_publication_identifiers or [] @@ -2268,16 +2262,7 @@ async def create_score_set( logger.info(msg="Failed to create score set; Could not infer target type.", extra=logging_context()) raise ValueError("One of either `target_accession` or `target_gene` should be present") - for external_gene_identifier_offset_create in gene.external_identifiers: - offset = external_gene_identifier_offset_create.offset - identifier_create = external_gene_identifier_offset_create.identifier - await create_external_gene_identifier_offset( - db, - target_gene, - identifier_create.db_name, - identifier_create.identifier, - offset, - ) + target_gene.external_identifiers = target_gene_external_identifiers(gene.external_identifiers) targets.append(target_gene) diff --git a/src/mavedb/routers/statistics.py b/src/mavedb/routers/statistics.py index 5deea563..ae30b80c 100644 --- a/src/mavedb/routers/statistics.py +++ b/src/mavedb/routers/statistics.py @@ -9,33 +9,20 @@ from mavedb.deps import get_db from mavedb.models.controlled_keyword import ControlledKeyword -from mavedb.models.doi_identifier import DoiIdentifier -from mavedb.models.ensembl_identifier import EnsemblIdentifier -from mavedb.models.ensembl_offset import EnsemblOffset -from mavedb.models.experiment import ( - Experiment, - experiments_doi_identifiers_association_table, - experiments_raw_read_identifiers_association_table, -) +from mavedb.models.doi_identifier import ExperimentDoiIdentifier, ScoreSetDoiIdentifier +from mavedb.models.experiment import Experiment from mavedb.models.experiment_controlled_keyword import ExperimentControlledKeywordAssociation from mavedb.models.experiment_publication_identifier import ExperimentPublicationIdentifierAssociation from mavedb.models.publication_identifier import PublicationIdentifier from mavedb.models.published_variant import PublishedVariantsMV -from mavedb.models.raw_read_identifier import RawReadIdentifier -from mavedb.models.refseq_identifier import RefseqIdentifier -from mavedb.models.refseq_offset import RefseqOffset -from mavedb.models.score_set import ( - ScoreSet, - score_sets_doi_identifiers_association_table, - score_sets_raw_read_identifiers_association_table, -) +from mavedb.models.raw_read_identifier import ExperimentRawReadIdentifier, ScoreSetRawReadIdentifier +from mavedb.models.score_set import ScoreSet from mavedb.models.score_set_publication_identifier import ScoreSetPublicationIdentifierAssociation from mavedb.models.target_accession import TargetAccession from mavedb.models.target_gene import TargetGene +from mavedb.models.target_gene_external_identifier import TargetGeneExternalIdentifier from mavedb.models.target_sequence import TargetSequence from mavedb.models.taxonomy import Taxonomy -from mavedb.models.uniprot_identifier import UniprotIdentifier -from mavedb.models.uniprot_offset import UniprotOffset from mavedb.models.user import User from mavedb.routers.shared import PUBLIC_ERROR_RESPONSES, ROUTER_BASE_PREFIX @@ -61,6 +48,16 @@ type[ExperimentControlledKeywordAssociation], type[ExperimentPublicationIdentifierAssociation], type[ScoreSetPublicationIdentifierAssociation], + type[ExperimentDoiIdentifier], + type[ExperimentRawReadIdentifier], + type[ScoreSetDoiIdentifier], + type[ScoreSetRawReadIdentifier], +] +RecordIdentifierLinks = Union[ + type[ExperimentDoiIdentifier], + type[ExperimentRawReadIdentifier], + type[ScoreSetDoiIdentifier], + type[ScoreSetRawReadIdentifier], ] @@ -101,15 +98,15 @@ def _model_and_association_from_record_field( } record_to_assc_map: dict[RecordNames, dict[RecordFields, RecordAssociationTables]] = { RecordNames.experiment: { - RecordFields.doiIdentifiers: experiments_doi_identifiers_association_table, + RecordFields.doiIdentifiers: ExperimentDoiIdentifier, RecordFields.publicationIdentifiers: ExperimentPublicationIdentifierAssociation, - RecordFields.rawReadIdentifiers: experiments_raw_read_identifiers_association_table, + RecordFields.rawReadIdentifiers: ExperimentRawReadIdentifier, RecordFields.keywords: ExperimentControlledKeywordAssociation, }, RecordNames.scoreSet: { - RecordFields.doiIdentifiers: score_sets_doi_identifiers_association_table, + RecordFields.doiIdentifiers: ScoreSetDoiIdentifier, RecordFields.publicationIdentifiers: ScoreSetPublicationIdentifierAssociation, - RecordFields.rawReadIdentifiers: score_sets_raw_read_identifiers_association_table, + RecordFields.rawReadIdentifiers: ScoreSetRawReadIdentifier, }, } @@ -130,6 +127,31 @@ def _count_for_identifier_in_query(db: Session, query: Select[tuple[Any, int]]) return {value: count for value, count in db.execute(query).all() if value is not None} +def _count_identifiers_on_published_records(db: Session, record: RecordNames, field: RecordFields) -> dict[str, int]: + queried_model, queried_link = _model_and_association_from_record_field(record, field) + + if queried_link is None: + raise HTTPException(500, f"No association table associated with the {field.value} field when one was expected.") + + link: RecordIdentifierLinks = queried_link # type: ignore[assignment] + query = _join_model_and_filter_unpublished( + select(link.identifier, func.count(link.identifier)), queried_model + ).group_by(link.identifier) + + return _count_for_identifier_in_query(db, query) + + +def _count_target_gene_identifiers_on_published_score_sets(db: Session, db_name: str) -> dict[str, int]: + query = _join_model_and_filter_unpublished( + select(TargetGeneExternalIdentifier.identifier, func.count(TargetGeneExternalIdentifier.identifier)) + .where(TargetGeneExternalIdentifier.db_name == db_name) + .join(TargetGene), + ScoreSet, + ).group_by(TargetGeneExternalIdentifier.identifier) + + return _count_for_identifier_in_query(db, query) + + ######################################################################################## # Record statistics ######################################################################################## @@ -216,21 +238,10 @@ def experiment_publication_identifier_statistics( ) def experiment_raw_read_identifier_statistics(record: RecordNames, db: Session = Depends(get_db)) -> dict[str, int]: """ - Returns a dictionary of counts for the distinct values of the `identifier` field (member of the `raw_read_identifiers` table). + Returns a dictionary of counts for each distinct raw read identifier. Don't include any raw read identifiers from unpublished experiments. """ - queried_model, queried_assc = _model_and_association_from_record_field(record, RecordFields.rawReadIdentifiers) - - if queried_assc is None: - raise HTTPException( - 500, "No association table associated with the raw read identifiers field when one was expected." - ) - - query = _join_model_and_filter_unpublished( - select(RawReadIdentifier.identifier, func.count(RawReadIdentifier.identifier)).join(queried_assc), queried_model - ).group_by(RawReadIdentifier.identifier) - - return _count_for_identifier_in_query(db, query) + return _count_identifiers_on_published_records(db, record, RecordFields.rawReadIdentifiers) @router.get( @@ -241,21 +252,10 @@ def experiment_raw_read_identifier_statistics(record: RecordNames, db: Session = ) def experiment_doi_identifiers_statistics(record: RecordNames, db: Session = Depends(get_db)) -> dict[str, int]: """ - Returns a dictionary of counts for the distinct values of the `identifier` field (member of the `doi_identifiers` table). + Returns a dictionary of counts for each distinct DOI identifier. Don't include any DOI identifiers from unpublished experiments. """ - queried_model, queried_assc = _model_and_association_from_record_field(record, RecordFields.doiIdentifiers) - - if queried_assc is None: - raise HTTPException( - 500, "No association table associated with the doi identifiers field when one was expected." - ) - - query = _join_model_and_filter_unpublished( - select(DoiIdentifier.identifier, func.count(DoiIdentifier.identifier)).join(queried_assc), queried_model - ).group_by(DoiIdentifier.identifier) - - return _count_for_identifier_in_query(db, query) + return _count_identifiers_on_published_records(db, record, RecordFields.doiIdentifiers) @router.get( @@ -519,17 +519,10 @@ def target_genes_organism_counts(db: Session = Depends(get_db)) -> dict[str, int ) def target_genes_ensembl_identifier_counts(db: Session = Depends(get_db)) -> dict[str, int]: """ - Returns a dictionary of counts for the distinct values of the `identifier` field (member of the `ensembl_identifiers` table). + Returns a dictionary of counts for each distinct Ensembl identifier of a target gene. Don't include any NULL field values. Don't include any targets from unpublished score sets. """ - query = _join_model_and_filter_unpublished( - select(EnsemblIdentifier.identifier, func.count(EnsemblIdentifier.identifier)) - .join(EnsemblOffset) - .join(TargetGene), - ScoreSet, - ).group_by(EnsemblIdentifier.identifier) - - return _count_for_identifier_in_query(db, query) + return _count_target_gene_identifiers_on_published_score_sets(db, "Ensembl") @router.get( @@ -540,17 +533,10 @@ def target_genes_ensembl_identifier_counts(db: Session = Depends(get_db)) -> dic ) def target_genes_refseq_identifier_counts(db: Session = Depends(get_db)) -> dict[str, int]: """ - Returns a dictionary of counts for the distinct values of the `identifier` field (member of the `refseq_identifiers` table). + Returns a dictionary of counts for each distinct RefSeq identifier of a target gene. Don't include any NULL field values. Don't include any targets from unpublished score sets. """ - query = _join_model_and_filter_unpublished( - select(RefseqIdentifier.identifier, func.count(RefseqIdentifier.identifier)) - .join(RefseqOffset) - .join(TargetGene), - ScoreSet, - ).group_by(RefseqIdentifier.identifier) - - return _count_for_identifier_in_query(db, query) + return _count_target_gene_identifiers_on_published_score_sets(db, "RefSeq") @router.get( @@ -561,17 +547,10 @@ def target_genes_refseq_identifier_counts(db: Session = Depends(get_db)) -> dict ) def target_genes_uniprot_identifier_counts(db: Session = Depends(get_db)) -> dict[str, int]: """ - Returns a dictionary of counts for the distinct values of the `identifier` field (member of the `uniprot_identifiers` table). + Returns a dictionary of counts for each distinct UniProt identifier of a target gene. Don't include any NULL field values. Don't include any targets from unpublished score sets. """ - query = _join_model_and_filter_unpublished( - select(UniprotIdentifier.identifier, func.count(UniprotIdentifier.identifier)) - .join(UniprotOffset) - .join(TargetGene), - ScoreSet, - ).group_by(UniprotIdentifier.identifier) - - return _count_for_identifier_in_query(db, query) + return _count_target_gene_identifiers_on_published_score_sets(db, "UniProt") @router.get( diff --git a/src/mavedb/routers/target_gene_identifiers.py b/src/mavedb/routers/target_gene_identifiers.py index 06180209..656c495d 100644 --- a/src/mavedb/routers/target_gene_identifiers.py +++ b/src/mavedb/routers/target_gene_identifiers.py @@ -1,11 +1,18 @@ from typing import Any, List from fastapi import APIRouter, Depends, HTTPException -from sqlalchemy import func +from sqlalchemy import select from sqlalchemy.orm import Session from mavedb import deps -from mavedb.lib.identifiers import EXTERNAL_GENE_IDENTIFIER_CLASSES +from mavedb.lib.authorization import get_principal +from mavedb.lib.identifiers import search_identifiers_on_readable_records +from mavedb.lib.permissions.principal import Principal +from mavedb.lib.permissions.score_set import ScoreSetViewer +from mavedb.lib.validation.constants.identifier import valid_dbnames +from mavedb.models.score_set import ScoreSet +from mavedb.models.target_gene import TargetGene +from mavedb.models.target_gene_external_identifier import TargetGeneExternalIdentifier from mavedb.routers.shared import BASE_400_RESPONSE, PUBLIC_ERROR_RESPONSES, ROUTER_BASE_PREFIX from mavedb.view_models import external_gene_identifier from mavedb.view_models.search import TextSearch @@ -31,28 +38,36 @@ summary="Search target gene identifiers", responses={**BASE_400_RESPONSE}, ) -def search_target_gene_identifiers(db_name: str, search: TextSearch, db: Session = Depends(deps.get_db)) -> Any: +def search_target_gene_identifiers( + db_name: str, + search: TextSearch, + db: Session = Depends(deps.get_db), + principal: Principal = Depends(get_principal), +) -> Any: """ - Search target gene identifiers. + Search the target gene identifiers of score sets you can read. """ - if db_name not in EXTERNAL_GENE_IDENTIFIER_CLASSES: + if db_name not in valid_dbnames: raise HTTPException( status_code=422, - detail=f"Unexpected db_name: {db_name}. Expected one of: {list(EXTERNAL_GENE_IDENTIFIER_CLASSES.keys())}", + detail=f"Unexpected db_name: {db_name}. Expected one of: {valid_dbnames}", ) - identifier_class = EXTERNAL_GENE_IDENTIFIER_CLASSES[db_name] - assert hasattr(identifier_class, "identifier") - - query = db.query(identifier_class) - - if search.text and len(search.text.strip()) > 0: - lower_search_text = search.text.strip().lower() - query = query.filter(func.lower(identifier_class.identifier).contains(lower_search_text)) - else: + if not search.text or not search.text.strip(): raise HTTPException(status_code=400, detail="Search text is required") - items = query.order_by(identifier_class.identifier).limit(50).all() - if not items: - items = [] - return items + return search_identifiers_on_readable_records( + db, + principal, + search.text.strip(), + [ + ( + TargetGeneExternalIdentifier, + ScoreSetViewer, + select(TargetGeneExternalIdentifier, ScoreSet) + .join(TargetGeneExternalIdentifier.target_gene) + .join(TargetGene.score_set) + .where(TargetGeneExternalIdentifier.db_name == db_name), + ) + ], + ) diff --git a/src/mavedb/view_models/components/doi.py b/src/mavedb/view_models/components/doi.py new file mode 100644 index 00000000..9995e9b3 --- /dev/null +++ b/src/mavedb/view_models/components/doi.py @@ -0,0 +1,30 @@ +from typing import Annotated + +import idutils +from pydantic import AfterValidator + +from mavedb.lib.validation.exceptions import ValidationError + + +def canonical_doi(value: str) -> str: + """Validate a DOI and return its canonical form. + + Args: + value: A DOI, bare or with a ``doi:`` or resolver URL prefix. + + Returns: + The bare DOI, upper-cased. + + Raises: + ValidationError: If ``value`` is not a DOI. + """ + if not idutils.is_doi(value): + raise ValidationError("'{}' is not a valid DOI identifier.".format(value)) + + # DOIs are case-insensitive, and the DOI registry stores them upper-cased. Storing that form keeps a record + # from citing the same DOI twice. + return idutils.normalize_doi(value).upper() + + +Doi = Annotated[str, AfterValidator(canonical_doi)] +"""A DOI field, validated and stored bare and upper-cased.""" diff --git a/src/mavedb/view_models/doi_identifier.py b/src/mavedb/view_models/doi_identifier.py index d9afdb24..f94f172c 100644 --- a/src/mavedb/view_models/doi_identifier.py +++ b/src/mavedb/view_models/doi_identifier.py @@ -1,10 +1,6 @@ -import idutils - -from pydantic import field_validator - -from mavedb.lib.validation.exceptions import ValidationError from mavedb.view_models import record_type_validator, set_record_type from mavedb.view_models.base.base import BaseModel +from mavedb.view_models.components.doi import Doi class DoiIdentifierBase(BaseModel): @@ -12,16 +8,11 @@ class DoiIdentifierBase(BaseModel): class DoiIdentifierCreate(DoiIdentifierBase): - @field_validator("identifier") - def must_be_valid_doi(cls, v: str): - if not idutils.is_doi(v): - raise ValidationError("'{}' is not a valid DOI identifier.".format(v)) - return v + identifier: Doi # Properties shared by models stored in DB class SavedDoiIdentifier(DoiIdentifierBase): - id: int record_type: str = None # type: ignore url: str diff --git a/src/mavedb/view_models/external_gene_identifier.py b/src/mavedb/view_models/external_gene_identifier.py index 0dd8a7e2..738a898e 100644 --- a/src/mavedb/view_models/external_gene_identifier.py +++ b/src/mavedb/view_models/external_gene_identifier.py @@ -30,7 +30,6 @@ class SavedExternalGeneIdentifier(ExternalGeneIdentifierBase): record_type: str = None # type: ignore db_version: Optional[str] = None url: Optional[str] = None - reference_html: Optional[str] = None _record_type_factory = record_type_validator()(set_record_type) diff --git a/src/mavedb/view_models/external_gene_identifier_offset.py b/src/mavedb/view_models/external_gene_identifier_offset.py index 51876ae7..e49313b1 100644 --- a/src/mavedb/view_models/external_gene_identifier_offset.py +++ b/src/mavedb/view_models/external_gene_identifier_offset.py @@ -1,5 +1,8 @@ -from pydantic import field_validator +from typing import Any +from pydantic import field_validator, model_validator + +from mavedb.models.target_gene_external_identifier import TargetGeneExternalIdentifier from mavedb.view_models import external_gene_identifier, record_type_validator, set_record_type from mavedb.view_models.base.base import BaseModel @@ -29,6 +32,18 @@ class SavedExternalGeneIdentifierOffset(ExternalGeneIdentifierOffsetBase): class Config: from_attributes = True + # A stored row holds the identifier beside its offset. The api nests it, as it did when each database had + # its own identifier table. + @model_validator(mode="before") + @classmethod + def nest_stored_identifier(cls, data: Any) -> Any: + if isinstance(data, TargetGeneExternalIdentifier): + return { + "identifier": {"db_name": data.db_name, "identifier": data.identifier, "url": data.url}, + "offset": data.offset, + } + return data + # Properties to return to non-admin clients class ExternalGeneIdentifierOffset(SavedExternalGeneIdentifierOffset): diff --git a/src/mavedb/view_models/raw_read_identifier.py b/src/mavedb/view_models/raw_read_identifier.py index 2a273ac1..d91f3e4e 100644 --- a/src/mavedb/view_models/raw_read_identifier.py +++ b/src/mavedb/view_models/raw_read_identifier.py @@ -12,7 +12,6 @@ class RawReadIdentifierCreate(RawReadIdentifierBase): # Properties shared by models stored in DB class SavedRawReadIdentifier(RawReadIdentifierBase): - id: int record_type: str = None # type: ignore url: str diff --git a/src/mavedb/view_models/target_gene.py b/src/mavedb/view_models/target_gene.py index eb27ca86..5e62f2f9 100644 --- a/src/mavedb/view_models/target_gene.py +++ b/src/mavedb/view_models/target_gene.py @@ -5,7 +5,7 @@ from typing_extensions import Self from mavedb.lib.validation.exceptions import ValidationError -from mavedb.lib.validation.transform import transform_external_identifier_offsets_to_list, transform_score_set_to_urn +from mavedb.lib.validation.transform import transform_score_set_to_urn from mavedb.models.enums.target_category import TargetCategory from mavedb.view_models import external_gene_identifier_offset, record_type_validator, set_record_type from mavedb.view_models.base.base import BaseModel @@ -69,20 +69,6 @@ class Config: from_attributes = True arbitrary_types_allowed = True - # These 'synthetic' fields are generated from other model properties. Transform data from other properties as needed, setting - # the appropriate field on the model itself. Then, proceed with Pydantic ingestion once fields are created. Only perform these - # transformations if the relevant attributes are present on the input data (i.e., when creating from an ORM object). - @model_validator(mode="before") - def generate_external_identifiers_list(cls, data: Any): - if hasattr(data, "ensembl_offset") or hasattr(data, "refseq_offset") or hasattr(data, "uniprot_offset"): - try: - data.__setattr__("external_identifiers", transform_external_identifier_offsets_to_list(data)) - except (AttributeError, KeyError) as exc: - raise ValidationError( - f"Unable to coerce external identifiers for {cls.__name__}: {exc}." # type: ignore - ) - return data - @model_validator(mode="after") def check_seq_or_accession(self) -> Self: if self.target_sequence is None and self.target_accession is None: diff --git a/tests/helpers/constants.py b/tests/helpers/constants.py index f1d7b81f..03d0ecc9 100644 --- a/tests/helpers/constants.py +++ b/tests/helpers/constants.py @@ -261,7 +261,6 @@ "recordType": "DoiIdentifier", "identifier": TEST_CROSSREF_IDENTIFIER, "url": f"https://doi.org/{TEST_CROSSREF_IDENTIFIER}", - "id": 1, } # VRS 1.X @@ -2244,7 +2243,6 @@ TEST_SAVED_MINIMAL_RAW_READ_IDENTIFIER = { **TEST_MINIMAL_RAW_READ_IDENTIFIER, - "id": 1, "url": "https://www.test.rawreadidentifiers.org", } diff --git a/tests/lib/test_external_identifier_urls.py b/tests/lib/test_external_identifier_urls.py new file mode 100644 index 00000000..3b4f9560 --- /dev/null +++ b/tests/lib/test_external_identifier_urls.py @@ -0,0 +1,40 @@ +import pytest + +from mavedb.lib.external_identifier_urls import doi_url, external_gene_identifier_url, raw_read_url + + +@pytest.mark.unit +def test_doi_url_links_to_the_doi_resolver(): + assert doi_url("10.7554/ELIFE.58026") == "https://doi.org/10.7554/ELIFE.58026" + + +@pytest.mark.unit +@pytest.mark.parametrize( + "identifier, expected_url", + [ + ("PRJNA941815", "https://www.ncbi.nlm.nih.gov/bioproject/PRJNA941815"), + ("GSE226732", "https://www.ncbi.nlm.nih.gov/geo/query/acc.cgi?acc=GSE226732"), + ("E-MTAB-1234", "https://www.ebi.ac.uk/biostudies/arrayexpress/studies/E-MTAB-1234"), + ("A-AFFY-44", "https://www.ebi.ac.uk/biostudies/arrayexpress/arrays/A-AFFY-44"), + ("SRP002725", "https://www.ebi.ac.uk/ena/browser/view/SRP002725"), + ("not-an-accession", "https://www.ebi.ac.uk/ena/browser/view/not-an-accession"), + ], + ids=["bioproject", "geo", "arrayexpress-experiment", "arrayexpress-array", "sra", "unrecognized"], +) +def test_raw_read_url_links_by_accession_type(identifier, expected_url): + assert raw_read_url(identifier) == expected_url + + +@pytest.mark.unit +@pytest.mark.parametrize( + "db_name, identifier, expected_url", + [ + ("Ensembl", "ENSG00000103275", "https://www.ensembl.org/id/ENSG00000103275"), + ("UniProt", "Q9Y617", "https://www.uniprot.org/uniprotkb/Q9Y617"), + ("RefSeq", "NP_478059.1", "https://www.ncbi.nlm.nih.gov/protein/NP_478059.1"), + ("RefSeq", "NM_003352.8", "https://www.ncbi.nlm.nih.gov/nuccore/NM_003352.8"), + ], + ids=["ensembl", "uniprot", "refseq-protein", "refseq-nucleotide"], +) +def test_external_gene_identifier_url_links_to_the_database_page(db_name, identifier, expected_url): + assert external_gene_identifier_url(db_name, identifier) == expected_url diff --git a/tests/routers/test_record_identifiers.py b/tests/routers/test_record_identifiers.py new file mode 100644 index 00000000..a88453c1 --- /dev/null +++ b/tests/routers/test_record_identifiers.py @@ -0,0 +1,233 @@ +# ruff: noqa: E402 + +import pytest + +arq = pytest.importorskip("arq") +cdot = pytest.importorskip("cdot") +fastapi = pytest.importorskip("fastapi") + +from copy import deepcopy + +from sqlalchemy import select + +from mavedb.models.doi_identifier import ExperimentDoiIdentifier, ScoreSetDoiIdentifier +from mavedb.models.raw_read_identifier import ExperimentRawReadIdentifier +from mavedb.models.target_gene_external_identifier import TargetGeneExternalIdentifier +from tests.helpers.constants import TEST_MINIMAL_EXPERIMENT, TEST_MINIMAL_SEQ_SCORESET +from tests.helpers.dependency_overrider import DependencyOverrider +from tests.helpers.util.experiment import create_experiment +from tests.helpers.util.score_set import create_seq_score_set + + +def _target_genes_with(external_identifiers): + target_genes = deepcopy(TEST_MINIMAL_SEQ_SCORESET["targetGenes"]) + target_genes[0]["externalIdentifiers"] = external_identifiers + return target_genes + + +ENSEMBL = {"identifier": {"dbName": "Ensembl", "identifier": "ENSG00000103275"}, "offset": 0} +UNIPROT = {"identifier": {"dbName": "UniProt", "identifier": "Q9Y617"}, "offset": 3} +REFSEQ = {"identifier": {"dbName": "RefSeq", "identifier": "NP_478059.1"}, "offset": 6} + + +def test_create_experiment_with_identifiers_stores_one_upper_cased_doi_and_typed_raw_read_urls( + session, client, setup_router_db +): + experiment = create_experiment( + client, + { + "doiIdentifiers": [{"identifier": "10.7554/eLife.58026"}, {"identifier": "10.7554/ELIFE.58026"}], + "rawReadIdentifiers": [ + {"identifier": "SRP002725"}, + {"identifier": "PRJNA941815"}, + {"identifier": "GSE226732"}, + {"identifier": "E-MTAB-1234"}, + ], + }, + ) + + assert experiment["doiIdentifiers"] == [ + { + "recordType": "DoiIdentifier", + "identifier": "10.7554/ELIFE.58026", + "url": "https://doi.org/10.7554/ELIFE.58026", + } + ] + assert {identifier["identifier"]: identifier["url"] for identifier in experiment["rawReadIdentifiers"]} == { + "SRP002725": "https://www.ebi.ac.uk/ena/browser/view/SRP002725", + "PRJNA941815": "https://www.ncbi.nlm.nih.gov/bioproject/PRJNA941815", + "GSE226732": "https://www.ncbi.nlm.nih.gov/geo/query/acc.cgi?acc=GSE226732", + "E-MTAB-1234": "https://www.ebi.ac.uk/biostudies/arrayexpress/studies/E-MTAB-1234", + } + assert session.scalars(select(ExperimentDoiIdentifier.identifier)).all() == ["10.7554/ELIFE.58026"] + + +def test_update_experiment_identifiers_keeps_resubmitted_and_drops_omitted(session, client, setup_router_db): + experiment = create_experiment( + client, + { + "doiIdentifiers": [{"identifier": "10.1000/182"}], + "rawReadIdentifiers": [{"identifier": "SRP002725"}], + }, + ) + + payload = deepcopy(TEST_MINIMAL_EXPERIMENT) + payload.update( + { + "doiIdentifiers": [{"identifier": "10.1000/182"}, {"identifier": "10.1000/183"}], + "rawReadIdentifiers": [], + } + ) + response = client.put(f"/api/v1/experiments/{experiment['urn']}", json=payload) + + assert response.status_code == 200 + assert [identifier["identifier"] for identifier in response.json()["doiIdentifiers"]] == [ + "10.1000/182", + "10.1000/183", + ] + assert response.json()["rawReadIdentifiers"] == [] + assert session.scalars(select(ExperimentRawReadIdentifier)).all() == [] + + +def test_create_score_set_with_doi_stores_it_on_the_score_set(session, client, setup_router_db): + experiment = create_experiment(client) + score_set = create_seq_score_set(client, experiment["urn"], {"doiIdentifiers": [{"identifier": "10.1000/abc"}]}) + + assert score_set["doiIdentifiers"] == [ + {"recordType": "DoiIdentifier", "identifier": "10.1000/ABC", "url": "https://doi.org/10.1000/ABC"} + ] + assert session.scalars(select(ScoreSetDoiIdentifier.identifier)).all() == ["10.1000/ABC"] + + +def test_create_score_set_with_external_identifiers_returns_them_nested_with_urls(session, client, setup_router_db): + experiment = create_experiment(client) + score_set = create_seq_score_set( + client, experiment["urn"], {"targetGenes": _target_genes_with([UNIPROT, ENSEMBL, REFSEQ])} + ) + + assert score_set["targetGenes"][0]["externalIdentifiers"] == [ + { + "recordType": "ExternalGeneIdentifierOffset", + "offset": 0, + "identifier": { + "recordType": "ExternalGeneIdentifier", + "dbName": "Ensembl", + "identifier": "ENSG00000103275", + "url": "https://www.ensembl.org/id/ENSG00000103275", + }, + }, + { + "recordType": "ExternalGeneIdentifierOffset", + "offset": 6, + "identifier": { + "recordType": "ExternalGeneIdentifier", + "dbName": "RefSeq", + "identifier": "NP_478059.1", + "url": "https://www.ncbi.nlm.nih.gov/protein/NP_478059.1", + }, + }, + { + "recordType": "ExternalGeneIdentifierOffset", + "offset": 3, + "identifier": { + "recordType": "ExternalGeneIdentifier", + "dbName": "UniProt", + "identifier": "Q9Y617", + "url": "https://www.uniprot.org/uniprotkb/Q9Y617", + }, + }, + ] + + +def test_update_score_set_external_identifiers_replaces_the_target_genes_set(session, client, setup_router_db): + experiment = create_experiment(client) + score_set = create_seq_score_set(client, experiment["urn"], {"targetGenes": _target_genes_with([UNIPROT, ENSEMBL])}) + + payload = deepcopy(TEST_MINIMAL_SEQ_SCORESET) + payload.update( + { + "experimentUrn": experiment["urn"], + "targetGenes": _target_genes_with([{**UNIPROT, "offset": 9}]), + } + ) + response = client.put(f"/api/v1/score-sets/{score_set['urn']}", json=payload) + + assert response.status_code == 200 + rows = session.execute(select(TargetGeneExternalIdentifier.db_name, TargetGeneExternalIdentifier.offset)).all() + assert rows == [("UniProt", 9)] + + +@pytest.mark.parametrize( + "route, text, identifier, make_record", + [ + ( + "/api/v1/doi-identifiers/search", + "10.1000", + "10.1000/182", + lambda client: create_experiment(client, {"doiIdentifiers": [{"identifier": "10.1000/182"}]}), + ), + ( + "/api/v1/raw-read-identifiers/search", + "SRP", + "SRP002725", + lambda client: create_experiment(client, {"rawReadIdentifiers": [{"identifier": "SRP002725"}]}), + ), + ( + "/api/v1/target-gene-identifiers/search?db_name=UniProt", + "Q9Y", + "Q9Y617", + lambda client: create_seq_score_set( + client, create_experiment(client)["urn"], {"targetGenes": _target_genes_with([UNIPROT])} + ), + ), + ], + ids=["doi", "raw-read", "target-gene"], +) +def test_search_identifiers_on_private_record_visible_only_to_those_who_can_read_it( + client, setup_router_db, anonymous_app_overrides, extra_user_app_overrides, route, text, identifier, make_record +): + make_record(client) + + owner_response = client.post(route, json={"text": text}) + with DependencyOverrider(anonymous_app_overrides): + anonymous_response = client.post(route, json={"text": text}) + with DependencyOverrider(extra_user_app_overrides): + other_user_response = client.post(route, json={"text": text}) + + assert owner_response.status_code == 200 + assert [result["identifier"] for result in owner_response.json()] == [identifier] + assert anonymous_response.json() == [] + assert other_user_response.json() == [] + + +def test_search_doi_identifiers_cited_twice_returns_one_result(client, setup_router_db): + experiment = create_experiment(client, {"doiIdentifiers": [{"identifier": "10.1000/182"}]}) + create_seq_score_set(client, experiment["urn"], {"doiIdentifiers": [{"identifier": "10.1000/182"}]}) + + response = client.post("/api/v1/doi-identifiers/search", json={"text": "10.1000"}) + + assert response.status_code == 200 + assert response.json() == [ + {"recordType": "DoiIdentifier", "identifier": "10.1000/182", "url": "https://doi.org/10.1000/182"} + ] + + +@pytest.mark.parametrize( + "route", + [ + "/api/v1/doi-identifiers/search", + "/api/v1/raw-read-identifiers/search", + "/api/v1/target-gene-identifiers/search?db_name=UniProt", + ], + ids=["doi", "raw-read", "target-gene"], +) +def test_search_identifiers_without_text_returns_400(client, setup_router_db, route): + response = client.post(route, json={"text": " "}) + + assert response.status_code == 400 + + +def test_search_target_gene_identifiers_unknown_database_returns_422(client, setup_router_db): + response = client.post("/api/v1/target-gene-identifiers/search?db_name=GenBank", json={"text": "Q9Y"}) + + assert response.status_code == 422 diff --git a/tests/view_models/test_doi_identifier.py b/tests/view_models/test_doi_identifier.py index 09bf9cd4..48cdba10 100644 --- a/tests/view_models/test_doi_identifier.py +++ b/tests/view_models/test_doi_identifier.py @@ -20,3 +20,17 @@ def test_invalid_doi_fails(): ) assert f"'{invalid_doi}' is not a valid DOI identifier" in str(exc_info.value) + + +@pytest.mark.parametrize( + "doi, expected", + [ + ("10.7554/eLife.58026", "10.7554/ELIFE.58026"), + ("https://doi.org/10.7554/elife.58026", "10.7554/ELIFE.58026"), + ], + ids=["mixed-case", "url"], +) +def test_create_doi_identifier_mixed_case_is_upper_cased(doi, expected): + doi_identifier = DoiIdentifierCreate(identifier=doi) + + assert doi_identifier.identifier == expected diff --git a/tests/view_models/test_score_set.py b/tests/view_models/test_score_set.py index 03698974..2c9079e3 100644 --- a/tests/view_models/test_score_set.py +++ b/tests/view_models/test_score_set.py @@ -291,10 +291,7 @@ def test_saved_score_set_synthetic_properties(): score_set.pop("secondaryPublicationIdentifiers") # Convert fields expecting an object to attributed objects - external_identifiers = {"refseq_offset": None, "ensembl_offset": None, "uniprot_offset": None} - target_genes = [ - dummy_attributed_object_from_dict({**target, **external_identifiers}) for target in score_set["targetGenes"] - ] + target_genes = [dummy_attributed_object_from_dict(target) for target in score_set["targetGenes"]] score_set["targetGenes"] = [SavedTargetGene.model_validate(target) for target in target_genes] # Set synthetic properties with dummy attributed objects to mock SQLAlchemy model objects. @@ -397,10 +394,7 @@ def test_cannot_create_saved_score_set_without_all_attributed_properties(exclude score_set.pop("secondaryPublicationIdentifiers") # Convert fields expecting an object to attributed objects - external_identifiers = {"refseq_offset": None, "ensembl_offset": None, "uniprot_offset": None} - target_genes = [ - dummy_attributed_object_from_dict({**target, **external_identifiers}) for target in score_set["targetGenes"] - ] + target_genes = [dummy_attributed_object_from_dict(target) for target in score_set["targetGenes"]] score_set["targetGenes"] = [SavedTargetGene.model_validate(target) for target in target_genes] # Set synthetic properties with dummy attributed objects to mock SQLAlchemy model objects. diff --git a/tests/view_models/test_target_gene.py b/tests/view_models/test_target_gene.py index 71b497b9..db71974f 100644 --- a/tests/view_models/test_target_gene.py +++ b/tests/view_models/test_target_gene.py @@ -1,5 +1,6 @@ import pytest +from mavedb.models.target_gene_external_identifier import TargetGeneExternalIdentifier from mavedb.view_models.target_gene import SavedTargetGene, TargetGene, TargetGeneCreate, TargetGeneWithScoreSetUrn from tests.helpers.constants import ( SEQUENCE, @@ -142,46 +143,39 @@ def test_cannot_create_target_gene_with_both_sequence_and_accession(): assert "Expected either a `target_sequence` or a `target_accession`, not both." in str(exc_info.value) +def _stored_external_identifier(identifier, offset): + return TargetGeneExternalIdentifier( + db_name=identifier["db_name"], identifier=identifier["identifier"], offset=offset + ) + + @pytest.mark.parametrize( "identifier", - [ - {"key": "ensembl_offset", "value": TEST_ENSEMBLE_EXTERNAL_IDENTIFIER}, - {"key": "refseq_offset", "value": TEST_REFSEQ_EXTERNAL_IDENTIFIER}, - {"key": "uniprot_offset", "value": TEST_UNIPROT_EXTERNAL_IDENTIFIER}, - ], + [TEST_ENSEMBLE_EXTERNAL_IDENTIFIER, TEST_REFSEQ_EXTERNAL_IDENTIFIER, TEST_UNIPROT_EXTERNAL_IDENTIFIER], + ids=["ensembl", "refseq", "uniprot"], ) -def test_external_identifiers_parsed_into_containing_list(identifier): - target_gene = { - "id": 1, - "name": "UBE2I", - "category": "regulatory", - "ensembl_offset": None, - "refseq_offset": None, - "uniprot_offset": None, - } - target_gene["target_sequence"] = dummy_attributed_object_from_dict( +def test_saved_target_gene_from_stored_external_identifier_nests_identifier_beside_offset(identifier): + offset = 10 + target_gene = dummy_attributed_object_from_dict( { - "sequenceType": "dna", - "sequence": SEQUENCE, - "taxonomy": TEST_SAVED_TAXONOMY, + "id": 1, + "name": "UBE2I", + "category": "regulatory", + "external_identifiers": [_stored_external_identifier(identifier, offset)], + "target_sequence": dummy_attributed_object_from_dict( + { + "sequenceType": "dna", + "sequence": SEQUENCE, + "taxonomy": TEST_SAVED_TAXONOMY, + } + ), } ) - - # Tranform external identifier constant into an attributed object, with an added offset property - external_identifier = identifier["key"] - offset = 10 - target_gene[external_identifier] = { - "offset": offset, - "identifier": dummy_attributed_object_from_dict(identifier["value"]), - } - target_gene[external_identifier] = dummy_attributed_object_from_dict(target_gene[external_identifier]) - - target_gene = dummy_attributed_object_from_dict(target_gene) saved_target_gene = SavedTargetGene.model_validate(target_gene) assert len(saved_target_gene.external_identifiers) == 1 - assert saved_target_gene.external_identifiers[0].identifier.identifier == identifier["value"]["identifier"] - assert saved_target_gene.external_identifiers[0].identifier.db_name == identifier["value"]["db_name"] + assert saved_target_gene.external_identifiers[0].identifier.identifier == identifier["identifier"] + assert saved_target_gene.external_identifiers[0].identifier.db_name == identifier["db_name"] assert saved_target_gene.external_identifiers[0].offset == offset @@ -190,9 +184,7 @@ def test_cannot_create_saved_target_without_seq_or_acc(): "id": 1, "name": "UBE2I", "category": "regulatory", - "ensembl_offset": None, - "refseq_offset": None, - "uniprot_offset": None, + "external_identifiers": [], } target_gene = dummy_attributed_object_from_dict(target_gene) @@ -208,11 +200,7 @@ def test_saved_target_gene_can_be_created_from_orm(): "id": 1, "name": "UBE2I", "category": "regulatory", - "ensembl_offset": dummy_attributed_object_from_dict( - {"offset": 1, "identifier": dummy_attributed_object_from_dict(TEST_ENSEMBLE_EXTERNAL_IDENTIFIER)} - ), - "refseq_offset": None, - "uniprot_offset": None, + "external_identifiers": [_stored_external_identifier(TEST_ENSEMBLE_EXTERNAL_IDENTIFIER, 1)], "target_sequence": dummy_attributed_object_from_dict( { "sequenceType": "dna", @@ -236,14 +224,7 @@ def test_target_gene_with_score_set_urn_can_be_created_from_orm(): "id": 1, "name": "UBE2I", "category": "regulatory", - "ensembl_offset": dummy_attributed_object_from_dict( - { - "offset": 1, - "identifier": dummy_attributed_object_from_dict(TEST_ENSEMBLE_EXTERNAL_IDENTIFIER), - } - ), - "refseq_offset": None, - "uniprot_offset": None, + "external_identifiers": [_stored_external_identifier(TEST_ENSEMBLE_EXTERNAL_IDENTIFIER, 1)], "target_sequence": dummy_attributed_object_from_dict( { "sequenceType": "dna", From e6b8563bd11fc90da97d6a3b6e1a9acc11fa216f Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Fri, 23 Oct 2026 10:09:23 -0700 Subject: [PATCH 05/10] fix(db): strip DOI prefixes when folding stored DOIs Before this change the api accepted DOIs written with a doi: or resolver prefix and stored them as given. The fold only upper-cased them, so such a row would have become HTTPS://DOI.ORG/..., with a broken link and no match against the bare form new submissions store. Strip the prefixes idutils.normalize_doi strips. The 2026.3.0 dump has none, but production accepts them until this ships. --- .../2dfdc4a88870_store_identifiers_on_owner_rows.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/alembic/versions/2dfdc4a88870_store_identifiers_on_owner_rows.py b/alembic/versions/2dfdc4a88870_store_identifiers_on_owner_rows.py index ccd080f8..cc781c81 100644 --- a/alembic/versions/2dfdc4a88870_store_identifiers_on_owner_rows.py +++ b/alembic/versions/2dfdc4a88870_store_identifiers_on_owner_rows.py @@ -9,7 +9,8 @@ moves onto the row that uses it (#944): - The six DOI and raw read link tables hold the identifier string, keyed by ``(_id, identifier)``. - DOIs are upper-cased on the way, the form the DOI registry stores, since they're case-insensitive and a record + DOIs are stored bare and upper-cased, as the api now stores new ones: ``doi:`` and resolver prefixes are + stripped, and upper case is the form the DOI registry stores, since DOIs are case-insensitive and a record shouldn't cite one twice. - ``ensembl_offsets``, ``refseq_offsets`` and ``uniprot_offsets`` merge into ``target_gene_external_identifiers``, keyed by ``(target_gene_id, db_name)``: a target gene has at most one @@ -34,6 +35,10 @@ branch_labels = None depends_on = None +# The api accepted DOIs with a ``doi:`` or resolver prefix before #944 and stored them as given. This strips the +# prefixes idutils.normalize_doi strips, which is what the api applies to new DOIs. +DOI_NORMALIZE = r"upper(regexp_replace({}, '^(doi:\s*|(https?://)?(dx\.)?doi\.org/)', '', 'i'))" + # (link table, parent column, shared table, shared id column) DOI_LINKS = [ ("scoreset_doi_identifiers", "scoreset_id", "doi_identifiers", "doi_identifier_id"), @@ -60,7 +65,7 @@ def _fold_link_table(link: str, parent: str, shared: str, shared_id: str, normal f"UPDATE {link} l SET identifier = {normalize.format('s.identifier')} FROM {shared} s WHERE s.id = l.{shared_id}" ) - # Two shared rows differing only in case would collide once upper-cased. There are none today. + # Two shared rows differing only in case or prefix would collide once normalized. There are none today. op.execute( f"DELETE FROM {link} a USING {link} b " f"WHERE a.{parent} = b.{parent} AND a.identifier = b.identifier AND a.ctid > b.ctid" @@ -105,7 +110,7 @@ def _unfold_link_table(link: str, parent: str, shared: str, shared_id: str) -> N def upgrade() -> None: for link, parent, shared, shared_id in DOI_LINKS: - _fold_link_table(link, parent, shared, shared_id, normalize="upper({})") + _fold_link_table(link, parent, shared, shared_id, normalize=DOI_NORMALIZE) for link, parent, shared, shared_id in RAW_READ_LINKS: _fold_link_table(link, parent, shared, shared_id, normalize="{}") From cb419845d636556752b82c138f257451a28f4bcb Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Fri, 23 Oct 2026 10:10:05 -0700 Subject: [PATCH 06/10] fix(search): match LIKE wildcards in identifier search literally A search for % or _ matched every identifier. Read access is checked in Python before the result limit, so that loaded every record that cites one, on a route anyone can call. --- src/mavedb/lib/identifiers.py | 2 +- tests/routers/test_record_identifiers.py | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/mavedb/lib/identifiers.py b/src/mavedb/lib/identifiers.py index f6c97cf1..b1ada50b 100644 --- a/src/mavedb/lib/identifiers.py +++ b/src/mavedb/lib/identifiers.py @@ -430,7 +430,7 @@ def search_identifiers_on_readable_records( found: dict[str, ExternalIdentifier] = {} for link, viewer_class, query in sources: viewer = principal.viewer_for(viewer_class) - rows = db.execute(query.where(func.lower(link.identifier).contains(text.lower()))).all() + rows = db.execute(query.where(func.lower(link.identifier).contains(text.lower(), autoescape=True))).all() for link_row, record in rows: if link_row.identifier in found: continue diff --git a/tests/routers/test_record_identifiers.py b/tests/routers/test_record_identifiers.py index a88453c1..d11f0016 100644 --- a/tests/routers/test_record_identifiers.py +++ b/tests/routers/test_record_identifiers.py @@ -212,6 +212,16 @@ def test_search_doi_identifiers_cited_twice_returns_one_result(client, setup_rou ] +@pytest.mark.parametrize("text", ["%", "_"], ids=["percent", "underscore"]) +def test_search_doi_identifiers_with_like_wildcard_matches_it_literally(client, setup_router_db, text): + create_experiment(client, {"doiIdentifiers": [{"identifier": "10.1000/182"}]}) + + response = client.post("/api/v1/doi-identifiers/search", json={"text": text}) + + assert response.status_code == 200 + assert response.json() == [] + + @pytest.mark.parametrize( "route", [ From 53be9701eb549355aed1f29eeb4a18d831ff0d05 Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Fri, 23 Oct 2026 10:10:52 -0700 Subject: [PATCH 07/10] refactor(statistics): map identifier link tables separately The DOI and raw read link models were listed both among the association tables and in their own union, and the count narrowed from one to the other with a type: ignore. Give them their own typed map, so a new link goes in one place and mypy checks it. --- src/mavedb/routers/statistics.py | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/src/mavedb/routers/statistics.py b/src/mavedb/routers/statistics.py index ae30b80c..dfda4015 100644 --- a/src/mavedb/routers/statistics.py +++ b/src/mavedb/routers/statistics.py @@ -48,10 +48,6 @@ type[ExperimentControlledKeywordAssociation], type[ExperimentPublicationIdentifierAssociation], type[ScoreSetPublicationIdentifierAssociation], - type[ExperimentDoiIdentifier], - type[ExperimentRawReadIdentifier], - type[ScoreSetDoiIdentifier], - type[ScoreSetRawReadIdentifier], ] RecordIdentifierLinks = Union[ type[ExperimentDoiIdentifier], @@ -98,15 +94,11 @@ def _model_and_association_from_record_field( } record_to_assc_map: dict[RecordNames, dict[RecordFields, RecordAssociationTables]] = { RecordNames.experiment: { - RecordFields.doiIdentifiers: ExperimentDoiIdentifier, RecordFields.publicationIdentifiers: ExperimentPublicationIdentifierAssociation, - RecordFields.rawReadIdentifiers: ExperimentRawReadIdentifier, RecordFields.keywords: ExperimentControlledKeywordAssociation, }, RecordNames.scoreSet: { - RecordFields.doiIdentifiers: ScoreSetDoiIdentifier, RecordFields.publicationIdentifiers: ScoreSetPublicationIdentifierAssociation, - RecordFields.rawReadIdentifiers: ScoreSetRawReadIdentifier, }, } @@ -127,13 +119,22 @@ def _count_for_identifier_in_query(db: Session, query: Select[tuple[Any, int]]) return {value: count for value, count in db.execute(query).all() if value is not None} -def _count_identifiers_on_published_records(db: Session, record: RecordNames, field: RecordFields) -> dict[str, int]: - queried_model, queried_link = _model_and_association_from_record_field(record, field) +# Identifiers are stored on the rows that link them to their records, so these count the link rows themselves. +RECORD_IDENTIFIER_LINKS: dict[RecordNames, dict[RecordFields, RecordIdentifierLinks]] = { + RecordNames.experiment: { + RecordFields.doiIdentifiers: ExperimentDoiIdentifier, + RecordFields.rawReadIdentifiers: ExperimentRawReadIdentifier, + }, + RecordNames.scoreSet: { + RecordFields.doiIdentifiers: ScoreSetDoiIdentifier, + RecordFields.rawReadIdentifiers: ScoreSetRawReadIdentifier, + }, +} - if queried_link is None: - raise HTTPException(500, f"No association table associated with the {field.value} field when one was expected.") - link: RecordIdentifierLinks = queried_link # type: ignore[assignment] +def _count_identifiers_on_published_records(db: Session, record: RecordNames, field: RecordFields) -> dict[str, int]: + queried_model, _ = _model_and_association_from_record_field(record, None) + link = RECORD_IDENTIFIER_LINKS[record][field] query = _join_model_and_filter_unpublished( select(link.identifier, func.count(link.identifier)), queried_model ).group_by(link.identifier) From 8c2f4bfcb2a090f7acce7f4c5cd6c7e2f13494c3 Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Fri, 23 Oct 2026 10:11:53 -0700 Subject: [PATCH 08/10] refactor(search): build record identifier search sources in one place The DOI and raw read search routes each spelled out the same three link-record joins. identifier_sources_on_records builds them from the three link models, so a new record type changes one function. --- src/mavedb/lib/identifiers.py | 57 ++++++++++++++++++++-- src/mavedb/routers/doi_identifiers.py | 33 +------------ src/mavedb/routers/raw_read_identifiers.py | 35 ++----------- 3 files changed, 60 insertions(+), 65 deletions(-) diff --git a/src/mavedb/lib/identifiers.py b/src/mavedb/lib/identifiers.py index b1ada50b..318fdaad 100644 --- a/src/mavedb/lib/identifiers.py +++ b/src/mavedb/lib/identifiers.py @@ -14,11 +14,23 @@ from mavedb.lib.exceptions import AmbiguousIdentifierError, NonexistentIdentifierError from mavedb.lib.external_publications import Crossref, CrossrefWork, PublicationAuthors, Rxiv, RxivContentDetail +from mavedb.lib.permissions.experiment import ExperimentViewer +from mavedb.lib.permissions.experiment_set import ExperimentSetViewer from mavedb.lib.permissions.principal import Principal +from mavedb.lib.permissions.score_set import ScoreSetViewer from mavedb.lib.permissions.viewer import Viewer from mavedb.lib.validation.publication import identifier_valid_for, infer_identifier_from_url, validate_db_name +from mavedb.models.doi_identifier import ExperimentDoiIdentifier, ExperimentSetDoiIdentifier, ScoreSetDoiIdentifier +from mavedb.models.experiment import Experiment +from mavedb.models.experiment_set import ExperimentSet from mavedb.models.mixins.external_identifier import ExternalIdentifier from mavedb.models.publication_identifier import PublicationIdentifier +from mavedb.models.raw_read_identifier import ( + ExperimentRawReadIdentifier, + ExperimentSetRawReadIdentifier, + ScoreSetRawReadIdentifier, +) +from mavedb.models.score_set import ScoreSet from mavedb.models.target_gene_external_identifier import TargetGeneExternalIdentifier from mavedb.view_models.external_gene_identifier_offset import ExternalGeneIdentifierOffsetCreate @@ -26,6 +38,10 @@ IDENTIFIER_SEARCH_LIMIT = 50 +# A table to search for identifiers: its link model, the viewer for the record that governs each row's +# visibility, and a query selecting ``(link row, record)``. +IdentifierSource = tuple[type[ExternalIdentifier], type[Viewer[Any]], Select] + class ExternalPublication: """ @@ -408,7 +424,7 @@ def search_identifiers_on_readable_records( db: Session, principal: Principal, text: str, - sources: Sequence[tuple[type[ExternalIdentifier], type[Viewer[Any]], Select]], + sources: Sequence[IdentifierSource], ) -> list[ExternalIdentifier]: """Find identifiers containing ``text`` that are attached to records the caller may read. @@ -419,8 +435,7 @@ def search_identifiers_on_readable_records( db: An active database session. principal: The caller. text: The text to search for, matched case-insensitively anywhere in the identifier. - sources: For each table to search, its link model, the viewer for the record that governs each row's - visibility, and a query selecting ``(link row, record)``. + sources: The tables to search. Returns: Up to ``IDENTIFIER_SEARCH_LIMIT`` link rows, one per distinct identifier. @@ -440,6 +455,42 @@ def search_identifiers_on_readable_records( return [found[identifier] for identifier in sorted(found)][:IDENTIFIER_SEARCH_LIMIT] +def identifier_sources_on_records( + score_set_link: Union[type[ScoreSetDoiIdentifier], type[ScoreSetRawReadIdentifier]], + experiment_link: Union[type[ExperimentDoiIdentifier], type[ExperimentRawReadIdentifier]], + experiment_set_link: Union[type[ExperimentSetDoiIdentifier], type[ExperimentSetRawReadIdentifier]], +) -> list[IdentifierSource]: + """The sources for an identifier that score sets, experiments and experiment sets each cite. + + Args: + score_set_link: The model linking the identifier to score sets. + experiment_link: The model linking the identifier to experiments. + experiment_set_link: The model linking the identifier to experiment sets. + + Returns: + A source for each record type, for :func:`search_identifiers_on_readable_records`. + """ + return [ + ( + score_set_link, + ScoreSetViewer, + select(score_set_link, ScoreSet).join(ScoreSet, ScoreSet.id == score_set_link.score_set_id), + ), + ( + experiment_link, + ExperimentViewer, + select(experiment_link, Experiment).join(Experiment, Experiment.id == experiment_link.experiment_id), + ), + ( + experiment_set_link, + ExperimentSetViewer, + select(experiment_set_link, ExperimentSet).join( + ExperimentSet, ExperimentSet.id == experiment_set_link.experiment_set_id + ), + ), + ] + + def sanitize_target_sequence_label(label: str): """ Strip whitespace from the supplied string and replace any diff --git a/src/mavedb/routers/doi_identifiers.py b/src/mavedb/routers/doi_identifiers.py index 53946244..b6a42869 100644 --- a/src/mavedb/routers/doi_identifiers.py +++ b/src/mavedb/routers/doi_identifiers.py @@ -1,20 +1,13 @@ from typing import Any, List from fastapi import APIRouter, Depends, HTTPException -from sqlalchemy import select from sqlalchemy.orm import Session from mavedb import deps from mavedb.lib.authorization import get_principal -from mavedb.lib.identifiers import search_identifiers_on_readable_records +from mavedb.lib.identifiers import identifier_sources_on_records, search_identifiers_on_readable_records from mavedb.lib.permissions.principal import Principal -from mavedb.lib.permissions.experiment_set import ExperimentSetViewer -from mavedb.lib.permissions.experiment import ExperimentViewer -from mavedb.lib.permissions.score_set import ScoreSetViewer from mavedb.models.doi_identifier import ExperimentDoiIdentifier, ExperimentSetDoiIdentifier, ScoreSetDoiIdentifier -from mavedb.models.experiment import Experiment -from mavedb.models.experiment_set import ExperimentSet -from mavedb.models.score_set import ScoreSet from mavedb.routers.shared import BASE_400_RESPONSE, PUBLIC_ERROR_RESPONSES, ROUTER_BASE_PREFIX from mavedb.view_models import doi_identifier from mavedb.view_models.search import TextSearch @@ -55,27 +48,5 @@ def search_doi_identifiers( db, principal, search.text.strip(), - [ - ( - ScoreSetDoiIdentifier, - ScoreSetViewer, - select(ScoreSetDoiIdentifier, ScoreSet).join( - ScoreSet, ScoreSet.id == ScoreSetDoiIdentifier.score_set_id - ), - ), - ( - ExperimentDoiIdentifier, - ExperimentViewer, - select(ExperimentDoiIdentifier, Experiment).join( - Experiment, Experiment.id == ExperimentDoiIdentifier.experiment_id - ), - ), - ( - ExperimentSetDoiIdentifier, - ExperimentSetViewer, - select(ExperimentSetDoiIdentifier, ExperimentSet).join( - ExperimentSet, ExperimentSet.id == ExperimentSetDoiIdentifier.experiment_set_id - ), - ), - ], + identifier_sources_on_records(ScoreSetDoiIdentifier, ExperimentDoiIdentifier, ExperimentSetDoiIdentifier), ) diff --git a/src/mavedb/routers/raw_read_identifiers.py b/src/mavedb/routers/raw_read_identifiers.py index 075b94a4..8968db46 100644 --- a/src/mavedb/routers/raw_read_identifiers.py +++ b/src/mavedb/routers/raw_read_identifiers.py @@ -1,24 +1,17 @@ from typing import Any, List from fastapi import APIRouter, Depends, HTTPException -from sqlalchemy import select from sqlalchemy.orm import Session from mavedb import deps from mavedb.lib.authorization import get_principal -from mavedb.lib.identifiers import search_identifiers_on_readable_records +from mavedb.lib.identifiers import identifier_sources_on_records, search_identifiers_on_readable_records from mavedb.lib.permissions.principal import Principal -from mavedb.lib.permissions.experiment_set import ExperimentSetViewer -from mavedb.lib.permissions.experiment import ExperimentViewer -from mavedb.lib.permissions.score_set import ScoreSetViewer -from mavedb.models.experiment import Experiment -from mavedb.models.experiment_set import ExperimentSet from mavedb.models.raw_read_identifier import ( ExperimentRawReadIdentifier, ExperimentSetRawReadIdentifier, ScoreSetRawReadIdentifier, ) -from mavedb.models.score_set import ScoreSet from mavedb.routers.shared import BASE_400_RESPONSE, PUBLIC_ERROR_RESPONSES, ROUTER_BASE_PREFIX from mavedb.view_models import raw_read_identifier from mavedb.view_models.search import TextSearch @@ -59,27 +52,7 @@ def search_raw_read_identifiers( db, principal, search.text.strip(), - [ - ( - ScoreSetRawReadIdentifier, - ScoreSetViewer, - select(ScoreSetRawReadIdentifier, ScoreSet).join( - ScoreSet, ScoreSet.id == ScoreSetRawReadIdentifier.score_set_id - ), - ), - ( - ExperimentRawReadIdentifier, - ExperimentViewer, - select(ExperimentRawReadIdentifier, Experiment).join( - Experiment, Experiment.id == ExperimentRawReadIdentifier.experiment_id - ), - ), - ( - ExperimentSetRawReadIdentifier, - ExperimentSetViewer, - select(ExperimentSetRawReadIdentifier, ExperimentSet).join( - ExperimentSet, ExperimentSet.id == ExperimentSetRawReadIdentifier.experiment_set_id - ), - ), - ], + identifier_sources_on_records( + ScoreSetRawReadIdentifier, ExperimentRawReadIdentifier, ExperimentSetRawReadIdentifier + ), ) From d1098950b12d1bbf27dfef1ddb6a39553b55704c Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Fri, 23 Oct 2026 10:13:01 -0700 Subject: [PATCH 09/10] perf(score-sets): load target gene external identifiers with selectinload The three offsets these replace were one row each, so joining them in was cheap. External identifiers are a collection of up to three rows per gene, and joining a collection in multiplies the rows of every other joined collection on the target gene and its score set. Load it in its own query instead. --- src/mavedb/lib/score_sets.py | 4 ++-- src/mavedb/routers/genes.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/mavedb/lib/score_sets.py b/src/mavedb/lib/score_sets.py index 67714de8..e43e3a7d 100644 --- a/src/mavedb/lib/score_sets.py +++ b/src/mavedb/lib/score_sets.py @@ -241,7 +241,7 @@ def search_score_sets(db: Session, owner_or_contributor: Optional[User], search: ScoreSetPublicationIdentifierAssociation.publication ), joinedload(ScoreSet.target_genes).options( - joinedload(TargetGene.external_identifiers), + selectinload(TargetGene.external_identifiers), joinedload(TargetGene.target_sequence).joinedload(TargetSequence.taxonomy), joinedload(TargetGene.target_accession), ), @@ -253,7 +253,7 @@ def search_score_sets(db: Session, owner_or_contributor: Optional[User], search: ScoreSetPublicationIdentifierAssociation.publication ), selectinload(ScoreSet.target_genes).options( - joinedload(TargetGene.external_identifiers), + selectinload(TargetGene.external_identifiers), joinedload(TargetGene.target_sequence).joinedload(TargetSequence.taxonomy), joinedload(TargetGene.target_accession), ), diff --git a/src/mavedb/routers/genes.py b/src/mavedb/routers/genes.py index c3c88e4f..04fabf86 100644 --- a/src/mavedb/routers/genes.py +++ b/src/mavedb/routers/genes.py @@ -86,7 +86,7 @@ def _score_set_load_options(): ScoreSetPublicationIdentifierAssociation.publication ), selectinload(ScoreSet.target_genes).options( - joinedload(TargetGene.external_identifiers), + selectinload(TargetGene.external_identifiers), joinedload(TargetGene.target_sequence).joinedload(TargetSequence.taxonomy), joinedload(TargetGene.target_accession), ), From 447b47a24d583d6af0063dfadb40b0dbb7bb0107 Mon Sep 17 00:00:00 2001 From: Benjamin Capodanno Date: Fri, 23 Oct 2026 10:40:18 -0700 Subject: [PATCH 10/10] fix(search): move identifier search out of lib/identifiers The search checks read access through the permission viewers, which import fastapi. View models import lib/identifiers for publication authors, so they could no longer load without the server extras, and the core-dependency test jobs failed to collect. The search now lives in lib/identifier_search, which only the routers import. --- src/mavedb/lib/identifier_search.py | 103 ++++++++++++++++++ src/mavedb/lib/identifiers.py | 96 +--------------- src/mavedb/routers/doi_identifiers.py | 2 +- src/mavedb/routers/raw_read_identifiers.py | 2 +- src/mavedb/routers/target_gene_identifiers.py | 2 +- 5 files changed, 108 insertions(+), 97 deletions(-) create mode 100644 src/mavedb/lib/identifier_search.py diff --git a/src/mavedb/lib/identifier_search.py b/src/mavedb/lib/identifier_search.py new file mode 100644 index 00000000..4a91b1c8 --- /dev/null +++ b/src/mavedb/lib/identifier_search.py @@ -0,0 +1,103 @@ +"""Search for the external identifiers that records cite, limited to records the caller may read. + +This is kept apart from :mod:`mavedb.lib.identifiers` because it needs the permission checks, which import the +web framework, and view models import that module. +""" + +from typing import Any, Sequence, Union + +from sqlalchemy import Select, func, select +from sqlalchemy.orm import Session + +from mavedb.lib.permissions.experiment import ExperimentViewer +from mavedb.lib.permissions.experiment_set import ExperimentSetViewer +from mavedb.lib.permissions.principal import Principal +from mavedb.lib.permissions.score_set import ScoreSetViewer +from mavedb.lib.permissions.viewer import Viewer +from mavedb.models.doi_identifier import ExperimentDoiIdentifier, ExperimentSetDoiIdentifier, ScoreSetDoiIdentifier +from mavedb.models.experiment import Experiment +from mavedb.models.experiment_set import ExperimentSet +from mavedb.models.mixins.external_identifier import ExternalIdentifier +from mavedb.models.raw_read_identifier import ( + ExperimentRawReadIdentifier, + ExperimentSetRawReadIdentifier, + ScoreSetRawReadIdentifier, +) +from mavedb.models.score_set import ScoreSet + +IDENTIFIER_SEARCH_LIMIT = 50 + +# A table to search for identifiers: its link model, the viewer for the record that governs each row's +# visibility, and a query selecting ``(link row, record)``. +IdentifierSource = tuple[type[ExternalIdentifier], type[Viewer[Any]], Select] + + +def search_identifiers_on_readable_records( + db: Session, + principal: Principal, + text: str, + sources: Sequence[IdentifierSource], +) -> list[ExternalIdentifier]: + """Find identifiers containing ``text`` that are attached to records the caller may read. + + Identifiers are stored on the rows that link them to their records, so one identifier can appear on many + rows. This returns one row per distinct identifier, sorted by identifier. + + Args: + db: An active database session. + principal: The caller. + text: The text to search for, matched case-insensitively anywhere in the identifier. + sources: The tables to search. + + Returns: + Up to ``IDENTIFIER_SEARCH_LIMIT`` link rows, one per distinct identifier. + """ + # Read access to a record is decided in Python, so the filter runs before the limit. These tables hold a + # few hundred rows. Once row-level security enforces reads (#808), the database does this filtering. + found: dict[str, ExternalIdentifier] = {} + for link, viewer_class, query in sources: + viewer = principal.viewer_for(viewer_class) + rows = db.execute(query.where(func.lower(link.identifier).contains(text.lower(), autoescape=True))).all() + for link_row, record in rows: + if link_row.identifier in found: + continue + if viewer.may_read(record): + found[link_row.identifier] = link_row + + return [found[identifier] for identifier in sorted(found)][:IDENTIFIER_SEARCH_LIMIT] + + +def identifier_sources_on_records( + score_set_link: Union[type[ScoreSetDoiIdentifier], type[ScoreSetRawReadIdentifier]], + experiment_link: Union[type[ExperimentDoiIdentifier], type[ExperimentRawReadIdentifier]], + experiment_set_link: Union[type[ExperimentSetDoiIdentifier], type[ExperimentSetRawReadIdentifier]], +) -> list[IdentifierSource]: + """The sources for an identifier that score sets, experiments and experiment sets each cite. + + Args: + score_set_link: The model linking the identifier to score sets. + experiment_link: The model linking the identifier to experiments. + experiment_set_link: The model linking the identifier to experiment sets. + + Returns: + A source for each record type, for :func:`search_identifiers_on_readable_records`. + """ + return [ + ( + score_set_link, + ScoreSetViewer, + select(score_set_link, ScoreSet).join(ScoreSet, ScoreSet.id == score_set_link.score_set_id), + ), + ( + experiment_link, + ExperimentViewer, + select(experiment_link, Experiment).join(Experiment, Experiment.id == experiment_link.experiment_id), + ), + ( + experiment_set_link, + ExperimentSetViewer, + select(experiment_set_link, ExperimentSet).join( + ExperimentSet, ExperimentSet.id == experiment_set_link.experiment_set_id + ), + ), + ] diff --git a/src/mavedb/lib/identifiers.py b/src/mavedb/lib/identifiers.py index 318fdaad..436ef8b3 100644 --- a/src/mavedb/lib/identifiers.py +++ b/src/mavedb/lib/identifiers.py @@ -1,7 +1,7 @@ import json import logging import os -from typing import Any, Iterable, Mapping, Optional, Sequence, Union +from typing import Iterable, Mapping, Optional, Union import eutils # type: ignore import requests @@ -9,39 +9,18 @@ from eutils._internal.xmlfacades.pubmedarticle import PubmedArticle # type: ignore from eutils._internal.xmlfacades.pubmedarticleset import PubmedArticleSet # type: ignore from idutils import is_doi, normalize_doi -from sqlalchemy import Select, func, select +from sqlalchemy import select from sqlalchemy.orm import Session from mavedb.lib.exceptions import AmbiguousIdentifierError, NonexistentIdentifierError from mavedb.lib.external_publications import Crossref, CrossrefWork, PublicationAuthors, Rxiv, RxivContentDetail -from mavedb.lib.permissions.experiment import ExperimentViewer -from mavedb.lib.permissions.experiment_set import ExperimentSetViewer -from mavedb.lib.permissions.principal import Principal -from mavedb.lib.permissions.score_set import ScoreSetViewer -from mavedb.lib.permissions.viewer import Viewer from mavedb.lib.validation.publication import identifier_valid_for, infer_identifier_from_url, validate_db_name -from mavedb.models.doi_identifier import ExperimentDoiIdentifier, ExperimentSetDoiIdentifier, ScoreSetDoiIdentifier -from mavedb.models.experiment import Experiment -from mavedb.models.experiment_set import ExperimentSet -from mavedb.models.mixins.external_identifier import ExternalIdentifier from mavedb.models.publication_identifier import PublicationIdentifier -from mavedb.models.raw_read_identifier import ( - ExperimentRawReadIdentifier, - ExperimentSetRawReadIdentifier, - ScoreSetRawReadIdentifier, -) -from mavedb.models.score_set import ScoreSet from mavedb.models.target_gene_external_identifier import TargetGeneExternalIdentifier from mavedb.view_models.external_gene_identifier_offset import ExternalGeneIdentifierOffsetCreate logger = logging.getLogger(__name__) -IDENTIFIER_SEARCH_LIMIT = 50 - -# A table to search for identifiers: its link model, the viewer for the record that governs each row's -# visibility, and a query selecting ``(link row, record)``. -IdentifierSource = tuple[type[ExternalIdentifier], type[Viewer[Any]], Select] - class ExternalPublication: """ @@ -420,77 +399,6 @@ def target_gene_external_identifiers( ] -def search_identifiers_on_readable_records( - db: Session, - principal: Principal, - text: str, - sources: Sequence[IdentifierSource], -) -> list[ExternalIdentifier]: - """Find identifiers containing ``text`` that are attached to records the caller may read. - - Identifiers are stored on the rows that link them to their records, so one identifier can appear on many - rows. This returns one row per distinct identifier, sorted by identifier. - - Args: - db: An active database session. - principal: The caller. - text: The text to search for, matched case-insensitively anywhere in the identifier. - sources: The tables to search. - - Returns: - Up to ``IDENTIFIER_SEARCH_LIMIT`` link rows, one per distinct identifier. - """ - # Read access to a record is decided in Python, so the filter runs before the limit. These tables hold a - # few hundred rows. Once row-level security enforces reads (#808), the database does this filtering. - found: dict[str, ExternalIdentifier] = {} - for link, viewer_class, query in sources: - viewer = principal.viewer_for(viewer_class) - rows = db.execute(query.where(func.lower(link.identifier).contains(text.lower(), autoescape=True))).all() - for link_row, record in rows: - if link_row.identifier in found: - continue - if viewer.may_read(record): - found[link_row.identifier] = link_row - - return [found[identifier] for identifier in sorted(found)][:IDENTIFIER_SEARCH_LIMIT] - - -def identifier_sources_on_records( - score_set_link: Union[type[ScoreSetDoiIdentifier], type[ScoreSetRawReadIdentifier]], - experiment_link: Union[type[ExperimentDoiIdentifier], type[ExperimentRawReadIdentifier]], - experiment_set_link: Union[type[ExperimentSetDoiIdentifier], type[ExperimentSetRawReadIdentifier]], -) -> list[IdentifierSource]: - """The sources for an identifier that score sets, experiments and experiment sets each cite. - - Args: - score_set_link: The model linking the identifier to score sets. - experiment_link: The model linking the identifier to experiments. - experiment_set_link: The model linking the identifier to experiment sets. - - Returns: - A source for each record type, for :func:`search_identifiers_on_readable_records`. - """ - return [ - ( - score_set_link, - ScoreSetViewer, - select(score_set_link, ScoreSet).join(ScoreSet, ScoreSet.id == score_set_link.score_set_id), - ), - ( - experiment_link, - ExperimentViewer, - select(experiment_link, Experiment).join(Experiment, Experiment.id == experiment_link.experiment_id), - ), - ( - experiment_set_link, - ExperimentSetViewer, - select(experiment_set_link, ExperimentSet).join( - ExperimentSet, ExperimentSet.id == experiment_set_link.experiment_set_id - ), - ), - ] - - def sanitize_target_sequence_label(label: str): """ Strip whitespace from the supplied string and replace any diff --git a/src/mavedb/routers/doi_identifiers.py b/src/mavedb/routers/doi_identifiers.py index b6a42869..762df7ba 100644 --- a/src/mavedb/routers/doi_identifiers.py +++ b/src/mavedb/routers/doi_identifiers.py @@ -5,7 +5,7 @@ from mavedb import deps from mavedb.lib.authorization import get_principal -from mavedb.lib.identifiers import identifier_sources_on_records, search_identifiers_on_readable_records +from mavedb.lib.identifier_search import identifier_sources_on_records, search_identifiers_on_readable_records from mavedb.lib.permissions.principal import Principal from mavedb.models.doi_identifier import ExperimentDoiIdentifier, ExperimentSetDoiIdentifier, ScoreSetDoiIdentifier from mavedb.routers.shared import BASE_400_RESPONSE, PUBLIC_ERROR_RESPONSES, ROUTER_BASE_PREFIX diff --git a/src/mavedb/routers/raw_read_identifiers.py b/src/mavedb/routers/raw_read_identifiers.py index 8968db46..3762fd85 100644 --- a/src/mavedb/routers/raw_read_identifiers.py +++ b/src/mavedb/routers/raw_read_identifiers.py @@ -5,7 +5,7 @@ from mavedb import deps from mavedb.lib.authorization import get_principal -from mavedb.lib.identifiers import identifier_sources_on_records, search_identifiers_on_readable_records +from mavedb.lib.identifier_search import identifier_sources_on_records, search_identifiers_on_readable_records from mavedb.lib.permissions.principal import Principal from mavedb.models.raw_read_identifier import ( ExperimentRawReadIdentifier, diff --git a/src/mavedb/routers/target_gene_identifiers.py b/src/mavedb/routers/target_gene_identifiers.py index 656c495d..88387633 100644 --- a/src/mavedb/routers/target_gene_identifiers.py +++ b/src/mavedb/routers/target_gene_identifiers.py @@ -6,7 +6,7 @@ from mavedb import deps from mavedb.lib.authorization import get_principal -from mavedb.lib.identifiers import search_identifiers_on_readable_records +from mavedb.lib.identifier_search import search_identifiers_on_readable_records from mavedb.lib.permissions.principal import Principal from mavedb.lib.permissions.score_set import ScoreSetViewer from mavedb.lib.validation.constants.identifier import valid_dbnames