From dfa54d6297b1feebc2c6d92d766f2e9ea901aff9 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 6 Mar 2026 20:36:47 +0000 Subject: [PATCH 1/3] Initial plan From eb205ee53adbfbf7cc7d1f1ce972a6df4916d553 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 6 Mar 2026 20:42:25 +0000 Subject: [PATCH 2/3] fix: incorporate PR #9358 fixes - thread safety, collection element defaults, IsVisible roundtrip Co-authored-by: philnach <19275540+philnach@users.noreply.github.com> --- .gitignore | 1 + ...aptiveCollectionElementConverterFactory.cs | 33 +++- .../Library/AdaptiveCards/AdaptiveElement.cs | 8 +- .../AdaptiveFallbackConverter.cs | 15 +- .../Library/AdaptiveCards/SafeJsonHelper.cs | 4 +- .../AdaptiveCards.Test.csproj | 2 +- .../SystemTextJsonMigrationTests.cs | 160 ++++++++++++++++++ 7 files changed, 212 insertions(+), 11 deletions(-) diff --git a/.gitignore b/.gitignore index aa895406c8..220ab4210e 100644 --- a/.gitignore +++ b/.gitignore @@ -321,3 +321,4 @@ _deps *-prefix/ **/.nx/* +.nuget/ diff --git a/source/dotnet/Library/AdaptiveCards/AdaptiveCollectionElementConverterFactory.cs b/source/dotnet/Library/AdaptiveCards/AdaptiveCollectionElementConverterFactory.cs index 0153d38c04..94741aa3e3 100644 --- a/source/dotnet/Library/AdaptiveCards/AdaptiveCollectionElementConverterFactory.cs +++ b/source/dotnet/Library/AdaptiveCards/AdaptiveCollectionElementConverterFactory.cs @@ -167,9 +167,11 @@ public override void Write(Utf8JsonWriter writer, T value, JsonSerializerOptions { if (!prop.CanRead) continue; if (prop.GetIndexParameters().Length > 0) continue; // Skip indexers - if (prop.GetCustomAttribute() is JsonIgnoreAttribute ignore && ignore.Condition == JsonIgnoreCondition.Always) continue; if (prop.GetCustomAttribute() != null) continue; + var ignoreAttr = prop.GetCustomAttribute(); + if (ignoreAttr != null && ignoreAttr.Condition == JsonIgnoreCondition.Always) continue; + string jsonName; var nameAttr = prop.GetCustomAttribute(); if (nameAttr != null) @@ -185,15 +187,34 @@ public override void Write(Utf8JsonWriter writer, T value, JsonSerializerOptions jsonName = prop.Name; } + if (string.IsNullOrEmpty(jsonName)) continue; + var propValue = prop.GetValue(value); - - // Handle null suppression - if (propValue == null && options.DefaultIgnoreCondition == JsonIgnoreCondition.WhenWritingNull) continue; + var propType = prop.PropertyType; - if (string.IsNullOrEmpty(jsonName)) continue; + // Determine the effective ignore condition: per-property attribute overrides the global option. + var effectiveCondition = (ignoreAttr != null) + ? ignoreAttr.Condition + : options.DefaultIgnoreCondition; + + // Apply the effective ignore condition + if (effectiveCondition == JsonIgnoreCondition.WhenWritingNull && propValue == null) continue; + if (effectiveCondition == JsonIgnoreCondition.WhenWritingDefault) + { + if (propValue == null) continue; + // For value types compare against the type's default (e.g. false for bool, 0 for enum). + // Activator.CreateInstance always returns a non-null boxed value for value types so + // the null-conditional guard here is purely defensive. + if (propType.IsValueType) + { + var underlyingType = Nullable.GetUnderlyingType(propType) ?? propType; + var typeDefault = Activator.CreateInstance(underlyingType); + if (typeDefault == null || propValue.Equals(typeDefault)) continue; + } + } writer.WritePropertyName(jsonName); - JsonSerializer.Serialize(writer, propValue, prop.PropertyType, options); + JsonSerializer.Serialize(writer, propValue, propType, options); } // Write extension data diff --git a/source/dotnet/Library/AdaptiveCards/AdaptiveElement.cs b/source/dotnet/Library/AdaptiveCards/AdaptiveElement.cs index 6eaf783151..b459ea0403 100644 --- a/source/dotnet/Library/AdaptiveCards/AdaptiveElement.cs +++ b/source/dotnet/Library/AdaptiveCards/AdaptiveElement.cs @@ -46,7 +46,13 @@ public abstract class AdaptiveElement : AdaptiveTypedElement /// /// Indicates whether the element should be visible when the card has been rendered. /// - [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)] + /// + /// The spec default is true (visible). Because the .NET type default for bool + /// is false, using would suppress + /// false values during serialization — which would then be read back as true + /// (the initialised default) and silently make hidden elements visible. To avoid this roundtrip + /// regression the property is always serialised regardless of its value. + /// [XmlElement] [DefaultValue(true)] public bool IsVisible { get; set; } = true; diff --git a/source/dotnet/Library/AdaptiveCards/AdaptiveFallbackConverter.cs b/source/dotnet/Library/AdaptiveCards/AdaptiveFallbackConverter.cs index 4854c6f380..2e7494b65c 100644 --- a/source/dotnet/Library/AdaptiveCards/AdaptiveFallbackConverter.cs +++ b/source/dotnet/Library/AdaptiveCards/AdaptiveFallbackConverter.cs @@ -40,7 +40,20 @@ public AdaptiveFallbackConverter(List warnings, ParseContext pa /// /// State tracking to determine whether we're currently processing a fallback request. /// - public static bool IsInFallback = false; + /// + /// Marked [ThreadStatic] to avoid race conditions when multiple threads parse + /// cards concurrently. Each thread has its own copy of this flag so that one thread's + /// fallback state cannot corrupt another thread's ID collision detection. + /// + [System.ThreadStatic] + private static bool _isInFallback; + + /// + public static bool IsInFallback + { + get => _isInFallback; + set => _isInFallback = value; + } /// public override AdaptiveFallbackElement Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) diff --git a/source/dotnet/Library/AdaptiveCards/SafeJsonHelper.cs b/source/dotnet/Library/AdaptiveCards/SafeJsonHelper.cs index 180a4324a5..ed355b6a83 100644 --- a/source/dotnet/Library/AdaptiveCards/SafeJsonHelper.cs +++ b/source/dotnet/Library/AdaptiveCards/SafeJsonHelper.cs @@ -10,8 +10,8 @@ namespace AdaptiveCards /// duplicate keys (which is valid JSON per RFC 8259 but not handled by JsonObject.Create). /// /// - /// System.Text.Json's throws - /// when duplicate keys are present. + /// throws + /// when duplicate keys are present in a JsonElement. /// This helper uses indexer assignment so duplicates silently keep the last value, /// matching the previous Newtonsoft.Json behavior. A debug warning is emitted /// when duplicates are detected to help identify malformed payloads. diff --git a/source/dotnet/Test/AdaptiveCards.Test/AdaptiveCards.Test.csproj b/source/dotnet/Test/AdaptiveCards.Test/AdaptiveCards.Test.csproj index 28103f8d8b..3e238ebd7b 100644 --- a/source/dotnet/Test/AdaptiveCards.Test/AdaptiveCards.Test.csproj +++ b/source/dotnet/Test/AdaptiveCards.Test/AdaptiveCards.Test.csproj @@ -1,7 +1,7 @@  - net6.0 + net8.0 false false diff --git a/source/dotnet/Test/AdaptiveCards.Test/SystemTextJsonMigrationTests.cs b/source/dotnet/Test/AdaptiveCards.Test/SystemTextJsonMigrationTests.cs index 843fe465b8..1d004d1e1a 100644 --- a/source/dotnet/Test/AdaptiveCards.Test/SystemTextJsonMigrationTests.cs +++ b/source/dotnet/Test/AdaptiveCards.Test/SystemTextJsonMigrationTests.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. using System; +using System.Collections.Concurrent; using System.Collections.Generic; using System.Linq; using System.Text.Json; @@ -812,5 +813,164 @@ public void Table_DeserializesCorrectly() Assert.AreEqual(1, cell1.Items.Count); Assert.AreEqual("Cell 1", (cell1.Items[0] as AdaptiveTextBlock)?.Text); } + + // ===================================================================== + // Category 6: Bug-fix Regression Tests + // ===================================================================== + + /// + /// Verifies that does not leak + /// across threads. Before the ThreadStatic fix, concurrent parsing of cards that contain + /// fallback elements could corrupt the shared flag, causing valid cards to throw a + /// spurious ID-collision exception or silently accept real collisions. + /// + [TestMethod] + public void ConcurrentFallbackParsing_IsThreadSafe() + { + // A card with a fallback element that has the same ID as its parent (allowed per spec). + var json = @"{ + ""type"": ""AdaptiveCard"", + ""version"": ""1.2"", + ""body"": [ + { + ""type"": ""TextBlock"", + ""id"": ""shared"", + ""text"": ""Primary"", + ""fallback"": { + ""type"": ""TextBlock"", + ""id"": ""shared"", + ""text"": ""Fallback"" + } + } + ] + }"; + + var exceptions = new ConcurrentBag(); + var tasks = new Task[10]; + + for (int i = 0; i < 10; i++) + { + tasks[i] = Task.Run(() => + { + try + { + for (int j = 0; j < 50; j++) + { + var result = AdaptiveCard.FromJson(json); + Assert.IsNotNull(result.Card); + Assert.AreEqual(1, result.Card.Body.Count); + } + } + catch (Exception ex) + { + exceptions.Add(ex); + } + }); + } + + Task.WaitAll(tasks); + Assert.AreEqual(0, exceptions.Count, + $"Thread-safety failures: {string.Join("; ", exceptions.Select(e => e.Message))}"); + } + + /// + /// Verifies that collection element types (Container, Column, ColumnSet) do not emit + /// properties that have their default values. Before the fix, the + /// AdaptiveCollectionElementConverter wrote every property regardless of + /// [JsonIgnore(Condition = WhenWritingNull/WhenWritingDefault)], producing + /// verbose (and sometimes null-valued) JSON that violated the spec. + /// + [TestMethod] + public void CollectionElements_DefaultPropertiesAreNotSerialized() + { + var card = new AdaptiveCard("1.2") + { + Body = + { + new AdaptiveContainer { Items = { new AdaptiveTextBlock("Hello") } }, + new AdaptiveColumnSet + { + Columns = { new AdaptiveColumn { Items = { new AdaptiveTextBlock("Col") } } } + } + } + }; + + var json = card.ToJson(); + + // These are all default values — they must NOT appear in the output. + Assert.IsFalse(json.Contains("\"separator\""), + "Default 'separator: false' must not be serialized"); + Assert.IsFalse(json.Contains("\"bleed\""), + "Default 'bleed: false' must not be serialized"); + Assert.IsFalse(json.Contains("\"horizontalAlignment\""), + "Default 'horizontalAlignment: Left' must not be serialized"); + Assert.IsFalse(json.Contains("\"verticalContentAlignment\""), + "Default 'verticalContentAlignment: Top' (null-written by enum converter) must not be serialized"); + + // Non-default values must still round-trip correctly. + var card2 = new AdaptiveCard("1.2") + { + Body = + { + new AdaptiveContainer + { + Bleed = true, + Style = AdaptiveContainerStyle.Emphasis, + Items = { new AdaptiveTextBlock("Hello") } + } + } + }; + + var json2 = card2.ToJson(); + Assert.IsTrue(json2.Contains("\"bleed\""), "Non-default 'bleed: true' must be serialized"); + Assert.IsTrue(json2.Contains("emphasis"), "Non-default 'style: emphasis' must be serialized"); + + var reparsed = AdaptiveCard.FromJson(json2).Card; + var container = reparsed.Body[0] as AdaptiveContainer; + Assert.IsTrue(container.Bleed); + Assert.AreEqual(AdaptiveContainerStyle.Emphasis, container.Style); + } + + /// + /// Verifies that setting to false is + /// preserved after a serialise-then-parse round-trip. Before the fix, the property used + /// [JsonIgnore(Condition = WhenWritingDefault)] which silently dropped + /// false (the bool type-default) from the JSON, causing hidden elements to + /// reappear as visible after re-parsing. + /// + [TestMethod] + public void IsVisible_False_RoundtripsCorrectly() + { + var card = new AdaptiveCard("1.0") + { + Body = + { + new AdaptiveTextBlock("Hidden") { IsVisible = false }, + new AdaptiveTextBlock("Visible") { IsVisible = true }, + new AdaptiveContainer + { + IsVisible = false, + Items = { new AdaptiveTextBlock("Inside hidden container") } + } + } + }; + + var json = card.ToJson(); + + // "isVisible": false must be present in the serialized JSON + Assert.IsTrue(json.Contains("\"isVisible\""), + "The isVisible property must always be serialized"); + Assert.IsTrue(json.Contains("false"), + "isVisible: false must be present in the JSON"); + + var reparsed = AdaptiveCard.FromJson(json).Card; + + Assert.IsFalse(((AdaptiveTextBlock)reparsed.Body[0]).IsVisible, + "TextBlock with IsVisible=false must remain hidden after roundtrip"); + Assert.IsTrue(((AdaptiveTextBlock)reparsed.Body[1]).IsVisible, + "TextBlock with IsVisible=true must remain visible after roundtrip"); + Assert.IsFalse(((AdaptiveContainer)reparsed.Body[2]).IsVisible, + "Container with IsVisible=false must remain hidden after roundtrip"); + } } } From d37050cd1f0ef5c5c8c9440f11f7a2754070c759 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 6 Mar 2026 20:43:25 +0000 Subject: [PATCH 3/3] fix: tighten IsVisible_False_RoundtripsCorrectly assertion to match exact JSON pattern Co-authored-by: philnach <19275540+philnach@users.noreply.github.com> --- .../Test/AdaptiveCards.Test/SystemTextJsonMigrationTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/dotnet/Test/AdaptiveCards.Test/SystemTextJsonMigrationTests.cs b/source/dotnet/Test/AdaptiveCards.Test/SystemTextJsonMigrationTests.cs index 1d004d1e1a..ddd18a1dbf 100644 --- a/source/dotnet/Test/AdaptiveCards.Test/SystemTextJsonMigrationTests.cs +++ b/source/dotnet/Test/AdaptiveCards.Test/SystemTextJsonMigrationTests.cs @@ -960,7 +960,7 @@ public void IsVisible_False_RoundtripsCorrectly() // "isVisible": false must be present in the serialized JSON Assert.IsTrue(json.Contains("\"isVisible\""), "The isVisible property must always be serialized"); - Assert.IsTrue(json.Contains("false"), + Assert.IsTrue(json.Contains("\"isVisible\":false") || json.Contains("\"isVisible\": false"), "isVisible: false must be present in the JSON"); var reparsed = AdaptiveCard.FromJson(json).Card;