Enforce sequence coordinate bounds

src/ga4gh/vrs/dataproxy.py

@@ -16,13 +16,26 @@
 import requests
 from bioutils.accessions import coerce_namespace
 
+from ga4gh.vrs.models import Range
+
 _logger = logging.getLogger(__name__)
 
 
 class DataProxyValidationError(Exception):
     """Class for validation errors during data proxy methods"""
 
 
+def validate_start_le_end(identifier: str, start: int | None, end: int | None) -> None:
+    """Reject an interval whose ``start`` is after its ``end``, since vrs-python treats
+    every sequence as linear.
+
+    :raises DataProxyValidationError: If both are defined and ``start > end``
+    """
+    if start is not None and end is not None and start > end:
+        err_msg = f"Interval start must not be after end on {identifier}: start={start}, end={end}"
+        raise DataProxyValidationError(err_msg)
+
+
 class _DataProxy(ABC):
     """abstract class / interface for VRS data needs
 
@@ -42,7 +55,8 @@ def get_sequence(
 
         start and end are optional
 
-        If the given sequence does not exist, KeyError is raised.
+        If the given sequence does not exist, KeyError is raised. If start is
+        after end, DataProxyValidationError may be raised.
 
         >> dp.get_sequence("NM_000551.3", 0, 10)
         'CCTCGCCTCC'
@@ -171,7 +185,12 @@ def validate_ref_seq(
             logged.
         :raises DataProxyValidationError: If excepted reference sequence does not match
             the actual reference sequence and ``require_validation`` is ``True``.
+            Raised regardless of ``require_validation`` if the location is invalid
+            (see ``validate_location_bounds``).
         """
+        # An out-of-bounds fetch may be silently truncated, which would be misreported
+        # as a reference mismatch
+        self.validate_location_bounds(sequence_id, start_pos, end_pos)
         correct_ref = self.get_sequence(sequence_id, start_pos, end_pos)
 
         if correct_ref != ref:
@@ -181,6 +200,65 @@ def validate_ref_seq(
             if require_validation:
                 raise DataProxyValidationError(err_msg)
 
+    def validate_location_bounds(
+        self,
+        sequence_id: str,
+        start_pos: int | Range | None,
+        end_pos: int | Range | None,
+    ) -> None:
+        """Ensure that ``start_pos`` and ``end_pos`` are representable on ``sequence_id``.
+
+        Each defined coordinate must be within ``[0, len(sequence)]`` (inter-residue).
+        Undefined (``None``) endpoints are skipped, and for a ``Range`` every defined
+        member is checked.
+
+        ``start_pos`` must also not be after ``end_pos``, comparing a ``Range`` by its
+        lowest possible start and highest possible end.
+
+        There is no ``require_validation`` option: an out-of-bounds location has no
+        meaning, so the error is always raised, including from ``validate_ref_seq``.
+        Sequence backends may silently truncate out-of-range fetches, so this check
+        must be made before relying on fetched sequence.
+
+        :param sequence_id: Sequence ID to use
+        :param start_pos: Start pos (inter-residue) on the sequence_id
+        :param end_pos: End pos (inter-residue) on the sequence_id
+        :raises DataProxyValidationError: If a defined coordinate is out of bounds, or
+            ``start_pos`` is after ``end_pos``
+        :raises KeyError: If ``sequence_id`` is not found
+        """
+        # same cache key as derive_refget_accession
+        sequence_id = coerce_namespace(sequence_id)
+        md = self.get_metadata(sequence_id)
+        seq_len = md["length"]
+        bad = []
+        for name, pos in (("start", start_pos), ("end", end_pos)):
+            values = pos.root if isinstance(pos, Range) else [pos]
+            if any(v is not None and not 0 <= v <= seq_len for v in values):
+                bad.append((name, pos))
+        if bad:
+            # Range is shown as its list form, e.g. end=[4500, 4600]
+            detail = ", ".join(
+                f"{name}={pos.root if isinstance(pos, Range) else pos}"
+                for name, pos in bad
+            )
+            # Name the sequence as given, plus the refget accession it resolved to
+            refget_ac = next((a for a in md["aliases"] if a.startswith("ga4gh:")), None)
+            seq_name = sequence_id
+            if refget_ac and refget_ac != sequence_id:
+                seq_name += f" ({refget_ac})"
+            err_msg = (
+                f"Location out of bounds on {seq_name}: {detail} "
+                f"not within [0, {seq_len}]"
+            )
+            raise DataProxyValidationError(err_msg)
+
+        validate_start_le_end(
+            sequence_id,
+            start_pos.root[0] if isinstance(start_pos, Range) else start_pos,
+            end_pos.root[1] if isinstance(end_pos, Range) else end_pos,
+        )
+
 
 class _SeqRepoDataProxyBase(_DataProxy):
     # wraps seqreqpo classes in order to provide translation to/from
@@ -196,6 +274,8 @@ def get_metadata(self, identifier: str) -> dict:
     def get_sequence(
         self, identifier: str, start: int | None = None, end: int | None = None
     ) -> str:
+        # Checked here because backends disagree on how to handle it
+        validate_start_le_end(identifier, start, end)
         return self._get_sequence(identifier, start=start, end=end)
 
     @abstractmethod

src/ga4gh/vrs/extras/translator.py

@@ -202,6 +202,8 @@ def _create_allele(self, values: dict, **kwargs) -> models.Allele:
 
         Args:
             values (dict): The values to use for creating the allele object.
+                'sequence_id' (str): The sequence identifier from the input
+                    expression, used to validate `start` and `end`.
                 'refget_accession' (str): The accession ID of the reference genome.
                 'start' (int): The start position of the allele.
                 'end' (int): The end position of the allele.
@@ -212,6 +214,9 @@ def _create_allele(self, values: dict, **kwargs) -> models.Allele:
             models.Allele: The created allele object.
 
         """
+        self.data_proxy.validate_location_bounds(
+            values["sequence_id"], values["start"], values["end"]
+        )
         seq_ref = models.SequenceReference(refgetAccession=values["refget_accession"])
         location = models.SequenceLocation(
             sequenceReference=seq_ref, start=values["start"], end=values["end"]
@@ -275,6 +280,7 @@ def _from_beacon(self, beacon_expr: str, **kwargs) -> models.Allele | None:
         ins_seq = alt
 
         values = {
+            "sequence_id": sequence,
             "refget_accession": refget_accession,
             "start": start,
             "end": end,
@@ -348,6 +354,7 @@ def _from_gnomad(self, gnomad_expr: str, **kwargs) -> models.Allele | None:
         )
 
         values = {
+            "sequence_id": sequence,
             "refget_accession": refget_accession,
             "start": start,
             "end": end,
@@ -413,6 +420,7 @@ def _from_spdi(self, spdi_expr: str, **kwargs) -> models.Allele | None:
         ins_seq = g["ins_seq"]
 
         values = {
+            "sequence_id": g["ac"],
             "refget_accession": refget_accession,
             "start": start,
             "end": end,
@@ -569,12 +577,16 @@ def _from_hgvs(
         if not refget_accession:
             return None
 
+        start = sv.posedit.pos.start.base - 1
+        end = sv.posedit.pos.end.base
+        self.data_proxy.validate_location_bounds(sv.ac, start, end)
+
         location = models.SequenceLocation(
             sequenceReference=models.SequenceReference(
                 refgetAccession=refget_accession
             ),
-            start=sv.posedit.pos.start.base - 1,
-            end=sv.posedit.pos.end.base,
+            start=start,
+            end=end,
         )
 
         copies = kwargs.get("copies")

src/ga4gh/vrs/normalize.py

@@ -111,6 +111,8 @@ def _normalize_allele(
         of the `sequence`.
         To exclude `sequence` from the response, set to 0.
         For no limit, set to `None`.
+    :raises DataProxyValidationError: If the allele location is out of bounds on its
+        sequence
     """
     # Algorithm applies to LiteralSequenceExpression alleles only; other states are returned unchanged
     if not isinstance(input_allele.state, models.LiteralSequenceExpression):
@@ -130,6 +132,14 @@ def _normalize_allele(
 
     # 0: Get reference sequence and interval
     ref_seq = SequenceProxy(data_proxy, alias)
+
+    # Reject locations that do not exist on the sequence before anything is fetched,
+    # since out-of-range fetches may be silently truncated by the sequence backend.
+    # Done before the early returns below, which skip definite ranges.
+    data_proxy.validate_location_bounds(
+        alias, input_allele.location.start, input_allele.location.end
+    )
+
     start = _get_allele_location_pos(input_allele, use_start=True)
     if start is None:
         return input_allele

src/ga4gh/vrs/utils/hgvs_tools.py

@@ -107,6 +107,8 @@ def get_position_and_state(self, sv: HgvsSequenceVariant) -> tuple[int, int, str
 
         Raises:
             ValueError: If the HGVS variant type is unsupported.
+            DataProxyValidationError: If an identity or duplication's location is
+                invalid.
 
         """
         if sv.posedit.edit.type == "ins":
@@ -118,20 +120,17 @@ def get_position_and_state(self, sv: HgvsSequenceVariant) -> tuple[int, int, str
             start = sv.posedit.pos.start.base - 1
             end = sv.posedit.pos.end.base
             if sv.posedit.edit.type == "identity":
-                state = self.data_proxy.get_sequence(
-                    sv.ac,
-                    start=sv.posedit.pos.start.base - 1,
-                    end=sv.posedit.pos.end.base,
-                )
+                # Checked before fetching, which a backend may silently truncate
+                self.data_proxy.validate_location_bounds(sv.ac, start, end)
+                state = self.data_proxy.get_sequence(sv.ac, start=start, end=end)
             else:
                 state = sv.posedit.edit.alt or ""
 
         elif sv.posedit.edit.type == "dup":
             start = sv.posedit.pos.start.base - 1
             end = sv.posedit.pos.end.base
-            ref = self.data_proxy.get_sequence(
-                sv.ac, start=sv.posedit.pos.start.base - 1, end=sv.posedit.pos.end.base
-            )
+            self.data_proxy.validate_location_bounds(sv.ac, start, end)
+            ref = self.data_proxy.get_sequence(sv.ac, start=start, end=end)
             state = ref + ref
 
         else:
@@ -191,6 +190,7 @@ def extract_allele_values(self, hgvs_expr: str) -> dict | None:
         (start, end, state) = self.get_position_and_state(sv)
 
         return {
+            "sequence_id": sv.ac,
             "refget_accession": refget_accession,
             "start": start,
             "end": end,

tests/extras/cassettes/test_in_bounds[spdi-insertion-at-end].yaml

@@ -0,0 +1,58 @@
+interactions:
+- request:
+    body: null
+    headers: {}
+    method: GET
+    uri: http://localhost:5000/seqrepo/1/metadata/refseq:NM_000551.3
+  response:
+    body:
+      string: "{\n  \"added\": \"2016-08-24T05:03:11Z\",\n  \"aliases\": [\n    \"MD5:215137b1973c1a5afcf86be7d999574a\",\n
+        \   \"NCBI:NM_000551.3\",\n    \"refseq:NM_000551.3\",\n    \"SEGUID:T12L0p2X5E8DbnL0+SwI4Wc1S6g\",\n
+        \   \"SHA1:4f5d8bd29d97e44f036e72f4f92c08e167354ba8\",\n    \"VMC:GS_v_QTc1p-MUYdgrRv4LMT6ByXIOsdw3C_\",\n
+        \   \"sha512t24u:v_QTc1p-MUYdgrRv4LMT6ByXIOsdw3C_\",\n    \"ga4gh:SQ.v_QTc1p-MUYdgrRv4LMT6ByXIOsdw3C_\"\n
+        \ ],\n  \"alphabet\": \"ACGT\",\n  \"length\": 4560\n}\n"
+    headers: {}
+    status:
+      code: 200
+      message: OK
+- request:
+    body: null
+    headers: {}
+    method: GET
+    uri: http://localhost:5000/seqrepo/1/metadata/ga4gh:SQ.v_QTc1p-MUYdgrRv4LMT6ByXIOsdw3C_
+  response:
+    body:
+      string: "{\n  \"added\": \"2016-08-24T05:03:11Z\",\n  \"aliases\": [\n    \"MD5:215137b1973c1a5afcf86be7d999574a\",\n
+        \   \"NCBI:NM_000551.3\",\n    \"refseq:NM_000551.3\",\n    \"SEGUID:T12L0p2X5E8DbnL0+SwI4Wc1S6g\",\n
+        \   \"SHA1:4f5d8bd29d97e44f036e72f4f92c08e167354ba8\",\n    \"VMC:GS_v_QTc1p-MUYdgrRv4LMT6ByXIOsdw3C_\",\n
+        \   \"sha512t24u:v_QTc1p-MUYdgrRv4LMT6ByXIOsdw3C_\",\n    \"ga4gh:SQ.v_QTc1p-MUYdgrRv4LMT6ByXIOsdw3C_\"\n
+        \ ],\n  \"alphabet\": \"ACGT\",\n  \"length\": 4560\n}\n"
+    headers: {}
+    status:
+      code: 200
+      message: OK
+- request:
+    body: null
+    headers: {}
+    method: GET
+    uri: http://localhost:5000/seqrepo/1/sequence/ga4gh:SQ.v_QTc1p-MUYdgrRv4LMT6ByXIOsdw3C_?start=4560&end=4560
+  response:
+    body:
+      string: ''
+    headers: {}
+    status:
+      code: 200
+      message: OK
+- request:
+    body: null
+    headers: {}
+    method: GET
+    uri: http://localhost:5000/seqrepo/1/sequence/ga4gh:SQ.v_QTc1p-MUYdgrRv4LMT6ByXIOsdw3C_?start=4559&end=4560
+  response:
+    body:
+      string: G
+    headers: {}
+    status:
+      code: 200
+      message: OK
+version: 1

tests/extras/cassettes/test_out_of_bounds[beacon-past-end].yaml

@@ -0,0 +1,25 @@
+interactions:
+- request:
+    body: null
+    headers: {}
+    method: GET
+    uri: http://localhost:5000/seqrepo/1/metadata/GRCh38:1
+  response:
+    body:
+      string: "{\n  \"added\": \"2016-08-27T21:17:00Z\",\n  \"aliases\": [\n    \"GRCh38:1\",\n
+        \   \"GRCh38:chr1\",\n    \"GRCh38.p1:1\",\n    \"GRCh38.p1:chr1\",\n    \"GRCh38.p10:1\",\n
+        \   \"GRCh38.p10:chr1\",\n    \"GRCh38.p11:1\",\n    \"GRCh38.p11:chr1\",\n
+        \   \"GRCh38.p12:1\",\n    \"GRCh38.p12:chr1\",\n    \"GRCh38.p2:1\",\n    \"GRCh38.p2:chr1\",\n
+        \   \"GRCh38.p3:1\",\n    \"GRCh38.p3:chr1\",\n    \"GRCh38.p4:1\",\n    \"GRCh38.p4:chr1\",\n
+        \   \"GRCh38.p5:1\",\n    \"GRCh38.p5:chr1\",\n    \"GRCh38.p6:1\",\n    \"GRCh38.p6:chr1\",\n
+        \   \"GRCh38.p7:1\",\n    \"GRCh38.p7:chr1\",\n    \"GRCh38.p8:1\",\n    \"GRCh38.p8:chr1\",\n
+        \   \"GRCh38.p9:1\",\n    \"GRCh38.p9:chr1\",\n    \"MD5:6aef897c3d6ff0c78aff06ac189178dd\",\n
+        \   \"NCBI:NC_000001.11\",\n    \"refseq:NC_000001.11\",\n    \"SEGUID:FCUd6VJ6uikS/VWLbhGdVmj2rOA\",\n
+        \   \"SHA1:14251de9527aba2912fd558b6e119d5668f6ace0\",\n    \"VMC:GS_Ya6Rs7DHhDeg7YaOSg1EoNi3U_nQ9SvO\",\n
+        \   \"sha512t24u:Ya6Rs7DHhDeg7YaOSg1EoNi3U_nQ9SvO\",\n    \"ga4gh:SQ.Ya6Rs7DHhDeg7YaOSg1EoNi3U_nQ9SvO\"\n
+        \ ],\n  \"alphabet\": \"ACGMNRT\",\n  \"length\": 248956422\n}\n"
+    headers: {}
+    status:
+      code: 200
+      message: OK
+version: 1

tests/extras/cassettes/test_out_of_bounds[cnv-hgvs-copy-number-change-past-end].yaml

@@ -0,0 +1,25 @@
+interactions:
+- request:
+    body: null
+    headers: {}
+    method: GET
+    uri: http://localhost:5000/seqrepo/1/metadata/refseq:NC_000007.14
+  response:
+    body:
+      string: "{\n  \"added\": \"2016-08-27T21:23:35Z\",\n  \"aliases\": [\n    \"GRCh38:7\",\n
+        \   \"GRCh38:chr7\",\n    \"GRCh38.p1:7\",\n    \"GRCh38.p1:chr7\",\n    \"GRCh38.p10:7\",\n
+        \   \"GRCh38.p10:chr7\",\n    \"GRCh38.p11:7\",\n    \"GRCh38.p11:chr7\",\n
+        \   \"GRCh38.p12:7\",\n    \"GRCh38.p12:chr7\",\n    \"GRCh38.p2:7\",\n    \"GRCh38.p2:chr7\",\n
+        \   \"GRCh38.p3:7\",\n    \"GRCh38.p3:chr7\",\n    \"GRCh38.p4:7\",\n    \"GRCh38.p4:chr7\",\n
+        \   \"GRCh38.p5:7\",\n    \"GRCh38.p5:chr7\",\n    \"GRCh38.p6:7\",\n    \"GRCh38.p6:chr7\",\n
+        \   \"GRCh38.p7:7\",\n    \"GRCh38.p7:chr7\",\n    \"GRCh38.p8:7\",\n    \"GRCh38.p8:chr7\",\n
+        \   \"GRCh38.p9:7\",\n    \"GRCh38.p9:chr7\",\n    \"MD5:cc044cc2256a1141212660fb07b6171e\",\n
+        \   \"NCBI:NC_000007.14\",\n    \"refseq:NC_000007.14\",\n    \"SEGUID:4+JjCcBVhPCr8vdIhUKFycPv8bY\",\n
+        \   \"SHA1:e3e26309c05584f0abf2f748854285c9c3eff1b6\",\n    \"VMC:GS_F-LrLMe1SRpfUZHkQmvkVKFEGaoDeHul\",\n
+        \   \"sha512t24u:F-LrLMe1SRpfUZHkQmvkVKFEGaoDeHul\",\n    \"ga4gh:SQ.F-LrLMe1SRpfUZHkQmvkVKFEGaoDeHul\"\n
+        \ ],\n  \"alphabet\": \"ACGNRSTY\",\n  \"length\": 159345973\n}\n"
+    headers: {}
+    status:
+      code: 200
+      message: OK
+version: 1

tests/extras/cassettes/test_out_of_bounds[gnomad-negative-start].yaml

@@ -0,0 +1,25 @@
+interactions:
+- request:
+    body: null
+    headers: {}
+    method: GET
+    uri: http://localhost:5000/seqrepo/1/metadata/GRCh38:1
+  response:
+    body:
+      string: "{\n  \"added\": \"2016-08-27T21:17:00Z\",\n  \"aliases\": [\n    \"GRCh38:1\",\n
+        \   \"GRCh38:chr1\",\n    \"GRCh38.p1:1\",\n    \"GRCh38.p1:chr1\",\n    \"GRCh38.p10:1\",\n
+        \   \"GRCh38.p10:chr1\",\n    \"GRCh38.p11:1\",\n    \"GRCh38.p11:chr1\",\n
+        \   \"GRCh38.p12:1\",\n    \"GRCh38.p12:chr1\",\n    \"GRCh38.p2:1\",\n    \"GRCh38.p2:chr1\",\n
+        \   \"GRCh38.p3:1\",\n    \"GRCh38.p3:chr1\",\n    \"GRCh38.p4:1\",\n    \"GRCh38.p4:chr1\",\n
+        \   \"GRCh38.p5:1\",\n    \"GRCh38.p5:chr1\",\n    \"GRCh38.p6:1\",\n    \"GRCh38.p6:chr1\",\n
+        \   \"GRCh38.p7:1\",\n    \"GRCh38.p7:chr1\",\n    \"GRCh38.p8:1\",\n    \"GRCh38.p8:chr1\",\n
+        \   \"GRCh38.p9:1\",\n    \"GRCh38.p9:chr1\",\n    \"MD5:6aef897c3d6ff0c78aff06ac189178dd\",\n
+        \   \"NCBI:NC_000001.11\",\n    \"refseq:NC_000001.11\",\n    \"SEGUID:FCUd6VJ6uikS/VWLbhGdVmj2rOA\",\n
+        \   \"SHA1:14251de9527aba2912fd558b6e119d5668f6ace0\",\n    \"VMC:GS_Ya6Rs7DHhDeg7YaOSg1EoNi3U_nQ9SvO\",\n
+        \   \"sha512t24u:Ya6Rs7DHhDeg7YaOSg1EoNi3U_nQ9SvO\",\n    \"ga4gh:SQ.Ya6Rs7DHhDeg7YaOSg1EoNi3U_nQ9SvO\"\n
+        \ ],\n  \"alphabet\": \"ACGMNRT\",\n  \"length\": 248956422\n}\n"
+    headers: {}
+    status:
+      code: 200
+      message: OK
+version: 1