Merge pull request #3218 from FirelyTeam/copilot/fix-3214

ewoutkramer · web-flow · commit af8e2101af19 · 2025-07-24T14:08:55.000+02:00
Fix thread-safety issue with SerializationFilter when reusing JsonSerializerOptions
diff --git a/src/Hl7.Fhir.Base/Serialization/BaseFhirJsonPocoSerializer.cs b/src/Hl7.Fhir.Base/Serialization/BaseFhirJsonPocoSerializer.cs
@@ -86,10 +86,19 @@ public string SerializeToString(IReadOnlyDictionary<string, object> members)
         private void serializeInternal(
             IReadOnlyDictionary<string, object> members,
             Utf8JsonWriter writer,
-            bool skipValue)
+            bool skipValue,
+            SerializationFilter? filter = null)
         {
             writer.WriteStartObject();
-            var filter = Settings.SummaryFilter;
+            
+            // 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
+                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);
@@ -132,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);
@@ -159,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<string, object> 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/BaseFhirXmlPocoSerializer.cs b/src/Hl7.Fhir.Base/Serialization/BaseFhirXmlPocoSerializer.cs
@@ -42,9 +42,12 @@ public BaseFhirXmlPocoSerializer(FhirRelease release)
         }
 
         /// <summary>
-        /// Serializes the given dictionary with FHIR data into Json.
+        /// Serializes the given dictionary with FHIR data into XML.
         /// </summary>
-        public void Serialize(IReadOnlyDictionary<string, object> members, XmlWriter writer, SerializationFilter? summary = default)
+        /// <param name="members">The dictionary containing FHIR data to serialize.</param>
+        /// <param name="writer">The XmlWriter to write the serialized data to.</param>
+        /// <param name="summaryFilterFactory">A factory function that creates a new filter instance for each serialization operation. This ensures thread-safety when reusing serializer instances in concurrent scenarios.</param>
+        public void Serialize(IReadOnlyDictionary<string, object> members, XmlWriter writer, Func<SerializationFilter>? summaryFilterFactory)
         {
             writer.WriteStartDocument();
 
@@ -57,17 +60,41 @@ public void Serialize(IReadOnlyDictionary<string, object> 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();
         }
 
         /// <summary>
-        /// Serializes the given dictionary with FHIR data into UTF8 encoded Json.
+        /// Serializes the given dictionary with FHIR data into XML.
+        /// </summary>
+        /// <param name="members">The dictionary containing FHIR data to serialize.</param>
+        /// <param name="writer">The XmlWriter to write the serialized data to.</param>
+        /// <param name="summary">The serialization filter to apply. NOTE: For thread-safety when reusing serializer instances, pass a fresh filter instance for each serialization operation.</param>
+        [Obsolete("Use the overload that takes Func<SerializationFilter> 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<string, object> members, XmlWriter writer, SerializationFilter? summary = default)
+        {
+            Serialize(members, writer, summary != null ? () => summary : (Func<SerializationFilter>?)null);
+        }
+
+        /// <summary>
+        /// Serializes the given dictionary with FHIR data into UTF8 encoded XML.
+        /// </summary>
+        /// <param name="members">The dictionary containing FHIR data to serialize.</param>
+        /// <param name="summaryFilterFactory">A factory function that creates a new filter instance for each serialization operation. This ensures thread-safety when reusing serializer instances in concurrent scenarios.</param>
+        public string SerializeToString(IReadOnlyDictionary<string, object> members, Func<SerializationFilter>? summaryFilterFactory) =>
+            SerializationUtil.WriteXmlToString(w => Serialize(members, w, summaryFilterFactory));
+
+        /// <summary>
+        /// Serializes the given dictionary with FHIR data into UTF8 encoded XML.
         /// </summary>
+        /// <param name="members">The dictionary containing FHIR data to serialize.</param>
+        /// <param name="summary">The serialization filter to apply. NOTE: For thread-safety when reusing serializer instances, pass a fresh filter instance for each serialization operation.</param>
+        [Obsolete("Use the overload that takes Func<SerializationFilter> 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<string, object> members, SerializationFilter? summary = default) =>
-            SerializationUtil.WriteXmlToString(w => Serialize(members, w, summary));
+            SerializeToString(members, summary != null ? () => summary : (Func<SerializationFilter>?)null);
 
         /// <summary>
         /// Serializes the given dictionary with FHIR data into Json, optionally skipping the "value" element.
diff --git a/src/Hl7.Fhir.Base/Serialization/FhirJsonPocoSerializerSettings.cs b/src/Hl7.Fhir.Base/Serialization/FhirJsonPocoSerializerSettings.cs
@@ -27,7 +27,14 @@ public record FhirJsonPocoSerializerSettings
         /// <summary>
         /// Specifies the filter to use for summary serialization.
         /// </summary>
+        [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;
+
+        /// <summary>
+        /// 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.
+        /// </summary>
+        public Func<SerializationFilter>? SummaryFilterFactory { get; set; } = default;
     }
 }
 
diff --git 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,44 +42,94 @@ public abstract class SerializationFilter
         /// <summary>
         /// Construct a new filter that conforms to the `_summary=true` summarized form.
         /// </summary>
-        public static SerializationFilter ForSummary() => new BundleFilter(new ElementMetadataFilter() { IncludeInSummary = true });
+        [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() => CreateSummaryFactory()();
 
         /// <summary>
         /// Construct a new filter that conforms to the `_summary=text` summarized form.
         /// </summary>
-        public static SerializationFilter ForText() => new BundleFilter(new TopLevelFilter(
-            new ElementMetadataFilter()
-            {
-                IncludeNames = new[] { "text", "id", "meta" },
-                IncludeMandatory = true
-            }));
-
-        public static SerializationFilter ForCount() => new BundleFilter(new TopLevelFilter(
-            new ElementMetadataFilter()
-            {
-                IncludeMandatory = true,
-                IncludeNames = new[] { "id", "total", "link" }
-            }));
+        [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() => 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() => CreateCountFactory()();
 
         /// <summary>
         /// Construct a new filter that conforms to the `_summary=data` summarized form.
         /// </summary>
-        public static SerializationFilter ForData() => new BundleFilter(new TopLevelFilter(
-          new ElementMetadataFilter()
-          {
-              IncludeNames = new[] { "text" },
-              Invert = true
-          }));
+        [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() => CreateDataFactory()();
 
         /// <summary>
         /// Construct a new filter that conforms to the `_elements=...` summarized form.
         /// </summary>
-        public static SerializationFilter ForElements(string[] elements) => new BundleFilter(new TopLevelFilter(
-          new ElementMetadataFilter()
-          {
-              IncludeNames = elements,
-              IncludeMandatory = true
-          }));
+        [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) => CreateElementsFactory(elements)();
+
+        /// <summary>
+        /// 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.
+        /// </summary>
+        public static Func<SerializationFilter> CreateSummaryFactory()
+        {
+            return () => new BundleFilter(new ElementMetadataFilter() { IncludeInSummary = true });
+        }
+
+        /// <summary>
+        /// 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.
+        /// </summary>
+        public static Func<SerializationFilter> CreateTextFactory()
+        {
+            return () => new BundleFilter(new TopLevelFilter(
+                new ElementMetadataFilter()
+                {
+                    IncludeNames = new[] { "text", "id", "meta" },
+                    IncludeMandatory = true
+                }));
+        }
+
+        /// <summary>
+        /// 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.
+        /// </summary>
+        public static Func<SerializationFilter> CreateCountFactory()
+        {
+            return () => new BundleFilter(new TopLevelFilter(
+                new ElementMetadataFilter()
+                {
+                    IncludeMandatory = true,
+                    IncludeNames = new[] { "id", "total", "link" }
+                }));
+        }
+
+        /// <summary>
+        /// 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.
+        /// </summary>
+        public static Func<SerializationFilter> CreateDataFactory()
+        {
+            return () => new BundleFilter(new TopLevelFilter(
+                new ElementMetadataFilter()
+                {
+                    IncludeNames = new[] { "text" },
+                    Invert = true
+                }));
+        }
+
+        /// <summary>
+        /// 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.
+        /// </summary>
+        public static Func<SerializationFilter> CreateElementsFactory(string[] elements)
+        {
+            return () => new BundleFilter(new TopLevelFilter(
+                new ElementMetadataFilter()
+                {
+                    IncludeNames = elements,
+                    IncludeMandatory = true
+                }));
+        }
     }
 }
 
diff --git 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)
     }
     
     /// <inheritdoc />
-    public string SerializeToXml(Resource instance) => getXmlSerializer().SerializeToString(instance);
+    public string SerializeToXml(Resource instance) => getXmlSerializer().SerializeToString(instance, (Func<SerializationFilter>?)null);
 
     /// <summary>
     /// Deserializes a resource from an XML reader
@@ -70,5 +70,5 @@ public Base DeserializeElementFromXml(Type targetType, XmlReader reader)
     /// </summary>
     /// <param name="instance">An instance of Base or any of its children</param>
     /// <param name="writer">The XML writer</param>
-    public void SerializeToXmlWriter(Base instance, XmlWriter writer) => getXmlSerializer().Serialize(instance, writer);
+    public void SerializeToXmlWriter(Base instance, XmlWriter writer) => getXmlSerializer().Serialize(instance, writer, (Func<SerializationFilter>?)null);
 }
diff --git 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);
+        }
     }
 }
diff --git a/src/Hl7.Fhir.Support.Poco.Tests/NewPocoSerializers/SummaryFilterThreadSafetyTests.cs b/src/Hl7.Fhir.Support.Poco.Tests/NewPocoSerializers/SummaryFilterThreadSafetyTests.cs

Original file line number	Diff line number	Diff line change
`@@ -27,7 +27,14 @@ public record FhirJsonPocoSerializerSettings`
`27`	`27`	`/// <summary>`
`28`	`28`	`/// Specifies the filter to use for summary serialization.`
`29`	`29`	`/// </summary>`
	`30`	`+ [Obsolete("Use SummaryFilterFactory instead to ensure thread-safety when reusing JsonSerializerOptions instances. This property will be removed in a future version.")]`
`30`	`31`	`public SerializationFilter? SummaryFilter { get; set; } = default;`
	`32`	`+`
	`33`	`+ /// <summary>`
	`34`	`+ /// Specifies a factory function that creates a new filter instance for each serialization operation.`
	`35`	`+ /// This ensures thread-safety when reusing JsonSerializerOptions instances in concurrent scenarios.`
	`36`	`+ /// </summary>`
	`37`	`+ public Func<SerializationFilter>? SummaryFilterFactory { get; set; } = default;`
`31`	`38`	`}`
`32`	`39`	`}`
`33`	`40`
Original file line number	Diff line number	Diff line change
`@@ -34,7 +34,7 @@ public Resource DeserializeFromXml(string data)`
`34`	`34`	`}`
`35`	`35`
`36`	`36`	`/// <inheritdoc />`
`37`		`- public string SerializeToXml(Resource instance) => getXmlSerializer().SerializeToString(instance);`
	`37`	`+ public string SerializeToXml(Resource instance) => getXmlSerializer().SerializeToString(instance, (Func<SerializationFilter>?)null);`
`38`	`38`
`39`	`39`	`/// <summary>`
`40`	`40`	`/// Deserializes a resource from an XML reader`
`@@ -70,5 +70,5 @@ public Base DeserializeElementFromXml(Type targetType, XmlReader reader)`
`70`	`70`	`/// </summary>`
`71`	`71`	`/// <param name="instance">An instance of Base or any of its children</param>`
`72`	`72`	`/// <param name="writer">The XML writer</param>`
`73`		`- public void SerializeToXmlWriter(Base instance, XmlWriter writer) => getXmlSerializer().Serialize(instance, writer);`
	`73`	`+ public void SerializeToXmlWriter(Base instance, XmlWriter writer) => getXmlSerializer().Serialize(instance, writer, (Func<SerializationFilter>?)null);`
`74`	`74`	`}`