nodejs-datastore icon indicating copy to clipboard operation
nodejs-datastore copied to clipboard

Should not throw TypeError when saving `null` for a field with subproperty index-exclusion

Open mrnagydavid opened this issue 1 year ago • 2 comments

Environment details

  • OS: N/A
  • Node.js version: N/A
  • npm version: N/A
  • @google-cloud/datastore version: v9.1.0, but in earlier versions too

Problem statement

save({ excludeFromIndexes: ['foo.bar'], entity: { sho: 1, foo: null })
// throws TypeError: Cannot read properties of undefined (reading 'properties')

save({ excludeFromIndexes: ['foo[].*'], entity: { sho: 1, foo: null })
// throws TypeError: Cannot read properties of undefined (reading 'values')

Considering the following 4 types of index exclusions:

  1. excludeFromindexes: ['foo.*']
  2. excludeFromindexes: ['foo.bar']
  3. excludeFromindexes: ['foo[].*']
  4. excludeFromindexes: ['foo[].bar']

Supplying { entity: { foo: null } } throws for 2 and 3, but not for 1 and 4.

It should either throw in all cases or in none. I think it should throw in none, and it is easily fixable.

Steps to reproduce

Easily reproducable via unit tests: repo with unit test showcasing error

Proposed solution

// entity.ts

function excludePathFromEntity(entity: EntityProto, path: string) {
  if (!entity) return; // <-- !! fixes 'foo.bar'
  // ...
  } else if (
    firstPathPartIsArray &&
    hasWildCard &&
    remainderPath === '*' &&
    isFirstPathPartDefined
  ) {
    const array = entity.properties![firstPathPart].arrayValue;
    if (!array) return; // <-- !! fixes 'foo[].*'
  // ...

This TypeError can be fixed with 2 lines of code and I am ready to open a PR.

mrnagydavid avatar Oct 01 '24 08:10 mrnagydavid

@mrnagydavid Instead of modifying the test, please add a new test that matches what you changed the old test to. After that it LGTM.

danieljbruce avatar Oct 03 '24 18:10 danieljbruce

@danieljbruce PR is open with the suggested change.

mrnagydavid avatar Oct 10 '24 08:10 mrnagydavid

This issue can be closed now that the user's PR is merged.

danieljbruce avatar Nov 29 '24 15:11 danieljbruce