Skip to content

fix: ga4gh_identify respects in_place=never for digest fields - #657

Open
developer-rpai wants to merge 3 commits into
ga4gh:mainfrom
developer-rpai:fix-440-identify-inplace-digest
Open

developer-rpai wants to merge 3 commits into
ga4gh:mainfrom
developer-rpai:fix-440-identify-inplace-digest

Conversation

@developer-rpai

Copy link
Copy Markdown

Bug

ga4gh_identify(allele, in_place="never") mutated the input object: it injected digest fields onto the Allele and its nested identifiable objects (e.g. SequenceLocation), even though the caller asked for no in-place edits. Reproducer from the issue: model_dump_json(exclude_none=True) grew from 323 to 411 chars after the call.

Closes #440

Root cause

The in_place="never" branch of get_or_create_ga4gh_identifier ran compute_ga4gh_identifier on the live object. That path always mutates: compute_digest() stores the digest on the object itself (store=True default), and ga4gh_serialize() calls _recurse_ga4gh_serialize, which calls get_or_create_digest() on every nested identifiable object, setting their digest fields in place. So both the top-level digest and nested digests were written into the caller's object.

Fix

When in_place is "never", compute the identifier on a copy.deepcopy of the object (src/ga4gh/vrs/models.py). The caller's object is left byte-identical; the only output is the returned identifier string. The default and always modes are unchanged.

Verification

  • Reproduced the bug on pristine upstream main (object JSON grew 275 to 363 chars; allele.digest and allele.location.digest injected), then confirmed the fix leaves the object unchanged and returns the same identifier value.
  • Added regression test test_identify_in_place_never_does_not_mutate (fails on pristine code, passes with the fix) and test_identify_in_place_modes_still_mutate (sanity check that default/always still set id).
  • tests/test_vrs.py: all 13 tests pass. Doctests in src/ga4gh/core/identifiers.py and src/ga4gh/vrs/models.py: 3 pass.

Not verified

  • Full test suite / CI: could not run here. tests/validation/test_models.py needs the submodules/vrs checkout (submodule fetch is blocked in this sandbox), and test_vrs_normalize.py / parts of test_dataproxy.py need the local seqrepo REST service on localhost:5000 (unavailable here; they fail with connection refused). Neither failure is related to this change.
  • Ruff: not installed in this sandbox, but E501 is ignored in the repo config and the change follows existing style.

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 ga4gh#440
@developer-rpai
developer-rpai requested review from a team as code owners September 24, 2026 06:20
@jsstevenson

Copy link
Copy Markdown
Contributor

I am not 100% on this fix; would really prefer to avoid an unnecessary deepcopy, but I'm not sure if that's avoidable

@korikuzma korikuzma left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See @jsstevenson comment above. Are you able to investigate this @developer-rpai ?

…ever

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).
@developer-rpai

Copy link
Copy Markdown
Author

Good news: the deepcopy is gone.

I replaced it with a no-store computation path: Ga4ghIdentifiableObject._compute_digest_no_store hashes the serialization without setting the digest field, backed by a _recurse_ga4gh_serialize_no_store variant of the serialization recursion that does the same for nested identifiable objects (returns their existing digest when set, otherwise computes one with store=False). default/always modes are untouched, and the CisPhasedBlock members-sorting override is preserved in the no-store path.

Verified locally: identifier values are unchanged vs the deepcopy approach (allele with and without recompute, CisPhasedBlock, pre-set nested digest), the caller's object is left byte-identical, and all 13 tests in tests/test_vrs.py pass, including the regression tests.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ga4gh_identify adds digest properties in-place regardless of in_place argument

3 participants