Skip to content

Remove all non-inheritable extensions from the base profile before snapshotting #2886

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.

Sign up for GitHub

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

Merged
mmsmits merged 14 commits into from
Oct 23, 2024
Merged

Remove all non-inheritable extensions from the base profile before snapshotting #2886

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
6d5ad14
Remove all non-inheritable extensions from the base profile before sn…
mmsmits Oct 8, 2024
f22faf9
Merge branch 'develop' into bugfix/2799-test-for-ultra-nested-extensions
mmsmits Oct 8, 2024
6da124f
Merge branch 'develop' into feature/1814-remove-all-non-inheritable-e…
mmsmits Oct 21, 2024
71bfd26
moved SnapshotGeneration extensions to Base
mmsmits Oct 21, 2024
33f2e70
Merge branch 'develop' into feature/1814-remove-all-non-inheritable-e…
mmsmits Oct 21, 2024
7244bd9
removed cref
mmsmits Oct 21, 2024
604abd7
Merge remote-tracking branch 'origin/feature/1814-remove-all-non-inhe…
mmsmits Oct 21, 2024
3eee269
added new suppression files
mmsmits Oct 21, 2024
806545e
Also for Conformance
mmsmits Oct 21, 2024
cbcd50e
marked `RemoveAllConstrainedByDiffExtensions` as obsolete
mmsmits Oct 21, 2024
016e7b0
Update src/Hl7.Fhir.Base/Specification/Snapshot/SnapshotGeneratorExte…
mmsmits Oct 22, 2024
40f323b
Update src/Hl7.Fhir.Base/Specification/Snapshot/SnapshotGeneratorExte…
mmsmits Oct 22, 2024
3d2a6ed
Merge branch 'develop' into feature/1814-remove-all-non-inheritable-e…
mmsmits Oct 22, 2024
16c9577
Merge branch 'develop' into feature/1814-remove-all-non-inheritable-e…
ewoutkramer Oct 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 45 additions & 2 deletions ...n/Snapshot/SnapshotGeneratorExtensions.cs → ...n/Snapshot/SnapshotGeneratorExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
*/

using Hl7.Fhir.Model;
using Hl7.Fhir.Support;
using Hl7.Fhir.Rest;
using Hl7.Fhir.Utility;
using System;
using System.Collections.Generic;
Expand All @@ -19,7 +19,7 @@ namespace Hl7.Fhir.Specification.Snapshot
// This extension indicates snapshot elements with associated differential constraints in the profile.
// Note: extensions are persisted to XML/JSON, whereas annotations are ephemeral (in-memory only)

/// <summary>Helper methods for the <see cref="SnapshotGenerator"/> class to generate and inspect custom extensions.</summary>
/// <summary>Helper methods for the SnapshotGenerator class to generate and inspect custom extensions.</summary>
public static class SnapshotGeneratorExtensions
{
/// <summary>The canonical url of the extension definition that marks snapshot elements with associated differential constraints.</summary>
Expand Down Expand Up @@ -54,6 +54,7 @@ public static void RemoveConstrainedByDiffExtension(this IExtendable element)
}

/// <summary>Recursively removes all instances of the <see cref="CONSTRAINED_BY_DIFF_EXT"/> extension from the specified element definition and all it's child objects.</summary>
[Obsolete("Use RemoveAllNonInheritableExtensions(this Element element) instead.")]
public static void RemoveAllConstrainedByDiffExtensions(this Element element)
{
if (element == null) { throw Error.ArgumentNull(nameof(element)); }
Expand All @@ -65,6 +66,7 @@ public static void RemoveAllConstrainedByDiffExtensions(this Element element)
}

/// <summary>Recursively removes all instances of the <see cref="CONSTRAINED_BY_DIFF_EXT"/> extension from all the elements and their respective child objects.</summary>
[Obsolete("Use RemoveAllNonInheritableExtensions(this IEnumerable<T> elements) instead.")]
public static void RemoveAllConstrainedByDiffExtensions<T>(this IEnumerable<T> elements) where T : Element
{
if (elements == null) { throw Error.ArgumentNull(nameof(elements)); }
Expand All @@ -74,6 +76,47 @@ public static void RemoveAllConstrainedByDiffExtensions<T>(this IEnumerable<T> e
}
}


internal static void RemoveAllNonInheritableExtensions(this Element element)
{
if (element == null) { throw Error.ArgumentNull(nameof(element)); }
element.RemoveNonInheritableExtensions();
foreach (var child in element.Children.OfType<Element>())
{
child.RemoveAllNonInheritableExtensions();
}
}

internal static void RemoveNonInheritableExtensions(this IExtendable element)
{
if (element == null) { throw Error.ArgumentNull(nameof(element)); }
foreach (var ext in _nonInheritableExtensions)
{
element.RemoveExtension(ext);
}
}

private static readonly List<string> _nonInheritableExtensions = [
ResourceIdentity.CORE_BASE_URL + "elementdefinition-isCommonBinding",
ResourceIdentity.CORE_BASE_URL + "structuredefinition-fmm",
ResourceIdentity.CORE_BASE_URL + "structuredefinition-fmm-no-warnings",
ResourceIdentity.CORE_BASE_URL + "structuredefinition-hierarchy",
ResourceIdentity.CORE_BASE_URL + "structuredefinition-interface",
ResourceIdentity.CORE_BASE_URL + "structuredefinition-normative-version",
ResourceIdentity.CORE_BASE_URL + "structuredefinition-applicable-version",
ResourceIdentity.CORE_BASE_URL + "structuredefinition-category",
ResourceIdentity.CORE_BASE_URL + "structuredefinition-codegen-super",
ResourceIdentity.CORE_BASE_URL + "structuredefinition-security-category",
ResourceIdentity.CORE_BASE_URL + "structuredefinition-standards-status",
ResourceIdentity.CORE_BASE_URL + "structuredefinition-summary",
ResourceIdentity.CORE_BASE_URL + "structuredefinition-wg",
ResourceIdentity.CORE_BASE_URL + "replaces",
ResourceIdentity.CORE_BASE_URL + "resource-approvalDate",
ResourceIdentity.CORE_BASE_URL + "resource-effectivePeriod",
ResourceIdentity.CORE_BASE_URL + "resource-lastReviewDate",
CONSTRAINED_BY_DIFF_EXT //this is our own extension to define differences compared to the base, this can't be inherited from the base profile
];

// ========== For internal use only ==========
// [WMR 20170209] OBSOLETE
#if false
Expand Down
18 changes: 18 additions & 0 deletions src/Hl7.Fhir.Conformance/CompatibilitySuppressions.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<?xml version="1.0" encoding="utf-8"?>
<!-- https://learn.microsoft.com/en-us/dotnet/fundamentals/package-validation/diagnostic-ids -->
<Suppressions xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema">
<Suppression>
<DiagnosticId>CP0001</DiagnosticId>
<Target>T:Hl7.Fhir.Specification.Snapshot.SnapshotGeneratorExtensions</Target>
<Left>lib/net8.0/Hl7.Fhir.Conformance.dll</Left>
<Right>lib/net8.0/Hl7.Fhir.Conformance.dll</Right>
<IsBaselineSuppression>true</IsBaselineSuppression>
</Suppression>
<Suppression>
<DiagnosticId>CP0001</DiagnosticId>
<Target>T:Hl7.Fhir.Specification.Snapshot.SnapshotGeneratorExtensions</Target>
<Left>lib/netstandard2.0/Hl7.Fhir.Conformance.dll</Left>
<Right>lib/netstandard2.0/Hl7.Fhir.Conformance.dll</Right>
<IsBaselineSuppression>true</IsBaselineSuppression>
</Suppression>
</Suppressions>
4 changes: 2 additions & 2 deletions src/Hl7.Fhir.Conformance/Specification/Snapshot/SnapshotGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,7 +465,7 @@ private async Tasks.Task<List<ElementDefinition>> generate(StructureDefinition s

// [WMR 20170208] Moved to *AFTER* ensureBaseComponents - emits annotations...
// [WMR 20160915] Derived profiles should never inherit the ChangedByDiff extension from the base structure
snapshot.Element.RemoveAllConstrainedByDiffExtensions();
snapshot.RemoveAllNonInheritableExtensions();
snapshot.Element.RemoveAllConstrainedByDiffAnnotations();

// Notify observers
Expand Down Expand Up @@ -1487,7 +1487,7 @@ private static bool copyChildren(ElementDefinitionNavigator nav, ElementDefiniti
var elem = elems[pos];

// [WMR 20160826] Never inherit Changed extension from base profile!
elem.RemoveAllConstrainedByDiffExtensions();
elem.RemoveAllNonInheritableExtensions();
elem.RemoveAllConstrainedByDiffAnnotations();

// [WMR 20160902] Initialize empty ElementDefinition.Base components if necessary
Expand Down
18 changes: 18 additions & 0 deletions src/Hl7.Fhir.STU3/CompatibilitySuppressions.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<?xml version="1.0" encoding="utf-8"?>
<!-- https://learn.microsoft.com/en-us/dotnet/fundamentals/package-validation/diagnostic-ids -->
<Suppressions xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema">
<Suppression>
<DiagnosticId>CP0001</DiagnosticId>
<Target>T:Hl7.Fhir.Specification.Snapshot.SnapshotGeneratorExtensions</Target>
<Left>lib/net8.0/Hl7.Fhir.STU3.dll</Left>
<Right>lib/net8.0/Hl7.Fhir.STU3.dll</Right>
<IsBaselineSuppression>true</IsBaselineSuppression>
</Suppression>
<Suppression>
<DiagnosticId>CP0001</DiagnosticId>
<Target>T:Hl7.Fhir.Specification.Snapshot.SnapshotGeneratorExtensions</Target>
<Left>lib/netstandard2.0/Hl7.Fhir.STU3.dll</Left>
<Right>lib/netstandard2.0/Hl7.Fhir.STU3.dll</Right>
<IsBaselineSuppression>true</IsBaselineSuppression>
</Suppression>
</Suppressions>
5 changes: 3 additions & 2 deletions src/Hl7.Fhir.STU3/Specification/Snapshot/SnapshotGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,8 @@ private async Tasks.Task<List<ElementDefinition>> generate(StructureDefinition s

// [WMR 20170208] Moved to *AFTER* ensureBaseComponents - emits annotations...
// [WMR 20160915] Derived profiles should never inherit the ChangedByDiff extension from the base structure
snapshot.Element.RemoveAllConstrainedByDiffExtensions();
// Also remove core extensions that are not supposed to be inherited by derived profiles
snapshot.RemoveAllNonInheritableExtensions();
snapshot.Element.RemoveAllConstrainedByDiffAnnotations();

// Notify observers
Expand Down Expand Up @@ -1309,7 +1310,7 @@ private static bool copyChildren(ElementDefinitionNavigator nav, ElementDefiniti
var elem = elems[pos];

// [WMR 20160826] Never inherit Changed extension from base profile!
elem.RemoveAllConstrainedByDiffExtensions();
elem.RemoveAllNonInheritableExtensions();
elem.RemoveAllConstrainedByDiffAnnotations();

// [WMR 20160902] Initialize empty ElementDefinition.Base components if necessary
Expand Down
118 changes: 0 additions & 118 deletions src/Hl7.Fhir.STU3/Specification/Snapshot/SnapshotGeneratorExtensions.cs
View file
Open in desktop

This file was deleted.

6 changes: 3 additions & 3 deletions src/Hl7.Fhir.Specification.STU3.Tests/Snapshot/SnapshotGeneratorTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@
using Hl7.Fhir.Support;
using Hl7.Fhir.Utility;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using System;
using NSubstitute;
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
Expand Down Expand Up @@ -2433,8 +2433,8 @@ private static bool isAlmostExactly(ElementDefinition elem, ElementDefinition ba
}

// Also ignore any Changed extensions on base and diff
elemClone.RemoveAllConstrainedByDiffExtensions();
baseClone.RemoveAllConstrainedByDiffExtensions();
elemClone.RemoveAllNonInheritableExtensions();

@ewoutkramer ewoutkramer Oct 14, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this method replace the original ones? So RemoveallNonInheritableExtension replaces RemoveAllConstrainedByDiffExtensions? If so, should we not remove/obsolete the original method?

@mmsmits mmsmits Oct 21, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Well, I thought about that, but didn't dare to since Forge might be using the ConstraintByDiff extension one maybe?
And that is a bit faster (I don't know if it might be noticeable in a UI).
@Rob5045 any feedback here?

@Rob5045 Rob5045 Oct 21, 2024

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.

Forge does not use the RemoveAllConstrainedByDiffExtensions extension.

@mmsmits mmsmits Oct 21, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks, I'll Obsolete is then

@Rob5045 Rob5045 Oct 21, 2024 •
edited
Loading

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.

I'm working on supporting extensions on element definition properties in Forge. That would mean that the UI won't be cluttered with these I guess? That would be nice.
image

@Rob5045 Rob5045 Oct 21, 2024 •
edited
Loading

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.

How about http://hl7.org/fhir/StructureDefinition/elementdefinition-bindingName? I see that one all the time as well.

@mmsmits mmsmits Oct 22, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is not on the list:
We only made this fix for category 5 of this list:
https://jira.hl7.org/browse/FHIR-28441?focusedCommentId=177188&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-177188

baseClone.RemoveAllNonInheritableExtensions();
elemClone.RemoveAllConstrainedByDiffAnnotations();
baseClone.RemoveAllConstrainedByDiffAnnotations();

Expand Down
35 changes: 33 additions & 2 deletions src/Hl7.Fhir.Specification.Shared.Tests/Snapshot/SnapshotGeneratorTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2562,8 +2562,8 @@ private static bool isAlmostExactly(ElementDefinition elem, ElementDefinition ba
}

// Also ignore any Changed extensions on base and diff
elemClone.RemoveAllConstrainedByDiffExtensions();
baseClone.RemoveAllConstrainedByDiffExtensions();
elemClone.RemoveAllNonInheritableExtensions();
baseClone.RemoveAllNonInheritableExtensions();
elemClone.RemoveAllConstrainedByDiffAnnotations();
baseClone.RemoveAllConstrainedByDiffAnnotations();

Expand Down Expand Up @@ -10155,6 +10155,37 @@ private StructureDefinition createVeryNestedExtension()
}


[TestMethod]
public void RemoveNonInhertitableSnapshotsTest()
{
var profile = new StructureDefinition
{
Url = "http://fire.ly/StructureDefinition/example",
Snapshot = new()
{
Element = new() {
new("Example")
{
Binding = new()
{
Strength = BindingStrength.Required,
ValueSet = "http://fire.ly/ValueSet/example"
}
},
}
}
};

profile.Snapshot.Element[0].AddExtension(ResourceIdentity.CORE_BASE_URL + "structuredefinition-hierarchy", new FhirString("foo"));
profile.Snapshot.Element[0].Binding.AddExtension(ResourceIdentity.CORE_BASE_URL + "elementdefinition-isCommonBinding", new FhirBoolean(true));

profile.Snapshot.RemoveAllNonInheritableExtensions();

profile.Snapshot.Element[0].Extension.Should().BeEmpty();
profile.Snapshot.Element[0].Binding.Extension.Should().BeEmpty();
}


[TestMethod]
public async Tasks.Task TestNewR5Elements()
{
Expand Down