ReturnValuesOnConditionCheckFailure is not available for non-transactional operations

[arkadiybutermanov]

Describe the bug Since Sep 2023, all write operations support ReturnValuesOnConditionCheckFailure property, however there is no way to set that flag using ElectroDB currently except for transactional operations: https://github.com/tywalch/electrodb/blob/773e0ba6a1d9c557608e7b731eeef80be9502f83/src/entity.js#L1733

It's quite important to us as it allows identifying cause of failure without making separate request.

ElectroDB Version 2.14.1

Expected behavior An additional option translating to ReturnValuesOnConditionCheckFailure (e.g. failureResponse) must be available for all write operations along with the existing response (ReturnValues) option.

[dil-mvallone]

I am able to reproduce the bug. The command is correctly built and the flag is correctly set. However, when processing the response, electrodb ends up discarding the Item property.

I was able to trace the bug down to the ownsItem(item) function in /electrodb/src/entity.js.

this.getName() === item[this.identifiers.entity] && During a transaction, this line resolves to false, because item[this.identifiers.entity] returns an invalid value.

In my example, it should be returning license, but it returns { S: 'license' }.

Some extra information:

  ownsItem(item) {
      console.log('ownsItem', item, this.getName(), item[this.identifiers.entity]  )
    return (
      item &&
      this.getName() === item[this.identifiers.entity] &&
      this.getVersion() === item[this.identifiers.version] &&
      validations.isStringHasLength(item[this.identifiers.entity]) &&
      validations.isStringHasLength(item[this.identifiers.version])
    );
  }

logs

   ownsItem {
      indexedName: { S: 'professional' },
      deleteAt: { N: '1753710692' },
      __edb_e__: { S: 'license' },
      licenseTypeUid: { S: 'professional' },
      GSI2PK: { S: '3#user-1' },
      orgId: { N: '3' },
      entityVersion: { N: '1' },
      createdAt: { S: '2023-11-22T16:24:55.776Z' },
      GSI1PK: { S: 'licenses_by_username#3' },
      GSI2SK: { S: 'professional' },
      GSI1SK: { S: 'professional#professional' },
      name: { S: 'Professional' },
      SK: { S: 'user-1#professional' },
      __edb_v__: { S: '1' },
      PK: { S: 'licenses#3' },
      userUid: { S: 'user-1' }
    } license { S: 'license' }

This is how I run it:

App.transaction
        .write(({ license }) => [
          license
            .update({
              licenseTypeUid: 'professional',
              orgId: 3,
              userUid: 'user-1',
            })
            .set({
              name: 'Professional',
              createdAt: '2023-11-22T16:24:55.776Z',
            })
            .where(notDeletedCondition)
            .commit({
              params: {
                ReturnValuesOnConditionCheckFailure: 'ALL_OLD',
              },
            }),
        ])
        .go()

[tywalch]

Interesting, so the response back is unmarshalled? What version of the sdk is being used to recreate this?

[dil-mvallone]

The response from Electrodb lacks the Item property. We get the exception back with the Code: ConditionCheckFailure but it lacks the Item property.

We are currently using aws-sdk 3.552.0 (its the NODEJS_20_X Lambda runtime). We discussed it with an AWS SDK maintainer and came to the conclusion the bug is not on aws-sdk.

Here is the discussion: https://github.com/aws/aws-sdk-js-v3/issues/6237

Thanks in advance,

[tywalch]

Thank you for all the detail here, I will look into this

[dil-mvallone]

hi @tywalch , where you able to research this issue?

Thanks,

[tywalch]

@dil-mvallone I have and I have some things cooking locally on my end, though not at the pace I'd normally like. I don't think this is a heavy lift outside of my recent availability. Thank you for actively following up, I appreciate the ping!

[dil-mvallone]

hi @tywalch, any progress on this issue? If we were to research and create a PR would you review it and merge it afterwards? Thanks,

[tywalch]

I have not made progress. I did start to look at this, but I quickly realized that it doesn't fit nicely into electro's existing return interface. Could you comment on the expected return interface you'd expect for this type of request? Presumably you'd expect some sort of feedback that request failed but you'd also prefer for the request to not reject and the values to be available similar to how they currently are returned.

[dil-mvallone]

hi @tywalch. I began preparing a PoC to better explain the issue, but I could not reproduce the bug on the PoC. Then I went back to our original issue on our codebase and the bug was not reproducible anymore. I did more research and it seems that version 2.14.2 fixed this issue. I was consistently able to reproduce it on 2.14.1.

We can close this issue for now and I'll create a new one if I find this bug again.

Thanks!