Skip to content

fix: reject duplicate resource ids in FHIR Bundle assembler#557

Open
gitcommit90 wants to merge 2 commits into
maziyarpanahi:masterfrom
gitcommit90:fix/fhir-bundle-duplicate-resource-ids
Open

fix: reject duplicate resource ids in FHIR Bundle assembler#557
gitcommit90 wants to merge 2 commits into
maziyarpanahi:masterfrom
gitcommit90:fix/fhir-bundle-duplicate-resource-ids

Conversation

@gitcommit90

Copy link
Copy Markdown

Description

to_bundle builds reference_map keyed by "ResourceType/id". When two resources share the same type and id, the second silently overwrites the first, corrupting internal cross-references. This PR detects the collision and raises a clear ValueError naming the colliding key. Resources without an id remain valid (unreferenceable).

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

Changes Made

  • Detect duplicate "ResourceType/id" keys in reference_map and raise ValueError
  • Document the duplicate-id contract in the to_bundle docstring (Raises section)
  • Add 5 regression tests covering collision detection, error message content, id-less resources, distinct types with same id, and position independence

Testing

  • I have added tests that prove my fix is effective
  • New and existing unit tests pass locally (1609 passed, 22 skipped)

Code Quality

  • I ran ruff check and ruff format --check — all passed
  • My changes generate no new warnings

Dependencies

  • I have not added any new dependencies

Related Issues

Closes #530

to_bundle builds reference_map keyed by ResourceType/id. When two
resources share the same resourceType and id, the second silently
overwrites the first, corrupting internal cross-references. Detect the
collision and raise a clear ValueError naming the colliding key.
Resources without an id remain valid (unreferenceable).
@maziyarpanahi maziyarpanahi added bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed P2 Medium roadmap-v2 OpenMed V2 roadmap backlog labels Jun 22, 2026

@maziyarpanahi maziyarpanahi left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thank you @gitcommit90. I reviewed this against #530 / OM-340.

The implementation matches the issue intent: duplicate ResourceType/id keys are rejected before the reference map can silently overwrite a target, the error names the colliding key, resources without an id remain valid/unreferenceable, and same-id resources of different FHIR types are still allowed. The docstring now records the duplicate-id contract.

I did not need to change the branch code. I copied the labels from #530 onto the PR.

Verification on the current PR checkout:

  • PYTHONPATH=/private/tmp/openmed-pr-557 /Users/maziyar/Developer/openmed/.venv/bin/python -m pytest tests/unit/clinical/test_fhir_bundle.py -q -> 17 passed
  • /Users/maziyar/Developer/openmed/.venv/bin/ruff check openmed/clinical/exporters/fhir/bundle.py tests/unit/clinical/test_fhir_bundle.py -> passed
  • /Users/maziyar/Developer/openmed/.venv/bin/ruff format --check openmed/clinical/exporters/fhir/bundle.py tests/unit/clinical/test_fhir_bundle.py -> passed

The branch is mergeable with no conflicts; GitHub has not attached hosted checks to this head commit yet, so I verified the touched behavior locally.

…licate-resource-ids

# Conflicts:
#	openmed/clinical/exporters/fhir/bundle.py
@maziyarpanahi

Copy link
Copy Markdown
Owner

Thank you @gitcommit90. I rechecked this PR against #530 after master moved and added a merge-resolution commit: Merge remote-tracking branch 'origin/master' into fix/fhir-bundle-duplicate-resource-ids.

What changed:

  • resolved the duplicate-resource-id conflict with the current FHIR bundle implementation on master;
  • kept a duplicate-id error message that preserves the current duplicate FHIR resource id: Resource/id contract while retaining the explicit warning that duplicate IDs would corrupt internal cross-references;
  • preserved the contributor regression cases together with the newer edge-case bundle tests from master.

Verification:

  • /Users/maziyar/Developer/openmed/.venv/bin/python -m pytest tests/unit/clinical/test_fhir_bundle.py tests/unit/clinical/test_fhir_bundle_edge_cases.py -q -> 25 passed
  • /Users/maziyar/Developer/openmed/.venv/bin/ruff check openmed/clinical/exporters/fhir/bundle.py tests/unit/clinical/test_fhir_bundle.py tests/unit/clinical/test_fhir_bundle_edge_cases.py -> passed
  • /Users/maziyar/Developer/openmed/.venv/bin/ruff format --check openmed/clinical/exporters/fhir/bundle.py tests/unit/clinical/test_fhir_bundle.py tests/unit/clinical/test_fhir_bundle_edge_cases.py -> passed

The issue labels are already on the PR. The branch is mergeable with no conflicts. GitHub has not reported hosted checks for this fork head, so the validation above is local.

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

Labels

bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed P2 Medium roadmap-v2 OpenMed V2 roadmap backlog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reject duplicate resource ids in the FHIR Bundle assembler

2 participants