Issues on update sub property and updating iso dates with set

[WolfWalter]

Describe the bug

We use onetable a lot and had some issues with some more advanced update use-cases. We ran into two issues:

• issue 1: setting a sub property to null throws because the generated DDB update statement tires to remove the sub property and set/overwrite the object
• issue 2: when trying to update a date field with set it throws when isoDates is set to true

To Reproduce

Demo repo with debug.ts: https://github.com/WolfWalter/dynamodb-onetable

Cut/Paste

/*
    debug.ts - Just for debug

    Edit your test case here and invoke via: "jest debug"

    Or run VS Code in the top level directory and just run.
 */
import { Client, Table } from './utils/init'

jest.setTimeout(7200 * 1000)

//  Change with your schema
const schema = {
    version: '0.0.1',
    indexes: {
        primary: {hash: 'pk', sort: 'sk'},
        gs1: {hash: 'gs1pk', sort: 'gs1sk', project: 'all'},
    },
    params: {
        createdField: 'createdAt',
        updatedField: 'updatedAt',
        isoDates: true,
        timestamps: true,
        separator: '#',
        warn: false,
    },
    models: {
        User: {
            pk: { type: String, value: 'user#' },
            sk: { type: String, value: 'user#${email}' },
            email: { type: String, required: true },
            address: {
              type: Object,
              default: {},
              schema: {
                  street: {type: String},
                  city: {type: String},
                  zip: {type: String, required: false},
              },
            },
            updatedAt: {type: Date },
        }
    } as const,
}

//  Change your table params as required
const table = new Table({
    name: 'DebugTable',
    client: Client,
    partial: false,
    schema,
    logger: true,
})

//  This will create a local table
test('Create Table', async () => {
    if (!(await table.exists())) {
        await table.createTable()
        expect(await table.exists()).toBe(true)
    }
})

test('Update sub property to null should not throw?', async () => {
  let User = table.getModel('User')

  // throws "OneTable execute failed \"update\" for \"User\", Invalid UpdateExpression: Two document paths overlap with each other; must remove or rewrite one of these paths; path one: [address, zip], path two: [address]"
  // because it tries to remove the `zip` and set the `address` in the same update
  await expect(
    () => User.update({email: '[email protected]', address: { street: 'aStreet', city: 'aCity', zip: null as unknown as undefined }})
  ).rejects.not.toThrow()
})

test('Update date field with set should not throw', async () => {
  let User = table.getModel('User')

  // throws: Unsupported type passed: Mon Oct 07 2024 16:21:29 GMT+0200 (Central European Summer Time). Pass options.convertClassInstanceToMap=true to marshall typeof object as map attribute. 
  await expect(
    () => User.update({email: '[email protected]'},{ set: { updatedAt: new Date() }})
  ).rejects.not.toThrow()


  // if setting options.convertClassInstanceToMap=true as suggested by the error it gets worse and the date gets set to `Invalid Date`for the local DDB and an empty object/DDB-Map in a real DDB
})

test('Destroy Table', async () => {
    await table.deleteTable('DeleteTableForever')
    expect(await table.exists()).toBe(false)
})

 FAIL  test/debug.ts (6.648 s)
  ✓ Create Table (339 ms)
  ✕ Update sub property to undefined/null should not trow (219 ms)
  ✕ Update date field with set should not throw (26 ms)
  ✓ Destroy Table (28 ms)

  ● Update sub property to undefined/null should not trow

    expect(received).rejects.not.toThrow()

    Error name:    "OneTableError"
    Error message: "OneTable execute failed \"update\" for \"User\", Invalid UpdateExpression: Two document paths overlap with each other; must remove or rewrite one of these paths; path one: [address, zip], path two: [address]"

          684 |                     this.log.error(`OneTable exception in "${op}" on "${model} ${err.message}"`, {err, trace})
          685 |                 }
        > 686 |                 throw new OneTableError(`OneTable execute failed "${op}" for "${model}", ${err.message}`, {
              |                       ^
          687 |                     code,
          688 |                     err,
          689 |                     trace,

          at Table.execute (src/Table.js:686:23)
          at Model.run (src/Model.js:375:22)
          at Model.updateItem (src/Model.js:1064:16)
          at Model.update (src/Model.js:827:16)
          at Object.<anonymous> (test/debug.ts:68:3)

      68 |   await expect(
      69 |     () => User.update({email: '[email protected]', address: { street: 'aStreet', city: 'aCity', zip: null as unknown as undefined }})
    > 70 |   ).rejects.not.toThrow()
         |                 ^
      71 | })
      72 |
      73 | test('Update date field with set should not throw', async () => {

      at Object.toThrow (node_modules/expect/build/index.js:218:22)
      at Object.<anonymous> (test/debug.ts:70:17)

  ● Update date field with set should not throw

    expect(received).rejects.not.toThrow()

    Error name:    "Error"
    Error message: "Unsupported type passed: Fri Oct 18 2024 13:15:28 GMT+0200 (Central European Summer Time). Pass options.convertClassInstanceToMap=true to marshall typeof object as map attribute."

          1074 |                 }
          1075 |             } else {
        > 1076 |                 item = client.marshall(item, options)
               |                               ^
          1077 |             }
          1078 |         } else {
          1079 |             if (Array.isArray(item)) {

          at convertToAttr (node_modules/@aws-sdk/util-dynamodb/dist-cjs/index.js:133:9)
          at node_modules/@aws-sdk/util-dynamodb/dist-cjs/index.js:197:20
          at convertToMapAttrFromEnumerableProps (node_modules/@aws-sdk/util-dynamodb/dist-cjs/index.js:201:5)
          at convertToAttr (node_modules/@aws-sdk/util-dynamodb/dist-cjs/index.js:111:12)
          at Dynamo.marshall (node_modules/@aws-sdk/util-dynamodb/dist-cjs/index.js:307:26)
          at Table.marshall (src/Table.js:1076:31)
          at Expression.command (src/Expression.js:509:29)
          at Model.run (src/Model.js:305:30)
          at Model.updateItem (src/Model.js:1064:27)
          at Model.update (src/Model.js:827:27)
          at test/debug.ts:78:16
          at Object.toThrow (node_modules/expect/build/index.js:202:58)
          at Object.<anonymous> (test/debug.ts:79:17)

      77 |   await expect(
      78 |     () => User.update({email: '[email protected]'},{ set: { updatedAt: new Date() }})
    > 79 |   ).rejects.not.toThrow()
         |                 ^
      80 |
      81 |
      82 |   // if setting options.convertClassInstanceToMap=true as suggested by the error it gets worse and the date gets set to `Invalid Date`for the local DDB and an empty object/DDB-Map in a real DDB

      at Object.toThrow (node_modules/expect/build/index.js:218:22)
      at Object.<anonymous> (test/debug.ts:79:17)

Test Suites: 1 failed, 1 total
Tests:       2 failed, 2 passed, 4 total
Snapshots:   0 total
Time:        6.691 s, estimated 26 s
Ran all test suites matching /debug.ts/i.

Expected behavior

• issue 1: do not try to remove the sub property if the whole field gets updated
• issue 2: convert the Date to an ISO-Date before passing it to DDB

Environment (please complete the following information):

• OS: Ubuntu 24.4 (found the issues in a current node Lambda)
• Node Version: 20
• OneTable Version: 2.7.5
• TypeScript Version: 5.5

[mobsense]

A couple of comments:

Regarding issue 1.

• When you use User.update inside an expect block, you need to use await and async() on the function definition
• Your first update fails because there isn't a "partial: true" to allow partial updates. When false, the entire object is updated and you cannot do partial removals. Also, set "exists: null" to allow an upsert. If you do a create first to ensure the object exists.

Regarding issue 2

• ISO dates are encoded as strings, when you do a "set", you are directly setting the property value. So use the following instead:

  set: {updatedAt: new Date().toISOString()}

[WolfWalter]

Hi @mobsense,
thanks for the feedback.

To use partial in issue 1 is not what I expected, because it works with undefined and I gave the full address. I guess I just hoped that null would be handled like undefined in this case, because onetable knows that it cannot be null and by using false as the the default for nulls.

For issue 2 that's what we implemented after running into it. I didn't realize that set does come without any of the onetable magic.

[mobsense]

In OneTable, undefined and null mean 2 very different things. Null is used to indicate a property that must be deleted. Undefined is exactly the same as completely omitting the property.

Yes, you could argue that "set" should apply the onetable data mapping routines. If you feel strongly about this, I'll change this to an enhancement issue.

[WolfWalter]

Ok, got it. Sorry for responding so late. Yes, applying the mapping routines on set, would be something I really wishes onetable would handle.