Skip to content

Fix versioned canonical profile resolution #5503

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

Open
rohithkg-code wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix versioned canonical profile resolution #5503

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
35 changes: 34 additions & 1 deletion ...lth.Fhir.Shared.Core.UnitTests/Features/Validation/ServerProvideProfileValidationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,38 @@ public async Task GivenStructureDefinitionsExist_WhenGettingSupportedProfiles_Th
Assert.Contains("http://example.org/fhir/StructureDefinition/custom-patient", profiles);
}

[Fact]
Copy link
Copy Markdown
Contributor

@feordin feordin Apr 29, 2026

Choose a reason for hiding this comment

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

More possible test cases:
Version mismatch returns null — call ResolveByCanonicalUriAsync("url|9.9.9") when only url|3.0.0 exists; assert the result is null. This ensures the fallback doesn't match on URL alone.

Unversioned URI still resolves — regression guard that an unversioned canonical URI resolves correctly after the restructured code path in ResolveByCanonicalUriAsync.

Two versions of the same SD, each resolves correctly — a test with both v2.0.0 and v3.0.0 of the same SD on the server, verifying each versioned URI resolves to the right resource. This will also expose the deduplication/cache bugs noted above.

public async Task GivenVersionedStructureDefinition_WhenGettingSupportedProfiles_ThenVersionedCanonicalIsReturned()
{
// Arrange
var patientProfile = CreateStructureDefinition("http://example.org/fhir/StructureDefinition/custom-patient", "Patient", "3.0.0");
SetupSearchServiceWithResults("StructureDefinition", patientProfile);

// Act
var profiles = await _serverProvideProfileValidation.GetSupportedProfilesAsync("Patient", CancellationToken.None);

// Assert
Assert.Single(profiles);
Assert.Contains("http://example.org/fhir/StructureDefinition/custom-patient|3.0.0", profiles);
}

[Fact]
public async Task GivenVersionedStructureDefinition_WhenResolvingByCanonicalUriWithVersion_ThenMatchingProfileIsReturned()
{
// Arrange
var patientProfile = CreateStructureDefinition("http://example.org/fhir/StructureDefinition/custom-patient", "Patient", "3.0.0");
SetupSearchServiceWithResults("StructureDefinition", patientProfile);

// Act
var profile = await _serverProvideProfileValidation.ResolveByCanonicalUriAsync("http://example.org/fhir/StructureDefinition/custom-patient|3.0.0");

// Assert
Assert.NotNull(profile);
var structureDefinition = Assert.IsType<StructureDefinition>(profile);
Assert.Equal("http://example.org/fhir/StructureDefinition/custom-patient", structureDefinition.Url);
Assert.Equal("3.0.0", structureDefinition.Version);
}

[Fact]
public async Task GivenANewStructureDefinition_WhenBackgroundLoopRuns_ThenSyncIsRequested()
{
Expand Down Expand Up @@ -353,12 +385,13 @@ public void Dispose()
_serverProvideProfileValidation?.Dispose();
}

private static StructureDefinition CreateStructureDefinition(string url, string type)
private static StructureDefinition CreateStructureDefinition(string url, string type, string version = null)
{
return new StructureDefinition
{
Id = Guid.NewGuid().ToString("N").Substring(0, 16), // Generate valid FHIR ID
Url = url,
Version = version,
Name = $"{type}Profile",
Status = PublicationStatus.Active,
Kind = StructureDefinition.StructureDefinitionKind.Resource,
Expand Down
37 changes: 36 additions & 1 deletion src/Microsoft.Health.Fhir.Shared.Core/Features/Validation/ServerProvideProfileValidation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,18 @@ public void Dispose()

public async Task<Resource> ResolveByCanonicalUriAsync(string uri)
{
var summary = (await ListSummariesAsync(CancellationToken.None)).ResolveByCanonicalUri(uri);
var summaries = (await ListSummariesAsync(CancellationToken.None)).ToList();
Copy link
Copy Markdown
Contributor

@feordin feordin Apr 29, 2026

Choose a reason for hiding this comment

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

Thanks for the contribution!

I have a question on this method:
It appears that this fallback only works correctly if at most one version of each StructureDefinition exists on the server.

_summaries is built in GetSummariesAsync using result[artifact.ResourceUri] = artifact — keyed on the base canonical URL with no version. When a server holds both v2.0.0 and v3.0.0 of the same SD, only the last one processed survives in _summaries, so url|2.0.0 will silently fail to resolve if v3.0.0 was stored last.

I think to fully support multi-version scenarios, GetSummariesAsync needs to key the dictionary by url + version (e.g. $"{artifact.ResourceUri}|{ver}"), and LoadBySummary's cache key needs the same treatment (see separate comment).

var summary = summaries.ResolveByCanonicalUri(uri);

if (summary == null &&
TrySplitVersionedCanonicalUri(uri, out string canonicalUri, out string version))
{
summary = summaries.FirstOrDefault(x =>
string.Equals(x.ResourceUri, canonicalUri, StringComparison.OrdinalIgnoreCase) &&
x.TryGetValue(_structureDefinitionVersionKey, out object summaryVersion) &&
string.Equals(summaryVersion?.ToString(), version, StringComparison.OrdinalIgnoreCase));
}

return LoadBySummary(summary);
Copy link
Copy Markdown
Contributor

@feordin feordin Apr 29, 2026

Choose a reason for hiding this comment

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

I think, if we fix the first comment I made, we will have a new issue:
The _resourcesByUri cache key doesn't include version, so it will return the wrong resource once multi-version support is added.

Steps to reproduce (after fixing the deduplication bug above):

ResolveByCanonicalUriAsync("url|v2") → loads v2 from JSON → stores in cache under key url
ResolveByCanonicalUriAsync("url|v3") → cache hit on url → returns v2 incorrectly

}

Expand Down Expand Up @@ -175,6 +186,30 @@ private static string GetCanonicalUrl(ArtifactSummary artifact)
return url;
}

private static bool TrySplitVersionedCanonicalUri(string uri, out string canonicalUri, out string version)
{
canonicalUri = uri;
version = null;

if (string.IsNullOrWhiteSpace(uri))
{
return false;
}

int fragmentIndex = uri.IndexOf('#');
string uriWithoutFragment = fragmentIndex >= 0 ? uri.Substring(0, fragmentIndex) : uri;
int versionSeparatorIndex = uriWithoutFragment.LastIndexOf('|');

if (versionSeparatorIndex <= 0 || versionSeparatorIndex == uriWithoutFragment.Length - 1)
{
return false;
}

canonicalUri = uriWithoutFragment.Substring(0, versionSeparatorIndex);
version = uriWithoutFragment.Substring(versionSeparatorIndex + 1);
return true;
}

private static string GetHashForSupportedProfiles(IReadOnlyCollection<ArtifactSummary> summaries)
{
if (summaries == null)
Expand Down
Loading