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

fix: check property existence for excludeFromIndexes with wildcard

Open tanishiking opened this issue 4 years ago • 7 comments

Fixes https://github.com/googleapis/nodejs-datastore/issues/787 🦕

As described in https://github.com/googleapis/nodejs-datastore/issues/787, nodejs-datastore raises TypeError: Cannot read property 'entityValue' of undefined if we try to exclude undefined properties from index using .*.

This is because we didn't check the property existence (e.g. for non_exist_data.*, we should check non_exist_data's existence), so this PR make entity.ts check their existence before traversing data for excludeFromIndexes with a wildcard.


Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • [x] Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • [x] Ensure the tests and linter pass
  • [x] Code coverage does not decrease (if any source code was changed)
  • [x] Appropriate docs were updated (if necessary)

tanishiking avatar Feb 01 '21 11:02 tanishiking

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

google-cla[bot] avatar Feb 01 '21 11:02 google-cla[bot]

Codecov Report

:exclamation: No coverage uploaded for pull request base (main@a5cc245). Click here to learn what that means. The diff coverage is n/a.

:exclamation: Current head e408c19 differs from pull request most recent head 96a4c11. Consider uploading reports for the commit 96a4c11 to get more accurate results

@@           Coverage Diff           @@
##             main     #788   +/-   ##
=======================================
  Coverage        ?   98.97%           
=======================================
  Files           ?       11           
  Lines           ?     8260           
  Branches        ?      486           
=======================================
  Hits            ?     8175           
  Misses          ?       83           
  Partials        ?        2           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov[bot] avatar Feb 01 '21 11:02 codecov[bot]

@googlebot I signed it!

tanishiking avatar Feb 01 '21 11:02 tanishiking

@crwilcox or @stephenplusplus could I trouble y'all to have a look here?

JustinBeckwith avatar Mar 09 '21 03:03 JustinBeckwith

No region tags are edited in this PR.

This comment is generated by snippet-bot. If you find problems with this result, please file an issue at: https://github.com/googleapis/repo-automation-bots/issues. To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • [ ] Refresh this comment

snippet-bot[bot] avatar Jun 10 '21 23:06 snippet-bot[bot]

Can someone please add this fix already? It is quite the detrimental bug.

udnes99 avatar Sep 08 '21 12:09 udnes99

It is a huge shame that this is still broken today in 7.0.0. In fact, two years ago my colleague posted about the same issue and the issue was closed with inaccurate information.

zomgbre avatar Sep 21 '22 19:09 zomgbre

Closed in favour of https://github.com/googleapis/nodejs-datastore/pull/1114.

danieljbruce avatar Oct 02 '23 13:10 danieljbruce