DynamoDB Marshaller does not handle undefined property values

[carlnordenfelt]

Describe the bug
The @aws-sdk/util-dynamodb marshaller throws an error if provided an undefined property value.
Version 2 of the equivalent marshaller handled undefined properties gracefully.

It may not necessarily be considered a bug but it is a breaking change that has a rather big impact on existing code relying on marshalling arbitrary objects without first checking for undefined properties.

SDK version number
@aws-sdk/util-dynamodb 3.0.0

Is the issue in the browser/Node.js/ReactNative?
Node.js

Details of the browser/Node.js/ReactNative version
v15.2.1

To Reproduce (observed behavior)
The following code example will throw an error when marshalling with the v3 marshaller.
The v2 marshalling is included to highlight the difference between v2/v3.

const ddbConverter = require('aws-sdk').DynamoDB.Converter;
const { marshall } = require('@aws-sdk/util-dynamodb');

const object = {
    foo: 'string',
    bar: undefined,
};
console.log('Obj', Object.keys(object));

const v2Result = ddbConverter.marshall(object, { convertEmptyValues: true });
console.log('v2Result', Object.keys(v2Result));

const v3Result = marshall(object, { convertEmptyValues: true });
console.log('v3Result', Object.keys(v3Result));

There is a fairly easy to use work-around but it can have some unforeseen consequences:

const v3Result = marshall(JSON.parse(JSON.stringify(object)), { convertEmptyValues: true });

Expected behavior
As per the v2 marshaller, undefined key/values should be ignored.

Somewhat related issue: #1647

[trivikr]

@carlnordenfelt When a value undefined in a JS Object, we explicitly want to call it that value is undefined instead of removing it.

Should marshaller remove keys for undefined values?

[carlnordenfelt]

Hi,

I guess it depends. Throwing an error seems a bit harsh. Keeping the value as undefined would be preferred IMO.
V2 removes the property altogether, keeping it would probably be although I am not sure what DDB would do with it, it would most probably cause an error downstream.

In a way this realtes to the ES6 issue #1535. We sue ES6 classes and we convert these to Dynamo items using the V2 marshaller. What we don't do is check every property of the class for undefined but optimistically push them through the marshaller which gives us a valid DDB Item without the undefined values.

Perhaps, if it is a concern, it could be a config option next to convertEmptyValues? That would also help with the major version change that was added to the ticket.

[trivikr]

Throwing an error seems a bit harsh. Keeping the value as undefined would be preferred IMO.

The DynamoDB AttributeValue doesn't have an undefined value. The closest attribute is of type NULL, and marshaller shouldn't convert undefined to null as they have different meanings in JavaScript.

V2 removes the property altogether

This should be considered as a bug, as the key is already declared but not defined and shouldn't be removed by marshaller. As per MDN:

undefined is a primitive value automatically assigned to variables that have just been declared, or to formal arguments for which there are no actual arguments.

WDYT?

[carlnordenfelt]

It should probably not be converted to NULL, I agree with that.

I've played around with the v2 sdk to see how the behaviour differs, for good and bad, and the following works, the bar property is not created in Dynamo but the Item is successfully persisted.

const aws = require('aws-sdk');
const dynamo = new aws.DynamoDB.DocumentClient();

(async () => {
    const v2Item = {
        'hashKey': 'example',
        bar: undefined,
    };
    await dynamo.put({ TableName: 'tableName', Item: v2Item }).promise();
})();

I tried an equivalent request with V3 PutItemCommand and it throws an error early on before sending the request.

/.../ddb-marshall/node_modules/@aws-sdk/client-dynamodb/dist/cjs/models/models_0.js:1081
        if (value.S !== undefined)
                  ^
TypeError: Cannot read property 'S' of undefined

To be honest, I am not sure what to think.
On the one hand, not having to bother with undefined properties is convenient but on the other hand one could consider the item invalid if it has an undefined property. Maybe this is a decision/manipulation that would be specific to the future DocumentClient that is being discussed in another issue(s).

[devonhumes]

This is also a problem with the types as seen in #1647. I'm adding this here because this is specific to the undefined case. With an interface, I may have optional properties such as

interface Test {
    expireAt?: string;
    actual: string;
}

DynamoDB allows for optional properties, such as TTL properties, but NativeAttributeValue does not. Unfortunately, it's complicated because TypeScript does not differentiate between a value explicitly set to undefined and one which is not provided at all. So while an object like:

const: Test = {
    expireAt: undefined,
    actual: "string"
}

may be invalid an object like:

const: Test = {
    actual: "string"
}

is expected and should be valid. I'm not sure how to handle this, but I've already had to struggle with this because our data model allows for optional properties on some objects.

[trivikr]

Update: We'll prioritize fixing this with v3.2.0 release currently scheduled for Friday 01/08

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.