Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SnapshotSource does not reuse self-generated snapshot #2802

Closed
cknaap opened this issue Jun 25, 2024 · 2 comments · Fixed by #2803
Closed

SnapshotSource does not reuse self-generated snapshot #2802

cknaap opened this issue Jun 25, 2024 · 2 comments · Fixed by #2803
Assignees

Comments

@cknaap
Copy link
Member

cknaap commented Jun 25, 2024

Describe the bug
SnapshotSource should return the same snapshot upon multiple retrievals. Instead, it regenerates the snapshot every time.

To Reproduce
Steps to reproduce the behavior:

  1. Create a SnapshotSource, with regenerate: true, based on a CachedResolver
  2. Get a StructureDefinition by canonical, e.g. from Element
  3. Save a reference to the resulting StructureDefinition.Snapshot
  4. Repeat step 2
  5. Save a reference to the second resulting StructureDefinition.Snapshot
  6. Observe that both references are not the same.

Expected behavior
When the SnapshotSource has generated (by the Generator) a snapshot, it is marked as IsCreatedBySnapshotGenerator(), and it should not be re-generated again upon subsequent retrievals.

Screenshots
n/a

Version used:

  • FHIR Version: All
  • Version: 5.8.1

Additional context

@ewoutkramer
Copy link
Member

ewoutkramer commented Jun 25, 2024

This is not a bug, but a feature. Set regenerate to false to get the desired behaviour. I must be missing something ;-)

@ewoutkramer
Copy link
Member

Offline discussion added some new insights:

  • The current Regenerate options will always regenerate a new snapshot, even though the documentation suggests it will not repeatedly do so.
  • Setting this to true seems to be not really useful, since -except for some testing usecases- it's not clear why you'd like to snapshot again and again.
  • Setting it to false is what most people want: generate the snapshot if it does not exist or re-generate it when it's not done by our tools. This is the default and many tools will depend on it.
  • It is not possible to allow snapshots created by other tools (probably: the java tooling) at this moment.
  • Since whether a snapshot is "ours" is done using an in-memory annotation, initially every snapshot will be regenerated, the flag only ensures we're not doing it twice.

If we'd apply #2803 we will certainly get regressions, since this PR will cause true to result in the old behaviour for false, and false (still the default) will cause 3rd-party snapshots to be used instead of being regenerated.

A first conclusion might be that the current regenerate option is not particularly useful (we'd be better of without it), is not documented clearly and does not allow for 3rd-party snapshots to be used. We should obsolete it and introduce a more explicit setting. Basically there are only two cases:

  • Add a snapshot if there is none or use whatever is there (new behaviour)
  • Regenerate snapshot once to ensure it is "ours" (and/or at least updated to the lastest version) (current default behaviour)
  • Maybe: heuristically try to determine whether the snapshot is ours (there must be obvious differences) and then avoid regenerating it.

@mmsmits mmsmits self-assigned this Jul 25, 2024
@mmsmits mmsmits assigned Kasdejong and unassigned mmsmits Aug 19, 2024
mmsmits added a commit that referenced this issue Sep 10, 2024
…pshot

fix: #2802 - let SnapshotSource reuse self-generated snapshot
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 a pull request may close this issue.

4 participants