fix: Check property existence for exclude from indexes with wildcard (#1114)

danieljbruce · web-flow · commit e6b8ef74ff10 · 2023-09-29T16:51:03.000-04:00
* Add a test for testing without mocks Testing without mocks allows us to look at the code how it is functioning right now so that we can evaluate the intended behavior. * Do a refactor on the test Objects shared by both tests should be moved into the outer block so that they can be shared. * improve the test so that the error bubbles up Now the test fails properly and passes properly by introducing callbacks * isolate the latter test right now 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. * test fixes 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 for excludeFromIndexes on array 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 * Move the tests over to the index file We should not create a nomocks file because we can just group the tests under a unique describe block of test/index.ts. * PR update Remove typeof and remove check for string ‘undefined’ * Create a function to refactor the tests Refactor the tests so that they are more readable. There is a lot of repeated code in each test which makes comparisons hard. * All tests for this feature now use same script 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 Add parameterized testing to eliminate repeated code. These tests need to be easier to read for the sake of the reviewer. * Eliminate function and inline code This function is only used once now. We should inline this code in the test. * Add JSON stringify to test title In the test title include the properties passed in so that it is easy to track what passed/failed. * Organize code so that similar variables are used Similar variables should be used together. Organize the code so that there is less confusion about which variables are related. * inline getExpectedConfig function The getExpectedConfig does not need to be used anymore because its code can just be inlined in the one place it is used. * Separate tests into blocks and inline code Separate the test into blocks and inline some parameters for better test readability. * Add types to parameters passed into async Stronger typing makes the code easier to read. * Eliminate unnecessary variable assignment Don’t assign to the Datastore variable. This is not necessary. Just use the OriginalDatastore variable directly. * Ran linter and simplified source changes 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. * Add blank line back for easier diff reading * Try again, eliminate the blank line * Additional adjustment to entity first path part Apply the first suggestion in the PR to all examples * Define isFirstPathPartUndefined There is a repeated code fragment for checking if the first path part is undefined that we should refactor. * Rename the variable to align with what it does * run linter * Revert "run linter" This reverts commit a101f27. * Revert "Rename the variable to align with what it does" This reverts commit 4f54d94. * Revert "Define isFirstPathPartUndefined" This reverts commit df3f376. * Refactor check out for seeing if defined We do four checks to see if the first path part is defined. We should refactor these checks out. * Move comment to more appropriate place The comment should reflect the next line of code. * Replace comments with description Replace comments with description property so that the tests will report the description when running the tests instead of the properties in the description. * Eliminate prefix. Only use description * fix typo * lowercase convention
diff --git a/src/entity.ts b/src/entity.ts
@@ -852,8 +852,11 @@ export namespace entity {
         return;
       }
 
+      const isFirstPathPartDefined =
+        entity.properties![firstPathPart] !== undefined;
       if (
         firstPathPartIsArray &&
+        isFirstPathPartDefined &&
         // check also if the property in question is actually an array value.
         entity.properties![firstPathPart].arrayValue &&
         // check if wildcard is not applied
@@ -879,7 +882,12 @@ export namespace entity {
             );
           }
         });
-      } else if (firstPathPartIsArray && hasWildCard && remainderPath === '*') {
+      } else if (
+        firstPathPartIsArray &&
+        hasWildCard &&
+        remainderPath === '*' &&
+        isFirstPathPartDefined
+      ) {
         const array = entity.properties![firstPathPart].arrayValue;
         // eslint-disable-next-line @typescript-eslint/no-explicit-any
         array.values.forEach((value: any) => {
@@ -898,7 +906,7 @@ export namespace entity {
             excludePathFromEntity(entity, newPath);
           });
         } else {
-          if (hasWildCard && remainderPath === '*') {
+          if (hasWildCard && remainderPath === '*' && isFirstPathPartDefined) {
             const parentEntity = entity.properties![firstPathPart].entityValue;
 
             if (parentEntity) {
@@ -911,7 +919,7 @@ export namespace entity {
             } else {
               excludePathFromEntity(entity, firstPathPart);
             }
-          } else {
+          } else if (isFirstPathPartDefined) {
             const parentEntity = entity.properties![firstPathPart].entityValue;
             excludePathFromEntity(parentEntity, remainderPath);
           }
diff --git a/test/index.ts b/test/index.ts
@@ -20,15 +20,23 @@ import {PassThrough, Readable} from 'stream';
 
 import * as ds from '../src';
 import {Datastore, DatastoreOptions} from '../src';
-import {entity, Entity, EntityProto, EntityObject} from '../src/entity';
-import {RequestConfig} from '../src/request';
+import {Datastore as OriginalDatastore} from '../src';
+import {
+  entity,
+  Entity,
+  EntityProto,
+  EntityObject,
+  Entities,
+} from '../src/entity';
+import {RequestCallback, RequestConfig} from '../src/request';
 import * as is from 'is';
 import * as sinon from 'sinon';
 import * as extend from 'extend';
-const async = require('async');
+import {google} from '../protos/protos';
 
 // eslint-disable-next-line @typescript-eslint/no-var-requires
 const v1 = require('../src/v1/index.js');
+const async = require('async');
 
 // eslint-disable-next-line @typescript-eslint/no-explicit-any
 const fakeEntityInit: any = {
@@ -2319,6 +2327,128 @@ async.each(
         });
       });
 
+      describe('without using mocks', () => {
+        describe('on save tests', () => {
+          const onSaveTests = [
+            {
+              description:
+                'should encode a save request without excludeFromIndexes',
+              properties: {k: {stringValue: 'v'}},
+              entitiesWithoutKey: {data: {k: 'v'}},
+            },
+            {
+              description:
+                'should add exclude from indexes to property k and ignore excludeFromIndexes with wildcard',
+              properties: {k: {stringValue: 'v', excludeFromIndexes: true}},
+              entitiesWithoutKey: {
+                data: {k: 'v'},
+                excludeFromIndexes: ['k', 'k.*'],
+              },
+            },
+            {
+              description:
+                'should encode a save request without properties and without excludeFromIndexes',
+              properties: {},
+              entitiesWithoutKey: {data: {}},
+            },
+            {
+              description:
+                'should encode a save request with no properties ignoring excludeFromIndexes for a property not on save data',
+              properties: {},
+              entitiesWithoutKey: {
+                data: {},
+                excludeFromIndexes: [
+                  'non_exist_property', // this just ignored
+                  'non_exist_property.*', // should also be ignored
+                ],
+              },
+            },
+            {
+              description:
+                'should encode a save request with one property ignoring excludeFromIndexes for a property not on save data',
+              properties: {k: {stringValue: 'v'}},
+              entitiesWithoutKey: {
+                data: {k: 'v'},
+                excludeFromIndexes: [
+                  'non_exist_property[]', // this just ignored
+                ],
+              },
+            },
+            {
+              description:
+                'should encode a save request with one property ignoring excludeFromIndexes for a property with a wildcard not on save data',
+              properties: {k: {stringValue: 'v'}},
+              entitiesWithoutKey: {
+                data: {k: 'v'},
+                excludeFromIndexes: [
+                  'non_exist_property[].*', // this just ignored
+                ],
+              },
+            },
+          ];
+
+          async.each(
+            onSaveTests,
+            (onSaveTest: {
+              description: string;
+              properties: google.datastore.v1.IValue;
+              entitiesWithoutKey: Entities;
+            }) => {
+              it(`${onSaveTest.description}`, async () => {
+                const datastore = new OriginalDatastore({
+                  namespace: `${Date.now()}`,
+                });
+                {
+                  // This block of code mocks out request_ to check values passed into it.
+                  const expectedConfig = {
+                    client: 'DatastoreClient',
+                    method: 'commit',
+                    gaxOpts: {},
+                    reqOpts: {
+                      mutations: [
+                        {
+                          upsert: {
+                            key: {
+                              path: [{kind: 'Post', name: 'Post1'}],
+                              partitionId: {
+                                namespaceId: datastore.namespace,
+                              },
+                            },
+                            properties: onSaveTest.properties,
+                          },
+                        },
+                      ],
+                    },
+                  };
+                  // Mock out the request function to compare config passed into it.
+                  datastore.request_ = (
+                    config: RequestConfig,
+                    callback: RequestCallback
+                  ) => {
+                    try {
+                      assert.deepStrictEqual(config, expectedConfig);
+                      callback(null, 'some-data');
+                    } catch (e: any) {
+                      callback(e);
+                    }
+                  };
+                }
+                {
+                  // Attach key to entities parameter passed in and run save with those parameters.
+                  const key = datastore.key(['Post', 'Post1']);
+                  const entities = Object.assign(
+                    {key},
+                    onSaveTest.entitiesWithoutKey
+                  );
+                  const results = await datastore.save(entities);
+                  assert.deepStrictEqual(results, ['some-data']);
+                }
+              });
+            }
+          );
+        });
+      });
+
       describe('multi-db support', () => {
         it('should get the database id from the client', async () => {
           const otherDatastore = new Datastore({
