refactor: Add separate function for adding large properties to the excludeFromIndexes list #1349
Conversation
This file contains hidden or 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
…into 1242-write-tests-for-findLargeProperties-revisit
Against the current source code, this test fails because the code throws an error, but it should pass.
Pull out the code that extends the excludeFromIndexes list
…tps://github.com/googleapis/nodejs-datastore into 376497482-separate-function-for-adding-large-properties
product-auto-label bot added size: l 
danieljbruce changed the base branch from main to exclude-from-indexes-cleanup …into 376497482-separate-function-for-adding-large-properties
…' of https://github.com/googleapis/nodejs-datastore into 376497482-separate-function-for-adding-large-properties
…' of https://github.com/googleapis/nodejs-datastore into 376497482-separate-function-for-adding-large-properties
danieljbruce changed the base branch from exclude-from-indexes-cleanup to main danieljbruce changed the base branch from main to exclude-from-indexes-cleanup 
| * large properties in the entity object. The extended excludeFromIndexes | ||
| * list is then used when building the entity proto. | ||
| */ | ||
| // TODO: Add params |
Contributor Author
There was a problem hiding this comment.
I just pushed updates to address this. This is about adding documentation for the parameters which is done now.
| if (entityObject.excludeLargeProperties) { | ||
| if (Array.isArray(entityObject.data)) { | ||
| // This code populates the excludeFromIndexes list with the right values. | ||
| if (entityObject.excludeLargeProperties) { |
There was a problem hiding this comment.
isn't this redundant from line 24?
Contributor Author
There was a problem hiding this comment.
Yup. I just pushed an update to remove this line.
danieljbruce deleted the 376497482-separate-function-for-adding-large-properties branch danieljbruce added a commit that referenced this pull request Nov 6, 2024
#1356) * fix: The save function should not throw an error for name/value properties in arrays (#1336) * Add some test cases for findLargeProperties_ * Add a test for arrays and skip it. * Add a test for the complex case wrapped in an arra * Add a test case for an array without name/value. * Add another test for name/value pair * Change the test case involving a name/value pair * Add two simple test cases for excludeFromIndexes * Add another unit test for findLargeProperties_ * Remove if block that stops recursion * Add a test for save with a `name/value` pair * Add some test code that ensures name/value encoded Against the current source code, this test fails because the code throws an error, but it should pass. * Add header * Remove only * Add a test that saves a long string value property * Add a unit test checking longString for value * Remove only * TODO is done * refactor: Add separate function for adding large properties to the excludeFromIndexes list (#1349) * Add some test cases for findLargeProperties_ * Add a test for arrays and skip it. * Add a test for the complex case wrapped in an arra * Add a test case for an array without name/value. * Add another test for name/value pair * Change the test case involving a name/value pair * Add two simple test cases for excludeFromIndexes * Add another unit test for findLargeProperties_ * Remove if block that stops recursion * Add a test for save with a `name/value` pair * Add some test code that ensures name/value encoded Against the current source code, this test fails because the code throws an error, but it should pass. * Add header * Remove only * Add a test that saves a long string value property * Add a unit test checking longString for value * Remove only * TODO is done * Refactor the save function Pull out the code that extends the excludeFromIndexes list * Add TODO * Write unit tests for the new excludeFromIndexes fn * Add the header * Document parameter for extendExcludeFromIndexes * Remove redundant check * refactor: Add a separate function for building the entity proto (#1350) * Add some test cases for findLargeProperties_ * Add a test for arrays and skip it. * Add a test for the complex case wrapped in an arra * Add a test case for an array without name/value. * Add another test for name/value pair * Change the test case involving a name/value pair * Add two simple test cases for excludeFromIndexes * Add another unit test for findLargeProperties_ * Remove if block that stops recursion * Add a test for save with a `name/value` pair * Add some test code that ensures name/value encoded Against the current source code, this test fails because the code throws an error, but it should pass. * Add header * Remove only * Add a test that saves a long string value property * Add a unit test checking longString for value * Remove only * TODO is done * Refactor the save function Pull out the code that extends the excludeFromIndexes list * Add TODO * Write unit tests for the new excludeFromIndexes fn * Add the header * Refactor the code for building the entity proto * Rename it buildEntityProto * The tests need another mock due to the structure The structure change requires another mock. * First two tests for buildEntityProto * Reintroduce extendExcludeFromIndexes.ts tests * Adjust the test to match expected proto * Add another test for an entity in an array * Add headers * Add another header * Eliminate space. Simplify diff * test: Ensure that using extendExcludeFromIndexes and then buildEntityProto always excludes large properties from indexing (#1355) * Add some test cases for findLargeProperties_ * Add a test for arrays and skip it. * Add a test for the complex case wrapped in an arra * Add a test case for an array without name/value. * Add another test for name/value pair * Change the test case involving a name/value pair * Add two simple test cases for excludeFromIndexes * Add another unit test for findLargeProperties_ * Remove if block that stops recursion * Add a test for save with a `name/value` pair * Add some test code that ensures name/value encoded Against the current source code, this test fails because the code throws an error, but it should pass. * Add header * Remove only * Add a test that saves a long string value property * Add a unit test checking longString for value * Remove only * TODO is done * Refactor the save function Pull out the code that extends the excludeFromIndexes list * Add TODO * Write unit tests for the new excludeFromIndexes fn * Add the header * Refactor the code for building the entity proto * Rename it buildEntityProto * The tests need another mock due to the structure The structure change requires another mock. * First two tests for buildEntityProto * Reintroduce extendExcludeFromIndexes.ts tests * Adjust the test to match expected proto * Add another test for an entity in an array * Add headers * Add another header * Add some tests for checkEntityProto Make sure that checkEntityProto is working correctly. * Empty commit * Pull out complexCaseEntities * Add a few more tests to excludeIndexesAndBuildProto * Add generated test cases ensuring completeness * Add a few comments that explain behaviour of block * Add comments explaining code blocks * Add a bunch of other generated tests * Make this test more meaningful for name/value chek * Fix the generator * Remove console logs * Add better test name descriptions * Add TODO * Add a comment about the array of arrays test case * Increase the max depth * Add headers to the codebase * Update parameter descriptions * Add a bunch of parameter documentation Correct only as well.
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.
Summary:
Currently in the save method, if the user sets excludeLargeProperties to true then some code in this method will extend the excludeFromIndexes list based on the position of large properties in the entity object. This PR refactors all of this code into a separate function called
extendExcludeFromIndexesbecause it allows us to unit test this code to make sure it is working correctly. We cannot unit test this code when it is buried inside the save function like it is right now and it allows us to complete the next steps listed below.This is a follow-up to this PR.
Next steps:
Next we need to do the following:
buildEntityProto.extendExcludeFromIndexesis used and thenbuildEntityProtois used, the resulting entity proto always hasexcludeFromIndexes: truein all the places of the entity proto where the large properties are.