Skip to content

Commit 75dc869

Browse files
sebelgasebelga
authored
fix(entitykey): remove convertion of string number to integer for entity key id (#179)
The live Datastore treats the same way User.get("123") than User.get(123). Both return the entity with the Key.id being "123". The conversion from string to an integer has been removed inside gstore and let to the consumer to implement if needed. BREAKING CHANGE: The "keyType" Schema option has been removed as it is no longer needed. Also, as gstore does not parse the id anymore, running your project against the Datastore emulator locally might break as the emulator treats differently User.get(123) than User.get("123"). Auto-allocated ids are integers and need to be provided as integers for the Emulator. #168
1 parent 16999cb commit 75dc869

File tree

7 files changed

+40
-97
lines changed

7 files changed

+40
-97
lines changed

‎lib/entity.js

Lines changed: 4 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -318,6 +318,10 @@ class Entity {
318318
// Private
319319
// -------
320320
function createKey(self, id, ancestors, namespace) {
321+
if (id && !is.number(id) && !is.string(id)) {
322+
throw new Error('id must be a string or a number');
323+
}
324+
321325
const hasAncestors = typeof ancestors !== 'undefined' && ancestors !== null && is.array(ancestors);
322326

323327
/*
@@ -329,7 +333,6 @@ function createKey(self, id, ancestors, namespace) {
329333

330334
let path;
331335
if (id) {
332-
id = parseId(self, id);
333336
path = hasAncestors ? ancestors.concat([self.entityKind, id]) : [self.entityKind, id];
334337
} else {
335338
if (hasAncestors) {
@@ -344,32 +347,6 @@ function createKey(self, id, ancestors, namespace) {
344347
return namespace ? self.gstore.ds.key({ namespace, path }) : self.gstore.ds.key(path);
345348
}
346349

347-
/**
348-
* Parse the id and according to the keyType config in the Schema ("name"|"id"|<undefined>)
349-
* it will convert an '123'(string) id to 123 (int).
350-
* @param {*} self -- the entity instance
351-
* @param {*} id -- id passed in constructor
352-
*/
353-
function parseId(self, id) {
354-
const { options } = self.schema;
355-
356-
if (is.string(id)) {
357-
if (options && options.keyType === 'name') {
358-
return id;
359-
} if (options.keyType === 'id') {
360-
return self.gstore.ds.int(id);
361-
}
362-
// auto convert string number to number
363-
return isFinite(id) ? self.gstore.ds.int(id) : id;
364-
}
365-
366-
if (!is.number(id)) {
367-
throw new Error('id must be a string or a number');
368-
}
369-
370-
return id;
371-
}
372-
373350
function buildEntityData(self, data) {
374351
const { schema } = self;
375352
const isJoiSchema = schema.isJoi;

‎lib/index.d.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,7 @@ export class Schema<T = { [key: string]: any }> {
187187

188188
/**
189189
* Executes joi.validate on given data. If schema does not have a joi config object data is returned
190-
*
190+
*
191191
* @param {*} data The data to sanitize
192192
* @returns {*} The data sanitized
193193
*/
@@ -755,7 +755,6 @@ export interface SchemaOptions {
755755
format?: JSONFormatType | EntityFormatType;
756756
showKey?: string;
757757
};
758-
keyType?: "id" | "name" | "auto";
759758
joi?: boolean | { extra?: any; options?: any };
760759
}
761760

‎lib/model.js

Lines changed: 2 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -132,11 +132,9 @@ class Model extends Entity {
132132
* Get and entity from the Datastore
133133
*/
134134
static get(id, ancestors, namespace, transaction, options = {}) {
135-
let ids = arrify(id);
135+
const ids = arrify(id);
136136
const _this = this;
137137

138-
ids = ids.map(parseId);
139-
140138
const key = this.key(ids, ancestors, namespace);
141139
const refsToPopulate = [];
142140
const { dataloader } = options;
@@ -226,8 +224,6 @@ class Model extends Entity {
226224
let entityUpdated;
227225
let error = {};
228226

229-
id = parseId(id);
230-
231227
const key = this.key(id, ancestors, namespace);
232228
const replace = options && options.replace === true;
233229

@@ -379,10 +375,6 @@ class Model extends Entity {
379375
const _this = this;
380376
this.__hooksEnabled = true;
381377

382-
const multiple = is.array(id);
383-
384-
id = multiple ? id.map(parseId) : parseId(id);
385-
386378
if (!key) {
387379
key = this.key(id, ancestors, namespace);
388380
}
@@ -587,7 +579,6 @@ class Model extends Entity {
587579
let path = [_this.entityKind];
588580

589581
if (typeof id !== 'undefined' && id !== null) {
590-
id = parseId(id);
591582
path.push(id);
592583
}
593584

@@ -899,15 +890,14 @@ class Model extends Entity {
899890
* For "multiple" ids to delete, we obviously can't set any scope.
900891
*/
901892
function getScopeForDeleteHooks() {
902-
let id = is.object(args[0]) && {}.hasOwnProperty.call(args[0], '__override')
893+
const id = is.object(args[0]) && {}.hasOwnProperty.call(args[0], '__override')
903894
? arrify(args[0].__override)[0]
904895
: args[0];
905896

906897
if (is.array(id)) {
907898
return null;
908899
}
909900

910-
id = parseId(id);
911901
let ancestors;
912902
let namespace;
913903
let key;
@@ -984,10 +974,6 @@ function applyStatics(_Model, schema) {
984974
return _Model;
985975
}
986976

987-
function parseId(id) {
988-
return id !== null && isFinite(id) ? parseInt(id, 10) : id;
989-
}
990-
991977
// Bind Query methods
992978
const {
993979
initQuery,

‎test/entity-test.js

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -231,18 +231,6 @@ describe('Entity', () => {
231231

232232
it('---> with a full Key ("string" Integer keyname passed)', () => {
233233
entity = new GstoreModel({}, '123');
234-
235-
expect(entity.entityKey.id).equal('123');
236-
});
237-
238-
it('---> with a full Key ("string" Integer **not** converted)', () => {
239-
schema = new Schema({
240-
name: { type: 'string' },
241-
}, { keyType: 'name' });
242-
GstoreModel = gstore.model('EntityKind', schema);
243-
244-
entity = new GstoreModel({}, '123');
245-
246234
expect(entity.entityKey.name).equal('123');
247235
});
248236

‎test/integration/cache.js

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,14 @@
44

55
const redisStore = require('cache-manager-redis-store');
66
const chai = require('chai');
7+
const Chance = require('chance');
78
const { Datastore } = require('@google-cloud/datastore');
89
const { argv } = require('yargs');
910

1011
const { Gstore } = require('../../lib');
1112

1213
const ds = new Datastore({ projectId: 'gstore-integration-tests' });
13-
14+
const chance = new Chance();
1415
const { expect } = chai;
1516

1617
const allKeys = [];
@@ -74,11 +75,11 @@ describe('Integration Tests (Cache)', () => {
7475
});
7576

7677
it('should set KEY symbol on query result', () => {
77-
const user = new Model({ email: '[email protected]' });
78-
78+
const id = chance.string({ pool: 'abcdefghijklmnopqrstuvwxyz0123456789' });
79+
const user = new Model({ email: '[email protected]' }, id);
7980
return user.save().then(entity => {
8081
addKey(entity.entityKey);
81-
return Model.get(entity.entityKey.id)
82+
return Model.get(entity.entityKey.name)
8283
.then(e => {
8384
expect(e.email).equal('[email protected]');
8485
});

‎test/integration/model.js

Lines changed: 28 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,10 @@ const Entity = require('../../lib/entity');
1515
const gstore = new Gstore();
1616
const chance = new Chance();
1717

18-
const ds = new Datastore({ projectId: 'gstore-integration-testsx' });
18+
const ds = new Datastore({
19+
projectId: 'gstore-integration-tests',
20+
keyFilename: '/Users/sebastien/secure-keys/gstore-integration-tests-67ddd52037cf.json',
21+
});
1922
gstore.connect(ds);
2023

2124
const { expect, assert } = chai;
@@ -40,7 +43,7 @@ const cleanUp = cb => {
4043
});
4144
};
4245

43-
const randomName = () => chance.string({ pool: 'abcdefghijklmnopqrstuvwxyz' });
46+
const randomName = () => chance.string({ pool: 'abcdefghijklmnopqrstuvwxyz0123456789' });
4447

4548
describe('Model (Integration Tests)', () => {
4649
after(function afterAllIntTest(done) {
@@ -92,7 +95,7 @@ describe('Model (Integration Tests)', () => {
9295
const name = randomName();
9396
const user = new UserModel({
9497
name, company, email: chance.email(), private: randomName(),
95-
});
98+
}, randomName());
9699
return user.save()
97100
.then(({ entityKey }) => {
98101
addKey(entityKey);
@@ -102,7 +105,7 @@ describe('Model (Integration Tests)', () => {
102105

103106
const addPost = (userKey = null, publicationKey = null) => {
104107
const title = randomName();
105-
const post = new PostModel({ title, user: userKey, publication: publicationKey });
108+
const post = new PostModel({ title, user: userKey, publication: publicationKey }, randomName());
106109
return post.save()
107110
.then(({ entityKey }) => {
108111
addKey(entityKey);
@@ -125,24 +128,24 @@ describe('Model (Integration Tests)', () => {
125128
const { name: userName, entityKey: userKey } = await addUser();
126129
const { entityKey: postKey } = await addPost(userKey);
127130

128-
const { entityData } = await PostModel.get(postKey.id).populate('user');
129-
expect(entityData.user.id).equal(userKey.id);
131+
const { entityData } = await PostModel.get(postKey.name).populate('user');
132+
expect(entityData.user.id).equal(userKey.name);
130133
expect(entityData.user.name).equal(userName);
131134
assert.isUndefined(entityData.user.private); // make sure "read: false" is not leaked
132135
});
133136

134137
it('should return "null" if trying to populate a prop that does not exist', async () => {
135138
const { entityKey: postKey } = await addPost();
136139

137-
const { entityData } = await PostModel.get(postKey.id).populate('unknown');
140+
const { entityData } = await PostModel.get(postKey.name).populate('unknown');
138141
expect(entityData.unknown).equal(null);
139142
});
140143

141144
it('should populate multiple props', async () => {
142145
const { name: userName, entityKey: userKey } = await addUser();
143146
const { entityKey: postKey } = await addPost(userKey);
144147

145-
const { entityData } = await PostModel.get(postKey.id).populate(['user', 'publication', 'unknown']);
148+
const { entityData } = await PostModel.get(postKey.name).populate(['user', 'publication', 'unknown']);
146149
expect(entityData.user.name).equal(userName);
147150
expect(entityData.publication).equal(null);
148151
expect(entityData.unknown).equal(null);
@@ -153,7 +156,7 @@ describe('Model (Integration Tests)', () => {
153156
const { title: publicationTitle, entityKey: publicationKey } = await addPublication(userKey);
154157
const { entityKey: postKey } = await addPost(userKey, publicationKey);
155158

156-
const { entityData } = await PostModel.get(postKey.id).populate(['user', 'publication']);
159+
const { entityData } = await PostModel.get(postKey.name).populate(['user', 'publication']);
157160
expect(entityData.user.name).equal(userName);
158161
expect(entityData.publication.title).equal(publicationTitle);
159162
});
@@ -163,7 +166,7 @@ describe('Model (Integration Tests)', () => {
163166
const { title: publicationTitle, entityKey: publicationKey } = await addPublication(userKey);
164167
const { entityKey: postKey } = await addPost(userKey, publicationKey);
165168

166-
const { entityData } = await PostModel.get(postKey.id)
169+
const { entityData } = await PostModel.get(postKey.name)
167170
.populate('user')
168171
.populate('publication');
169172
expect(entityData.user.name).equal(userName);
@@ -174,7 +177,7 @@ describe('Model (Integration Tests)', () => {
174177
const { entityKey: userKey } = await addUser();
175178
const { entityKey: postKey } = await addPost(userKey);
176179

177-
const { entityData } = await PostModel.get(postKey.id).populate('user', ['email', 'private']);
180+
const { entityData } = await PostModel.get(postKey.name).populate('user', ['email', 'private']);
178181
assert.isDefined(entityData.user.email);
179182
assert.isUndefined(entityData.user.name);
180183
assert.isDefined(entityData.user.private); // force get private fields
@@ -185,7 +188,7 @@ describe('Model (Integration Tests)', () => {
185188
const { entityKey: postKey } = await addPost(userKey);
186189

187190
try {
188-
await PostModel.get(postKey.id).populate(['user', 'publication'], ['email', 'private']);
191+
await PostModel.get(postKey.name).populate(['user', 'publication'], ['email', 'private']);
189192
throw new Error('Shoud not get here.');
190193
} catch (err) {
191194
expect(err.message).equal('Only 1 property can be populated when fields to select are provided');
@@ -198,10 +201,10 @@ describe('Model (Integration Tests)', () => {
198201
const { entityKey: postKey1 } = await addPost(userKey1);
199202
const { entityKey: postKey2 } = await addPost(userKey2);
200203

201-
const [post1, post2] = await PostModel.get([postKey1.id, postKey2.id]).populate('user');
202-
expect(post1.entityData.user.id).equal(userKey1.id);
204+
const [post1, post2] = await PostModel.get([postKey1.name, postKey2.name]).populate('user');
205+
expect(post1.entityData.user.id).equal(userKey1.name);
203206
expect(post1.entityData.user.name).equal(userName1);
204-
expect(post2.entityData.user.id).equal(userKey2.id);
207+
expect(post2.entityData.user.id).equal(userKey2.name);
205208
expect(post2.entityData.user.name).equal(userName2);
206209
});
207210

@@ -211,14 +214,14 @@ describe('Model (Integration Tests)', () => {
211214
const { title: publicationTitle, entityKey: publicationKey } = await addPublication(userKey);
212215
const { entityKey: postKey } = await addPost(userKey, publicationKey);
213216

214-
const { entityData } = await PostModel.get(postKey.id)
217+
const { entityData } = await PostModel.get(postKey.name)
215218
.populate(['user', 'user.company'])
216219
.populate('publication')
217220
.populate('publication.user')
218221
.populate('publication.user.company')
219222
.populate('path.that.does.not.exist');
220223

221-
expect(entityData.user.id).equal(userKey.id);
224+
expect(entityData.user.id).equal(userKey.name);
222225
expect(entityData.user.name).equal(userName);
223226
expect(entityData.user.company.name).equal(companyName);
224227
expect(entityData.publication.title).equal(publicationTitle);
@@ -230,7 +233,7 @@ describe('Model (Integration Tests)', () => {
230233
const { name: userName, entityKey: userKey } = await addUser();
231234
const { entityKey: postKey } = await addPost(userKey);
232235

233-
const { entityData } = await PostModel.get(postKey.id).populate();
236+
const { entityData } = await PostModel.get(postKey.name).populate();
234237
expect(entityData.user.name).equal(userName);
235238
expect(entityData.publication).equal(null);
236239
assert.isUndefined(entityData.user.private); // make sure "read: false" is not leaked
@@ -244,7 +247,7 @@ describe('Model (Integration Tests)', () => {
244247
const { entityKey: postKey } = await addPost(userKey);
245248

246249
await transaction.run();
247-
const { entityData } = await PostModel.get(postKey.id, null, null, transaction).populate('user');
250+
const { entityData } = await PostModel.get(postKey.name, null, null, transaction).populate('user');
248251
await transaction.commit();
249252
expect(transaction.get.called).equal(true);
250253
expect(transaction.get.callCount).equal(2);
@@ -274,19 +277,19 @@ describe('Model (Integration Tests)', () => {
274277
const transaction = gstore.transaction();
275278
return transaction.run()
276279
.then(async () => {
277-
await User.update(fromUser.entityKey.id, {
280+
await User.update(fromUser.entityKey.name, {
278281
coins: fromUser.coins - amount,
279282
}, null, null, transaction);
280283

281-
await User.update(toUser.entityKey.id, {
284+
await User.update(toUser.entityKey.name, {
282285
coins: toUser.coins + amount,
283286
}, null, null, transaction);
284287

285288
transaction.commit()
286289
.then(async () => {
287290
const [user1, user2] = await User.get([
288-
fromUser.entityKey.id,
289-
toUser.entityKey.id,
291+
fromUser.entityKey.name,
292+
toUser.entityKey.name,
290293
], null, null, null, { preserveOrder: true });
291294
expect(user1.name).equal('User1');
292295
expect(user1.coins).equal(0);
@@ -303,8 +306,8 @@ describe('Model (Integration Tests)', () => {
303306
});
304307
}
305308

306-
const fromUser = new User({ name: 'User1', coins: 1000 });
307-
const toUser = new User({ name: 'User2', coins: 50 });
309+
const fromUser = new User({ name: 'User1', coins: 1000 }, randomName());
310+
const toUser = new User({ name: 'User2', coins: 50 }, randomName());
308311

309312
return fromUser.save()
310313
.then(({ entityKey }) => {
@@ -319,7 +322,6 @@ describe('Model (Integration Tests)', () => {
319322
});
320323
});
321324

322-
323325
describe('hooks', () => {
324326
it('post delete hook should set scope on entity instance', () => {
325327
const schema = new Schema({ name: { type: 'string' } });

0 commit comments

Comments
 (0)