Fixed EnhancedClient UpdateItem operation to make it work on nested attributes as well

[anirudh9391]

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

SET #AMZN_MAPPED_mainAddress = :AMZN_MAPPED_mainAddress
:AMZN_MAPPED_mainAddress -> "AttributeValue(M={state=AttributeValue(S=WA)})"

This operates like a putItem and updates the map with a new state.

Intead modified the update query to

aws dynamodb update-item \
    --table-name TEST_TABLE3 \
    --key '{"id":{"N":"111"}}' \
    --region us-west-2 \
    --update-expression "SET #AMZN_MAPPED_mainAddress.#AMZN_MAPPED_city = :AMZN_MAPPED_mainAddress_city" \
    --expression-attribute-values '{":AMZN_MAPPED_mainAddress_city": {"S": "Salem"}}' \  
    --expression-attribute-names '{"#AMZN_MAPPED_mainAddress": "mainAddress","#AMZN_MAPPED_city":"city"}'

Testing

Unit Tests pass

Screenshots (if appropriate)

Types of changes

• [ x] Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)

[cenedhryn]

The PR also needs a changelog item.

[joviegas]

It will be great if we mention the user experience in the description with these changes since these changes are made in Public classes
Something like

1. Args with ignoreNulls as true
2. Args with ignoreNulls as true and DynamoDBEnhancedRequestConfiguration
   and when to use DynamoDBEnhancedRequestConfigurations values for different value

[natedenh]

Is there any timeline for getting this fix merged? I'm working on a project that would benefit from using this fix.

[anirudh9391]

Is there any timeline for getting this fix merged? I'm working on a project that would benefit from using this fix.

Yes, this fix involves non-trivial changes to behaviour that break backwards compatibility. We are working on getting the required documentation changes to eventually get this merged in.

[joviegas]

I modified it so that an empty map is stored and preserved. Wrote an integ test that validates it

    public class newTest {

    private static final String TABLE_NAME = "anirudkr-update-test-table";
    private static final TableSchema<RecordWithUpdateBehaviors> TABLE_SCHEMA = TableSchema.fromClass(RecordWithUpdateBehaviors.class);

    private static final DynamoDbEnhancedClient ENHANCED_CLIENT = DynamoDbEnhancedClient.builder()
                                                                                        .dynamoDbClient(DynamoDbClient.create())
                                                                                        .extensions(
                                                                                            Stream.concat(
                                                                                                ExtensionResolver.defaultExtensions().stream(),
                                                                                                Stream.of(AutoGeneratedTimestampRecordExtension.create())
                                                                                            ).collect(Collectors.toList())
                                                                                        )
                                                                                        .build();

    private static final DynamoDbTable<RecordWithUpdateBehaviors> MAPPED_TABLE = ENHANCED_CLIENT.table(TABLE_NAME, TABLE_SCHEMA);

    String randomId = "abc";

    @Test
    public void main() {

        RecordWithUpdateBehaviors update = new RecordWithUpdateBehaviors();
        update.setId("abc");
        //update.setLastUpdatedOn(Instant.now());
        //update.setVersion(1L);
        update.setNestedRecord(new NestedRecordWithUpdateBehavior());

        MAPPED_TABLE.updateItem(r -> r.item(update).ignoreNulls(true));

        // Retrieve the element
        DynamoDbClient dynamoDbClient = DynamoDbClient.create();
        RecordWithUpdateBehaviors persistedRecord = MAPPED_TABLE.getItem(r -> r.key(k -> k.partitionValue("abc")));
        GetItemResponse getItemResponse = dynamoDbClient.getItem(GetItemRequest.builder()
                                                                               .key(Collections.singletonMap("id", AttributeValue.fromS(randomId)))
                                                                               .tableName(TABLE_NAME)
                                                                               .build());

        assertThat(persistedRecord.getNestedRecord()).isNull();
        assertThat(getItemResponse.item().get("nestedRecord")).isNotNull();

        assertTrue(getItemResponse.item().get("nestedRecord").toString().equals("AttributeValue(M={nestedTimeAttribute"
                                                                                 + "=AttributeValue(NUL=true), "
                                                                                + "nestedRecord=AttributeValue(NUL=true), attribute=AttributeValue(NUL=true), id=AttributeValue(NUL=true), nestedUpdateBehaviorAttribute=AttributeValue(NUL=true), nestedCounter=AttributeValue(NUL=true), nestedVersionedAttribute=AttributeValue(NUL=true)})"));
    }

[anirudh9391]

When a map is set or updated to an empty map, the same should be sent to DynamoDB. The current behavior supports this. Specifically, when we execute record.setNestedRecord(new NestedRecordWithUpdateBehavior());, it is transmitted as an empty map over the wire and is correctly stored as an empty AttributeValue in DynamoDB.

However, with the recent change, the map is now being set to null, which is incorrect. The test case below demonstrates the issue: previously, getItemResponse.item().get("nestedRecord") would return an empty attribute value map, but now it returns null.

public class TestCaseForEmptyMapUpdate {

    private static final String TABLE_NAME = "joviegas-update-test-table";
    private static final TableSchema<RecordWithUpdateBehaviors> TABLE_SCHEMA = TableSchema.fromClass(RecordWithUpdateBehaviors.class);

    private static final DynamoDbEnhancedClient ENHANCED_CLIENT = DynamoDbEnhancedClient.builder()
                                                                                        .dynamoDbClient(DynamoDbClient.create())
                                                                                        .extensions(
                                                                                            Stream.concat(
                                                                                                ExtensionResolver.defaultExtensions().stream(),
                                                                                                Stream.of(AutoGeneratedTimestampRecordExtension.create())
                                                                                            ).collect(Collectors.toList())
                                                                                        )
                                                                                        .build();

    private static final DynamoDbTable<RecordWithUpdateBehaviors> MAPPED_TABLE = ENHANCED_CLIENT.table(TABLE_NAME, TABLE_SCHEMA);

    public static void main(String[] args) {
        String randomId = UUID.randomUUID().toString();

        RecordWithUpdateBehaviors record = new RecordWithUpdateBehaviors();
        record.setId(randomId);
        record.setNestedRecord(new NestedRecordWithUpdateBehavior());

        MAPPED_TABLE.updateItem(r -> r.item(record).ignoreNulls(true));

        // Retrieve the element
        DynamoDbClient dynamoDbClient = DynamoDbClient.create();
        GetItemResponse getItemResponse = dynamoDbClient.getItem(GetItemRequest.builder()
                                                                               .key(Collections.singletonMap("id", AttributeValue.fromS(randomId)))
                                                                               .tableName(TABLE_NAME)
                                                                               .build());

        System.out.println("Retrieved nestedRecord: " + getItemResponse.item().get("nestedRecord"));
// This should be an empty map , you can test the same in master branch
    }
}

also in cli

aws dynamodb update-item \
    --table-name joviegas-update-test-table \
    --key '{"id": {"S": "clitest"}}' \
    --update-expression "SET #NR = :emptyMap" \
    --expression-attribute-names '{"#NR": "nestedRecord"}' \
    --expression-attribute-values '{":emptyMap": {"M": {}}}' \
    --return-values ALL_NEW -
    ```

Seems like this is testing out a case where the customer uses an enhancedClient for the update but a lower level client for get ? I have modified it so that we store empty map values in the table, but try testing this out using the enhanced client like this

@Test
    public void when_updatingNestedObjectToEmptyWithSingleLevel_existingInformationIsPreserved_ignoreNulls() {

        NestedRecordWithUpdateBehavior nestedRecord = createNestedWithDefaults("id456", 5L);
        nestedRecord.setAttribute(TEST_ATTRIBUTE);

        RecordWithUpdateBehaviors record = new RecordWithUpdateBehaviors();
        record.setId("id123");
        record.setNestedRecord(nestedRecord);

        mappedTable.putItem(record);

        NestedRecordWithUpdateBehavior updatedNestedRecord = new NestedRecordWithUpdateBehavior();

        RecordWithUpdateBehaviors update_record = new RecordWithUpdateBehaviors();
        update_record.setId("id123");
        update_record.setVersion(1L);
        update_record.setNestedRecord(updatedNestedRecord);

        mappedTable.updateItem(r -> r.item(update_record).ignoreNulls(true));

        RecordWithUpdateBehaviors persistedRecord = mappedTable.getItem(r -> r.key(k -> k.partitionValue("id123")));

        assertThat(persistedRecord.getNestedRecord()).isNull();
    }

[joviegas]

Thank you so much for handling the changes. Unfortunately, I wasn't able to test further due to some other issues. Could you kindly confirm if the following cases work as expected?
Updating a nested attribute value for the following types: BinaryAttributeValue, StringSetAttributeValue, NumberSetAttributeValue, BinarySetAttributeValue, ListAttributeValue, NullAttributeValue, and BooleanAttributeValue.

(Covering all possible AttributeValues-Link)

There’s no need to write additional test cases, but a quick integration test would be greatly appreciated.

[sonarqubecloud]

Quality Gate passed

Issues
10 New issues
0 Accepted issues

Measures
0 Security Hotspots
98.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[anirudh9391]

Thank you so much for handling the changes. Unfortunately, I wasn't able to test further due to some other issues. Could you kindly confirm if the following cases work as expected? Updating a nested attribute value for the following types: BinaryAttributeValue, StringSetAttributeValue, NumberSetAttributeValue, BinarySetAttributeValue, ListAttributeValue, NullAttributeValue, and BooleanAttributeValue.

(Covering all possible AttributeValues-Link)

There’s no need to write additional test cases, but a quick integration test would be greatly appreciated.

@Test
    public void nestedRecord() {
        RecordWithUpdateBehaviors item = new RecordWithUpdateBehaviors();
        item.setId("abc");
        item.setVersion(1L);

        MAPPED_TABLE.putItem(r -> r.item(item));*/

        NestedRecordWithUpdateBehavior nested = new NestedRecordWithUpdateBehavior();
        nested.setFlag(false);
        nested.setList(Arrays.asList("a", "b", "c"));
        nested.setNumber(8);
        nested.setNumberSet(new HashSet<>(Arrays.asList(1, 2, 3)));
        nested.setStringSet(new HashSet<>(Arrays.asList("d", "e", "f")));
        nested.setBinary(ByteBuffer.allocateDirect(1));
        nested.setByteList(Arrays.asList(ByteBuffer.allocateDirect(1), ByteBuffer.allocateDirect(1)));

        RecordWithUpdateBehaviors update = new RecordWithUpdateBehaviors();
        update.setId("abc");
        update.setVersion(16L);
        update.setNestedRecord(nested);

        MAPPED_TABLE.updateItem(r -> r.item(update).ignoreNulls(true));

        // Retrieve the element
        DynamoDbClient dynamoDbClient = DynamoDbClient.create();

        GetItemResponse getItemResponse = dynamoDbClient.getItem(GetItemRequest.builder()
                                                                               .key(Collections.singletonMap("id",
                                                                                                             AttributeValue.fromS(randomId)))
                                                                               .tableName(TABLE_NAME)
                                                                               .build());

        assertThat(getItemResponse.item().get("nestedRecord").toString()).isEqualTo("AttributeValue(M={flag=AttributeValue"
                                                                                    + "(BOOL=false), stringSet=AttributeValue"
                                                                                    + "(SS=[d, e, f]), "
                                                                                    + "nestedRecord=AttributeValue(NUL=true), "
                                                                                    + "byteList=AttributeValue"
                                                                                    + "(L=[AttributeValue(B=SdkBytes"
                                                                                    + "(bytes=0x00)), AttributeValue(B=SdkBytes"
                                                                                    + "(bytes=0x00))]), list=AttributeValue"
                                                                                    + "(L=[AttributeValue(S=a), AttributeValue"
                                                                                    + "(S=b), AttributeValue(S=c)]), "
                                                                                    + "nestedUpdateBehaviorAttribute"
                                                                                    + "=AttributeValue(NUL=true), "
                                                                                    + "nestedCounter=AttributeValue(NUL=true), "
                                                                                    + "numberSet=AttributeValue(NS=[3, 2, 1]), "
                                                                                    + "nestedVersionedAttribute=AttributeValue"
                                                                                    + "(NUL=true), number=AttributeValue(N=8), "
                                                                                    + "nestedTimeAttribute=AttributeValue"
                                                                                    + "(NUL=true), binary=AttributeValue"
                                                                                    + "(B=SdkBytes(bytes=0x00)), "
                                                                                    + "attribute=AttributeValue(NUL=true), "
                                                                                    + "id=AttributeValue(NUL=true)})");
    }

[phillipberndt]

This backwards incompatible change broke our existing code that relied on this behavior. I cut #5584 to track that.