Repository: jeremydaly/dynamodb-toolbox (ts)

Issue:
`removeNullAttributes` circumvents null checks for required fields
If you mark a field as `required: true` or `required: 'always'`, the `removeNullAttributes` config can still result in null values for that field. You can get dynamodb-toolbox to insert a null value by passing it an empty string or a whitespace-only value and getting it to coerce to `null` under the hood. In this case, the db will remove the attribute without complaint, effectively circumventing the null checks for that field. This even occurs if you set `required` to `'always'` instead of `true`.

Here's an example of a unit test that would fail currently:
```
  it('fails when given a null-like value for an \'always\' required field', () => {
    expect(() => TestEntity3.updateParams({
      'pk': 'test-pk',
      'test2': '',
    })).toThrow(`'test2' is a required field`)
  })
````


This fails with:
```
  - update › fails when given a null-like value for an 'always' required field
    Expected substring: "'test2' is a required field"
    Received function did not throw
```

The resulting behavior is that the key `test2` gets removed from the db, and the application will probably crash because it's expecting to always get a non-null value out of dynamo.

This is related to #252, but would be a separate fix.

Pull request:
Validate that removed fields are not required
Fixes #255
Defining a table attribute with `required: true` should mean that values always exist for that field (barring extraneous factors like schema changes and writes that may not go through this library). Updates may or may not specify a new value, but that new value should not be null, and the field cannot be removed outright.

This fixes two cases of this bug:
1) You can remove a `required: true` value by passing $remove: attr or `$remove: [attr]`
2) You can remove a `required: true` value by passing `attr: ''` if `removeNullAttributes` is set.


Patch:
diff --git a/src/classes/Entity.ts b/src/classes/Entity.ts
index fd29de5d..b5add6ed 100644
--- a/src/classes/Entity.ts
+++ b/src/classes/Entity.ts
@@ -1498,6 +1498,9 @@ class Entity<
                 schema.attributes[attrs[i]].partitionKey === true ? 'partitionKey' : 'sortKey'
               } and cannot be removed`
             )
+          // Verify attribute is not required
+          if (schema.attributes[attrs[i]].required)
+            error(`'${attrs[i]}' is required and cannot be removed`)
           // Grab the attribute name and add to REMOVE and names
           const attr = schema.attributes[attrs[i]].map || attrs[i]
           REMOVE.push(`#${attr}`)
@@ -1508,6 +1511,8 @@ class Entity<
         (data[field] === null || String(data[field]).trim() === '') &&
         (!mapping.link || mapping.save)
       ) {
+        // Verify attribute is not required
+        if (schema.attributes[field].required) error(`'${field}' is required and cannot be removed`)
         REMOVE.push(`#${field}`)
         names[`#${field}`] = field
       } else if (


Test patch:
diff --git a/src/__tests__/type-infering.unit.test.ts b/src/__tests__/type-infering.unit.test.ts
index 700216a9..debc795d 100644
--- a/src/__tests__/type-infering.unit.test.ts
+++ b/src/__tests__/type-infering.unit.test.ts
@@ -1468,39 +1468,42 @@ describe('Entity', () => {
       })
 
       it('throws when trying to delete req/always attr', () => {
-        // 🔨 TOIMPROVE: Not sure this should not throw
-        // @ts-expect-error
-        ent.updateParams({ ...testedParams, reqAttr: null })
+        expect(() =>
+          // @ts-expect-error
+          ent.updateParams({ ...testedParams, reqAttr: null })
+        ).toThrow()
 
-        // 🔨 TOIMPROVE: Not sure this should not throw
-        // @ts-expect-error
-        ent.updateParams({ ...testedParams, $remove: ['reqAttr'] })
+        expect(() => ent.updateParams({ ...testedParams, reqAttr: '' })).toThrow()
 
-        // 🔨 TOIMPROVE: Not sure this should not throw
-        // @ts-expect-error
-        ent.updateParams({ ...testedParams, reqAttrDef: null })
+        expect(() =>
+          // @ts-expect-error
+          ent.updateParams({ ...testedParams, $remove: ['reqAttr'] })
+        ).toThrow()
 
-        // 🔨 TOIMPROVE: Not sure this should not throw
-        // @ts-expect-error
-        ent.updateParams({ ...testedParams, reqAttrDef: null })
+        expect(() =>
+          // @ts-expect-error
+          ent.updateParams({ ...testedParams, reqAttrDef: null })
+        ).toThrow
 
         expect(() =>
           // @ts-expect-error
           ent.updateParams({ ...testedParams, alwAttr: null })
         ).toThrow()
 
-        // 🔨 TOIMPROVE: Not sure this is expected behavior (alwAttr is both set AND removed)
-        // @ts-expect-error
-        ent.updateParams({ ...testedParams, $remove: ['alwAttr'] })
+        expect(() =>
+          // @ts-expect-error
+          ent.updateParams({ ...testedParams, $remove: ['alwAttr'] })
+        ).toThrow()
 
         expect(() =>
           // @ts-expect-error
           ent.updateParams({ ...testedParams, alwAttrDef: null })
         ).toThrow()
 
-        // 🔨 TOIMPROVE: Not sure this is expected behavior (alwAttrDef is both set AND removed)
-        // @ts-expect-error
-        ent.updateParams({ ...testedParams, $remove: ['alwAttrDef'] })
+        expect(() =>
+          // @ts-expect-error
+          ent.updateParams({ ...testedParams, $remove: ['alwAttrDef'] })
+        ).toThrow()
       })
 
       it('throws with bad returnValues parameter', () => {