refactor: Add a separate function for building the entity proto by danieljbruce · Pull Request #1350 · googleapis/nodejs-datastore

danieljbruce · 2024-10-31T19:38:34Z

Summary:

Currently in the save method, the entity objects that the user passed in will be converted to entity protos. The save method has code that does this for arrays and code that does this for non-arrays. This PR refactors all of this code into a separate function called buildEntityProto because 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.

Changes

In src/index.ts, code inside the save function is moved out into buildEntityProto.ts so that it can be unit tested.
In src/utils/entity/buildEntityProto.ts the buildEntityProto function is created that now contains the code that used to be in src/index.ts.
In test/entity.ts, some constants in a test are moved out into a fixtures file so that they can be reused by another test.
In test/entity/buildEntityProto.ts, some tests have been created for the buildEntityProto function and some of those tests use the constants that have been moved to the fixtures file.
In test/fixtures/entityObjectAndProto.ts, constants are added that used to be in test/entity.ts.
In test/index.ts, new mocks need to be added to the proxyquire object due to the fact that the location of the code that builds the entity proto is now in src/utils/entity/buildEntityProto.ts. So mocks that used to apply for src/index.ts will now apply for ../src/utils/entity/buildEntityProto.js.

Next steps:

Next we need to write tests to ensure that for any input, whenever extendExcludeFromIndexes is used and then buildEntityProto is used, the resulting entity proto always has excludeFromIndexes: true in all the places of the entity proto where the large properties are.

…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

The structure change requires another mock.

…into 376498249-separate-function-for-building-the-entity-proto

# Conflicts: # src/index.ts # src/utils/entity/extendExcludeFromIndexes.ts

danieljbruce · 2024-11-05T14:51:55Z

src/index.ts

-        } else {
-          // This code builds the right entityProto from the entityObject
-          entityProto = entity.entityToEntityProto(entityObject);
-        }


This entire code block has been pulled into the buildEntityProto function.

danieljbruce · 2024-11-05T14:52:59Z

src/utils/entity/buildEntityProto.ts

+ * designed to be used for non-array entities.
+ *
+ */
+export function buildEntityProto(entityObject: Entity) {


This is the new function that all the code from src/index.ts is being pulled into

danieljbruce · 2024-11-05T14:56:19Z

test/entity.ts

@@ -1107,426 +1111,6 @@ describe('entity', () => {
    });

    it('should respect excludeFromIndexes', () => {


The fixed constants defined below are now going to get reused for the new buildEntityProto function so we pull them out into a separate file so that they can be used here and for buildEntityProto tests as well.

danieljbruce · 2024-11-05T14:58:20Z

test/entity/buildEntityProto.ts

+import {
+  entityObject,
+  expectedEntityProto,
+} from '../fixtures/entityObjectAndProto';


Import the fixed constants that were moved out of test/entity.ts.

danieljbruce · 2024-11-05T15:10:11Z

test/index.ts

+          {
+            '../../entity.js': {entity: fakeEntity},
+          }
+        );


These tests replace entity with fakeEntity using './entity.js': {entity: fakeEntity}, in the src/index.ts file. However, since the code that builds the entity proto used in these tests is no longer in src/index.ts, but is in buildEntityProto.js instead then the entity in buildEntityProto.js needs to be mocked out as well.

daniel-sanche

LGTM (although I mostly looked at the code, and not the tests)

danieljbruce · 2024-11-05T20:28:00Z

LGTM (although I mostly looked at the code, and not the tests)

Sounds good. You'll have the opportunity to re-review the tests when all the PRs are put together for a merge onto main.

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

danieljbruce added 30 commits August 14, 2024 17:11

Add some test cases for findLargeProperties_

b337a5c

Add a test for arrays and skip it.

59889b5

Add a test for the complex case wrapped in an arra

3ca8d1c

Add a test case for an array without name/value.

d2c0138

Add another test for name/value pair

3307e7e

Change the test case involving a name/value pair

03bd79e

Add two simple test cases for excludeFromIndexes

63427a6

Merge branch 'main' of https://github.com/googleapis/nodejs-datastore …

7c8b7be

…into 1242-write-tests-for-findLargeProperties-revisit

Add another unit test for findLargeProperties_

111a968

Remove if block that stops recursion

2587728

Add a test for save with a name/value pair

bc963cc

Add some test code that ensures name/value encoded

ce23c71

Against the current source code, this test fails because the code throws an error, but it should pass.

Add header

c96cf51

Remove only

3aef723

Add a test that saves a long string value property

83cfff5

Add a unit test checking longString for value

b1a4dcf

Remove only

891bc65

TODO is done

459a3e0

Refactor the save function

9bef8e4

Pull out the code that extends the excludeFromIndexes list

Add TODO

c8de349

Merge branch '1242-write-tests-for-findLargeProperties-revisit' of ht…

52034a8

…tps://github.com/googleapis/nodejs-datastore into 376497482-separate-function-for-adding-large-properties

Write unit tests for the new excludeFromIndexes fn

7bc65dd

Add the header

feab4b8

Refactor the code for building the entity proto

35f6831

Rename it buildEntityProto

7eb39e5

The tests need another mock due to the structure

e5b4011

The structure change requires another mock.

First two tests for buildEntityProto

516a4a7

Reintroduce extendExcludeFromIndexes.ts tests

2a64930

Adjust the test to match expected proto

b9e2cb1

Add another test for an entity in an array

ff38fd0

product-auto-label bot added size: xl Pull request size is extra large. api: datastore Issues related to the googleapis/nodejs-datastore API. labels Oct 31, 2024

Add headers

e00ee11

danieljbruce changed the base branch from main to exclude-from-indexes-cleanup October 31, 2024 19:53

Add another header

360e382

danieljbruce mentioned this pull request Nov 4, 2024

test: Ensure that using extendExcludeFromIndexes and then buildEntityProto always excludes large properties from indexing #1355

Merged

danieljbruce added 2 commits November 5, 2024 09:40

Merge branch 'main' of https://github.com/googleapis/nodejs-datastore …

c8c0a2a

…into 376498249-separate-function-for-building-the-entity-proto

Merge branch 'exclude-from-indexes-cleanup' of

a4c5508

# Conflicts: # src/index.ts # src/utils/entity/extendExcludeFromIndexes.ts

danieljbruce commented Nov 5, 2024

View reviewed changes

Eliminate space. Simplify diff

ffa8956

danieljbruce commented Nov 5, 2024

View reviewed changes

danieljbruce marked this pull request as ready for review November 5, 2024 15:30

danieljbruce requested a review from a team November 5, 2024 15:30

danieljbruce requested a review from a team as a code owner November 5, 2024 15:30

danieljbruce requested a review from daniel-sanche November 5, 2024 15:31

daniel-sanche approved these changes Nov 5, 2024

View reviewed changes

danieljbruce merged commit 6e83861 into exclude-from-indexes-cleanup Nov 5, 2024

danieljbruce deleted the 376498249-separate-function-for-building-the-entity-proto branch November 5, 2024 20:29

danieljbruce mentioned this pull request Nov 5, 2024

fix: Address edge cases for excluding large properties when using save #1356

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Add a separate function for building the entity proto#1350

refactor: Add a separate function for building the entity proto#1350
danieljbruce merged 35 commits intoexclude-from-indexes-cleanupfrom
376498249-separate-function-for-building-the-entity-proto

danieljbruce commented Oct 31, 2024 •

edited

Loading

Uh oh!

danieljbruce Nov 5, 2024

Uh oh!

danieljbruce Nov 5, 2024

Uh oh!

danieljbruce Nov 5, 2024

Uh oh!

danieljbruce Nov 5, 2024

Uh oh!

danieljbruce Nov 5, 2024

Uh oh!

daniel-sanche left a comment

Uh oh!

danieljbruce commented Nov 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -1107,426 +1111,6 @@ describe('entity', () => {
		});

		it('should respect excludeFromIndexes', () => {

Conversation

danieljbruce commented Oct 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danieljbruce Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

danieljbruce Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

danieljbruce Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

danieljbruce Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

danieljbruce Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

daniel-sanche left a comment

Choose a reason for hiding this comment

Uh oh!

danieljbruce commented Nov 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

danieljbruce commented Oct 31, 2024 •

edited

Loading