Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion Directory.Packages.props
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
<Hl7FhirLegacyVersion>5.11.0</Hl7FhirLegacyVersion>
<DotNetSdkPackageVersion>10.0.10</DotNetSdkPackageVersion>
<OpenIddictPackageVersion>6.2.0</OpenIddictPackageVersion>
<IgnixaPackageVersion>0.0.163</IgnixaPackageVersion>
<IgnixaPackageVersion>0.6.74</IgnixaPackageVersion>
</PropertyGroup>
<!-- SDK Packages -->
<Choose>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,10 @@ public ImportResource Parse(long index, long offset, int length, string rawResou
ImportResourceIdValidator.Validate(resource.Id);
CheckConditionalReferenceInResource(resource, importMode);

resource.Meta ??= new MetaJsonNode();

var lastUpdatedIsNull = importMode == ImportMode.InitialLoad || resource.Meta.LastUpdated == null;
var lastUpdated = lastUpdatedIsNull ? Clock.UtcNow : resource.Meta.LastUpdated.Value;
resource.Meta.LastUpdated = new DateTimeOffset(lastUpdated.DateTime.TruncateToMillisecond(), lastUpdated.Offset);
if (!lastUpdatedIsNull && resource.Meta.LastUpdated.Value > Clock.UtcNow.AddSeconds(10)) // 10 sec is the max for the computers in the domain
var lastUpdatedIsNull = importMode == ImportMode.InitialLoad || resource.Meta.LastUpdatedOffset == null;
var lastUpdated = lastUpdatedIsNull ? Clock.UtcNow : resource.Meta.LastUpdatedOffset.Value;
resource.Meta.LastUpdatedOffset = new DateTimeOffset(lastUpdated.DateTime.TruncateToMillisecond(), lastUpdated.Offset);
if (!lastUpdatedIsNull && resource.Meta.LastUpdatedOffset.Value > Clock.UtcNow.AddSeconds(10)) // 10 sec is the max for the computers in the domain
{
throw new NotSupportedException("LastUpdated in the resource cannot be in the future.");
}
Expand Down Expand Up @@ -134,7 +132,7 @@ public ImportResource Parse(long index, long offset, int length, string rawResou
/// </remarks>
private void CheckConditionalReferenceInResource(ResourceJsonNode resource, ImportMode importMode)
{
if (importMode == ImportMode.IncrementalLoad || resource.MutableNode is not JsonObject root)
if (importMode == ImportMode.IncrementalLoad || resource.ToSourceNavigator().Meta<JsonNode>() is not JsonObject root)
{
return;
}
Expand All @@ -156,7 +154,7 @@ private void CheckConditionalReferenceInResource(ResourceJsonNode resource, Impo
{
foreach (var item in array)
{
ThrowIfConditionalReference(item, resource.FhirVersion);
ThrowIfConditionalReference(item);
}
}
else
Expand All @@ -166,23 +164,21 @@ private void CheckConditionalReferenceInResource(ResourceJsonNode resource, Impo
// Match that leniency here (field.IsCollection but value isn't a JsonArray) instead of
// rejecting it - ThrowIfConditionalReference still throws below if this value isn't even
// a JSON object.
ThrowIfConditionalReference(value, resource.FhirVersion);
ThrowIfConditionalReference(value);
}
}
}

/// <summary>
/// Reads the reference field through the typed <see cref="ReferenceJsonNode"/> model instead of casting
/// through raw <see cref="JsonValue"/>. A missing "reference" property (e.g. an identifier-only or
/// display-only reference, both valid FHIR) yields a null <see cref="ReferenceJsonNode.Reference"/> and
/// is skipped, matching the Firely parser. A non-string "reference" scalar (e.g. <c>"reference": 123</c>)
/// is deliberately not guarded against - <see cref="ReferenceJsonNode.Reference"/> throws in that case.
/// Reads the reference field from the raw JSON object. A missing "reference" property (e.g. an
/// identifier-only or display-only reference, both valid FHIR) is skipped, matching the Firely parser.
/// A non-string "reference" scalar (e.g. <c>"reference": 123</c>) deliberately throws.
/// A reference field that is present but isn't a JSON object at all (schema-invalid, e.g. a bare string
/// or number) also throws here rather than being silently skipped - confirmed empirically that
/// <c>resource.ToElement(schema)</c> does NOT reject this shape on its own, so this is the only place
/// that catches it. A null array item (e.g. <c>[null, {...}]</c>) is treated as absent, not malformed.
/// </summary>
private static void ThrowIfConditionalReference(JsonNode referenceNode, FhirVersion? fhirVersion)
private static void ThrowIfConditionalReference(JsonNode referenceNode)
{
if (referenceNode is null)
{
Expand All @@ -194,7 +190,7 @@ private static void ThrowIfConditionalReference(JsonNode referenceNode, FhirVers
throw new FormatException($"Expected a Reference object but found {referenceNode.GetValueKind()}.");
}

var reference = new ReferenceJsonNode(referenceObject, fhirVersion).Reference;
var reference = referenceObject["reference"]?.GetValue<string>();
if (!string.IsNullOrWhiteSpace(reference) && reference.Contains('?', StringComparison.Ordinal))
{
throw new NotSupportedException($"Conditional reference is not supported for $import in {ImportMode.InitialLoad}.");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@
// -------------------------------------------------------------------------------------------------

using System;
using System.Linq;
using System.Reflection;
using Hl7.Fhir.ElementModel;
using Hl7.Fhir.Model;
using Hl7.Fhir.Specification.Source;
using Hl7.Fhir.Validation;
using Microsoft.Extensions.Logging;
Expand Down Expand Up @@ -75,4 +78,79 @@ public void GivenStu3OrR5FhirVersion_WhenCreatingValidator_ThenCid0ConstraintIsN
// Asser
Assert.DoesNotContain("cid-0", internalValidator.Settings.ConstraintsToIgnore ?? []);
}

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.

Can we add a test that covers the issue the customer ran into? A bundle being validated with resource entries that contained references shouldn't report validation errors.

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.

I believe it was this { "request": { "method": "DELETE", "url": "Observation/no-such-id" } } hitting the unguarded entry.Children("resource").First().ToScopedNode() on https://github.com/FirelyTeam/firely-net-sdk/pull/3099/changes#diff-872a636c2e24971aa1d68c2aac5ba903d2b34b3f82f738ce74e534bbd8a86ec5R238-R239

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.

I'll add the breaking payload to the bundle e2e tests too

[Fact]
public void GivenABundleWithAnEntryThatHasNoResource_WhenValidating_ThenNoInternalLogicFailureIsReported()
{
// Arrange
var validator = new ProfileValidator(_profilesResolver, _options, _logger, ModelInfoProvider.Instance);
Bundle bundle = CreateTransactionBundleWithAResourcelessEntry();

// Act
OperationOutcomeIssue[] issues = validator.TryValidate(bundle.ToTypedElement());

// Assert - ordinary validation issues are expected and fine, an internal failure is not.
Assert.DoesNotContain(issues, IsCatastrophicFailure);
}

private static bool IsCatastrophicFailure(OperationOutcomeIssue issue)
{
if (issue.DetailsText?.Contains("Internal logic failure", StringComparison.OrdinalIgnoreCase) == true)
{
return true;
}

return string.Equals(issue.Severity, "Fatal", StringComparison.OrdinalIgnoreCase)
&& issue.DetailsCodes?.Coding.Any(coding => string.Equals(coding.Code, "5003", StringComparison.Ordinal)) == true;
}

private static Bundle CreateTransactionBundleWithAResourcelessEntry()
{
const string organizationFullUrl = "urn:uuid:6e2b8f22-6f2c-4a5f-9bd2-0f5f8c0a0001";

var bundle = new Bundle
{
Type = Bundle.BundleType.Transaction,
};

bundle.Entry.Add(new Bundle.EntryComponent
{
FullUrl = "urn:uuid:6e2b8f22-6f2c-4a5f-9bd2-0f5f8c0a0002",
Resource = new Patient
{
ManagingOrganization = new ResourceReference(organizationFullUrl),
},
Request = new Bundle.RequestComponent
{
Method = Bundle.HTTPVerb.POST,
Url = "Patient",
},
});

bundle.Entry.Add(new Bundle.EntryComponent
{
FullUrl = organizationFullUrl,
Resource = new Organization
{
Name = "Contoso Health",
},
Request = new Bundle.RequestComponent
{
Method = Bundle.HTTPVerb.POST,
Url = "Organization",
},
});

// The trigger: a transaction DELETE entry has a request but no resource.
bundle.Entry.Add(new Bundle.EntryComponent
{
Request = new Bundle.RequestComponent
{
Method = Bundle.HTTPVerb.DELETE,
Url = "Patient/does-not-exist",
},
});

return bundle;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@
<None Remove="TestFiles\Normative\Bundle-BatchWithDuplicatedItems.json" />
<None Remove="TestFiles\R5\Bundle-TransactionWithMetaHistory.json" />
<None Remove="TestFiles\Normative\Bundle-TransactionWithMetaHistory.json" />
<None Remove="TestFiles\Normative\Bundle-TransactionWithResourcelessEntry.json" />
</ItemGroup>
<ItemGroup>
<EmbeddedResource Include="TestFiles\Normative\Bundle-InvalidBundleType.json" />
Expand All @@ -174,6 +175,7 @@
<EmbeddedResource Include="TestFiles\Normative\Bundle-BatchWithDuplicatedItems.json" />
<EmbeddedResource Include="TestFiles\Normative\Bundle-BatchWithConditionalUpdateByIdentifier.json" />
<EmbeddedResource Include="TestFiles\Normative\Bundle-TransactionForRollBackWithDelete.json" />
<EmbeddedResource Include="TestFiles\Normative\Bundle-TransactionWithResourcelessEntry.json" />
<EmbeddedResource Include="TestFiles\Normative\capabilitystatement-example.json" />
<EmbeddedResource Include="TestFiles\Normative\codesystem-abstract-types.json" />
<EmbeddedResource Include="TestFiles\Normative\Flag.json" />
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
{
"resourceType": "Bundle",
"type": "transaction",
"entry": [
{
"fullUrl": "urn:uuid:6e2b8f22-6f2c-4a5f-9bd2-0f5f8c0a0002",
"resource": {
"resourceType": "Patient",
"name": [
{
"use": "official",
"family": "Foo",
"given": [ "Bar" ]
}
],
"gender": "male",
"birthDate": "2005-02-05",
"managingOrganization": {
"reference": "urn:uuid:6e2b8f22-6f2c-4a5f-9bd2-0f5f8c0a0001"
}
},
"request": {
"method": "POST",
"url": "Patient"
}
},
{
"fullUrl": "urn:uuid:6e2b8f22-6f2c-4a5f-9bd2-0f5f8c0a0001",
"resource": {
"resourceType": "Organization",
"name": "Contoso Health"
},
"request": {
"method": "POST",
"url": "Organization"
}
},
{
"request": {
"method": "DELETE",
"url": "Patient/does-not-exist"
}
}
]
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,28 @@ public async Task GivenABundle_WhenAnInvalidBundleTypeIsUsed_ThenHttp400IsReturn
Assert.True(fhirException.Message.Contains("The provided Bundle type is not supported.", StringComparison.OrdinalIgnoreCase));
}

[Fact]
[HttpIntegrationFixtureArgumentSets(DataStore.All, Format.Json)]
public async Task GivenABundleWithAnEntryThatHasNoResource_WhenValidated_ThenNoInternalLogicFailureIsReturned()
{
// The payload is posted as a raw JSON string, so this test is JSON-only - the Xml argument set would
// set an Xml content type and fail to parse it.
// A transaction bundle that mixes entries carrying a resource with a DELETE entry that carries only a
// request. Resolving the reference in the first entry forces the Firely bundle cache to build, which
// walks every entry looking for a "resource" child - including the resource-less one.
var bundleAsString = Samples.GetJson("Bundle-TransactionWithResourcelessEntry");

OperationOutcome outcome = await _client.ValidateAsync("Bundle/$validate", bundleAsString);

// Ordinary validation issues are expected and fine. $validate returns HTTP 200 with the outcome in the
// body, so a catastrophic internal failure here would otherwise be invisible to status-code telemetry.
Assert.DoesNotContain(
outcome.Issue,
issue => issue.Details?.Text?.Contains("Internal logic failure", StringComparison.OrdinalIgnoreCase) == true
|| (issue.Severity == OperationOutcome.IssueSeverity.Fatal
&& issue.Details?.Coding?.Any(coding => string.Equals(coding.Code, "5003", StringComparison.Ordinal)) == true));
}

[Fact]
[Trait(Traits.Priority, Priority.One)]
public async Task GivenABundleWithConditionalUpdateByReference_WhenExecutedWithMaximizedConditionalQueryParallelism_RunsTheQueryInParallel()
Expand Down
Loading