Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
mrnagydavid opened this issue Oct 1, 2024 · 3 comments
Assignees
Labels
api: datastore Issues related to the googleapis/nodejs-datastore API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@mrnagydavid
Copy link
Contributor

mrnagydavid commented Oct 1, 2024

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 mrnagydavid added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Oct 1, 2024
@product-auto-label product-auto-label bot added the api: datastore Issues related to the googleapis/nodejs-datastore API. label Oct 1, 2024
@danieljbruce
Copy link
Contributor

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

@mrnagydavid
Copy link
Contributor Author

@danieljbruce PR is open with the suggested change.

@danieljbruce
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the googleapis/nodejs-datastore API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

2 participants