-
Notifications
You must be signed in to change notification settings - Fork 840
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
Fixed EnhancedClient UpdateItem operation to make it work on nested attributes as well #5593
Conversation
…ttributes as well
…e operations on nested DynamoDB object attributes.
// If ignoreNulls is set to true, check for nested params to be updated | ||
// If needed, Transform itemMap for it to be able to handle them. | ||
|
||
Map<String, AttributeValue> itemMap = ignoreNullsMode == IgnoreNullsMode.SCALAR_ONLY ? |
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.
This line is different from the approved PR
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.
So just to confirm, MAPS_ONLY mode is what we originally supported with ignoreNulls == true
, and SCALAR_ONLY
is new support we added?
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.
Yes that is correct. with MAPS_ONLY, essentially a new map is created as part of a every item update regardless of whether its scalar or non-scalar. This does not work for scalar updates, and hence the SCALAR_ONLY
@@ -0,0 +1,32 @@ | |||
/* |
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.
This enum is new
@@ -145,6 +162,355 @@ public void updateBehaviors_transactWriteItems_secondUpdate() { | |||
assertThat(persistedRecord.getCreatedAutoUpdateOn()).isEqualTo(firstUpdatedRecord.getCreatedAutoUpdateOn()); | |||
} | |||
|
|||
@Test |
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 have modified and added more tests
Since we are adding Public APIS for this could you please mention the Public APIS in PR description and user experience in PR description |
verifySingleLevelNestingDefaultUpdateBehavior(persistedRecord.getNestedRecord(), updatedNestedCounter); | ||
} | ||
|
||
@Test |
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.
Can we have parameterized test so that,its easy to find expected behaviors
public static List<Arguments> getDifferentCombinationOfignoreNullsModeAndExpectedResult() {
return Arrays.asList(
Arguments.of(simpleNestedAttrib, enum1, expectedMap1),
Arguments.of(simpleNestedAttrib, enum2, expectedMap1),
Arguments.of(nestedAttribWithEmpty, enum2, expectedMap1),
etc etc
@ParameterizedTest
@MethodSource("getDifferentCombinationOfignoreNullsModeAndExpectedResult")
void testUpdateWithDifferentModes(NestedAttibute attrib, IgnoreNullsMode ignoreNullsMode, Map<String, AttributeVale> expectedUpdate)
{
//existing common code
mappedTable.updateItem(r -> r.item(attrib).ignoreNullsMode(ignoreNullsMode));
Map<String, AttributeVale> valueInDDB = nativeClient.get(item);
assertThat(expectedUpdate).isEqualTo(valueInDDB)
}
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.
This is not going to make it neater. For each case, i.e. simple nested attribute, empty attribute or a multi nested attribute, the validation functions are different. If at all we group some methods together, it will not make it neater.
For eg. updating non-scalar object updates in scalar_only mode throws exception, whereas for maps_only mode has an expected output. Much more like this.
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.
To make it more readable, I have grouped some of the validation methods together in the next revision into :
- verifyMultipleLevelNestingTargetedUpdateBehavior
- verifySingleLevelNestingTargetedUpdateBehavior
/** | ||
* This enum offers users different modes of performing DDB item updates | ||
* <p> | ||
* In the SCALAR_ONLY mode, updates to nested scalar attributes are supported | ||
* <p> | ||
* In the MAPS_ONLY mode, updates to nested map structures are supported | ||
* <p> | ||
* The DEFAULT mode operates by setting ignoreNulls to false, and requires the user to | ||
* fetch existing DDB item, make modifications to it and then update the item | ||
*/ |
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.
This probably needs more detail, especially regarding tradeoffs
import software.amazon.awssdk.annotations.SdkPublicApi; | ||
|
||
/** | ||
* This enum offers users different modes of performing DDB item updates |
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.
different modes of performing DDB item updates
This statement seems too generic
MAPS_ONLY, | ||
DEFAULT |
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.
What is the difference between MAPS_ONLY and DEFAULT? In the code, seems like we behave the same unless its SCALAR_ONLY
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.
Good point. I have changed this and added a unit test.
- MAPS_ONLY - allows users to set non-null values to null valued / nonexistent maps in ignoreNulls = true mode.
- DEFAULT - ignoreNulls = false mode, expecting users to retrieve and modify the item
* <p> | ||
* In the SCALAR_ONLY mode, updates to nested scalar attributes are supported in the ignoreNulls = true mode, i.e. when the user | ||
* wants to update nested scalar attributes by providing only the delta of changes to be updated. This mode does not support | ||
* updates to maps and is expected to throw a 400x DynamoDB exception if done so. |
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.
nit: 400x looks like "400 times" to me. Should this be 4xx
?
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.
Yeah i will change it
// If ignoreNulls is set to true, check for nested params to be updated | ||
// If needed, Transform itemMap for it to be able to handle them. | ||
|
||
Map<String, AttributeValue> itemMap = ignoreNullsMode == IgnoreNullsMode.SCALAR_ONLY ? |
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.
So just to confirm, MAPS_ONLY mode is what we originally supported with ignoreNulls == true
, and SCALAR_ONLY
is new support we added?
Quality Gate passedIssues Measures |
Modified Enhanced Client to facilitate nested update operations.
Motivation and Context
Customer reported an issue where UpdateItem does not work for nested attributes.
Modifications
Modified UpdateItem query path to form the right updateQuery for nested update attributes.
Previously nested update queries looked like
This operates like a putItem and updates the map with a new state.
Intead modified the update query to
Public APIs
Usage :
Usage :
Testing
Unit Tests pass
Screenshots (if appropriate)
Types of changes