The call to ApiClientBuilder.EnableBackingStoreForParseNodeFactory would not check to see if the supplied ParseNodeFactoryRegistry was already a BackingStoreParseNodeFactory. This means that if it was called in a loop it would add multiple layers of Backing Storage and the call to GetRootParseNode could end up making lots of calls. In dotnet 4.8 projects with Prefer 32 Bit Turned off, the large call stack was causing StackOverflow Exceptions.

Author: grippstick

src/abstractions/ApiClientBuilder.cs

@@ -50,6 +50,9 @@ public static ISerializationWriterFactory EnableBackingStoreForSerializationWrit
                 if(registry != SerializationWriterFactoryRegistry.DefaultInstance)// if the registry is the default instance, we already enabled it above. No need to do it twice
                     EnableBackingStoreForSerializationRegistry(SerializationWriterFactoryRegistry.DefaultInstance);
             }
+            if(result is BackingStoreSerializationWriterProxyFactory)
+                //We are already enabled so use it.
+                return result;
             else
                 result = new BackingStoreSerializationWriterProxyFactory(original);
 
@@ -69,6 +72,9 @@ public static IParseNodeFactory EnableBackingStoreForParseNodeFactory(IParseNode
                 if(registry != ParseNodeFactoryRegistry.DefaultInstance)// if the registry is the default instance, we already enabled it above. No need to do it twice
                     EnableBackingStoreForParseNodeRegistry(ParseNodeFactoryRegistry.DefaultInstance);
             }
+            if(result is BackingStoreParseNodeFactory)
+                //We are already enabled so use it.
+                return result;
             else
                 result = new BackingStoreParseNodeFactory(original);
 
@@ -80,7 +86,7 @@ private static void EnableBackingStoreForParseNodeRegistry(ParseNodeFactoryRegis
             var keysToUpdate = new List<string>();
             foreach(var entry in registry.ContentTypeAssociatedFactories)
             {
-                if(entry.Value is not (BackingStoreSerializationWriterProxyFactory or SerializationWriterFactoryRegistry))
+                if(entry.Value is not BackingStoreParseNodeFactory)
                 {
                     keysToUpdate.Add(entry.Key);
                 }
@@ -97,7 +103,7 @@ private static void EnableBackingStoreForSerializationRegistry(SerializationWrit
             var keysToUpdate = new List<string>();
             foreach(var entry in registry.ContentTypeAssociatedFactories)
             {
-                if(entry.Value is not (BackingStoreSerializationWriterProxyFactory or SerializationWriterFactoryRegistry))
+                if(entry.Value is not BackingStoreSerializationWriterProxyFactory)
                 {
                     keysToUpdate.Add(entry.Key);
                 }

tests/abstractions/ApiClientBuilderTests.cs

@@ -62,6 +62,45 @@ public void EnableBackingStoreForParseNodeFactory()
             Assert.IsType<BackingStoreParseNodeFactory>(parseNodeRegistry.ContentTypeAssociatedFactories[StreamContentType]);
         }
 
+        [Fact]
+        public void EnableBackingStoreForParseNodeFactoryMultipleCallsDoesNotDoubleWrap()
+        {
+            // Arrange
+            //it is not normal to test a private field, but it is the purpose of the test
+            var concreteFieldInfo = typeof(ParseNodeProxyFactory)
+                                        .GetField("_concrete", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance);
+
+
+            var parseNodeRegistry = new ParseNodeFactoryRegistry();
+            var mockParseNodeFactory = new Mock<IAsyncParseNodeFactory>();
+            parseNodeRegistry.ContentTypeAssociatedFactories.TryAdd(StreamContentType, mockParseNodeFactory.Object);
+
+            Assert.IsNotType<BackingStoreParseNodeFactory>(parseNodeRegistry.ContentTypeAssociatedFactories[StreamContentType]);
+
+            // Act
+            var firstResult = ApiClientBuilder.EnableBackingStoreForParseNodeFactory(parseNodeRegistry);
+            var secondResult = ApiClientBuilder.EnableBackingStoreForParseNodeFactory(firstResult);
+            var thirdResult = ApiClientBuilder.EnableBackingStoreForParseNodeFactory(secondResult);
+
+
+            //make sure the original was not modifed
+            Assert.IsNotType<BackingStoreParseNodeFactory>(parseNodeRegistry);
+
+            // Assert the type has changed due to backing store enabling
+            Assert.IsType<BackingStoreParseNodeFactory>(firstResult);
+
+            //make sure the second call returned the original wrapper
+            Assert.Equal(firstResult, secondResult);
+            Assert.Equal(firstResult, thirdResult);
+
+            //make sure what is in the registry is a BackingStore
+            var factory = parseNodeRegistry.ContentTypeAssociatedFactories[StreamContentType];
+            Assert.IsType<BackingStoreParseNodeFactory>(factory);
+
+            //make sure the concrete version of the factory is the same as the orginal
+            Assert.Equal(mockParseNodeFactory.Object, concreteFieldInfo!.GetValue(factory));
+        }
+
         [Fact]
         public void EnableBackingStoreForParseNodeFactoryAlsoEnablesForDefaultInstance()
         {
@@ -80,5 +119,38 @@ public void EnableBackingStoreForParseNodeFactoryAlsoEnablesForDefaultInstance()
             Assert.IsType<BackingStoreParseNodeFactory>(parseNodeRegistry.ContentTypeAssociatedFactories[StreamContentType]);
             Assert.IsType<BackingStoreParseNodeFactory>(ParseNodeFactoryRegistry.DefaultInstance.ContentTypeAssociatedFactories[StreamContentType]);
         }
+
+        [Fact]
+        public void EnableBackingStoreForParseNodeFactoryAlsoEnablesForDefaultInstanceMultipleCallsDoesNotDoubleWrap()
+        {
+            // Arrange
+            var parseNodeRegistry = new ParseNodeFactoryRegistry();
+            var mockParseNodeFactory = new Mock<IAsyncParseNodeFactory>();
+            parseNodeRegistry.ContentTypeAssociatedFactories.TryAdd(StreamContentType, mockParseNodeFactory.Object);
+            ParseNodeFactoryRegistry.DefaultInstance.ContentTypeAssociatedFactories.TryAdd(StreamContentType, mockParseNodeFactory.Object);
+
+            Assert.IsNotType<BackingStoreParseNodeFactory>(parseNodeRegistry.ContentTypeAssociatedFactories[StreamContentType]);
+
+            // Act
+            var firstResult = ApiClientBuilder.EnableBackingStoreForParseNodeFactory(parseNodeRegistry);
+            var secondResult = ApiClientBuilder.EnableBackingStoreForParseNodeFactory(firstResult);
+            var thirdResult = ApiClientBuilder.EnableBackingStoreForParseNodeFactory(secondResult);
+
+
+            //make sure the original was not modifed
+            Assert.IsNotType<BackingStoreParseNodeFactory>(parseNodeRegistry);
+
+            // Assert the type has changed due to backing store enabling
+            Assert.IsType<BackingStoreParseNodeFactory>(firstResult);
+
+            //make sure the second call returned the original wrapper
+            Assert.Equal(firstResult, secondResult);
+            Assert.Equal(firstResult, thirdResult);
+
+            //make sure what is in the registry is a BackingStore, it will be a new object so we can only check the type
+            var factory = ParseNodeFactoryRegistry.DefaultInstance.ContentTypeAssociatedFactories[StreamContentType];
+            Assert.IsType<BackingStoreParseNodeFactory>(factory);
+
+        }
     }
 }