Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Handle excludeLarge properties for arrays properly #1265

Merged
merged 44 commits into from
Aug 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
6a1ea09
Write a sample test for what save output should be
danieljbruce Aug 12, 2024
0c6348f
Add a test for a bunch of complex excludeFromIndex
danieljbruce Aug 12, 2024
457a4bb
Change the replace function so it works everywhere
danieljbruce Aug 13, 2024
f48a226
Modify the expected value
danieljbruce Aug 13, 2024
781b08a
Correct the test for evaluating arrays
danieljbruce Aug 13, 2024
60dabd7
Break up the entityToEntityProto into separate par
danieljbruce Aug 13, 2024
ba4e82b
Fix the tests to work with new addExcludeFromIndex
danieljbruce Aug 13, 2024
5c68be5
Break up the entityToEntityProto into separate par
danieljbruce Aug 13, 2024
c52ae75
Fix the tests to work with new addExcludeFromIndex
danieljbruce Aug 13, 2024
1037dfb
Revert "Break up the entityToEntityProto into separate par"
danieljbruce Aug 14, 2024
208a763
Revert "Fix the tests to work with new addExcludeFromIndex"
danieljbruce Aug 14, 2024
3b5594e
Remove source code changes
danieljbruce Aug 14, 2024
8314152
Skip the test that looks at arrays
danieljbruce Aug 14, 2024
757049a
Remove some of the wildcard indexes
danieljbruce Aug 14, 2024
099c4f3
Remove another boolean value from excludeFromIndex
danieljbruce Aug 14, 2024
110b919
Eliminate duplicate expectedMutations
danieljbruce Aug 14, 2024
9de7041
Eliminate duplicate runTest function
danieljbruce Aug 14, 2024
db9175b
Begin to set up the async tests
danieljbruce Aug 14, 2024
b154407
Pack all tests into the async framework
danieljbruce Aug 14, 2024
1620737
Delete tests addressed by async
danieljbruce Aug 14, 2024
70356c1
Get rid of test functions and inline everything
danieljbruce Aug 14, 2024
012f026
Add comment
danieljbruce Aug 14, 2024
ee91c85
Add comments describing each test case
danieljbruce Aug 14, 2024
10b6d87
Remove only
danieljbruce Aug 14, 2024
4e61d5f
Add another test looks at name/value not in array
danieljbruce Aug 15, 2024
c3152e5
Add a test for excludeLarge properties and name/va
danieljbruce Aug 15, 2024
2ddb8cc
Fix the test so that the array case matches
danieljbruce Aug 15, 2024
8969dcc
should pass the right properties for an array
danieljbruce Aug 15, 2024
50e7b24
Rename the test
danieljbruce Aug 15, 2024
1a7ce59
Change name to entityName
danieljbruce Aug 15, 2024
c0988c8
Add two tests that capture the array encoding
danieljbruce Aug 15, 2024
88ba08d
Change expected output of arrays
danieljbruce Aug 16, 2024
e7ac45a
Remove only
danieljbruce Aug 16, 2024
f12f58f
Add 2 more tests to ensure behaviour is preserved
danieljbruce Aug 19, 2024
d265437
Merge branch '1242-write-tests-on-save' of https://github.com/googlea…
danieljbruce Aug 19, 2024
6172570
Make argument more specific for excludeFromIndexes
danieljbruce Aug 19, 2024
6544665
Use excludeLargeProperties for array/non-array cas
danieljbruce Aug 19, 2024
79f2a2a
Fix the test to include entity proto
danieljbruce Aug 19, 2024
d011c98
Correct mistakes in initial implementation
danieljbruce Aug 19, 2024
b3e7515
Don’t skip any of the tests
danieljbruce Aug 19, 2024
64eba57
Remove only
danieljbruce Aug 19, 2024
8115a98
Add check for excludeLargeProperties
danieljbruce Aug 19, 2024
288682b
Merge branch '1242-to-main-2' of https://github.com/googleapis/nodejs…
danieljbruce Aug 28, 2024
ee87adb
Remove TODO
danieljbruce Aug 28, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/entity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -778,7 +778,6 @@ export namespace entity {
*/
export function entityToEntityProto(entityObject: EntityObject): EntityProto {
const properties = entityObject.data;
const excludeFromIndexes = entityObject.excludeFromIndexes;

const entityProto: EntityProto = {
key: null,
Expand All @@ -793,6 +792,15 @@ export namespace entity {
),
};

addExcludeFromIndexes(entityObject.excludeFromIndexes, entityProto);

return entityProto;
}

export function addExcludeFromIndexes(
excludeFromIndexes: string[] | undefined,
entityProto: EntityProto
): EntityProto {
if (excludeFromIndexes && excludeFromIndexes.length > 0) {
excludeFromIndexes.forEach((excludePath: string) => {
excludePathFromEntity(entityProto, excludePath);
Expand Down
42 changes: 31 additions & 11 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ import {AggregateQuery} from './aggregate';
import {SaveEntity} from './interfaces/save';

const {grpc} = new GrpcClient();
const addExcludeFromIndexes = entity.addExcludeFromIndexes;

export type PathType = string | number | entity.Int;
export interface BooleanObject {
Expand Down Expand Up @@ -1110,22 +1111,30 @@ class Datastore extends DatastoreRequest {
}
}

if (entityObject.excludeLargeProperties) {
entityObject.excludeFromIndexes = entity.findLargeProperties_(
entityObject.data,
'',
entityObject.excludeFromIndexes
);
}

if (!entity.isKeyComplete(entityObject.key)) {
insertIndexes[index] = true;
}

// @TODO remove in @google-cloud/[email protected]
// This was replaced with a more efficient mechanism in the top-level
// `excludeFromIndexes` option.
if (Array.isArray(entityObject.data)) {
// This code populates the excludeFromIndexes list with the right values.
if (entityObject.excludeLargeProperties) {
entityObject.data.forEach(
(data: {
name: {
toString(): string;
};
value: Entity;
excludeFromIndexes?: boolean;
}) => {
entityObject.excludeFromIndexes = entity.findLargeProperties_(
data.value,
data.name.toString(),
entityObject.excludeFromIndexes
);
}
);
}
// This code builds the right entityProto from the entityObject
entityProto.properties = entityObject.data.reduce(
(
acc: EntityProtoReduceAccumulator,
Expand Down Expand Up @@ -1156,7 +1165,18 @@ class Datastore extends DatastoreRequest {
},
{}
);
// This code adds excludeFromIndexes in the right places
addExcludeFromIndexes(entityObject.excludeFromIndexes, entityProto);
} else {
// This code populates the excludeFromIndexes list with the right values.
if (entityObject.excludeLargeProperties) {
entityObject.excludeFromIndexes = entity.findLargeProperties_(
entityObject.data,
'',
entityObject.excludeFromIndexes
);
}
// This code builds the right entityProto from the entityObject
entityProto = entity.entityToEntityProto(entityObject);
}

Expand Down
31 changes: 3 additions & 28 deletions test/gapic-mocks/commit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,31 +28,6 @@ describe('Commit', () => {
const clientName = 'DatastoreClient';
const datastore = getInitializedDatastoreClient();

function setCommitComparison(
compareFn: (request: protos.google.datastore.v1.ICommitRequest) => void
) {
const dataClient = datastore.clients_.get(clientName);
if (dataClient) {
dataClient.commit = (
request: protos.google.datastore.v1.ICommitRequest,
options: CallOptions,
callback: (
err?: unknown,
res?: protos.google.datastore.v1.ICommitResponse
) => void
) => {
try {
compareFn(request);
} catch (e) {
callback(e);
}
callback(null, {
mutationResults: [],
});
};
}
}

const key = {
path: [
{
Expand Down Expand Up @@ -315,7 +290,7 @@ describe('Commit', () => {
},
{
name: 'should pass the right properties for a simple name/value pair in an array with excludeFromIndexes list',
skipped: true,
skipped: false,
entities: [
{
name: 'entityName',
Expand Down Expand Up @@ -380,7 +355,7 @@ describe('Commit', () => {
{
// This test case reproduces https://github.com/googleapis/nodejs-datastore/issues/1242
name: 'should pass the right request with a nested field',
skipped: true,
skipped: false,
entities: [
{
name: 'field_b',
Expand Down Expand Up @@ -480,7 +455,7 @@ describe('Commit', () => {
{
// Just like the previous test, but entities are wrapped in an array
name: 'should pass the right properties for an array with name/value pairs and excludeLargeProperties',
skipped: true,
skipped: false,
entities: [
{
name: 'entityName',
Expand Down
1 change: 1 addition & 0 deletions test/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ const fakeEntityInit: any = {
keyToKeyProto: entity.keyToKeyProto,
encodeValue: entity.encodeValue,
entityToEntityProto: entity.entityToEntityProto,
addExcludeFromIndexes: entity.addExcludeFromIndexes,
findLargeProperties_: entity.findLargeProperties_,
URLSafeKey: entity.URLSafeKey,
};
Expand Down
Loading