Skip to content

Commit

Permalink
fix: Check property existence for exclude from indexes with wildcard (#…
Browse files Browse the repository at this point in the history 
…1114)

* 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
  • Loading branch information
@danieljbruce
danieljbruce committed Sep 29, 2023
1 parent a41741b commit e6b8ef7
Show file tree
Hide file tree
Showing 2 changed files with 144 additions and 6 deletions.
14 changes: 11 additions & 3 deletions src/entity.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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) => {
Expand All @@ -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) {
Expand All @@ -911,7 +919,7 @@ export namespace entity {
} else {
excludePathFromEntity(entity, firstPathPart);
}
} else {
} else if (isFirstPathPartDefined) {
const parentEntity = entity.properties![firstPathPart].entityValue;
excludePathFromEntity(parentEntity, remainderPath);
}
Expand Down
136 changes: 133 additions & 3 deletions test/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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({
Expand Down

0 comments on commit e6b8ef7

Please sign in to comment.