Added support for @DynamoDbUpdateBehavior on attributes within nested objects #6708
Conversation
anasatirbasa
force-pushed
the
feature/added-support-for-updateBehavior-annotations-in-nested-objects
branch
from
4c585a0 to
6f6f15b
Compare
anasatirbasa
force-pushed
the
feature/added-support-for-updateBehavior-annotations-in-nested-objects
branch
from
6f6f15b to
3e5c890
Compare
| TableSchema<?> elementListSchema = getTableSchemaForListElement(context.tableSchema(), key); | ||
| if (elementListSchema != null) { | ||
| TableSchema<?> cachedSchema = getOrCacheSchema(schemaInstanceCache, elementListSchema); | ||
| Collection<AttributeValue> updatedList = new ArrayList<>(value.l().size()); |
There was a problem hiding this comment.
you can avoid creating new arraylist and create it only when you use it in the loop
| TableSchema<?> listElementSchema = getTableSchemaForListElement(nestedSchema, nestedKey); | ||
| if (listElementSchema != null) { | ||
| TableSchema<?> cachedSchema = getOrCacheSchema(schemaInstanceCache, listElementSchema); | ||
| Collection<AttributeValue> updatedList = new ArrayList<>(nestedValue.l().size()); |
There was a problem hiding this comment.
This is more of a question than ask to change immediately.
So, we are doing eager arraylist creation which will create a new array list (with 8kb I guess) in memory. You can lazy init the array list to avoid that. It will for sure make the implementation a bit more complex but creating so many objects in memory also wouldn't be ideal.
What's your thoughts on this, leave it simple in favor of code readability but sacrifice memory or not ?
} else if (nestedValue.hasL() && !nestedValue.l().isEmpty()) {
AttributeValue firstElement = nestedValue.l().stream()
.filter(Objects::nonNull)
.findFirst()
.orElse(null);
if (firstElement != null && firstElement.hasM()) {
TableSchema<?> listElementSchema = getTableSchemaForListElement(nestedSchema, nestedKey);
if (listElementSchema != null) {
TableSchema<?> cachedSchema = getOrCacheSchema(schemaInstanceCache, listElementSchema);
// LAZY: Start with null
Collection<AttributeValue> updatedList = null;
boolean listModified = false;
List<AttributeValue> originalList = nestedValue.l();
for (int i = 0; i < originalList.size(); i++) {
AttributeValue listItem = originalList.get(i);
if (listItem != null && listItem.hasM()) {
Map<String, AttributeValue> processedMap = processNestedObject(
listItem.m(),
cachedSchema,
currentInstant,
schemaInstanceCache
);
AttributeValue processedItem = AttributeValue.builder().m(processedMap).build();
// Check if this item was modified
if (!Objects.equals(processedItem, listItem)) {
// First modification detected!
if (!listModified) {
// Create list NOW and copy all previous items
updatedList = new ArrayList<>(originalList.size());
for (int j = 0; j < i; j++) {
updatedList.add(originalList.get(j));
}
listModified = true;
}
// Add the modified item
updatedList.add(processedItem);
} else if (listModified) {
// Already modifying, add unchanged item
updatedList.add(listItem);
}
// else: No changes yet, don't create list
} else {
// Handle null or non-map items
if (listModified) {
updatedList.add(listItem);
}
}
}
// Only update if something actually changed
if (listModified) {
if (!updated) {
updatedNestedMap = new HashMap<>(nestedMap);
updated = true;
}
updatedNestedMap.put(nestedKey, AttributeValue.builder().l(updatedList).build());
}
}
}
}| staticSchema.isPresent() | ||
| ? staticSchema.get() | ||
| : TableSchema.fromClass(Class.forName( | ||
| rootSchema.converterForAttribute(key).type().rawClassParameters().get(0).rawClass().getName())); |
There was a problem hiding this comment.
Is there any chance converterForAttribute(key) might return null or rawClassParameters() might return an empty list, causing get(0) to throw IndexOutOfBoundsException ?
|
|
||
| for (int i = 0; i < parts.length - 1; i++) { | ||
| Optional<? extends TableSchema<?>> nestedSchema = getNestedSchema(currentSchema, parts[i]); | ||
| if (nestedSchema.isPresent()) { |
There was a problem hiding this comment.
When nestedSchema.isPresent() is false, currentSchema is not updated, but the loop continues. Doesn't it mean that the subsequent iterations might be working with the wrong schema level ?
| return schemaMap; | ||
| } | ||
|
|
||
| public static String reconstructCompositeKey(String path, String attributeName) { |
| .build(); | ||
| } | ||
|
|
||
| private static TableSchema<?> getOrCacheSchema( |
There was a problem hiding this comment.
Im not sure I understand how this caching helps. We are adding same object as key and value.
// First call with a schema instance
TableSchema<?> schema1 = getNestedSchema(context.tableSchema(), "address");
// schema1 = TableSchema@12345
// Call getOrCacheSchema
schemaInstanceCache.get(schema1); // Returns null (first time)
schemaInstanceCache.put(schema1, schema1); // Stores: {TableSchema@12345 -> TableSchema@12345}
return schema1; // Returns TableSchema@12345
// Second call with THE SAME schema instance
TableSchema<?> schema2 = getNestedSchema(context.tableSchema(), "address");
// schema2 = TableSchema@12345 (SAME INSTANCE!)
// Call getOrCacheSchema again
schemaInstanceCache.get(schema2); // Returns TableSchema@12345 (cache hit!)
return schema2; // Returns TableSchema@12345getNestedSchema() already returns the same instance for the same attribute path. The cache is just checking if we've seen this exact object reference before, but we're passing in the same reference we just got, am i missing something ?
| List<String> pathFieldNames = Arrays.asList(PATTERN.split(key)); | ||
| String attributeName = pathFieldNames.get(pathFieldNames.size() - 1); | ||
|
|
||
| for (int i = 0; i < pathFieldNames.size() - 1; i++) { |
There was a problem hiding this comment.
If getNestedSchema() returns Optional.empty() at any level, the code silently continues with the wrong schema, then currentSchema stays at the previous level UpdateBehaviorTag.resolveForAttribute() is called with potentially wrong schema ? or is this not possible ?
| * @return an {@link Optional} containing the nested attribute's {@link TableSchema}, or empty if unavailable | ||
| */ | ||
| public static Optional<? extends TableSchema<?>> getNestedSchema(TableSchema<?> parentSchema, String attributeName) { | ||
| EnhancedType<?> enhancedType = parentSchema.converterForAttribute(attributeName).type(); |
There was a problem hiding this comment.
do we need any null check on converterForAttribute() result ?
| } | ||
|
|
||
| @DynamoDbUpdateBehavior(UpdateBehavior.WRITE_IF_NOT_EXISTS) | ||
| public Instant getAttr_NESTED_ATTR_UPDATE_() { |
There was a problem hiding this comment.
Fieldname is not matching with getter name, Field is named childAttr_NESTED_ATTR_UPDATE_
Getter is named getAttr_NESTED_ATTR_UPDATE_() (missing "child" prefix), Setter parameter is named attr_NESTED_ATTR_UPDATE_ (different from field name)
Either: Rename field to attr_NESTED_ATTR_UPDATE_ to match getter/setter or rename getter/setter to getChildAttr_NESTED_ATTR_UPDATE_() to match field
| assertThat(result.getId()).isEqualTo("initial_id"); | ||
|
|
||
| // update id (no annotation, defaults to WRITE_ALWAYS) - should change | ||
| result.setId("updated_id"); |
There was a problem hiding this comment.
Are we updating partition key ? if we do, next line should fail. I think this is the case for many other tests below.
Is it possible to update partition key ?
| .setChildList(Arrays.asList( | ||
| new UpdateBehaviorTestModels.SimpleBeanChild().setId("child1").setAttr("attr_child1"), | ||
| new UpdateBehaviorTestModels.SimpleBeanChild().setId("child2").setAttr("attr_child2"))); | ||
| initial.setId("initial_id"); |
There was a problem hiding this comment.
we first set the id to 1 , then initial_id, is there a particular reason or just a typo ?
2 participants
Description
Added support for @DynamoDbUpdateBehavior on attributes within nested objects. The @DynamoDbUpdateBehavior annotation will only take effect for nested attributes when using IgnoreNullsMode.SCALAR_ONLY.
Motivation and Context
@DynamoDbUpdateBehavior to work on nested objects too.
Modifications
Support for
@DynamoDbUpdateBehavioron nested attributes was implemented by accounting for the two possible representations of nested objects during update operations:IgnoreNullsMode.IgnoreNullsMode.SCALAR_ONLYis used, nested attributes are flattened using the internal_NESTED_ATTR_UPDATE_marker. This behavior is handled inUpdateItemOperation.transformItemToMapForUpdateExpression.Both scenarios are now supported, and the same generated timestamp is applied consistently across both top-level and nested attributes.
In parallel, the evaluation of the
IgnoreNullsModeparameter within update requests was reviewed and refined. As part of this change:UpdateExpressionUtilsnow evaluates the@DynamoDbUpdateBehaviorannotation only whenIgnoreNullsMode.SCALAR_ONLYis configured._NESTED_ATTR_UPDATE_marker.@DynamoDbUpdateBehaviorto lists of nested objects is explicitly not supported, as DynamoDB update operations replace the entire list and do not allow updates to individual list elements.Testing
Existing test coverage was updated where necessary, and additional tests were introduced to validate the new behavior across nested object scenarios and different
IgnoreNullsModeconfigurations.Test Coverage Checklist
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License