From 8ddf51ac3b8e0c23a0edafa792f147c149f4951f Mon Sep 17 00:00:00 2001 From: developer-rpai Date: Wed, 23 Sep 2026 23:19:44 -0700 Subject: [PATCH 1/3] fix: ga4gh_identify respects in_place=never for digest fields With in_place="never", ga4gh_identify computed digests on the live object: compute_digest() stores the digest on the object itself and the serialization step stores digests on every nested identifiable object (e.g. SequenceLocation). As a result the input object was mutated even though the caller asked for no in-place edits. Compute the identifier on a deep copy when in_place is "never", so the caller's object (including its digest fields) is left byte-identical and the only output is the returned identifier string. The "default" and "always" modes are unchanged. Closes #440 --- src/ga4gh/vrs/models.py | 11 +++++++++-- tests/test_vrs.py | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/src/ga4gh/vrs/models.py b/src/ga4gh/vrs/models.py index ad2978d5..8acdf33e 100644 --- a/src/ga4gh/vrs/models.py +++ b/src/ga4gh/vrs/models.py @@ -11,6 +11,7 @@ module name, e.g., `ga4gh.vrs.models.Allele` """ +import copy import inspect import sys from abc import ABC @@ -327,7 +328,10 @@ def get_or_create_ga4gh_identifier( - 'always': this will update the vro.id field any time the identifier is computed - 'never': the vro.id field will not be edited in-place, - even when empty + even when empty. The vro object is not mutated in any way: + digest computation is performed on a deep copy, so neither + the vro.digest field nor digest fields on nested objects + are set. Digests will be recalculated even if present if recompute is True. @@ -344,7 +348,10 @@ def get_or_create_ga4gh_identifier( elif in_place == "always": self.id = self.compute_ga4gh_identifier(recompute) elif in_place == "never": - return self.compute_ga4gh_identifier(recompute) + # Digest computation stores digests on the object and its nested + # identifiable objects, so compute on a deep copy to leave the + # caller's object (including its digest fields) untouched. + return copy.deepcopy(self).compute_ga4gh_identifier(recompute) else: msg = "Expected 'in_place' to be one of 'default', 'always', or 'never'" raise ValueError(msg) diff --git a/tests/test_vrs.py b/tests/test_vrs.py index 975854ab..ae5d120a 100644 --- a/tests/test_vrs.py +++ b/tests/test_vrs.py @@ -194,6 +194,38 @@ def test_cpb(): assert ga4gh_identify(cpb_431012) == "ga4gh:CPB.x8GH5G73cPMs37jy1-9mJjWynu324rxI" +def test_identify_in_place_never_does_not_mutate(): + """ga4gh_identify(..., in_place="never") must not mutate the input object. + + Regression test for https://github.com/ga4gh/vrs-python/issues/440: + digest computation used to set `digest` fields on the object and its + nested identifiable objects even when in_place="never". + """ + allele = models.Allele(**allele_dict) + before = allele.model_dump_json(exclude_none=True) + assert allele.digest is None + assert allele.location.digest is None + + obj_id = ga4gh_identify(allele, in_place="never") + + assert obj_id == "ga4gh:VA.Hy2XU_-rp4IMh6I_1NXNecBo8Qx8n0oE" + assert allele.id is None + assert allele.digest is None + assert allele.location.digest is None + assert allele.model_dump_json(exclude_none=True) == before + + +def test_identify_in_place_modes_still_mutate(): + """Sanity check: in_place="default"/"always" keep their mutating behavior.""" + allele = models.Allele(**allele_dict) + assert ga4gh_identify(allele, in_place="default") == "ga4gh:VA.Hy2XU_-rp4IMh6I_1NXNecBo8Qx8n0oE" + assert allele.id == "ga4gh:VA.Hy2XU_-rp4IMh6I_1NXNecBo8Qx8n0oE" + + allele = models.Allele(**allele_dict) + assert ga4gh_identify(allele, in_place="always") == "ga4gh:VA.Hy2XU_-rp4IMh6I_1NXNecBo8Qx8n0oE" + assert allele.id == "ga4gh:VA.Hy2XU_-rp4IMh6I_1NXNecBo8Qx8n0oE" + + def test_ga4gh_iri(): iri = models.iriReference.model_construct( "ga4gh:VA.Hy2XU_-rp4IMh6I_1NXNecBo8Qx8n0oE" From e246e0ea4b8bb0abd9476c076b82629825d6b69d Mon Sep 17 00:00:00 2001 From: Rakesh Pai <41351936+developer-rpai@users.noreply.github.com> Date: Thu, 24 Sep 2026 09:15:21 -0700 Subject: [PATCH 2/3] style: apply ruff format (fix lint CI) --- tests/test_vrs.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/test_vrs.py b/tests/test_vrs.py index ae5d120a..add14df4 100644 --- a/tests/test_vrs.py +++ b/tests/test_vrs.py @@ -218,11 +218,17 @@ def test_identify_in_place_never_does_not_mutate(): def test_identify_in_place_modes_still_mutate(): """Sanity check: in_place="default"/"always" keep their mutating behavior.""" allele = models.Allele(**allele_dict) - assert ga4gh_identify(allele, in_place="default") == "ga4gh:VA.Hy2XU_-rp4IMh6I_1NXNecBo8Qx8n0oE" + assert ( + ga4gh_identify(allele, in_place="default") + == "ga4gh:VA.Hy2XU_-rp4IMh6I_1NXNecBo8Qx8n0oE" + ) assert allele.id == "ga4gh:VA.Hy2XU_-rp4IMh6I_1NXNecBo8Qx8n0oE" allele = models.Allele(**allele_dict) - assert ga4gh_identify(allele, in_place="always") == "ga4gh:VA.Hy2XU_-rp4IMh6I_1NXNecBo8Qx8n0oE" + assert ( + ga4gh_identify(allele, in_place="always") + == "ga4gh:VA.Hy2XU_-rp4IMh6I_1NXNecBo8Qx8n0oE" + ) assert allele.id == "ga4gh:VA.Hy2XU_-rp4IMh6I_1NXNecBo8Qx8n0oE" From 47c7629c5484e1e31044973339d78c2ffb01ca5d Mon Sep 17 00:00:00 2001 From: developer-rpai Date: Wed, 30 Sep 2026 06:52:31 -0700 Subject: [PATCH 3/3] fix: replace deepcopy with no-store digest computation for in_place=never Address reviewer feedback on the deepcopy in get_or_create_ga4gh_identifier: compute the identifier for in_place="never" without copying the object. - Add _recurse_ga4gh_serialize_no_store, a serialization recursion that never stores digests: nested identifiable objects contribute their existing digest (unless recompute), otherwise one computed with store=False. - Add _ValueObject._ga4gh_serialize_no_store (+ CisPhasedBlock override preserving the sorted-members tweak) and Ga4ghIdentifiableObject._compute_digest_no_store. - in_place="never" now builds the identifier from the no-store digest; default/always modes are untouched. Identifier values are unchanged vs the deepcopy approach (verified against the pinned values in test_identify_in_place_never_does_not_mutate and test_identify_in_place_modes_still_mutate). --- src/ga4gh/vrs/models.py | 62 +++++++++++++++++++++++++++++++++++------ 1 file changed, 54 insertions(+), 8 deletions(-) diff --git a/src/ga4gh/vrs/models.py b/src/ga4gh/vrs/models.py index 8acdf33e..495261b2 100644 --- a/src/ga4gh/vrs/models.py +++ b/src/ga4gh/vrs/models.py @@ -11,7 +11,6 @@ module name, e.g., `ga4gh.vrs.models.Allele` """ -import copy import inspect import sys from abc import ABC @@ -244,6 +243,28 @@ def _recurse_ga4gh_serialize(obj): return obj +def _recurse_ga4gh_serialize_no_store(obj, recompute: bool = False): + """Serialize without mutating any object. + + Mirrors `_recurse_ga4gh_serialize`, but digest computation never stores + digests: nested identifiable objects contribute their existing digest when + set (unless `recompute`), otherwise a digest computed with `store=False`. + """ + if isinstance(obj, Ga4ghIdentifiableObject): + if obj.digest is not None and not recompute: + return obj.digest + return obj._compute_digest_no_store(recompute) + if isinstance(obj, _ValueObject): + return obj._ga4gh_serialize_no_store(recompute) + if isinstance(obj, RootModel): + return _recurse_ga4gh_serialize_no_store(obj.model_dump(), recompute) + if isinstance(obj, str): + return obj + if isinstance(obj, list): + return [_recurse_ga4gh_serialize_no_store(x, recompute) for x in obj] + return obj + + class _ValueObject(Entity, ABC): """A contextual value whose equality is based on value, not identity. See https://en.wikipedia.org/wiki/Value_object for more on Value Objects. @@ -259,6 +280,15 @@ def ga4gh_serialize(self) -> dict: out[k] = _recurse_ga4gh_serialize(v) return out + def _ga4gh_serialize_no_store(self, recompute: bool = False) -> dict: + """Serialize like `ga4gh_serialize`, but digest computation for nested + identifiable objects never stores digests on those objects.""" + out = OrderedDict() + for k in self.ga4gh.inherent: + v = getattr(self, k) + out[k] = _recurse_ga4gh_serialize_no_store(v, recompute) + return out + class ga4gh: # noqa: N801 inherent: list[str] @@ -329,9 +359,9 @@ def get_or_create_ga4gh_identifier( identifier is computed - 'never': the vro.id field will not be edited in-place, even when empty. The vro object is not mutated in any way: - digest computation is performed on a deep copy, so neither - the vro.digest field nor digest fields on nested objects - are set. + digests are computed with ``store=False`` at every level of the + serialization, so neither the vro.digest field nor digest fields + on nested objects are set. Digests will be recalculated even if present if recompute is True. @@ -348,10 +378,11 @@ def get_or_create_ga4gh_identifier( elif in_place == "always": self.id = self.compute_ga4gh_identifier(recompute) elif in_place == "never": - # Digest computation stores digests on the object and its nested - # identifiable objects, so compute on a deep copy to leave the - # caller's object (including its digest fields) untouched. - return copy.deepcopy(self).compute_ga4gh_identifier(recompute) + # Compute the identifier without mutating the object: digests are + # computed with store=False at every level, so the caller's + # object (including its digest fields) is left untouched. + digest = self._compute_digest_no_store(recompute) + return f"{CURIE_NAMESPACE}{CURIE_SEP}{self.ga4gh.prefix}{GA4GH_PREFIX_SEP}{digest}" else: msg = "Expected 'in_place' to be one of 'default', 'always', or 'never'" raise ValueError(msg) @@ -383,6 +414,16 @@ def get_or_create_digest(self, recompute: bool = False) -> str: return self.compute_digest() return self.digest + def _compute_digest_no_store(self, recompute: bool = False) -> str: + """Compute a sha512t24u digest without mutating any object. + + Like `compute_digest(store=False)`, but the serialization step also + avoids storing digests on nested identifiable objects. + """ + return sha512t24u( + encode_canonical_json(self._ga4gh_serialize_no_store(recompute)) + ) + class ga4gh(_ValueObject.ga4gh): # noqa: N801 prefix: str @@ -745,6 +786,11 @@ def ga4gh_serialize(self) -> dict: out["members"] = sorted(out["members"]) return out + def _ga4gh_serialize_no_store(self, recompute: bool = False) -> dict: + out = _ValueObject._ga4gh_serialize_no_store(self, recompute) + out["members"] = sorted(out["members"]) + return out + class ga4gh(Ga4ghIdentifiableObject.ga4gh): prefix = "CPB" inherent = ["members", "type"]