From e34f676f4a615fddf69a98de2be92c4d3675c35c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 16 Jul 2025 20:54:39 +0000 Subject: [PATCH 1/7] Initial plan From 8ec009948a49d219ef2f956838ee8b23515f325c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 16 Jul 2025 21:15:14 +0000 Subject: [PATCH 2/7] Add factory pattern for thread-safe serialization filters Co-authored-by: ewoutkramer <2276305+ewoutkramer@users.noreply.github.com> --- .../BaseFhirJsonPocoSerializer.cs | 5 +- .../FhirJsonPocoSerializerSettings.cs | 7 + .../Serialization/SerializationFilter.cs | 61 +++++++++ .../SummaryFilterThreadSafetyTests.cs | 129 ++++++++++++++++++ 4 files changed, 201 insertions(+), 1 deletion(-) create mode 100644 src/Hl7.Fhir.Support.Poco.Tests/NewPocoSerializers/SummaryFilterThreadSafetyTests.cs diff --git a/src/Hl7.Fhir.Base/Serialization/BaseFhirJsonPocoSerializer.cs b/src/Hl7.Fhir.Base/Serialization/BaseFhirJsonPocoSerializer.cs index 0d978d33d9..968888c5e7 100644 --- a/src/Hl7.Fhir.Base/Serialization/BaseFhirJsonPocoSerializer.cs +++ b/src/Hl7.Fhir.Base/Serialization/BaseFhirJsonPocoSerializer.cs @@ -89,7 +89,10 @@ private void serializeInternal( bool skipValue) { writer.WriteStartObject(); - var filter = Settings.SummaryFilter; + // Use factory if available, otherwise fall back to the static instance for backward compatibility +#pragma warning disable CS0618 // Type or member is obsolete + var filter = Settings.SummaryFilterFactory?.Invoke() ?? Settings.SummaryFilter; +#pragma warning restore CS0618 // Type or member is obsolete if (members is Resource r) writer.WriteString("resourceType", r.TypeName); diff --git a/src/Hl7.Fhir.Base/Serialization/FhirJsonPocoSerializerSettings.cs b/src/Hl7.Fhir.Base/Serialization/FhirJsonPocoSerializerSettings.cs index 5987f242c7..01e9d32d47 100644 --- a/src/Hl7.Fhir.Base/Serialization/FhirJsonPocoSerializerSettings.cs +++ b/src/Hl7.Fhir.Base/Serialization/FhirJsonPocoSerializerSettings.cs @@ -27,7 +27,14 @@ public record FhirJsonPocoSerializerSettings /// /// Specifies the filter to use for summary serialization. /// + [Obsolete("Use SummaryFilterFactory instead to ensure thread-safety when reusing JsonSerializerOptions instances. This property will be removed in a future version.")] public SerializationFilter? SummaryFilter { get; set; } = default; + + /// + /// Specifies a factory function that creates a new filter instance for each serialization operation. + /// This ensures thread-safety when reusing JsonSerializerOptions instances in concurrent scenarios. + /// + public Func? SummaryFilterFactory { get; set; } = default; } } diff --git a/src/Hl7.Fhir.Base/Serialization/SerializationFilter.cs b/src/Hl7.Fhir.Base/Serialization/SerializationFilter.cs index 5873a2787d..044239f1e0 100644 --- a/src/Hl7.Fhir.Base/Serialization/SerializationFilter.cs +++ b/src/Hl7.Fhir.Base/Serialization/SerializationFilter.cs @@ -9,6 +9,7 @@ #nullable enable using Hl7.Fhir.Introspection; +using System; namespace Hl7.Fhir.Serialization { @@ -41,11 +42,13 @@ public abstract class SerializationFilter /// /// Construct a new filter that conforms to the `_summary=true` summarized form. /// + [Obsolete("Use CreateSummaryFactory() instead to ensure thread-safety when reusing JsonSerializerOptions instances. This method will be removed in a future version.")] public static SerializationFilter ForSummary() => new BundleFilter(new ElementMetadataFilter() { IncludeInSummary = true }); /// /// Construct a new filter that conforms to the `_summary=text` summarized form. /// + [Obsolete("Use CreateTextFactory() instead to ensure thread-safety when reusing JsonSerializerOptions instances. This method will be removed in a future version.")] public static SerializationFilter ForText() => new BundleFilter(new TopLevelFilter( new ElementMetadataFilter() { @@ -53,6 +56,7 @@ public abstract class SerializationFilter IncludeMandatory = true })); + [Obsolete("Use CreateCountFactory() instead to ensure thread-safety when reusing JsonSerializerOptions instances. This method will be removed in a future version.")] public static SerializationFilter ForCount() => new BundleFilter(new TopLevelFilter( new ElementMetadataFilter() { @@ -63,6 +67,7 @@ public abstract class SerializationFilter /// /// Construct a new filter that conforms to the `_summary=data` summarized form. /// + [Obsolete("Use CreateDataFactory() instead to ensure thread-safety when reusing JsonSerializerOptions instances. This method will be removed in a future version.")] public static SerializationFilter ForData() => new BundleFilter(new TopLevelFilter( new ElementMetadataFilter() { @@ -73,12 +78,68 @@ public abstract class SerializationFilter /// /// Construct a new filter that conforms to the `_elements=...` summarized form. /// + [Obsolete("Use CreateElementsFactory() instead to ensure thread-safety when reusing JsonSerializerOptions instances. This method will be removed in a future version.")] public static SerializationFilter ForElements(string[] elements) => new BundleFilter(new TopLevelFilter( new ElementMetadataFilter() { IncludeNames = elements, IncludeMandatory = true })); + + /// + /// Create a factory function that produces new filter instances conforming to the `_summary=true` summarized form. + /// Using this factory ensures thread-safety when reusing JsonSerializerOptions instances. + /// + public static Func CreateSummaryFactory() => + () => new BundleFilter(new ElementMetadataFilter() { IncludeInSummary = true }); + + /// + /// Create a factory function that produces new filter instances conforming to the `_summary=text` summarized form. + /// Using this factory ensures thread-safety when reusing JsonSerializerOptions instances. + /// + public static Func CreateTextFactory() => + () => new BundleFilter(new TopLevelFilter( + new ElementMetadataFilter() + { + IncludeNames = new[] { "text", "id", "meta" }, + IncludeMandatory = true + })); + + /// + /// Create a factory function that produces new filter instances conforming to the `_summary=count` summarized form. + /// Using this factory ensures thread-safety when reusing JsonSerializerOptions instances. + /// + public static Func CreateCountFactory() => + () => new BundleFilter(new TopLevelFilter( + new ElementMetadataFilter() + { + IncludeMandatory = true, + IncludeNames = new[] { "id", "total", "link" } + })); + + /// + /// Create a factory function that produces new filter instances conforming to the `_summary=data` summarized form. + /// Using this factory ensures thread-safety when reusing JsonSerializerOptions instances. + /// + public static Func CreateDataFactory() => + () => new BundleFilter(new TopLevelFilter( + new ElementMetadataFilter() + { + IncludeNames = new[] { "text" }, + Invert = true + })); + + /// + /// Create a factory function that produces new filter instances conforming to the `_elements=...` summarized form. + /// Using this factory ensures thread-safety when reusing JsonSerializerOptions instances. + /// + public static Func CreateElementsFactory(string[] elements) => + () => new BundleFilter(new TopLevelFilter( + new ElementMetadataFilter() + { + IncludeNames = elements, + IncludeMandatory = true + })); } } diff --git a/src/Hl7.Fhir.Support.Poco.Tests/NewPocoSerializers/SummaryFilterThreadSafetyTests.cs b/src/Hl7.Fhir.Support.Poco.Tests/NewPocoSerializers/SummaryFilterThreadSafetyTests.cs new file mode 100644 index 0000000000..e7b72c2e83 --- /dev/null +++ b/src/Hl7.Fhir.Support.Poco.Tests/NewPocoSerializers/SummaryFilterThreadSafetyTests.cs @@ -0,0 +1,129 @@ +using FluentAssertions; +using Hl7.Fhir.Model; +using Hl7.Fhir.Serialization; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using System.Collections.Concurrent; +using System.Linq; +using System.Text.Json; +using System.Threading.Tasks; + +namespace Hl7.Fhir.Support.Poco.Tests +{ + [TestClass] + public class SummaryFilterThreadSafetyTests + { + [TestMethod] + public void ConcurrentSerializationWithFactory_ShouldBeThreadSafe() + { + // Arrange + var options = new JsonSerializerOptions() + .ForFhir(typeof(Patient).Assembly, new FhirJsonPocoSerializerSettings + { + SummaryFilterFactory = SerializationFilter.CreateElementsFactory(["id", "active"]) + }) + .Pretty(); + + var patient = new Patient + { + Id = "123", + Active = true, + Name = [new() { Family = "Doe", Given = ["John"] }], + MultipleBirth = new FhirBoolean(false), + }; + var bundle = new Bundle + { + Type = Bundle.BundleType.Collection, + Entry = [new() { Resource = patient }] + }; + + ConcurrentBag serialized = []; + + // Act + Parallel.For(0, 100, i => + { + serialized.Add(JsonSerializer.Serialize(bundle, options)); + }); + + // Assert + serialized.Count.Should().Be(100); + + // All results should include the entry field + var resultsWithEntry = serialized.Where(json => json.Contains("\"entry\"")).Count(); + resultsWithEntry.Should().Be(100, "all results should contain the entry field"); + + // No results should contain unfiltered fields + var resultsWithUnfilteredFields = serialized.Where(json => + json.Contains("\"name\"") || json.Contains("\"multipleBirthBoolean\"")).Count(); + resultsWithUnfilteredFields.Should().Be(0, "no results should contain unfiltered fields"); + + // All results should contain the filtered fields + var resultsWithId = serialized.Where(json => json.Contains("\"id\": \"123\"")).Count(); + var resultsWithActive = serialized.Where(json => json.Contains("\"active\": true")).Count(); + resultsWithId.Should().Be(100, "all results should contain the id field"); + resultsWithActive.Should().Be(100, "all results should contain the active field"); + } + + [TestMethod] + public void ConcurrentSerializationWithLegacyFilter_ShouldShowInconsistentResults() + { + // This test documents the issue with the legacy approach + // and demonstrates why the factory approach is needed + + // Arrange +#pragma warning disable CS0618 // Type or member is obsolete + var options = new JsonSerializerOptions() + .ForFhir(typeof(Patient).Assembly, new FhirJsonPocoSerializerSettings + { + SummaryFilter = SerializationFilter.ForElements(["id", "active"]) + }) + .Pretty(); +#pragma warning restore CS0618 // Type or member is obsolete + + var patient = new Patient + { + Id = "123", + Active = true, + Name = [new() { Family = "Doe", Given = ["John"] }], + MultipleBirth = new FhirBoolean(false), + }; + var bundle = new Bundle + { + Type = Bundle.BundleType.Collection, + Entry = [new() { Resource = patient }] + }; + + ConcurrentBag serialized = []; + + // Act + Parallel.For(0, 100, i => + { + serialized.Add(JsonSerializer.Serialize(bundle, options)); + }); + + // Assert + serialized.Count.Should().Be(100); + + // With legacy approach, many results will be missing the entry field due to race conditions + var resultsWithEntry = serialized.Where(json => json.Contains("\"entry\"")).Count(); + resultsWithEntry.Should().BeLessThan(100, "legacy approach should show inconsistent results due to race conditions"); + } + + [TestMethod] + public void AllFactoryMethods_ShouldCreateNewInstances() + { + // Verify that each factory method call creates a new instance + var summaryFactory = SerializationFilter.CreateSummaryFactory(); + var textFactory = SerializationFilter.CreateTextFactory(); + var countFactory = SerializationFilter.CreateCountFactory(); + var dataFactory = SerializationFilter.CreateDataFactory(); + var elementsFactory = SerializationFilter.CreateElementsFactory(["id", "name"]); + + // Each call should return a different instance + summaryFactory().Should().NotBeSameAs(summaryFactory()); + textFactory().Should().NotBeSameAs(textFactory()); + countFactory().Should().NotBeSameAs(countFactory()); + dataFactory().Should().NotBeSameAs(dataFactory()); + elementsFactory().Should().NotBeSameAs(elementsFactory()); + } + } +} \ No newline at end of file From c6c5a91c72104726cc7665c26e742c2e1940b731 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 16 Jul 2025 21:34:51 +0000 Subject: [PATCH 3/7] Complete thread-safe serialization filter implementation Co-authored-by: ewoutkramer <2276305+ewoutkramer@users.noreply.github.com> --- .../Serialization/SerializationFilter.cs | 86 ++++++++++++------- .../SummaryFilterThreadSafetyTests.cs | 62 ++----------- 2 files changed, 65 insertions(+), 83 deletions(-) diff --git a/src/Hl7.Fhir.Base/Serialization/SerializationFilter.cs b/src/Hl7.Fhir.Base/Serialization/SerializationFilter.cs index 044239f1e0..150d0c12fe 100644 --- a/src/Hl7.Fhir.Base/Serialization/SerializationFilter.cs +++ b/src/Hl7.Fhir.Base/Serialization/SerializationFilter.cs @@ -10,6 +10,7 @@ using Hl7.Fhir.Introspection; using System; +using System.Threading; namespace Hl7.Fhir.Serialization { @@ -90,56 +91,81 @@ public abstract class SerializationFilter /// Create a factory function that produces new filter instances conforming to the `_summary=true` summarized form. /// Using this factory ensures thread-safety when reusing JsonSerializerOptions instances. /// - public static Func CreateSummaryFactory() => - () => new BundleFilter(new ElementMetadataFilter() { IncludeInSummary = true }); + public static Func CreateSummaryFactory() + { + var threadLocalFilter = new ThreadLocal(() => + new BundleFilter(new ElementMetadataFilter() { IncludeInSummary = true })); + + return () => threadLocalFilter.Value!; + } /// /// Create a factory function that produces new filter instances conforming to the `_summary=text` summarized form. /// Using this factory ensures thread-safety when reusing JsonSerializerOptions instances. /// - public static Func CreateTextFactory() => - () => new BundleFilter(new TopLevelFilter( - new ElementMetadataFilter() - { - IncludeNames = new[] { "text", "id", "meta" }, - IncludeMandatory = true - })); + public static Func CreateTextFactory() + { + var threadLocalFilter = new ThreadLocal(() => + new BundleFilter(new TopLevelFilter( + new ElementMetadataFilter() + { + IncludeNames = new[] { "text", "id", "meta" }, + IncludeMandatory = true + }))); + + return () => threadLocalFilter.Value!; + } /// /// Create a factory function that produces new filter instances conforming to the `_summary=count` summarized form. /// Using this factory ensures thread-safety when reusing JsonSerializerOptions instances. /// - public static Func CreateCountFactory() => - () => new BundleFilter(new TopLevelFilter( - new ElementMetadataFilter() - { - IncludeMandatory = true, - IncludeNames = new[] { "id", "total", "link" } - })); + public static Func CreateCountFactory() + { + var threadLocalFilter = new ThreadLocal(() => + new BundleFilter(new TopLevelFilter( + new ElementMetadataFilter() + { + IncludeMandatory = true, + IncludeNames = new[] { "id", "total", "link" } + }))); + + return () => threadLocalFilter.Value!; + } /// /// Create a factory function that produces new filter instances conforming to the `_summary=data` summarized form. /// Using this factory ensures thread-safety when reusing JsonSerializerOptions instances. /// - public static Func CreateDataFactory() => - () => new BundleFilter(new TopLevelFilter( - new ElementMetadataFilter() - { - IncludeNames = new[] { "text" }, - Invert = true - })); + public static Func CreateDataFactory() + { + var threadLocalFilter = new ThreadLocal(() => + new BundleFilter(new TopLevelFilter( + new ElementMetadataFilter() + { + IncludeNames = new[] { "text" }, + Invert = true + }))); + + return () => threadLocalFilter.Value!; + } /// /// Create a factory function that produces new filter instances conforming to the `_elements=...` summarized form. /// Using this factory ensures thread-safety when reusing JsonSerializerOptions instances. /// - public static Func CreateElementsFactory(string[] elements) => - () => new BundleFilter(new TopLevelFilter( - new ElementMetadataFilter() - { - IncludeNames = elements, - IncludeMandatory = true - })); + public static Func CreateElementsFactory(string[] elements) + { + var threadLocalFilter = new ThreadLocal(() => + new BundleFilter(new TopLevelFilter( + new ElementMetadataFilter() + { + IncludeNames = elements, + IncludeMandatory = true + }))); + + return () => threadLocalFilter.Value!; + } } } diff --git a/src/Hl7.Fhir.Support.Poco.Tests/NewPocoSerializers/SummaryFilterThreadSafetyTests.cs b/src/Hl7.Fhir.Support.Poco.Tests/NewPocoSerializers/SummaryFilterThreadSafetyTests.cs index e7b72c2e83..7c8c3d5941 100644 --- a/src/Hl7.Fhir.Support.Poco.Tests/NewPocoSerializers/SummaryFilterThreadSafetyTests.cs +++ b/src/Hl7.Fhir.Support.Poco.Tests/NewPocoSerializers/SummaryFilterThreadSafetyTests.cs @@ -64,66 +64,22 @@ public void ConcurrentSerializationWithFactory_ShouldBeThreadSafe() } [TestMethod] - public void ConcurrentSerializationWithLegacyFilter_ShouldShowInconsistentResults() + public void AllFactoryMethods_ShouldUseSameInstancePerThread() { - // This test documents the issue with the legacy approach - // and demonstrates why the factory approach is needed - - // Arrange -#pragma warning disable CS0618 // Type or member is obsolete - var options = new JsonSerializerOptions() - .ForFhir(typeof(Patient).Assembly, new FhirJsonPocoSerializerSettings - { - SummaryFilter = SerializationFilter.ForElements(["id", "active"]) - }) - .Pretty(); -#pragma warning restore CS0618 // Type or member is obsolete - - var patient = new Patient - { - Id = "123", - Active = true, - Name = [new() { Family = "Doe", Given = ["John"] }], - MultipleBirth = new FhirBoolean(false), - }; - var bundle = new Bundle - { - Type = Bundle.BundleType.Collection, - Entry = [new() { Resource = patient }] - }; - - ConcurrentBag serialized = []; - - // Act - Parallel.For(0, 100, i => - { - serialized.Add(JsonSerializer.Serialize(bundle, options)); - }); - - // Assert - serialized.Count.Should().Be(100); - - // With legacy approach, many results will be missing the entry field due to race conditions - var resultsWithEntry = serialized.Where(json => json.Contains("\"entry\"")).Count(); - resultsWithEntry.Should().BeLessThan(100, "legacy approach should show inconsistent results due to race conditions"); - } - - [TestMethod] - public void AllFactoryMethods_ShouldCreateNewInstances() - { - // Verify that each factory method call creates a new instance + // Verify that each factory method returns the same instance per thread + // (this ensures state consistency within a serialization operation) var summaryFactory = SerializationFilter.CreateSummaryFactory(); var textFactory = SerializationFilter.CreateTextFactory(); var countFactory = SerializationFilter.CreateCountFactory(); var dataFactory = SerializationFilter.CreateDataFactory(); var elementsFactory = SerializationFilter.CreateElementsFactory(["id", "name"]); - // Each call should return a different instance - summaryFactory().Should().NotBeSameAs(summaryFactory()); - textFactory().Should().NotBeSameAs(textFactory()); - countFactory().Should().NotBeSameAs(countFactory()); - dataFactory().Should().NotBeSameAs(dataFactory()); - elementsFactory().Should().NotBeSameAs(elementsFactory()); + // Each call on the same thread should return the same instance + summaryFactory().Should().BeSameAs(summaryFactory()); + textFactory().Should().BeSameAs(textFactory()); + countFactory().Should().BeSameAs(countFactory()); + dataFactory().Should().BeSameAs(dataFactory()); + elementsFactory().Should().BeSameAs(elementsFactory()); } } } \ No newline at end of file From ca432f4c326d5b04883f5bd3b47d24dbf7b2e7db Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 22 Jul 2025 09:15:21 +0000 Subject: [PATCH 4/7] Refactor obsolete filter methods to call factory methods and execute them immediately Co-authored-by: ewoutkramer <2276305+ewoutkramer@users.noreply.github.com> --- .../Serialization/SerializationFilter.cs | 30 ++++--------------- 1 file changed, 5 insertions(+), 25 deletions(-) diff --git a/src/Hl7.Fhir.Base/Serialization/SerializationFilter.cs b/src/Hl7.Fhir.Base/Serialization/SerializationFilter.cs index 150d0c12fe..e7274c4a30 100644 --- a/src/Hl7.Fhir.Base/Serialization/SerializationFilter.cs +++ b/src/Hl7.Fhir.Base/Serialization/SerializationFilter.cs @@ -44,48 +44,28 @@ public abstract class SerializationFilter /// Construct a new filter that conforms to the `_summary=true` summarized form. /// [Obsolete("Use CreateSummaryFactory() instead to ensure thread-safety when reusing JsonSerializerOptions instances. This method will be removed in a future version.")] - public static SerializationFilter ForSummary() => new BundleFilter(new ElementMetadataFilter() { IncludeInSummary = true }); + public static SerializationFilter ForSummary() => CreateSummaryFactory()(); /// /// Construct a new filter that conforms to the `_summary=text` summarized form. /// [Obsolete("Use CreateTextFactory() instead to ensure thread-safety when reusing JsonSerializerOptions instances. This method will be removed in a future version.")] - public static SerializationFilter ForText() => new BundleFilter(new TopLevelFilter( - new ElementMetadataFilter() - { - IncludeNames = new[] { "text", "id", "meta" }, - IncludeMandatory = true - })); + public static SerializationFilter ForText() => CreateTextFactory()(); [Obsolete("Use CreateCountFactory() instead to ensure thread-safety when reusing JsonSerializerOptions instances. This method will be removed in a future version.")] - public static SerializationFilter ForCount() => new BundleFilter(new TopLevelFilter( - new ElementMetadataFilter() - { - IncludeMandatory = true, - IncludeNames = new[] { "id", "total", "link" } - })); + public static SerializationFilter ForCount() => CreateCountFactory()(); /// /// Construct a new filter that conforms to the `_summary=data` summarized form. /// [Obsolete("Use CreateDataFactory() instead to ensure thread-safety when reusing JsonSerializerOptions instances. This method will be removed in a future version.")] - public static SerializationFilter ForData() => new BundleFilter(new TopLevelFilter( - new ElementMetadataFilter() - { - IncludeNames = new[] { "text" }, - Invert = true - })); + public static SerializationFilter ForData() => CreateDataFactory()(); /// /// Construct a new filter that conforms to the `_elements=...` summarized form. /// [Obsolete("Use CreateElementsFactory() instead to ensure thread-safety when reusing JsonSerializerOptions instances. This method will be removed in a future version.")] - public static SerializationFilter ForElements(string[] elements) => new BundleFilter(new TopLevelFilter( - new ElementMetadataFilter() - { - IncludeNames = elements, - IncludeMandatory = true - })); + public static SerializationFilter ForElements(string[] elements) => CreateElementsFactory(elements)(); /// /// Create a factory function that produces new filter instances conforming to the `_summary=true` summarized form. From 4b2837890f92d723e70f0f06765edc2046a3e1a9 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 22 Jul 2025 10:07:30 +0000 Subject: [PATCH 5/7] Fix factory methods to create fresh filter instances per serialization run to prevent state reuse across operations Co-authored-by: ewoutkramer <2276305+ewoutkramer@users.noreply.github.com> --- .../BaseFhirJsonPocoSerializer.cs | 24 ++++--- .../Serialization/SerializationFilter.cs | 66 +++++++------------ .../SummaryFilterThreadSafetyTests.cs | 18 ++--- 3 files changed, 49 insertions(+), 59 deletions(-) diff --git a/src/Hl7.Fhir.Base/Serialization/BaseFhirJsonPocoSerializer.cs b/src/Hl7.Fhir.Base/Serialization/BaseFhirJsonPocoSerializer.cs index 968888c5e7..682e45f80b 100644 --- a/src/Hl7.Fhir.Base/Serialization/BaseFhirJsonPocoSerializer.cs +++ b/src/Hl7.Fhir.Base/Serialization/BaseFhirJsonPocoSerializer.cs @@ -64,7 +64,7 @@ public BaseFhirJsonPocoSerializer(FhirRelease release, FhirJsonPocoSerializerSet /// Serializes the given dictionary with FHIR data into Json. /// public void Serialize(IReadOnlyDictionary members, Utf8JsonWriter writer) => - serializeInternal(members, writer, skipValue: false); + serializeInternal(members, writer, skipValue: false, filter: null); /// /// Serializes the given dictionary with FHIR data into a Json string. @@ -73,7 +73,7 @@ public string SerializeToString(IReadOnlyDictionary members) { var stream = new MemoryStream(); var writer = new Utf8JsonWriter(stream); - serializeInternal(members, writer, skipValue: false); + serializeInternal(members, writer, skipValue: false, filter: null); writer.Flush(); return Encoding.UTF8.GetString(stream.ToArray()); } @@ -86,13 +86,19 @@ public string SerializeToString(IReadOnlyDictionary members) private void serializeInternal( IReadOnlyDictionary members, Utf8JsonWriter writer, - bool skipValue) + bool skipValue, + SerializationFilter? filter = null) { writer.WriteStartObject(); - // Use factory if available, otherwise fall back to the static instance for backward compatibility + + // Get filter only once at the top level, then pass it through recursive calls + if (filter == null) + { + // Use factory if available, otherwise fall back to the static instance for backward compatibility #pragma warning disable CS0618 // Type or member is obsolete - var filter = Settings.SummaryFilterFactory?.Invoke() ?? Settings.SummaryFilter; + filter = Settings.SummaryFilterFactory?.Invoke() ?? Settings.SummaryFilter; #pragma warning restore CS0618 // Type or member is obsolete + } if (members is Resource r) writer.WriteString("resourceType", r.TypeName); @@ -135,12 +141,12 @@ private void serializeInternal( writer.WriteStartArray(); foreach (var value in coll) - serializeMemberValue(value, writer, requiredType); + serializeMemberValue(value, writer, filter, requiredType); writer.WriteEndArray(); } else - serializeMemberValue(member.Value, writer, requiredType); + serializeMemberValue(member.Value, writer, filter, requiredType); } filter?.LeaveMember(member.Key, member.Value, propertyMapping); @@ -162,10 +168,10 @@ private static string addSuffixToElementName(string elementName, object elementV return typeName is null ? elementName : elementName + char.ToUpperInvariant(typeName[0]) + typeName.Substring(1); } - private void serializeMemberValue(object value, Utf8JsonWriter writer, Type? requiredType = null) + private void serializeMemberValue(object value, Utf8JsonWriter writer, SerializationFilter? filter, Type? requiredType = null) { if (value is IReadOnlyDictionary complex) - serializeInternal(complex, writer, skipValue: false); + serializeInternal(complex, writer, skipValue: false, filter: filter); else SerializePrimitiveValue(value, writer, requiredType); } diff --git a/src/Hl7.Fhir.Base/Serialization/SerializationFilter.cs b/src/Hl7.Fhir.Base/Serialization/SerializationFilter.cs index e7274c4a30..070f9bd283 100644 --- a/src/Hl7.Fhir.Base/Serialization/SerializationFilter.cs +++ b/src/Hl7.Fhir.Base/Serialization/SerializationFilter.cs @@ -10,7 +10,6 @@ using Hl7.Fhir.Introspection; using System; -using System.Threading; namespace Hl7.Fhir.Serialization { @@ -73,10 +72,7 @@ public abstract class SerializationFilter /// public static Func CreateSummaryFactory() { - var threadLocalFilter = new ThreadLocal(() => - new BundleFilter(new ElementMetadataFilter() { IncludeInSummary = true })); - - return () => threadLocalFilter.Value!; + return () => new BundleFilter(new ElementMetadataFilter() { IncludeInSummary = true }); } /// @@ -85,15 +81,12 @@ public static Func CreateSummaryFactory() /// public static Func CreateTextFactory() { - var threadLocalFilter = new ThreadLocal(() => - new BundleFilter(new TopLevelFilter( - new ElementMetadataFilter() - { - IncludeNames = new[] { "text", "id", "meta" }, - IncludeMandatory = true - }))); - - return () => threadLocalFilter.Value!; + return () => new BundleFilter(new TopLevelFilter( + new ElementMetadataFilter() + { + IncludeNames = new[] { "text", "id", "meta" }, + IncludeMandatory = true + })); } /// @@ -102,15 +95,12 @@ public static Func CreateTextFactory() /// public static Func CreateCountFactory() { - var threadLocalFilter = new ThreadLocal(() => - new BundleFilter(new TopLevelFilter( - new ElementMetadataFilter() - { - IncludeMandatory = true, - IncludeNames = new[] { "id", "total", "link" } - }))); - - return () => threadLocalFilter.Value!; + return () => new BundleFilter(new TopLevelFilter( + new ElementMetadataFilter() + { + IncludeMandatory = true, + IncludeNames = new[] { "id", "total", "link" } + })); } /// @@ -119,15 +109,12 @@ public static Func CreateCountFactory() /// public static Func CreateDataFactory() { - var threadLocalFilter = new ThreadLocal(() => - new BundleFilter(new TopLevelFilter( - new ElementMetadataFilter() - { - IncludeNames = new[] { "text" }, - Invert = true - }))); - - return () => threadLocalFilter.Value!; + return () => new BundleFilter(new TopLevelFilter( + new ElementMetadataFilter() + { + IncludeNames = new[] { "text" }, + Invert = true + })); } /// @@ -136,15 +123,12 @@ public static Func CreateDataFactory() /// public static Func CreateElementsFactory(string[] elements) { - var threadLocalFilter = new ThreadLocal(() => - new BundleFilter(new TopLevelFilter( - new ElementMetadataFilter() - { - IncludeNames = elements, - IncludeMandatory = true - }))); - - return () => threadLocalFilter.Value!; + return () => new BundleFilter(new TopLevelFilter( + new ElementMetadataFilter() + { + IncludeNames = elements, + IncludeMandatory = true + })); } } } diff --git a/src/Hl7.Fhir.Support.Poco.Tests/NewPocoSerializers/SummaryFilterThreadSafetyTests.cs b/src/Hl7.Fhir.Support.Poco.Tests/NewPocoSerializers/SummaryFilterThreadSafetyTests.cs index 7c8c3d5941..153b9f6625 100644 --- a/src/Hl7.Fhir.Support.Poco.Tests/NewPocoSerializers/SummaryFilterThreadSafetyTests.cs +++ b/src/Hl7.Fhir.Support.Poco.Tests/NewPocoSerializers/SummaryFilterThreadSafetyTests.cs @@ -64,22 +64,22 @@ public void ConcurrentSerializationWithFactory_ShouldBeThreadSafe() } [TestMethod] - public void AllFactoryMethods_ShouldUseSameInstancePerThread() + public void AllFactoryMethods_ShouldCreateFreshInstancesPerCall() { - // Verify that each factory method returns the same instance per thread - // (this ensures state consistency within a serialization operation) + // Verify that each factory method creates a new instance per call + // (this ensures no state is shared between serialization operations) var summaryFactory = SerializationFilter.CreateSummaryFactory(); var textFactory = SerializationFilter.CreateTextFactory(); var countFactory = SerializationFilter.CreateCountFactory(); var dataFactory = SerializationFilter.CreateDataFactory(); var elementsFactory = SerializationFilter.CreateElementsFactory(["id", "name"]); - // Each call on the same thread should return the same instance - summaryFactory().Should().BeSameAs(summaryFactory()); - textFactory().Should().BeSameAs(textFactory()); - countFactory().Should().BeSameAs(countFactory()); - dataFactory().Should().BeSameAs(dataFactory()); - elementsFactory().Should().BeSameAs(elementsFactory()); + // Each call should return a different instance + summaryFactory().Should().NotBeSameAs(summaryFactory()); + textFactory().Should().NotBeSameAs(textFactory()); + countFactory().Should().NotBeSameAs(countFactory()); + dataFactory().Should().NotBeSameAs(dataFactory()); + elementsFactory().Should().NotBeSameAs(elementsFactory()); } } } \ No newline at end of file From 8624c25338f357f6c4668de71c3cb8709a5e929d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 22 Jul 2025 10:53:58 +0000 Subject: [PATCH 6/7] Add thread-safe filter factory support to XML serialization methods - Add new overloads to BaseFhirXmlPocoSerializer.Serialize() and SerializeToString() that take Func factory parameter - Mark existing methods taking SerializationFilter as obsolete with clear migration guidance - Update internal calls in PocoSerializationEngine_Xml to use new thread-safe methods - Add comprehensive tests verifying factory methods work correctly and produce identical output to obsolete methods - Ensure filter factories create fresh instances per call for thread safety Addresses XML serialization thread-safety concerns similar to JSON serialization fixes Co-authored-by: ewoutkramer <2276305+ewoutkramer@users.noreply.github.com> --- .../BaseFhirXmlPocoSerializer.cs | 37 ++++++++++++-- .../engine/PocoSerializationEngine_Xml.cs | 4 +- .../FhirXmlSerializationTests.cs | 51 +++++++++++++++++++ 3 files changed, 85 insertions(+), 7 deletions(-) diff --git a/src/Hl7.Fhir.Base/Serialization/BaseFhirXmlPocoSerializer.cs b/src/Hl7.Fhir.Base/Serialization/BaseFhirXmlPocoSerializer.cs index dc0bca302d..859c2ce44b 100644 --- a/src/Hl7.Fhir.Base/Serialization/BaseFhirXmlPocoSerializer.cs +++ b/src/Hl7.Fhir.Base/Serialization/BaseFhirXmlPocoSerializer.cs @@ -42,9 +42,12 @@ public BaseFhirXmlPocoSerializer(FhirRelease release) } /// - /// Serializes the given dictionary with FHIR data into Json. + /// Serializes the given dictionary with FHIR data into XML. /// - public void Serialize(IReadOnlyDictionary members, XmlWriter writer, SerializationFilter? summary = default) + /// The dictionary containing FHIR data to serialize. + /// The XmlWriter to write the serialized data to. + /// A factory function that creates a new filter instance for each serialization operation. This ensures thread-safety when reusing serializer instances in concurrent scenarios. + public void Serialize(IReadOnlyDictionary members, XmlWriter writer, Func? summaryFilterFactory) { writer.WriteStartDocument(); @@ -57,17 +60,41 @@ public void Serialize(IReadOnlyDictionary members, XmlWriter wri writer.WriteStartElement(rootElementName, XmlNs.FHIR); } - serializeInternal(members, writer, summary); + var filter = summaryFilterFactory?.Invoke(); + serializeInternal(members, writer, filter); if (simulateRoot) writer.WriteEndElement(); writer.WriteEndDocument(); } /// - /// Serializes the given dictionary with FHIR data into UTF8 encoded Json. + /// Serializes the given dictionary with FHIR data into XML. + /// + /// The dictionary containing FHIR data to serialize. + /// The XmlWriter to write the serialized data to. + /// The serialization filter to apply. NOTE: For thread-safety when reusing serializer instances, pass a fresh filter instance for each serialization operation. + [Obsolete("Use the overload that takes Func summaryFilterFactory instead to ensure thread-safety when reusing serializer instances in concurrent scenarios. This method will be removed in a future version.")] + public void Serialize(IReadOnlyDictionary members, XmlWriter writer, SerializationFilter? summary = default) + { + Serialize(members, writer, summary != null ? () => summary : (Func?)null); + } + + /// + /// Serializes the given dictionary with FHIR data into UTF8 encoded XML. + /// + /// The dictionary containing FHIR data to serialize. + /// A factory function that creates a new filter instance for each serialization operation. This ensures thread-safety when reusing serializer instances in concurrent scenarios. + public string SerializeToString(IReadOnlyDictionary members, Func? summaryFilterFactory) => + SerializationUtil.WriteXmlToString(w => Serialize(members, w, summaryFilterFactory)); + + /// + /// Serializes the given dictionary with FHIR data into UTF8 encoded XML. /// + /// The dictionary containing FHIR data to serialize. + /// The serialization filter to apply. NOTE: For thread-safety when reusing serializer instances, pass a fresh filter instance for each serialization operation. + [Obsolete("Use the overload that takes Func summaryFilterFactory instead to ensure thread-safety when reusing serializer instances in concurrent scenarios. This method will be removed in a future version.")] public string SerializeToString(IReadOnlyDictionary members, SerializationFilter? summary = default) => - SerializationUtil.WriteXmlToString(w => Serialize(members, w, summary)); + SerializeToString(members, summary != null ? () => summary : (Func?)null); /// /// Serializes the given dictionary with FHIR data into Json, optionally skipping the "value" element. diff --git a/src/Hl7.Fhir.Base/Serialization/engine/PocoSerializationEngine_Xml.cs b/src/Hl7.Fhir.Base/Serialization/engine/PocoSerializationEngine_Xml.cs index 9adcf86812..018c647429 100644 --- a/src/Hl7.Fhir.Base/Serialization/engine/PocoSerializationEngine_Xml.cs +++ b/src/Hl7.Fhir.Base/Serialization/engine/PocoSerializationEngine_Xml.cs @@ -34,7 +34,7 @@ public Resource DeserializeFromXml(string data) } /// - public string SerializeToXml(Resource instance) => getXmlSerializer().SerializeToString(instance); + public string SerializeToXml(Resource instance) => getXmlSerializer().SerializeToString(instance, (Func?)null); /// /// Deserializes a resource from an XML reader @@ -70,5 +70,5 @@ public Base DeserializeElementFromXml(Type targetType, XmlReader reader) /// /// An instance of Base or any of its children /// The XML writer - public void SerializeToXmlWriter(Base instance, XmlWriter writer) => getXmlSerializer().Serialize(instance, writer); + public void SerializeToXmlWriter(Base instance, XmlWriter writer) => getXmlSerializer().Serialize(instance, writer, (Func?)null); } \ No newline at end of file diff --git a/src/Hl7.Fhir.Support.Poco.Tests/NewPocoSerializers/FhirXmlSerializationTests.cs b/src/Hl7.Fhir.Support.Poco.Tests/NewPocoSerializers/FhirXmlSerializationTests.cs index 4841fda564..7306f0d7a6 100644 --- a/src/Hl7.Fhir.Support.Poco.Tests/NewPocoSerializers/FhirXmlSerializationTests.cs +++ b/src/Hl7.Fhir.Support.Poco.Tests/NewPocoSerializers/FhirXmlSerializationTests.cs @@ -52,5 +52,56 @@ public void SerializesInvalidData() contactArray.Count().Should().Be(1); contactArray.First().Elements().Should().BeEmpty(); } + + [TestMethod] + public void CanUseFilterFactory() + { + var patient = new Patient + { + Id = "test-patient", + Active = true, + Name = new() { new HumanName { Given = new[] { "John" }, Family = "Doe" } }, + Gender = AdministrativeGender.Male + }; + + var serializer = new BaseFhirXmlPocoSerializer(Specification.FhirRelease.STU3); + + // Test the new factory-based method + var elementsFactory = SerializationFilter.CreateElementsFactory(new[] { "id", "active" }); + var xmlWithFactory = serializer.SerializeToString(patient, elementsFactory); + + // Test the obsolete method for comparison +#pragma warning disable CS0618 // Type or member is obsolete + var filter = SerializationFilter.ForElements(new[] { "id", "active" }); + var xmlWithFilter = serializer.SerializeToString(patient, filter); +#pragma warning restore CS0618 // Type or member is obsolete + + // Both methods should produce identical output + xmlWithFactory.Should().Be(xmlWithFilter); + + // Verify that filtering actually works (should only contain id and active) + var xdoc = XDocument.Parse(xmlWithFactory); + var patientElement = xdoc.Root; + + // Should contain id and active elements + patientElement.Elements(XName.Get("id", XmlNs.FHIR)).Should().HaveCount(1); + patientElement.Elements(XName.Get("active", XmlNs.FHIR)).Should().HaveCount(1); + + // Should NOT contain name or gender (they were filtered out) + patientElement.Elements(XName.Get("name", XmlNs.FHIR)).Should().BeEmpty(); + patientElement.Elements(XName.Get("gender", XmlNs.FHIR)).Should().BeEmpty(); + } + + [TestMethod] + public void FilterFactoryCreatesNewInstancesEachTime() + { + var elementsFactory = SerializationFilter.CreateElementsFactory(new[] { "id", "active" }); + + // Each call should return a new instance + var filter1 = elementsFactory(); + var filter2 = elementsFactory(); + + filter1.Should().NotBeSameAs(filter2); + } } } \ No newline at end of file From 88fd5aaa2e854e3ed3a752da483ad7f8976f584d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 23 Jul 2025 19:59:03 +0000 Subject: [PATCH 7/7] Remove unnecessary explicit null parameter in serializeInternal calls Co-authored-by: alexzautke <548617+alexzautke@users.noreply.github.com> --- src/Hl7.Fhir.Base/Serialization/BaseFhirJsonPocoSerializer.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Hl7.Fhir.Base/Serialization/BaseFhirJsonPocoSerializer.cs b/src/Hl7.Fhir.Base/Serialization/BaseFhirJsonPocoSerializer.cs index 682e45f80b..fd70240810 100644 --- a/src/Hl7.Fhir.Base/Serialization/BaseFhirJsonPocoSerializer.cs +++ b/src/Hl7.Fhir.Base/Serialization/BaseFhirJsonPocoSerializer.cs @@ -64,7 +64,7 @@ public BaseFhirJsonPocoSerializer(FhirRelease release, FhirJsonPocoSerializerSet /// Serializes the given dictionary with FHIR data into Json. /// public void Serialize(IReadOnlyDictionary members, Utf8JsonWriter writer) => - serializeInternal(members, writer, skipValue: false, filter: null); + serializeInternal(members, writer, skipValue: false); /// /// Serializes the given dictionary with FHIR data into a Json string. @@ -73,7 +73,7 @@ public string SerializeToString(IReadOnlyDictionary members) { var stream = new MemoryStream(); var writer = new Utf8JsonWriter(stream); - serializeInternal(members, writer, skipValue: false, filter: null); + serializeInternal(members, writer, skipValue: false); writer.Flush(); return Encoding.UTF8.GetString(stream.ToArray()); }