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

SnapshotGenerator: Keep snapshot when differential is missing #2586

Open
mharthoorn opened this issue Sep 12, 2023 · 6 comments · Fixed by #2754
Open

SnapshotGenerator: Keep snapshot when differential is missing #2586

mharthoorn opened this issue Sep 12, 2023 · 6 comments · Fixed by #2754
Assignees
Labels

Comments

@mharthoorn
Copy link
Member

Problem:
If the snapshot generator encounters a StructureDefinition (of type LogicalModel) that already has a snapshot, but not a differential, it will remove that snapshot without replacing it (only when SnapshotGeneratorSettings.ForceRegenerateSnapshots is set to true)

Solution
The SnapshotGenerator should not remove a snapshot when there is no differential present in a StructureDefinition.

Alternatives

  • Create an extra setting SnapshotGeneratorSettings.SkipRegenerateOnMissingDifferential
  • Throw an Exception
  • Add a warning to the OperationOutcome

Additional context
In bulk snapshot operations this situation tends to go unnoticed and leads to loss of data / correctness in publications.

@Kasdejong
Copy link
Contributor

Kasdejong commented Mar 27, 2024

[TestMethod]
        public async T.Task TestMissingDifferential()
        {
            // Create a profile without a differential
            var profile = ObservationTypeSliceProfile;
            profile.Differential = null;
            profile.Kind = StructureDefinition.StructureDefinitionKind.Logical;
            var settings = new SnapshotGeneratorSettings { ForceRegenerateSnapshots = true };

            var resolver = new InMemoryResourceResolver(profile);
            var multiResolver = new MultiResolver(_testResolver, resolver);
            _generator = new SnapshotGenerator(multiResolver, settings);

            var (_, expanded) = await generateSnapshotAndCompare(profile);
            Assert.IsNotNull(expanded);
            Assert.IsTrue(expanded.HasSnapshot);

            expanded.Snapshot.Element.Dump();
        }

We are having issues reproducing the issue with this unit test, though the unit test seems to meet all the conditions. Is there something missing?

@mharthoorn

@Rob5045
Copy link
Contributor

Rob5045 commented Mar 28, 2024

Note that Simplifier uses version 5.1 of the SDK. Maybe the issue has been resolved in the mean time? I will check to see if I can reproduce the issue in the current Simplifier version.

@Rob5045
Copy link
Contributor

Rob5045 commented Mar 28, 2024

Unfortunately Martijn does not have reproduction steps. However I did find a possible cause. According to the specification for logical models (https://www.hl7.org/fhir/R5/logical.html) the base type can either be Element or Base. When the base type is set to Base the snapshot generator tries to resolve "http://hl7.org/fhir/StructureDefinition/Base". I created a resolver using ZipSource.CreateValidationSource() but it does not find the resource. It does find "http://hl7.org/fhir/StructureDefinition/Element". Looking at specification.zip I cannot find a definition for Base either. The snapshot generator always returns null when it cannot find the base definition.

@mmsmits
Copy link
Member

mmsmits commented Apr 2, 2024

Thanks Rob, we will create a unit test for this, and check if the snapshot is overwritten.

@mmsmits
Copy link
Member

mmsmits commented Apr 3, 2024

@Rob5045 Thanks for the pointer, otherwise we never would have found this :)

@mmsmits mmsmits reopened this May 1, 2024
@mmsmits
Copy link
Member

mmsmits commented May 1, 2024

This fix looks correct, and we still think it is.

However, a validator unit test crashes because of this. Because it results in different behavior of SnapshotSource, and we should take a look at the ensureSnapshot() function, to see if we need to copy this fix there too.

We should also check in the validator-api, why we don't crash on empty shapshots.

All reasons the revert this fix for and thoroughly investigate what's going on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In Progress
Development

Successfully merging a pull request may close this issue.

4 participants