Fix undefined behaviour in backing store

CHANGELOG.md

@@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+## [1.4.0] - 2024-09-11
+
+### Changed
+
+- Fix InMemoryBackingStore by preventing updates to underlying store's Map while iterating over it [#2106](https://github.com/microsoftgraph/msgraph-sdk-java/issues/2106)
+- Use concurrent HashMap for In memory backing store registry to avoid race conditions.
+
 ## [1.3.0] - 2024-08-22
 
 ### Changed

components/abstractions/spotBugsExcludeFilter.xml

@@ -52,7 +52,10 @@ xsi:schemaLocation="https://github.com/spotbugs/filter/3.0.0 https://raw.githubu
 	</Match>
     <Match>
         <Bug pattern="EI_EXPOSE_REP" />
-        <Class name="com.microsoft.kiota.TestEntity" />
+        <Or>
+            <Class name="com.microsoft.kiota.TestEntity" />
+            <Class name="com.microsoft.kiota.BaseCollectionPaginationCountResponse" />
+        </Or>
     </Match>
     <Match>
         <Bug pattern="NP_LOAD_OF_KNOWN_NULL_VALUE" />

components/abstractions/src/main/java/com/microsoft/kiota/store/InMemoryBackingStore.java

@@ -12,6 +12,7 @@
 import java.util.Map;
 import java.util.Objects;
 import java.util.UUID;
+import java.util.concurrent.ConcurrentHashMap;
 
 /** In-memory implementation of the backing store. Allows for dirty tracking of changes. */
 public class InMemoryBackingStore implements BackingStore {
@@ -48,24 +49,37 @@ public Pair<A, B> setValue1(B value1) {
 
     private boolean isInitializationCompleted = true;
     private boolean returnOnlyChangedValues;
-    private final Map<String, Pair<Boolean, Object>> store = new HashMap<>();
+    private final Map<String, Pair<Boolean, Object>> store = new ConcurrentHashMap<>();
     private final Map<String, TriConsumer<String, Object, Object>> subscriptionStore =
-            new HashMap<>();
+            new ConcurrentHashMap<>();
 
     public void setIsInitializationCompleted(final boolean value) {
         this.isInitializationCompleted = value;
+        ensureCollectionPropertiesAreConsistent();
         for (final Map.Entry<String, Pair<Boolean, Object>> entry : this.store.entrySet()) {
             final Pair<Boolean, Object> wrapper = entry.getValue();
+            Pair<Boolean, Object> updatedValue = new Pair<>(!value, wrapper.getValue1());
+            entry.setValue(updatedValue);
+
             if (wrapper.getValue1() instanceof BackedModel) {
                 BackedModel backedModel = (BackedModel) wrapper.getValue1();
                 backedModel
                         .getBackingStore()
                         .setIsInitializationCompleted(value); // propagate initialization
             }
-            ensureCollectionPropertyIsConsistent(
-                    entry.getKey(), this.store.get(entry.getKey()).getValue1());
-            final Pair<Boolean, Object> updatedValue = wrapper.setValue0(!value);
-            entry.setValue(updatedValue);
+            if (wrapper.getValue1() instanceof Pair) {
+                final Pair<?, Integer> collectionTuple = (Pair<?, Integer>) wrapper.getValue1();
+                Object[] items = getObjectArrayFromCollectionWrapper(collectionTuple);
+
+                for (final Object item : items) {
+                    if (!(item instanceof BackedModel)) break;
+
+                    BackedModel backedModel = (BackedModel) item;
+                    backedModel
+                            .getBackingStore()
+                            .setIsInitializationCompleted(value); // propagate initialization
+                }
+            }
         }
     }
 
@@ -89,10 +103,10 @@ public void clear() {
         final Map<String, Object> result = new HashMap<>();
         for (final Map.Entry<String, Pair<Boolean, Object>> entry : this.store.entrySet()) {
             final Pair<Boolean, Object> wrapper = entry.getValue();
-            final Object value = this.getValueFromWrapper(entry.getKey(), wrapper);
+            final Object value = this.get(entry.getKey());
 
             if (value != null) {
-                result.put(entry.getKey(), wrapper.getValue1());
+                result.put(entry.getKey(), value);
             } else if (Boolean.TRUE.equals(wrapper.getValue0())) {
                 result.put(entry.getKey(), null);
             }
@@ -112,20 +126,13 @@ public void clear() {
         return result;
     }
 
-    private Object getValueFromWrapper(final String entryKey, final Pair<Boolean, Object> wrapper) {
+    private Object getValueFromWrapper(final Pair<Boolean, Object> wrapper) {
         if (wrapper != null) {
-            final Boolean hasChanged = wrapper.getValue0();
-            if (!this.returnOnlyChangedValues || Boolean.TRUE.equals(hasChanged)) {
-                if (Boolean.FALSE.equals(
-                        hasChanged)) { // no need property has already been flagged.
-                    ensureCollectionPropertyIsConsistent(entryKey, wrapper.getValue1());
-                }
-                if (wrapper.getValue1() instanceof Pair) {
-                    Pair<?, ?> collectionTuple = (Pair<?, ?>) wrapper.getValue1();
-                    return collectionTuple.getValue0();
-                }
-                return wrapper.getValue1();
+            if (wrapper.getValue1() instanceof Pair) {
+                Pair<?, ?> collectionTuple = (Pair<?, ?>) wrapper.getValue1();
+                return collectionTuple.getValue0();
             }
+            return wrapper.getValue1();
         }
         return null;
     }
@@ -134,7 +141,20 @@ private Object getValueFromWrapper(final String entryKey, final Pair<Boolean, Ob
     @Nullable public <T> T get(@Nonnull final String key) {
         Objects.requireNonNull(key);
         final Pair<Boolean, Object> wrapper = this.store.get(key);
-        final Object value = this.getValueFromWrapper(key, wrapper);
+        if (wrapper == null) {
+            return null;
+        }
+        final Object value = this.getValueFromWrapper(wrapper);
+
+        boolean hasChanged = wrapper.getValue0();
+        if (getReturnOnlyChangedValues() && !hasChanged) {
+            ensureCollectionPropertiesAreConsistent();
+            hasChanged = this.store.get(key).getValue0();
+            if (!hasChanged) {
+                return null;
+            }
+        }
+
         try {
             return (T) value;
         } catch (ClassCastException ex) {
@@ -212,39 +232,50 @@ private void setupNestedSubscriptions(
         }
     }
 
-    private void ensureCollectionPropertyIsConsistent(final String key, final Object storeItem) {
-        if (storeItem instanceof Pair) { // check if we put in a collection annotated with the size
-            final Pair<?, Integer> collectionTuple = (Pair<?, Integer>) storeItem;
-            Object[] items;
-            if (collectionTuple.getValue0() instanceof Collection) {
-                items = ((Collection<Object>) collectionTuple.getValue0()).toArray();
-            } else { // it is a map
-                items = ((Map<?, Object>) collectionTuple.getValue0()).values().toArray();
-            }
+    private void ensureCollectionPropertiesAreConsistent() {
+        HashMap<String, Object> currentStoreDirtyCollections = new HashMap<>();
+        List<BackedModel> nestedBackedModelsToEnumerate = new ArrayList<>();
 
-            for (final Object item : items) {
-                touchNestedProperties(item); // call get on nested properties
+        for (final Map.Entry<String, Pair<Boolean, Object>> entry : this.store.entrySet()) {
+            final Pair<Boolean, Object> wrapper = entry.getValue();
+            if (wrapper.getValue1() instanceof Pair) {
+                final Pair<?, Integer> collectionTuple = (Pair<?, Integer>) wrapper.getValue1();
+                Object[] items = getObjectArrayFromCollectionWrapper(collectionTuple);
+                for (Object item : items) {
+                    if (!(item instanceof BackedModel)) break;
+                    nestedBackedModelsToEnumerate.add((BackedModel) item);
+                }
+                if (collectionTuple.getValue1()
+                        != items.length) { // and the size has changed since we last updated
+                    currentStoreDirtyCollections.put(entry.getKey(), collectionTuple.getValue0());
+                }
             }
+        }
+
+        // Enumerate nested backed models first since they may trigger the parent to be dirty
+        for (BackedModel nestedBackedModel : nestedBackedModelsToEnumerate) {
+            nestedBackedModel.getBackingStore().enumerate();
+        }
 
-            if (collectionTuple.getValue1()
-                    != items.length) { // and the size has changed since we last updated
-                set(
-                        key,
-                        collectionTuple.getValue0()); // ensure the store is notified the collection
-                // property is "dirty"
+        // Only update parent properties that haven't been marked as dirty by the nested models
+        for (Map.Entry<String, Object> entry : currentStoreDirtyCollections.entrySet()) {
+            // Always set() if there were no nested models
+            if (nestedBackedModelsToEnumerate.isEmpty()) {
+                set(entry.getKey(), entry.getValue());
+                continue;
+            }
+            boolean hasChanged = this.store.get(entry.getKey()).getValue0();
+            if (!hasChanged) {
+                set(entry.getKey(), entry.getValue());
             }
         }
-        touchNestedProperties(storeItem); // call get on nested properties
     }
 
-    private void touchNestedProperties(final Object nestedObject) {
-        if (nestedObject instanceof BackedModel) {
-            // Call Get<>() on nested properties so that this method may be called recursively to
-            // ensure collections are consistent
-            final BackedModel backedModel = (BackedModel) nestedObject;
-            for (final String itemKey : backedModel.getBackingStore().enumerate().keySet()) {
-                backedModel.getBackingStore().get(itemKey);
-            }
+    private Object[] getObjectArrayFromCollectionWrapper(final Pair<?, Integer> collectionTuple) {
+        if (collectionTuple.getValue0() instanceof Collection) {
+            return ((Collection<Object>) collectionTuple.getValue0()).toArray();
+        } else { // it is a map
+            return ((Map<?, Object>) collectionTuple.getValue0()).values().toArray();
         }
     }
 }

components/abstractions/src/test/java/com/microsoft/kiota/BaseCollectionPaginationCountResponse.java

@@ -0,0 +1,133 @@
+package com.microsoft.kiota;
+
+import com.microsoft.kiota.serialization.AdditionalDataHolder;
+import com.microsoft.kiota.serialization.Parsable;
+import com.microsoft.kiota.serialization.ParseNode;
+import com.microsoft.kiota.serialization.SerializationWriter;
+import com.microsoft.kiota.store.BackedModel;
+import com.microsoft.kiota.store.BackingStore;
+import com.microsoft.kiota.store.BackingStoreFactorySingleton;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+
+public class BaseCollectionPaginationCountResponse
+        implements AdditionalDataHolder, BackedModel, Parsable {
+
+    /**
+     * Stores model information.
+     */
+    @jakarta.annotation.Nonnull protected BackingStore backingStore;
+
+    /**
+     * Instantiates a new {@link BaseCollectionPaginationCountResponse} and sets the default values.
+     */
+    public BaseCollectionPaginationCountResponse() {
+        this.backingStore = BackingStoreFactorySingleton.instance.createBackingStore();
+        this.setAdditionalData(new HashMap<>());
+    }
+
+    /**
+     * Creates a new instance of the appropriate class based on discriminator value
+     * @param parseNode The parse node to use to read the discriminator value and create the object
+     * @return a {@link BaseCollectionPaginationCountResponse}
+     */
+    @jakarta.annotation.Nonnull public static BaseCollectionPaginationCountResponse createFromDiscriminatorValue(
+            @jakarta.annotation.Nonnull final ParseNode parseNode) {
+        Objects.requireNonNull(parseNode);
+        return new BaseCollectionPaginationCountResponse();
+    }
+
+    /**
+     * Gets the AdditionalData property value. Stores additional data not described in the OpenAPI description found when deserializing. Can be used for serialization as well.
+     * @return a {@link Map<String, Object>}
+     */
+    @jakarta.annotation.Nonnull public Map<String, Object> getAdditionalData() {
+        Map<String, Object> value = this.backingStore.get("additionalData");
+        if (value == null) {
+            value = new HashMap<>();
+            this.setAdditionalData(value);
+        }
+        return value;
+    }
+
+    /**
+     * Gets the backingStore property value. Stores model information.
+     * @return a {@link BackingStore}
+     */
+    @jakarta.annotation.Nonnull public BackingStore getBackingStore() {
+        return this.backingStore;
+    }
+
+    /**
+     * The deserialization information for the current model
+     * @return a {@link Map<String, java.util.function.Consumer<ParseNode>>}
+     */
+    @jakarta.annotation.Nonnull public Map<String, java.util.function.Consumer<ParseNode>> getFieldDeserializers() {
+        final HashMap<String, java.util.function.Consumer<ParseNode>> deserializerMap =
+                new HashMap<String, java.util.function.Consumer<ParseNode>>(2);
+        deserializerMap.put(
+                "@odata.count",
+                n -> {
+                    this.setOdataCount(n.getLongValue());
+                });
+        deserializerMap.put(
+                "@odata.nextLink",
+                n -> {
+                    this.setOdataNextLink(n.getStringValue());
+                });
+        return deserializerMap;
+    }
+
+    /**
+     * Gets the @odata.count property value. The OdataCount property
+     * @return a {@link Long}
+     */
+    @jakarta.annotation.Nullable public Long getOdataCount() {
+        return this.backingStore.get("odataCount");
+    }
+
+    /**
+     * Gets the @odata.nextLink property value. The OdataNextLink property
+     * @return a {@link String}
+     */
+    @jakarta.annotation.Nullable public String getOdataNextLink() {
+        return this.backingStore.get("odataNextLink");
+    }
+
+    /**
+     * Serializes information the current object
+     * @param writer Serialization writer to use to serialize this model
+     */
+    public void serialize(@jakarta.annotation.Nonnull final SerializationWriter writer) {
+        Objects.requireNonNull(writer);
+        writer.writeLongValue("@odata.count", this.getOdataCount());
+        writer.writeStringValue("@odata.nextLink", this.getOdataNextLink());
+        writer.writeAdditionalData(this.getAdditionalData());
+    }
+
+    /**
+     * Sets the AdditionalData property value. Stores additional data not described in the OpenAPI description found when deserializing. Can be used for serialization as well.
+     * @param value Value to set for the AdditionalData property.
+     */
+    public void setAdditionalData(@jakarta.annotation.Nullable final Map<String, Object> value) {
+        this.backingStore.set("additionalData", value);
+    }
+
+    /**
+     * Sets the @odata.count property value. The OdataCount property
+     * @param value Value to set for the @odata.count property.
+     */
+    public void setOdataCount(@jakarta.annotation.Nullable final Long value) {
+        this.backingStore.set("odataCount", value);
+    }
+
+    /**
+     * Sets the @odata.nextLink property value. The OdataNextLink property
+     * @param value Value to set for the @odata.nextLink property.
+     */
+    public void setOdataNextLink(@jakarta.annotation.Nullable final String value) {
+        this.backingStore.set("odataNextLink", value);
+    }
+}

components/abstractions/src/test/java/com/microsoft/kiota/TestEntity.java

@@ -13,6 +13,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.function.Consumer;
 
 public class TestEntity implements Parsable, AdditionalDataHolder, BackedModel {
@@ -107,4 +108,10 @@ public void serialize(@Nonnull SerializationWriter writer) {
         // TODO Auto-generated method stub
 
     }
+
+    public static TestEntity createFromDiscriminatorValue(
+            @jakarta.annotation.Nonnull final ParseNode parseNode) {
+        Objects.requireNonNull(parseNode);
+        return new TestEntity();
+    }
 }