Skip to content

Commit ceaff7e

Browse files
danieljbrucedanieljbruce
authored
fix: Address edge cases for excluding large properties when using save (googleapis#1356)
* fix: The save function should not throw an error for name/value properties in arrays (googleapis#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 (googleapis#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 (googleapis#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 (googleapis#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.
1 parent 536873e commit ceaff7e

File tree

14 files changed

+1523
-536
lines changed

14 files changed

+1523
-536
lines changed

‎src/entity.ts‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1000,16 +1000,6 @@ export namespace entity {
10001000
const MAX_DATASTORE_VALUE_LENGTH = 1500;
10011001
if (Array.isArray(entities)) {
10021002
for (const entry of entities) {
1003-
if (entry && entry.name && entry.value) {
1004-
if (
1005-
is.string(entry.value) &&
1006-
Buffer.from(entry.value).length > MAX_DATASTORE_VALUE_LENGTH
1007-
) {
1008-
entry.excludeFromIndexes = true;
1009-
} else {
1010-
continue;
1011-
}
1012-
}
10131003
findLargeProperties_(entry, path.concat('[]'), properties);
10141004
}
10151005
} else if (is.object(entities)) {

‎src/index.ts‎

Lines changed: 4 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -68,9 +68,10 @@ import {promisifyAll} from '@google-cloud/promisify';
6868
import {google} from '../protos/protos';
6969
import {AggregateQuery} from './aggregate';
7070
import {SaveEntity} from './interfaces/save';
71+
import {extendExcludeFromIndexes} from './utils/entity/extendExcludeFromIndexes';
72+
import {buildEntityProto} from './utils/entity/buildEntityProto';
7173

7274
const {grpc} = new GrpcClient();
73-
const addExcludeFromIndexes = entity.addExcludeFromIndexes;
7475

7576
export type PathType = string | number | entity.Int;
7677
export interface BooleanObject {
@@ -1098,7 +1099,6 @@ class Datastore extends DatastoreRequest {
10981099
.map(DatastoreRequest.prepareEntityObject_)
10991100
.forEach((entityObject: Entity, index: number) => {
11001101
const mutation: Mutation = {};
1101-
let entityProto: EntityProto = {};
11021102
let method = 'upsert';
11031103

11041104
if (entityObject.method) {
@@ -1115,70 +1115,8 @@ class Datastore extends DatastoreRequest {
11151115
insertIndexes[index] = true;
11161116
}
11171117

1118-
if (Array.isArray(entityObject.data)) {
1119-
// This code populates the excludeFromIndexes list with the right values.
1120-
if (entityObject.excludeLargeProperties) {
1121-
entityObject.data.forEach(
1122-
(data: {
1123-
name: {
1124-
toString(): string;
1125-
};
1126-
value: Entity;
1127-
excludeFromIndexes?: boolean;
1128-
}) => {
1129-
entityObject.excludeFromIndexes = entity.findLargeProperties_(
1130-
data.value,
1131-
data.name.toString(),
1132-
entityObject.excludeFromIndexes
1133-
);
1134-
}
1135-
);
1136-
}
1137-
// This code builds the right entityProto from the entityObject
1138-
entityProto.properties = entityObject.data.reduce(
1139-
(
1140-
acc: EntityProtoReduceAccumulator,
1141-
data: EntityProtoReduceData
1142-
) => {
1143-
const value = entity.encodeValue(
1144-
data.value,
1145-
data.name.toString()
1146-
);
1147-
1148-
if (typeof data.excludeFromIndexes === 'boolean') {
1149-
const excluded = data.excludeFromIndexes;
1150-
let values = value.arrayValue && value.arrayValue.values;
1151-
1152-
if (values) {
1153-
values = values.map((x: ValueProto) => {
1154-
x.excludeFromIndexes = excluded;
1155-
return x;
1156-
});
1157-
} else {
1158-
value.excludeFromIndexes = data.excludeFromIndexes;
1159-
}
1160-
}
1161-
1162-
acc[data.name] = value;
1163-
1164-
return acc;
1165-
},
1166-
{}
1167-
);
1168-
// This code adds excludeFromIndexes in the right places
1169-
addExcludeFromIndexes(entityObject.excludeFromIndexes, entityProto);
1170-
} else {
1171-
// This code populates the excludeFromIndexes list with the right values.
1172-
if (entityObject.excludeLargeProperties) {
1173-
entityObject.excludeFromIndexes = entity.findLargeProperties_(
1174-
entityObject.data,
1175-
'',
1176-
entityObject.excludeFromIndexes
1177-
);
1178-
}
1179-
// This code builds the right entityProto from the entityObject
1180-
entityProto = entity.entityToEntityProto(entityObject);
1181-
}
1118+
extendExcludeFromIndexes(entityObject);
1119+
const entityProto = buildEntityProto(entityObject);
11821120

11831121
entityProto.key = entity.keyToKeyProto(entityObject.key);
11841122

‎src/utils/entity/buildEntityProto.ts‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
// Copyright 2024 Google LLC
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
import {entity, Entity, EntityProto, ValueProto} from '../../entity';
16+
import {EntityProtoReduceAccumulator, EntityProtoReduceData} from '../../index';
17+
import addExcludeFromIndexes = entity.addExcludeFromIndexes;
18+
19+
/**
20+
* This function builds the entity proto from the entity object. We cannot
21+
* rely on entity.entityToEntityProto for this because this function is only
22+
* designed to be used for non-array entities.
23+
*
24+
*/
25+
export function buildEntityProto(entityObject: Entity) {
26+
let entityProto: EntityProto = {};
27+
if (Array.isArray(entityObject.data)) {
28+
// This code builds the right entityProto from the entityObject
29+
entityProto.properties = entityObject.data.reduce(
30+
(acc: EntityProtoReduceAccumulator, data: EntityProtoReduceData) => {
31+
const value = entity.encodeValue(data.value, data.name.toString());
32+
33+
if (typeof data.excludeFromIndexes === 'boolean') {
34+
const excluded = data.excludeFromIndexes;
35+
let values = value.arrayValue && value.arrayValue.values;
36+
37+
if (values) {
38+
values = values.map((x: ValueProto) => {
39+
x.excludeFromIndexes = excluded;
40+
return x;
41+
});
42+
} else {
43+
value.excludeFromIndexes = data.excludeFromIndexes;
44+
}
45+
}
46+
47+
acc[data.name] = value;
48+
49+
return acc;
50+
},
51+
{}
52+
);
53+
// This code adds excludeFromIndexes in the right places
54+
addExcludeFromIndexes(entityObject.excludeFromIndexes, entityProto);
55+
} else {
56+
// This code builds the right entityProto from the entityObject
57+
entityProto = entity.entityToEntityProto(entityObject);
58+
}
59+
return entityProto;
60+
}

‎src/utils/entity/extendExcludeFromIndexes.ts‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
// Copyright 2024 Google LLC
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
import {entity, Entity} from '../../entity';
16+
17+
/**
18+
* This function extends the excludeFromIndexes list when it finds
19+
* large properties in the entity object. The extended excludeFromIndexes
20+
* list is then used when building the entity proto.
21+
*
22+
* @param {Entity} entityObject The entity object to parse for properties to
23+
* add to the excludeFromIndexes list.
24+
*/
25+
export function extendExcludeFromIndexes(entityObject: Entity) {
26+
if (entityObject.excludeLargeProperties) {
27+
if (Array.isArray(entityObject.data)) {
28+
// This code populates the excludeFromIndexes list with the right values.
29+
entityObject.data.forEach(
30+
(data: {
31+
name: {
32+
toString(): string;
33+
};
34+
value: Entity;
35+
excludeFromIndexes?: boolean;
36+
}) => {
37+
entityObject.excludeFromIndexes = entity.findLargeProperties_(
38+
data.value,
39+
data.name.toString(),
40+
entityObject.excludeFromIndexes
41+
);
42+
}
43+
);
44+
} else {
45+
entityObject.excludeFromIndexes = entity.findLargeProperties_(
46+
entityObject.data,
47+
'',
48+
entityObject.excludeFromIndexes
49+
);
50+
}
51+
}
52+
}

‎system-test/datastore.ts‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -348,6 +348,37 @@ async.each(
348348
await datastore.delete(postKey);
349349
});
350350

351+
it('should save a nested name/value pair with name as a long string', async () => {
352+
const longString = Buffer.alloc(1501, '.').toString();
353+
const postKey = datastore.key(['Post', 'post1']);
354+
await datastore.save({
355+
key: postKey,
356+
data: {
357+
metadata: [
358+
{
359+
name: longString,
360+
value: 'some-value',
361+
},
362+
],
363+
},
364+
excludeLargeProperties: true,
365+
});
366+
});
367+
it('should save a nested name/value pair with value as a long string', async () => {
368+
const longString = Buffer.alloc(1501, '.').toString();
369+
const postKey = datastore.key(['Post', 'post1']);
370+
await datastore.save({
371+
key: postKey,
372+
data: [
373+
{
374+
name: 'some-name',
375+
value: longString,
376+
},
377+
],
378+
excludeLargeProperties: true,
379+
});
380+
});
381+
351382
describe('multi-db support for read and write operations', () => {
352383
const namespace = `${Date.now()}`;
353384
const keyHierarchy = ['Post', 'post1'];

0 commit comments

Comments
 (0)