Skip to content

do not mutate caller-supplied metadata in Parameter and Dimension - #64

Closed
jmccreight wants to merge 1 commit into
DOI-USGS:developmentfrom
jmccreight:fix_shared_metadata_mutation
Closed

do not mutate caller-supplied metadata in Parameter and Dimension#64
jmccreight wants to merge 1 commit into
DOI-USGS:developmentfrom
jmccreight:fix_shared_metadata_mutation

Conversation

@jmccreight

Copy link
Copy Markdown
Contributor

Problem

Strict-mode Parameter and Dimension construction alias the
per-entry sub-dict of the caller-supplied metadata
(self.meta = meta[name]). Two code paths then write through that
alias into the caller's dict:

  • Parameter.__init__: the creation-time bounded-maximum resolution
    writes meta['bounded_dimension_name'] and overwrites
    meta['maximum'] (dimension name -> numeric size).
  • Dimension: the size setter writes meta['size'].

The first Parameters instance built from a metadata dict works and
poisons it; any later instance sharing that dict crashes in
Parameters.add() on bounded parameters, which reads
metadata[name]['maximum'] expecting a dimension name:

ValueError: `5` does not exist in metadata     # Dimension(name=5)

Repro (second iteration raises on current development):

import pyPRMS as pp
meta = pp.MetaData(verbose=False).metadata
for _ in range(2):
    p = pp.Parameters(metadata=meta, verbose=False)
    p.dimensions.add(name='one', size=1)
    p.dimensions.add(name='npoigages', size=5)
    p.dimensions.add(name='nobs', size=5)
    p.add(name='outlet_sta')
    print(meta['parameters']['outlet_sta']['maximum'])  # 'nobs' -> 5

Found integration-testing pywatershed against development ahead of the
next release (as discussed): pywatershed holds a module-level
MetaData().metadata and builds multiple Parameters from it; 2 of 6
DomainSubset tests failed this way.

Fix

Copy the metadata sub-dict at strict-mode construction in both
classes (self.meta = dict(meta[name])). Instances keep their
resolved values (bounded_dimension_name, numeric maximum,
size); the caller's dict is never modified. The non-strict paths
are untouched -- they take ad-hoc dicts and internal code deliberately
aliases meta between objects (e.g. ParamDimensions from global
dimensions).

Testing

  • New regression test: two Parameters instances sharing one
    MetaData dict, adding a bounded parameter to each; asserts the
    instance sees the resolved bound and the shared dict is unchanged.
  • Full tests/func suite: 303 passed.
  • Downstream: pywatershed's test_domain_subset.py goes 4/6 -> 6/6
    against this branch (packaging 26.3 environment).

Strict-mode construction aliased the per-entry sub-dict of the
supplied metadata (self.meta = meta[name]), so creation-time bounded
maximum resolution (Parameter) and the size setter (Dimension) wrote
through to the caller's dict. A second Parameters instance built from
the same metadata then read a resolved numeric 'maximum' where
Parameters.add expects a dimension name, raising on bounded
parameters. Copy the sub-dict at construction; instances keep their
resolved values, shared metadata stays pristine. Adds a regression
test with two instances sharing one MetaData dict.
@pnorton-usgs

Copy link
Copy Markdown
Member

Thanks for finding this — the root cause you identified is correct. I've pushed an alternative fix on branch fix/deepcopy-metadata-isolation that addresses it at a higher level in Parameters.__init__ rather than in each leaf class.

The approach uses two separate deepcopy calls:

  1. __full_metadata — pristine copy, never mutated. Used by create_subset() to build child Parameters instances that need the original unresolved metadata (dimension names in bounded maximum fields, original dimension sizes, etc.).

  2. _working_metadata — separate working copy that self.metadata, self.__dimensions, and all Parameter instances are free to mutate (bounded resolution, dimension size changes, etc.).

Since the caller's dict is never passed into the internal machinery, the shallow copies in Parameter.__init__ and Dimension.__init__ become unnecessary (though harmless if left in place).

This also prevents a subtler issue: create_subset() passes self.__full_metadata to a new Parameters(...). If __full_metadata shared sub-dicts with self.metadata (which gets mutated during bounded resolution), the child instance would receive already-resolved metadata and crash. The two-copy approach guarantees __full_metadata stays pristine.

All 302 tests pass. See: development...fix/deepcopy-metadata-isolation

Comment thread tests/func/test_Parameters.py
@jmccreight

Copy link
Copy Markdown
Contributor Author

Agreed. See my comment about keeping the test and then you can close the PR. Thanks!

pnorton-usgs added a commit that referenced this pull request Aug 17, 2026
…#65)

Use two separate deep copies in Parameters.__init__:

1. __full_metadata: pristine copy, never mutated by the instance.
   Used by create_subset() to build child Parameters instances
   that need the original unresolved metadata (dimension names
   in bounded 'maximum' fields, original dimension sizes, etc.).

2. _working_metadata: separate working copy that self.metadata,
   self.__dimensions, and all Parameter instances are free to mutate
   (bounded resolution, dimension size changes, etc.).

This supersedes the need for shallow copies in Parameter.__init__
and Dimension.__init__ since the caller's dict is never passed into
the internal machinery. The approach also prevents create_subset()
from crashing due to mutated __full_metadata, which would happen
if __full_metadata and self.metadata shared sub-dicts.

Addresses the same root cause as PR #64 (caller-supplied metadata
being mutated across Parameters instances) but at a higher level,
providing stronger guarantees.
@pnorton-usgs

pnorton-usgs commented Aug 17, 2026

Copy link
Copy Markdown
Member

Issues addressed in this PR have been handled by PR #63 and #65. Closing this PR.

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.

2 participants