Repository: jeremydaly/dynamodb-toolbox (ts)

Issue:
Serializing bigint sets mutates the underlying entity schema
This was a surprise, the following test fails:
```ts
  it('converts arrays of bigints to sets', async () => {
    const attr = { type: 'set', setType: 'bigint' }
    const result = validateTypes()(attr, 'attr', [
      BigInt(-1234),
      BigInt('123000000000000000000001')
    ])
    expect(result).toEqual(new Set([
      toDynamoBigInt(BigInt(-1234)),
      toDynamoBigInt(BigInt('123000000000000000000001'))
    ]))
    // Should not mutate the attribute
    expect(attr.setType).toEqual('bigint')
  })
```

It seems that when we call `validateType` on a bigint set, we override the `setType` field to `number`, which then causes subsequent reads to lose precision.

I pinpointed this to `validateTypes.ts:64`
```ts
        if (mapping.setType === 'bigint') {
          mapping.setType = 'number'
          value = Array.from(value).map((n) => toDynamoBigInt(n))
        }
```

I don't see any reason we need to actually set `setType` to number since it does not get used elsewhere in this function. I plan to put up a PR to remove this line, but thought I'd open an issue to help track it in case that fix doesn't pan out.

Pull request description:
fix(lib/validate-types): resolve attr type changed to number when using bigint
Fix #592

Attribute definitions should _never_ be mutated, as this will result in changing the data types returned by future reads, and potentially cause catastrophic loss of precision. This PR removes the mutating code.


Patch:
diff --git a/src/lib/formatItem.ts b/src/lib/formatItem.ts
index 94418990..c557be56 100644
--- a/src/lib/formatItem.ts
+++ b/src/lib/formatItem.ts
@@ -4,13 +4,14 @@
  * @license MIT
  */
 
-import { PureAttributeDefinition } from '../classes/Entity'
+import type { NativeAttributeValue, NativeScalarAttributeValue, NumberValue } from '@aws-sdk/util-dynamodb'
+import type { PureAttributeDefinition } from '../classes/Entity'
 import validateTypes from './validateTypes'
-import { Linked } from './parseEntity'
+import type { Linked } from './parseEntity'
 
 // Convert from DocumentClient values, which may be wrapped sets or numbers,
 // into normal TS values.
-const convertDynamoValues = (value: unknown, attr?: PureAttributeDefinition): unknown => {
+const convertDynamoValues = (value: NativeAttributeValue, attr?: PureAttributeDefinition): unknown => {
   if (value === null) {
     return value
   }
@@ -18,20 +19,28 @@ const convertDynamoValues = (value: unknown, attr?: PureAttributeDefinition): un
   // Unwrap bigint/number sets to regular numbers/bigints
   if (attr && attr.type === 'set') {
     if (attr.setType === 'bigint') {
-      value = Array.from(value as Set<bigint>).map((n: any) => BigInt(n))
+      value = Array.from(value as Set<bigint>).map((n: any) => BigInt(unwrapAttributeValue(n)))
     } else if (attr.setType === 'number') {
-      value = Array.from(value as Set<number>).map((n: any) => Number(n))
+      value = Array.from(value as Set<number>).map((n: any) => Number(unwrapAttributeValue(n)))
     } else {
       value = Array.from(value as Set<unknown>)
     }
   }
 
-  // Convert wrapped number values to bigints
   if (attr && attr.type === 'bigint') {
-    value = BigInt(value as number)
+    value = BigInt(unwrapAttributeValue(value))
   }
+
   if (attr && attr.type === 'number') {
-    value = Number(value as number)
+    value = Number(unwrapAttributeValue(value))
+  }
+
+  return value
+}
+
+const unwrapAttributeValue = (value: NativeAttributeValue):  boolean | number | bigint | string => {
+  if( value?.value !== undefined ) {
+    return value.value
   }
 
   return value
diff --git a/src/lib/validateTypes.ts b/src/lib/validateTypes.ts
index 2fcd69ca..fab69ac4 100644
--- a/src/lib/validateTypes.ts
+++ b/src/lib/validateTypes.ts
@@ -61,7 +61,6 @@ export default () => (mapping: any, field: any, value: any) => {
       if (value instanceof Set) {
 
         if (mapping.setType === 'bigint') {
-          mapping.setType = 'number'
           value = Array.from(value).map((n) => toDynamoBigInt(n))
         }
 
@@ -78,7 +77,6 @@ export default () => (mapping: any, field: any, value: any) => {
         const actualSetType = typeOf(value[0])?.toLowerCase?.()
 
         if (mapping.setType === 'bigint') {
-          mapping.setType = 'number'
           value = value.map((n) => toDynamoBigInt(n))
         }
 


Test patch:
diff --git a/src/__tests__/entity.parse.unit.test.ts b/src/__tests__/entity.parse.unit.test.ts
index 09709e2c..e4aae71f 100644
--- a/src/__tests__/entity.parse.unit.test.ts
+++ b/src/__tests__/entity.parse.unit.test.ts
@@ -2,7 +2,6 @@ import { DocumentClientWithWrappedNumbers } from './bootstrap.test'
 
 import Table from '../classes/Table'
 import Entity from '../classes/Entity'
-import { toDynamoBigInt } from '../lib/utils'
 import { unmarshall } from '@aws-sdk/util-dynamodb'
 
 const TestTable = new Table({
@@ -206,7 +205,8 @@ describe('parse', () => {
 
   it('parses wrapped numbers', () => {
     const wrap = (value: number) =>
-      unmarshall({ valueToUnmarshall: {N: value.toString()} }, { wrapNumbers: true }).valueToUnmarshall.value
+      unmarshall({ valueToUnmarshall: { N: value.toString() } }, { wrapNumbers: true })
+        .valueToUnmarshall.value
 
     const item = TestEntity.parse({
       pk: 'test@test.com',
@@ -226,15 +226,17 @@ describe('parse', () => {
     const item = TestEntity.parse({
       pk: 'test@test.com',
       sk: 'bigint',
-      test_bigint: toDynamoBigInt(BigInt('90071992547409911234')),
+      test_bigint: { value: '99999999999999999999999999999999999999' },
       test_bigint_coerce: '12345'
     })
-    expect(item).toEqual({
-      email: 'test@test.com',
-      test_type: 'bigint',
-      test_bigint: BigInt('90071992547409911234'),
-      test_bigint_coerce: BigInt('12345')
-    })
+    expect(item).toMatchInlineSnapshot(`
+      {
+        "email": "test@test.com",
+        "test_bigint": 99999999999999999999999999999999999999n,
+        "test_bigint_coerce": 12345n,
+        "test_type": "bigint",
+      }
+    `)
   })
 
   it('parses bigint sets', () => {
@@ -242,19 +244,21 @@ describe('parse', () => {
       pk: 'test@test.com',
       sk: 'bigint',
       test_bigint_set_type: new Set([
-        toDynamoBigInt(BigInt('90071992547409911234')),
-        toDynamoBigInt(BigInt('-90071992547409911234')),
+        { value: '90071992547409911234' },
+        { value: '-90071992547409911234' },
         1234
-      ]),
-    })
-    expect(item).toEqual({
-      email: 'test@test.com',
-      test_type: 'bigint',
-      test_bigint_set_type: [
-        BigInt('90071992547409911234'),
-        BigInt('-90071992547409911234'),
-        BigInt(1234),
-      ]
+      ])
     })
+    expect(item).toMatchInlineSnapshot(`
+      {
+        "email": "test@test.com",
+        "test_bigint_set_type": [
+          90071992547409911234n,
+          -90071992547409911234n,
+          1234n,
+        ],
+        "test_type": "bigint",
+      }
+    `)
   })
 })
diff --git a/src/__tests__/formatItem.unit.test.ts b/src/__tests__/formatItem.unit.test.ts
index 1124cf18..c7fc75e3 100644
--- a/src/__tests__/formatItem.unit.test.ts
+++ b/src/__tests__/formatItem.unit.test.ts
@@ -23,6 +23,9 @@ DefaultTable.addEntity(
       set: { type: 'set', setType: 'string', alias: 'set_alias' },
       set_alias2: { type: 'set', setType: 'string', map: 'set2' },
       number: 'number',
+      numberSet: { type: 'set', setType: 'number' },
+      bigint: 'bigint',
+      bigintSet: { type: 'set', setType: 'bigint' },
       list: { type: 'list', alias: 'list_alias' },
       list_alias2: { type: 'list', map: 'list2' },
       test: 'map',
@@ -38,63 +41,95 @@ DefaultTable.addEntity(
   } as const)
 )
 
-// console.log(DefaultTable.User);
-
 describe('formatItem', () => {
   it('formats item with no alias', () => {
-    const result = formatItem()(
-      DefaultTable.User.schema.attributes,
-      DefaultTable.User.linked,
-      { pk: 'test' }
-    )
+    const result = formatItem()(DefaultTable.User.schema.attributes, DefaultTable.User.linked, {
+      pk: 'test'
+    })
     expect(result).toEqual({ pk: 'test' })
   })
 
   it('formats item with alias', () => {
-    const result = formatItem()(
-      DefaultTable.User.schema.attributes,
-      DefaultTable.User.linked,
-      { list: ['test'] }
-    )
+    const result = formatItem()(DefaultTable.User.schema.attributes, DefaultTable.User.linked, {
+      list: ['test']
+    })
     expect(result).toEqual({ list_alias: ['test'] })
   })
 
   it('formats item with mapping', () => {
-    const result = formatItem()(
-      DefaultTable.User.schema.attributes,
-      DefaultTable.User.linked,
-      { list2: ['test'] }
-    )
+    const result = formatItem()(DefaultTable.User.schema.attributes, DefaultTable.User.linked, {
+      list2: ['test']
+    })
     expect(result).toEqual({ list_alias2: ['test'] })
   })
 
   it('formats item with set (alias)', () => {
-    const result = formatItem()(
-      DefaultTable.User.schema.attributes,
-      DefaultTable.User.linked,
-      { set: new Set([1, 2, 3]) }
-    )
+    const result = formatItem()(DefaultTable.User.schema.attributes, DefaultTable.User.linked, {
+      set: new Set([1, 2, 3])
+    })
     expect(result).toEqual({ set_alias: [1, 2, 3] })
   })
 
   it('formats item with set (map)', () => {
-    const result = formatItem()(
-      DefaultTable.User.schema.attributes,
-      DefaultTable.User.linked,
-      { set2: new Set([1, 2, 3]) }
-    )
+    const result = formatItem()(DefaultTable.User.schema.attributes, DefaultTable.User.linked, {
+      set2: new Set([1, 2, 3])
+    })
     expect(result).toEqual({ set_alias2: [1, 2, 3] })
   })
 
   it('formats item with linked fields', () => {
-    const result = formatItem()(
-      DefaultTable.User.schema.attributes,
-      DefaultTable.User.linked,
-      { sk: 'test1#test2' }
-    )
+    const result = formatItem()(DefaultTable.User.schema.attributes, DefaultTable.User.linked, {
+      sk: 'test1#test2'
+    })
     expect(result).toEqual({ sk: 'test1#test2', linked1: 'test1', linked2: 'test2' })
   })
 
+  it('formats item with wrapped numbers', () => {
+    const result = formatItem()(DefaultTable.User.schema.attributes, DefaultTable.User.linked, {
+      number: { value: '1' },
+      numberSet: new Set([
+        {
+          value: '1'
+        },
+        {
+          value: '2'
+        },
+        {
+          value: '3'
+        }
+      ]),
+      bigint: { value: '1' },
+      bigintSet: new Set([
+        {
+          value: '11234567899999999'
+        },
+        {
+          value: '11234567899999999'
+        },
+        {
+          value: '11234567899999999'
+        }
+      ])
+    })
+
+    expect(result).toMatchInlineSnapshot(`
+      {
+        "bigint": 1n,
+        "bigintSet": [
+          11234567899999999n,
+          11234567899999999n,
+          11234567899999999n,
+        ],
+        "number": 1,
+        "numberSet": [
+          1,
+          2,
+          3,
+        ],
+      }
+    `)
+  })
+
   it('specifies attributes to include', () => {
     const result = formatItem()(
       DefaultTable.User.schema.attributes,
@@ -126,11 +161,9 @@ describe('formatItem', () => {
   })
 
   it('formats item with linked aliased composite field', () => {
-    const result = formatItem()(
-      DefaultTable.User.schema.attributes,
-      DefaultTable.User.linked,
-      { composite1: 'test1#test2' }
-    )
+    const result = formatItem()(DefaultTable.User.schema.attributes, DefaultTable.User.linked, {
+      composite1: 'test1#test2'
+    })
     expect(result).toEqual({
       composite1_alias: 'test1#test2',
       linked3: 'test1',
@@ -139,11 +172,9 @@ describe('formatItem', () => {
   })
 
   it('formats item with linked mapped composite field', () => {
-    const result = formatItem()(
-      DefaultTable.User.schema.attributes,
-      DefaultTable.User.linked,
-      { composite2: 'test1#test2' }
-    )
+    const result = formatItem()(DefaultTable.User.schema.attributes, DefaultTable.User.linked, {
+      composite2: 'test1#test2'
+    })
     expect(result).toEqual({
       composite2_alias: 'test1#test2',
       linked5: 'test1',
@@ -152,20 +183,16 @@ describe('formatItem', () => {
   })
 
   it('passes through attribute not specified in entity', () => {
-    const result = formatItem()(
-      DefaultTable.User.schema.attributes,
-      DefaultTable.User.linked,
-      { unspecified: 'value' }
-    )
+    const result = formatItem()(DefaultTable.User.schema.attributes, DefaultTable.User.linked, {
+      unspecified: 'value'
+    })
     expect(result).toEqual({ unspecified: 'value' })
   })
 
   it('passes through null attribute', () => {
-    const result = formatItem()(
-      DefaultTable.User.schema.attributes,
-      DefaultTable.User.linked,
-      { number: null }
-    )
+    const result = formatItem()(DefaultTable.User.schema.attributes, DefaultTable.User.linked, {
+      number: null
+    })
     expect(result).toEqual({ number: null })
   })
 })
diff --git a/src/__tests__/validateTypes.unit.test.ts b/src/__tests__/validateTypes.unit.test.ts
index 9e1079e7..418225c1 100644
--- a/src/__tests__/validateTypes.unit.test.ts
+++ b/src/__tests__/validateTypes.unit.test.ts
@@ -169,7 +169,8 @@ describe('validateTypes', () => {
   })
 
   it('converts arrays of bigints to sets', async () => {
-    const result = validateTypes()({ type: 'set', setType: 'bigint' }, 'attr', [
+    const attr = { type: 'set', setType: 'bigint' }
+    const result = validateTypes()(attr, 'attr', [
       BigInt(-1234),
       BigInt('123000000000000000000001')
     ])
@@ -177,5 +178,7 @@ describe('validateTypes', () => {
       toDynamoBigInt(BigInt(-1234)),
       toDynamoBigInt(BigInt('123000000000000000000001'))
     ]))
+    // Should not mutate the attribute
+    expect(attr.setType).toEqual('bigint')
   })
 })