Merge pull request #343 from grippstick/main

The call to ApiClientBuilder.EnableBackingStoreForParseNodeFactory wo…

Author: andrueastman

CHANGELOG.md

@@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+## [1.12.2] - 2024-08-23
+
+### Changed
+
+- Fixed a bug where calls to ApiClientBuilder.EnableBackingStoreForParseNodeFactory and ApiClientBuilder.EnableBackingStoreForSerializationWriterFactory would enable a BackingStore around BackingStores. [#2563] (https://github.com/microsoftgraph/msgraph-sdk-dotnet/issues/2563) [#2588] (https://github.com/microsoftgraph/msgraph-sdk-dotnet/issues/2588)
+
 ## [1.12.1] - 2024-08-21
 
 ### Changed

Directory.Build.props

@@ -1,7 +1,7 @@
 <Project>
   <!-- Common default project properties for ALL projects-->
   <PropertyGroup>
-    <VersionPrefix>1.12.1</VersionPrefix>
+    <VersionPrefix>1.12.2</VersionPrefix>
     <VersionSuffix></VersionSuffix>
     <!-- This is overidden in test projects by setting to true-->
     <IsTestProject>false</IsTestProject>

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);
+
+        }
     }
 }