fix: Check property existence for exclude from indexes with wildcard #1114
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Testing without mocks allows us to look at the code how it is functioning right now so that we can evaluate the intended behavior.
Objects shared by both tests should be moved into the outer block so that they can be shared.
Now the test fails properly and passes properly by introducing callbacks
The failure should be visible to the user and in the current state it is so now by isolating the test it becomes easier to explore solutions that work.
indent a describe block, modify tests to make sure they pass and refactor a function out that easily allows for testing modified parameters.
Add two more tests that address the array case for excludeIndexesFromPath. Also ensure that the conditions which check for these test cases properly ignore exclude from indexes when an array is used
product-auto-label
bot
added
size: m
We should not create a nomocks file because we can just group the tests under a unique describe block of test/index.ts.
…xes-with-wildcard
danieljbruce
added
the
owlbot:run
gcf-owl-bot
bot
removed
the
owlbot:run
ruyadorno
reviewed
May 26, 2023
ruyadorno
reviewed
May 26, 2023
…xes-with-wildcard
…into check-property-existence-for-excludeFromIndexes-with-wildcard
…ldcard' of https://github.com/danieljbruce/nodejs-datastore into check-property-existence-for-excludeFromIndexes-with-wildcard
Refactor the tests so that they are more readable. There is a lot of repeated code in each test which makes comparisons hard.
Replace all tests exercising the feature so that they all use the same test script. This will make the tests way easier to read.
Add parameterized testing to eliminate repeated code. These tests need to be easier to read for the sake of the reviewer.
This function is only used once now. We should inline this code in the test.
In the test title include the properties passed in so that it is easy to track what passed/failed.
Similar variables should be used together. Organize the code so that there is less confusion about which variables are related.
The getExpectedConfig does not need to be used anymore because its code can just be inlined in the one place it is used.
Separate the test into blocks and inline some parameters for better test readability.
Stronger typing makes the code easier to read.
Don’t assign to the Datastore variable. This is not necessary. Just use the OriginalDatastore variable directly.
The diff will be easier to read if we do not add a nested if statement and instead apply a condition on the if and change the else to else if.
Apply the first suggestion in the PR to all examples
There is a repeated code fragment for checking if the first path part is undefined that we should refactor.
This reverts commit a101f27.
This reverts commit 4f54d94.
This reverts commit df3f376.
We do four checks to see if the first path part is defined. We should refactor these checks out.
The comment should reflect the next line of code.
danieljbruce
added
the
owlbot:run
gcf-owl-bot
bot
removed
the
owlbot:run
ruyadorno
reviewed
Sep 29, 2023
test/index.ts
Outdated
Comment on lines
2229
to
2231
| it(`should pass the right properties to upsert on save with parameters: ${JSON.stringify( | ||
| onSaveTest | ||
| )}`, async () => { |
There was a problem hiding this comment.
What if we added a description property to each onSaveTest object with a human-readable, clear description of each scenario (it's probably fine to just reuse the wording for the comments on top of each object definition) and use it here instead of just dropping the blob of JSON.stringify() which I'm afraid will get really unreadable and harder to maintain with time.
There was a problem hiding this comment.
Sounds good. Then we can use the description as the test name.
…into check-property-existence-for-excludeFromIndexes-with-wildcard # Conflicts: # test/index.ts
Replace comments with description property so that the tests will report the description when running the tests instead of the properties in the description.
…xes-with-wildcard
danieljbruce
added
the
owlbot:run
gcf-owl-bot
bot
removed
the
owlbot:run
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes:
Adds a check in three places for the existence of a property in the path provided for exclude from indexes. Also adds test cases to explore what happens when a property is not contained in exclude from indexes, when the property is contained and when it is contained with an array in the path.
Take a look at the changes in the
srcfolder. All the changes in the PR involve adding an additional check to see ifentity.properties![firstPathPart]is defined before accessing a property ofentity.properties![firstPathPart]. That solves the issue users are seeing where the code throws an error. Instead it just doesn't explore the non-existent property path further and it doesn't addexcludeFromIndexes = trueto any of the entity properties.The exact test case corresponding to the issue the user reported is at https://github.com/googleapis/nodejs-datastore/pull/1114/files#diff-177d97aec07fd5b22f04049b34d74939b1d8364a7a83e8fb69ee3ee8695105fbR2187-R2198, but similar test cases where the same error would occur were added and source code changes were made to address those test cases.
Fixes #787