aws / aws-sdk-js-v3

Modularized AWS SDK for JavaScript.
Apache License 2.0
3.1k stars 578 forks source link

ReturnValuesOnConditionCheckFailure does not include Item in exception message with DynamoDbDocument #5045

Closed dmeehan1968 closed 1 year ago

dmeehan1968 commented 1 year ago

Checkboxes for prior research

Describe the bug

I'm using DynamoDbDocument as a client, and requesting the 'ALL_OLD' values for ReturnValuesOnConditionCheckFailure does not include the Item in the exception message.

const client = DynamoDBDocument.from(new DynamoDB({}))

try {
    await client.updateItem({
    // .. redacted update details
    ReturnValuesOnConditionCheckFailure: 'ALL_OLD'
} catch (error) {
    if (error instanceof ConditionalCheckFailedException) {
        console.log('Item:', error.Item)   // no item

        // the following dumps out all properties, not just the enumerable ones of the error object, and
        // it doesn't show an error item anywhere.  It's quite lengthy and not sure if it contains sensitive info
        // so not included here.
        console.log('error', Object.getOwnPropertyNames(error).reduce((acc, key) => ({
                            ...acc, [key]: error[key],
                        }), {}))
    }
}

SDK version number

3.382.0

Which JavaScript Runtime is this issue in?

Node.js

Details of the browser/Node.js/ReactNative version

Node v18

Reproduction Steps

This is occuring in a Lambda using Node 18 runtime that handling an IoT Core MQTT event, triggered both from an AWS registered device (thing) and an external network server (Loriot) using AWS IoT Code to publish

Observed Behavior

The server value is not returned in the error object as requested

Expected Behavior

The error.Item property should contain the server side record values

Possible Solution

No response

Additional Information/Context

No response

RanVaknin commented 1 year ago

Hi @dmeehan1968 ,

Thanks for opening this issue. Since your example is redacted, I can't tell if you are passing a ConditionExpression in the params. This param needs to be present with a condition that fails in order to trigger this exception.

Here is my working example:

const { DynamoDBClient, ListTablesCommand, ConditionalCheckFailedException, } = require("@aws-sdk/client-dynamodb");
const { DynamoDBDocumentClient, PutCommand, UpdateCommand } = require("@aws-sdk/lib-dynamodb");

const client = new DynamoDBClient({
    region: "us-east-1",
});

const ddbDocClient = DynamoDBDocumentClient.from(client);

async function updateItem() {
    try {
        const params = {
            TableName: "my-foo-table",
            Key: {
                pk: "testKey", 
            },
            UpdateExpression: "set #value = :value", 
            ExpressionAttributeNames: {
                "#value": "value",
            },
            ExpressionAttributeValues: {
                ":value": "testValue", 
            },
            ConditionExpression: "attribute_not_exists(pk)",
            ReturnValuesOnConditionCheckFailure: "ALL_OLD",
        };

        const data = await ddbDocClient.send(new UpdateCommand(params));
        console.log(data);
    } catch (error) {
        if (error instanceof ConditionalCheckFailedException) {
            console.log('Item:', error.Item); 

            console.log('error', Object.getOwnPropertyNames(error).reduce((acc, key) => ({
                                ...acc, [key]: error[key],
                            }), {}));
        }
    }
}

updateItem();

In this case I already have an item in my table called testKey, and im trying to update it with an expression saying it doesnt exist. This will result in ConditionalCheckFailedException and the item is present in the error:

Item: { pk: { S: 'testKey' }, value: { S: 'testValue' } }
error {
  stack: 'ConditionalCheckFailedException: The conditional request failed\n' +
    '    at de_ConditionalCheckFailedExceptionRes (REDACTED/node_modules/@aws-sdk/client-dynamodb/dist-cjs/protocols/Aws_json1_0.js:2671:23)\n' +
    '    at de_UpdateItemCommandError (REDACTED/node_modules/@aws-sdk/client-dynamodb/dist-cjs/protocols/Aws_json1_0.js:2487:25)\n' +
    '    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)\n' +
    '    at async /REDACTED/node_modules/@smithy/middleware-serde/dist-cjs/deserializerMiddleware.js:7:24\n' +
    '    at async /REDACTED/node_modules/@aws-sdk/lib-dynamodb/dist-cjs/baseCommand/DynamoDBDocumentClientCommand.js:26:34\n' +
    '    at async /REDACTED/node_modules/@aws-sdk/middleware-signing/dist-cjs/awsAuthMiddleware.js:14:20\n' +
    '    at async /REDACTED/node_modules/@smithy/middleware-retry/dist-cjs/retryMiddleware.js:27:46\n' +
    '    at async /REDACTED/node_modules/@aws-sdk/middleware-logger/dist-cjs/loggerMiddleware.js:7:26\n' +
    '    at async updateItem (/REDACTED/sample.js:28:22)',
  message: 'The conditional request failed',
  name: 'ConditionalCheckFailedException',
  '$fault': 'client',
  '$metadata': {
    httpStatusCode: 400,
    requestId: 'REDACTED',
    extendedRequestId: undefined,
    cfId: undefined,
    attempts: 1,
    totalRetryDelay: 0
  },
  Item: { pk: { S: 'testKey' }, value: { S: 'testValue' } },
  __type: 'com.amazonaws.dynamodb.v20120810#ConditionalCheckFailedException',
  ... TRUNCATED ...

I hope this helps. You can also consult the official Dynamo blogpost discussing this new exception.

Thanks again, Ran~

dmeehan1968 commented 1 year ago

@RanVaknin yes, I had a condition and the exception was being thrown but there wasn't no Item in the exception object.

Please note I was using DynamoDBDocument and DynamoDB classes, not the client base classes. Not sure if this makes a difference.

RanVaknin commented 1 year ago

HI @dmeehan1968 ,

yes, I had a condition and the exception was being thrown but there wasn't no Item in the exception object.

Without seeing your complete code , I'm not able to determine the root cause of your issue.

Please note I was using DynamoDBDocument and DynamoDB classes, not the client base classes. Not sure if this makes a difference.

I want to clarify that the example I provided indeed uses the DynamoDBDocumentClient - notice it uses UpdateCommand which is a high level method from the Document Client, instead of the base client method UpdateItemCommand.

You can see the base class implementation of this exact scenario on this comment as I have addressed it just recently.

I'll be happy to look into this if you can provide a complete code snippet.

Thanks, Ran~

dmeehan1968 commented 1 year ago

@RanVaknin I've since refactored the code, since it wasn't working as expected. Here is the content of the update command:

const  updateCommandProps: UpdateCommandInput = {
    TableName: 'Compliance',
    Key: {
        'PK': `DEVEUI#${device.eui}`,
        'SK': `DEVEUI#${device.eui}`,
    },
    ConditionExpression: `attribute_not_exists(#S.#TIMESTAMP) OR #S.#TIMESTAMP < :timestamp`,
    UpdateExpression: 'SET #S = :s, #LAST_UPDATED = :timestamp',
    ExpressionAttributeNames: {
        '#S': `S${sensorData.sensorId}`,
        '#TIMESTAMP': 'timestamp',
        '#LAST_UPDATED': 'lastUpdated',
    },
    ExpressionAttributeValues: {
        ':s': {
            min: sensorData.minC,
            max: sensorData.maxC,
            flowCount: sensorData.flowCount,
            reportCount: sensorData.reportCount,
            timestamp: sensorData.timestamp.getTime(),
            lastCompliance: (sensorData.flowCount ?? 0) > 0 && sensorData.flowCount === sensorData.reportCount ? sensorData.timestamp.getTime() : undefined,
            lastNonCompliance: (sensorData.flowCount ?? 0) > 0 && sensorData.flowCount !== sensorData.reportCount ? sensorData.timestamp.getTime() : undefined,
        },
        ':timestamp': sensorData.timestamp.getTime(),
    },
}

And here is how I was calling it:

try {
    const update = await this.dynamo.update(updateCommandProps)
} catch (error: any) {
    if (error instanceof ConditionalCheckFailedException) {
        this.logger.debug('ConditionalCheckFailed', { error })
        return
    }
}

this.dynamo is typed as DynamoDBDocument and constructed via:

DynamoDBDocument.from(new DynamoDB({}))

Forgive me if I'm misunderstanding, but you seem to be showing me how it works when using DynamoDBDocumentClient, and I'm specifically reporting that its not working with DynamoDBDocument, which inherits from DynamoDBDocumentClient and adds convenience methods like update and marshalling from native types.

My understanding is that it should still return the Item in the ConditionalCheckFailedException but I'm not seeing it (as I explained, I tried several ways of logging to be sure it wasn't just a logging failure, but there was no Item property in the exception.

RanVaknin commented 1 year ago

Hi @dmeehan1968 ,

Thanks for the additional info, this definitely helps.

DynamoDBDocument and DynamoDBDocumentClient are essentially the same as their differences mainly revolve around modularization and style.

With the introduction of v3, there were some major shifts like modularization and other enhancements. We recognized that our customers had grown used to the v2 way of doing things. Instead of forcing everyone to jump into a new syntax we decided to offer both.

You can read more about it in our Getting Started guide, as the very first paragraph addresses those differences.

I hope this helps clarify the differences between our implementation.

With regards to the problem at hand, I noticed that you did not specify ReturnValuesOnConditionCheckFailure: "ALL_OLD" in your request which explains why you are not getting your Item back.

From Dynamo's official blogpost:

By incorporating the ReturnValuesOnConditionCheckFailure parameter, you can reduce additional read operations and simplify error handling. You can now retrieve detailed information directly from the server side when a ConditionalCheckFailedException occurs, providing you with increased efficiency and improved decision-making. To get started, add the new parameter to your PutItem, UpdateItem, or DeleteItem operations and set the value to ALL_OLD

Hopefully this helps. All the best, Ran~

github-actions[bot] commented 1 year ago

This issue has not received a response in 1 week. If you still think there is a problem, please leave a comment to avoid the issue from automatically closing.

MagneticToad commented 1 year ago

I was struggling with this issue for a long time. In the end, the issue was that the SDK provided in the Lambda runtime was an old version that does not support this feature (https://docs.aws.amazon.com/lambda/latest/dg/lambda-runtimes.html). It looks like you need v3.363.0 or newer for v3 or v2.1408.0 or newer for v2. So, I had to npm install a newer SDK version and upload my code as zip rather than relying on the SDK available by default in the runtime.

dmeehan1968 commented 1 year ago

@MagneticToad Ahh, yes, that makes a lot of sense. NodeJsFunction construct automatically excludes @awk-sdk/* modules from the build in favour of those included in the runtime, without reference to the version.

In my case, I'm not sure which version I actually need, I've just been in the habit of upgrading the dependencies as soon as they are available. I might try downgrading to 3.188.0 (documented as the supported version in the Node 18 runtime in the link you gave). I actually did quite a bit of work to reduce my bundle sizes by taking advantage of the included SDK's (when adding my own layers) in which I had assumed that the SDK version would be 'latest' and not a specific version. Many thanks for the heads up.

MagneticToad commented 1 year ago

@dmeehan1968 Sure. Maybe I'm misunderstanding your message, but just to clarify - that link I included shows the SDK available in the runtime, not the version needed for the feature. So the Node.js 18 runtime includes the v3 SDK 3.188.0 by default which does not support the ReturnValuesOnConditionCheckFailure parameter; for that you will need v3.363.0 or newer. For reference, I'm using v2.1440.0 on a Node.js 14 runtime and the feature is working, but it didn't work when I tried to use the SDK available by default (v2.1374.0 for Node.js 14). Yes, I was also making the mistake of assuming the SDK version in the runtime was 'latest'.

github-actions[bot] commented 1 year ago

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.