fix: check property existence for excludeFromIndexes with wildcard #788

tanishiking · 2021-02-01T11:16:25Z

Fixes #787 🦕

As described in #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:

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
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Fix googleapis#787

google-cla · 2021-02-01T11:16:32Z

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).

📝 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

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

codecov · 2021-02-01T11:17:51Z

Codecov Report

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

❗ 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.

tanishiking · 2021-02-01T11:19:01Z

@googlebot I signed it!

JustinBeckwith · 2021-03-09T03:56:58Z

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

snippet-bot · 2021-06-10T23:59:30Z

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

See https://github.com/googleapis/repo-automation-bots/blob/master/packages/owl-bot/README.md

udnes99 · 2021-09-08T12:10:36Z

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

zomgbre · 2022-09-21T19:14:02Z

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.

danieljbruce

Looks fine, but a small difference is required in the way we test this feature.

danieljbruce · 2023-03-08T15:23:21Z

src/entity.ts

-              });
-            } else {
-              excludePathFromEntity(entity, firstPathPart);
+            if (entity.properties![firstPathPart]) {


So we just check to see that this property is defined before we move further as that prevents the error from being thrown. Makes sense. There seems to be no value in further traversing a path that the entity will not have any corresponding properties for.

danieljbruce · 2023-03-08T15:23:30Z

src/entity.ts

-            excludePathFromEntity(value, '');
-          }
-        });
+        if (entity.properties![firstPathPart]) {


So we just check to see that this property is defined before we move further as that prevents the error from being thrown. Makes sense. There seems to be no value in further traversing a path that the entity will not have any corresponding properties for.

danieljbruce · 2023-03-08T15:27:18Z

src/entity.ts

@@ -857,6 +857,7 @@ export namespace entity {
      if (
        firstPathPartIsArray &&
        // check also if the property in question is actually an array value.
+        entity.properties![firstPathPart] &&


I see no harm in this. If we don't check for this and entity.properties![firstPathPart] ever happens to be undefined then this will throw an error and I can't imagine that being desirable.

danieljbruce · 2023-03-08T15:31:52Z

test/index.ts

@@ -1677,6 +1677,8 @@ describe('Datastore', () => {
            'metadata.otherProperty',
            'metadata.obj.*',
            'metadata.longStringArray[].*',
+            'undefinedData.*',
+            'undefinedArray[].*',


We shouldn't modify the existing test. We should make a new test that tests for these specific values so we don't lose any existing test coverage.

danieljbruce · 2023-10-02T13:48:15Z

Closed in favour of #1114.

fix: check property existence for excludeFromIndexes with wildcard

67261f6

Fix googleapis#787

tanishiking requested a review from a team as a code owner February 1, 2021 11:16

product-auto-label bot added the api: datastore Issues related to the googleapis/nodejs-datastore API. label Feb 1, 2021

google-cla bot added the cla: no This human has *not* signed the Contributor License Agreement. label Feb 1, 2021

google-cla bot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Feb 1, 2021

Merge branch 'master' into check-property-for-wildcard

a5b30d3

JustinBeckwith and others added 2 commits June 8, 2021 12:45

Merge branch 'master' into check-property-for-wildcard

06f9249

Merge branch 'master' into check-property-for-wildcard

de98ad1

bcoe mentioned this pull request Jun 11, 2021

snippet-bot: pull request that has never touched region tags should not have comment added googleapis/repo-automation-bots#2021

Closed

bcoe added the owlbot:run Add this label to trigger the Owlbot post processor. label Jul 2, 2021

🦉 Updates from OwlBot

cfa33e9

See https://github.com/googleapis/repo-automation-bots/blob/master/packages/owl-bot/README.md

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jul 2, 2021

bcoe and others added 2 commits September 24, 2021 17:52

Merge branch 'main' into check-property-for-wildcard

76639d1

Merge branch 'main' into check-property-for-wildcard

96a4c11

meredithslota requested a review from a team as a code owner August 24, 2022 16:45

product-auto-label bot added the size: s Pull request size is small. label Aug 24, 2022

google-cla bot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Aug 24, 2022

meredithslota added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Aug 24, 2022

google-cla bot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Aug 24, 2022

danieljbruce reviewed Mar 8, 2023

View reviewed changes

danieljbruce closed this Oct 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: check property existence for excludeFromIndexes with wildcard #788

fix: check property existence for excludeFromIndexes with wildcard #788

tanishiking commented Feb 1, 2021 •

edited

Loading

google-cla bot commented Feb 1, 2021

codecov bot commented Feb 1, 2021 •

edited

Loading

tanishiking commented Feb 1, 2021

JustinBeckwith commented Mar 9, 2021

snippet-bot bot commented Jun 10, 2021

udnes99 commented Sep 8, 2021

zomgbre commented Sep 21, 2022

danieljbruce left a comment

danieljbruce Mar 8, 2023

danieljbruce Mar 8, 2023

danieljbruce Mar 8, 2023

danieljbruce Mar 8, 2023

danieljbruce commented Oct 2, 2023

fix: check property existence for excludeFromIndexes with wildcard #788

fix: check property existence for excludeFromIndexes with wildcard #788

Conversation

tanishiking commented Feb 1, 2021 • edited Loading

google-cla bot commented Feb 1, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

codecov bot commented Feb 1, 2021 • edited Loading

Codecov Report

tanishiking commented Feb 1, 2021

JustinBeckwith commented Mar 9, 2021

snippet-bot bot commented Jun 10, 2021

No region tags are edited in this PR.

udnes99 commented Sep 8, 2021

zomgbre commented Sep 21, 2022

danieljbruce left a comment

Choose a reason for hiding this comment

danieljbruce Mar 8, 2023

Choose a reason for hiding this comment

danieljbruce Mar 8, 2023

Choose a reason for hiding this comment

danieljbruce Mar 8, 2023

Choose a reason for hiding this comment

danieljbruce Mar 8, 2023

Choose a reason for hiding this comment

danieljbruce commented Oct 2, 2023

tanishiking commented Feb 1, 2021 •

edited

Loading

codecov bot commented Feb 1, 2021 •

edited

Loading