-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix undefined behaviour in backing store #1558
Changes from 15 commits
51809fa
71aab44
8e9d051
9414f6f
a03bdd8
90c576c
5fe4b45
f859bf5
fe63563
b337cbe
0c19f7a
0e73c97
f23d504
bb43852
0155fb9
36083aa
cde4c39
70bc888
631f1cf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
final 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 (isCollectionValue(wrapper)) { | ||
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,29 +126,37 @@ public void clear() { | |
return result; | ||
} | ||
|
||
private Object getValueFromWrapper(final String entryKey, 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(); | ||
} | ||
private Object getValueFromWrapper(final Pair<Boolean, Object> wrapper) { | ||
if (wrapper == null) { | ||
return null; | ||
} | ||
return null; | ||
if (isCollectionValue(wrapper)) { | ||
Pair<?, ?> collectionTuple = (Pair<?, ?>) wrapper.getValue1(); | ||
return collectionTuple.getValue0(); | ||
} | ||
return wrapper.getValue1(); | ||
} | ||
|
||
@SuppressWarnings("unchecked") | ||
@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) { | ||
if (isCollectionValue(wrapper)) { | ||
ensureCollectionPropertiesAreConsistent(); | ||
hasChanged = this.store.get(key).getValue0(); | ||
} | ||
if (!hasChanged) { | ||
return null; | ||
} | ||
} | ||
|
||
try { | ||
return (T) value; | ||
} catch (ClassCastException ex) { | ||
|
@@ -212,39 +234,61 @@ 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() { | ||
final HashMap<String, Object> currentStoreDirtyCollections = new HashMap<>(); | ||
final 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 (isCollectionValue(wrapper)) { | ||
final Pair<?, Integer> collectionTuple = (Pair<?, Integer>) wrapper.getValue1(); | ||
Object[] items = getObjectArrayFromCollectionWrapper(collectionTuple); | ||
for (Object item : items) { | ||
baywet marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (!(item instanceof BackedModel)) break; | ||
nestedBackedModelsToEnumerate.add((BackedModel) item); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar concern here, the cast will most likely fail if we called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for catching this. Made these updates & added tests. Tests also exposed a bug where we now need to propagate |
||
} | ||
if (collectionTuple.getValue1() | ||
!= items.length) { // and the size has changed since we last updated | ||
currentStoreDirtyCollections.put(entry.getKey(), collectionTuple.getValue0()); | ||
} | ||
} | ||
} | ||
|
||
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" | ||
// Enumerate nested backed models first since they may trigger the parent to be dirty | ||
for (BackedModel nestedBackedModel : nestedBackedModelsToEnumerate) { | ||
Ndiritu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
nestedBackedModel.getBackingStore().enumerate(); | ||
} | ||
|
||
// Only update parent properties that haven't been marked as dirty by the nested models | ||
for (final 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 == null) { | ||
throw new IllegalArgumentException("collectionTuple cannot be null"); | ||
} | ||
if (collectionTuple.getValue0() instanceof Collection) { | ||
final Collection<?> items = (Collection<?>) collectionTuple.getValue0(); | ||
return items.toArray(); | ||
} | ||
if (collectionTuple.getValue0() instanceof Map) { | ||
final Map<?, ?> items = (Map<?, ?>) collectionTuple.getValue0(); | ||
return items.values().toArray(); | ||
} | ||
throw new IllegalArgumentException("collectionTuple must be a Collection or a Map"); | ||
} | ||
|
||
private boolean isCollectionValue(final Pair<Boolean, Object> wrapper) { | ||
return wrapper.getValue1() instanceof Pair; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for this we may need to handle the case for maps in separately as this may be an incorrect assumption for them.
Since
getObjectArrayFromCollectionWrapper
will also unwrap the values of a map as an array, the key values pairs in the map are not guaranteed to have the same type. A map i.e. additionalData can have primitives and complex backed models too..