fix(entitykey): remove convertion of string number to integer for entity key id (#179)

sebelga · web-flow · commit 75dc8690861d · 2019-09-10T08:13:08.000+02:00
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
diff --git a/lib/entity.js b/lib/entity.js
@@ -318,6 +318,10 @@ class Entity {
 // Private
 // -------
 function createKey(self, id, ancestors, namespace) {
+    if (id && !is.number(id) && !is.string(id)) {
+        throw new Error('id must be a string or a number');
+    }
+
     const hasAncestors = typeof ancestors !== 'undefined' && ancestors !== null && is.array(ancestors);
 
     /*
@@ -329,7 +333,6 @@ function createKey(self, id, ancestors, namespace) {
 
     let path;
     if (id) {
-        id = parseId(self, id);
         path = hasAncestors ? ancestors.concat([self.entityKind, id]) : [self.entityKind, id];
     } else {
         if (hasAncestors) {
@@ -344,32 +347,6 @@ function createKey(self, id, ancestors, namespace) {
     return namespace ? self.gstore.ds.key({ namespace, path }) : self.gstore.ds.key(path);
 }
 
-/**
- * Parse the id and according to the keyType config in the Schema ("name"|"id"|<undefined>)
- * it will convert an '123'(string) id to 123 (int).
- * @param {*} self -- the entity instance
- * @param {*} id -- id passed in constructor
- */
-function parseId(self, id) {
-    const { options } = self.schema;
-
-    if (is.string(id)) {
-        if (options && options.keyType === 'name') {
-            return id;
-        } if (options.keyType === 'id') {
-            return self.gstore.ds.int(id);
-        }
-        // auto convert string number to number
-        return isFinite(id) ? self.gstore.ds.int(id) : id;
-    }
-
-    if (!is.number(id)) {
-        throw new Error('id must be a string or a number');
-    }
-
-    return id;
-}
-
 function buildEntityData(self, data) {
     const { schema } = self;
     const isJoiSchema = schema.isJoi;
diff --git a/lib/index.d.ts b/lib/index.d.ts
@@ -187,7 +187,7 @@ export class Schema<T = { [key: string]: any }> {
 
     /**
      * Executes joi.validate on given data. If schema does not have a joi config object data is returned
-     * 
+     *
      * @param {*} data The data to sanitize
      * @returns {*} The data sanitized
      */
@@ -755,7 +755,6 @@ export interface SchemaOptions {
         format?: JSONFormatType | EntityFormatType;
         showKey?: string;
     };
-    keyType?: "id" | "name" | "auto";
     joi?: boolean | { extra?: any; options?: any };
 }
 
diff --git a/lib/model.js b/lib/model.js
@@ -132,11 +132,9 @@ class Model extends Entity {
      * Get and entity from the Datastore
      */
     static get(id, ancestors, namespace, transaction, options = {}) {
-        let ids = arrify(id);
+        const ids = arrify(id);
         const _this = this;
 
-        ids = ids.map(parseId);
-
         const key = this.key(ids, ancestors, namespace);
         const refsToPopulate = [];
         const { dataloader } = options;
@@ -226,8 +224,6 @@ class Model extends Entity {
         let entityUpdated;
         let error = {};
 
-        id = parseId(id);
-
         const key = this.key(id, ancestors, namespace);
         const replace = options && options.replace === true;
 
@@ -379,10 +375,6 @@ class Model extends Entity {
         const _this = this;
         this.__hooksEnabled = true;
 
-        const multiple = is.array(id);
-
-        id = multiple ? id.map(parseId) : parseId(id);
-
         if (!key) {
             key = this.key(id, ancestors, namespace);
         }
@@ -587,7 +579,6 @@ class Model extends Entity {
             let path = [_this.entityKind];
 
             if (typeof id !== 'undefined' && id !== null) {
-                id = parseId(id);
                 path.push(id);
             }
 
@@ -899,15 +890,14 @@ class Model extends Entity {
          * For "multiple" ids to delete, we obviously can't set any scope.
          */
         function getScopeForDeleteHooks() {
-            let id = is.object(args[0]) && {}.hasOwnProperty.call(args[0], '__override')
+            const id = is.object(args[0]) && {}.hasOwnProperty.call(args[0], '__override')
                 ? arrify(args[0].__override)[0]
                 : args[0];
 
             if (is.array(id)) {
                 return null;
             }
 
-            id = parseId(id);
             let ancestors;
             let namespace;
             let key;
@@ -984,10 +974,6 @@ function applyStatics(_Model, schema) {
     return _Model;
 }
 
-function parseId(id) {
-    return id !== null && isFinite(id) ? parseInt(id, 10) : id;
-}
-
 // Bind Query methods
 const {
     initQuery,
diff --git a/test/entity-test.js b/test/entity-test.js
@@ -231,18 +231,6 @@ describe('Entity', () => {
 
             it('---> with a full Key ("string" Integer keyname passed)', () => {
                 entity = new GstoreModel({}, '123');
-
-                expect(entity.entityKey.id).equal('123');
-            });
-
-            it('---> with a full Key ("string" Integer **not** converted)', () => {
-                schema = new Schema({
-                    name: { type: 'string' },
-                }, { keyType: 'name' });
-                GstoreModel = gstore.model('EntityKind', schema);
-
-                entity = new GstoreModel({}, '123');
-
                 expect(entity.entityKey.name).equal('123');
             });
 
diff --git a/test/integration/cache.js b/test/integration/cache.js
@@ -4,13 +4,14 @@
 
 const redisStore = require('cache-manager-redis-store');
 const chai = require('chai');
+const Chance = require('chance');
 const { Datastore } = require('@google-cloud/datastore');
 const { argv } = require('yargs');
 
 const { Gstore } = require('../../lib');
 
 const ds = new Datastore({ projectId: 'gstore-integration-tests' });
-
+const chance = new Chance();
 const { expect } = chai;
 
 const allKeys = [];
@@ -74,11 +75,11 @@ describe('Integration Tests (Cache)', () => {
     });
 
     it('should set KEY symbol on query result', () => {
-        const user = new Model({ email: 'test@test.com' });
-
+        const id = chance.string({ pool: 'abcdefghijklmnopqrstuvwxyz0123456789' });
+        const user = new Model({ email: 'test@test.com' }, id);
         return user.save().then(entity => {
             addKey(entity.entityKey);
-            return Model.get(entity.entityKey.id)
+            return Model.get(entity.entityKey.name)
                 .then(e => {
                     expect(e.email).equal('test@test.com');
                 });
diff --git a/test/integration/model.js b/test/integration/model.js
@@ -15,7 +15,10 @@ const Entity = require('../../lib/entity');
 const gstore = new Gstore();
 const chance = new Chance();
 
-const ds = new Datastore({ projectId: 'gstore-integration-testsx' });
+const ds = new Datastore({
+    projectId: 'gstore-integration-tests',
+    keyFilename: '/Users/sebastien/secure-keys/gstore-integration-tests-67ddd52037cf.json',
+});
 gstore.connect(ds);
 
 const { expect, assert } = chai;
@@ -40,7 +43,7 @@ const cleanUp = cb => {
         });
 };
 
-const randomName = () => chance.string({ pool: 'abcdefghijklmnopqrstuvwxyz' });
+const randomName = () => chance.string({ pool: 'abcdefghijklmnopqrstuvwxyz0123456789' });
 
 describe('Model (Integration Tests)', () => {
     after(function afterAllIntTest(done) {
@@ -92,7 +95,7 @@ describe('Model (Integration Tests)', () => {
             const name = randomName();
             const user = new UserModel({
                 name, company, email: chance.email(), private: randomName(),
-            });
+            }, randomName());
             return user.save()
                 .then(({ entityKey }) => {
                     addKey(entityKey);
@@ -102,7 +105,7 @@ describe('Model (Integration Tests)', () => {
 
         const addPost = (userKey = null, publicationKey = null) => {
             const title = randomName();
-            const post = new PostModel({ title, user: userKey, publication: publicationKey });
+            const post = new PostModel({ title, user: userKey, publication: publicationKey }, randomName());
             return post.save()
                 .then(({ entityKey }) => {
                     addKey(entityKey);
@@ -125,24 +128,24 @@ describe('Model (Integration Tests)', () => {
                 const { name: userName, entityKey: userKey } = await addUser();
                 const { entityKey: postKey } = await addPost(userKey);
 
-                const { entityData } = await PostModel.get(postKey.id).populate('user');
-                expect(entityData.user.id).equal(userKey.id);
+                const { entityData } = await PostModel.get(postKey.name).populate('user');
+                expect(entityData.user.id).equal(userKey.name);
                 expect(entityData.user.name).equal(userName);
                 assert.isUndefined(entityData.user.private); // make sure "read: false" is not leaked
             });
 
             it('should return "null" if trying to populate a prop that does not exist', async () => {
                 const { entityKey: postKey } = await addPost();
 
-                const { entityData } = await PostModel.get(postKey.id).populate('unknown');
+                const { entityData } = await PostModel.get(postKey.name).populate('unknown');
                 expect(entityData.unknown).equal(null);
             });
 
             it('should populate multiple props', async () => {
                 const { name: userName, entityKey: userKey } = await addUser();
                 const { entityKey: postKey } = await addPost(userKey);
 
-                const { entityData } = await PostModel.get(postKey.id).populate(['user', 'publication', 'unknown']);
+                const { entityData } = await PostModel.get(postKey.name).populate(['user', 'publication', 'unknown']);
                 expect(entityData.user.name).equal(userName);
                 expect(entityData.publication).equal(null);
                 expect(entityData.unknown).equal(null);
@@ -153,7 +156,7 @@ describe('Model (Integration Tests)', () => {
                 const { title: publicationTitle, entityKey: publicationKey } = await addPublication(userKey);
                 const { entityKey: postKey } = await addPost(userKey, publicationKey);
 
-                const { entityData } = await PostModel.get(postKey.id).populate(['user', 'publication']);
+                const { entityData } = await PostModel.get(postKey.name).populate(['user', 'publication']);
                 expect(entityData.user.name).equal(userName);
                 expect(entityData.publication.title).equal(publicationTitle);
             });
@@ -163,7 +166,7 @@ describe('Model (Integration Tests)', () => {
                 const { title: publicationTitle, entityKey: publicationKey } = await addPublication(userKey);
                 const { entityKey: postKey } = await addPost(userKey, publicationKey);
 
-                const { entityData } = await PostModel.get(postKey.id)
+                const { entityData } = await PostModel.get(postKey.name)
                     .populate('user')
                     .populate('publication');
                 expect(entityData.user.name).equal(userName);
@@ -174,7 +177,7 @@ describe('Model (Integration Tests)', () => {
                 const { entityKey: userKey } = await addUser();
                 const { entityKey: postKey } = await addPost(userKey);
 
-                const { entityData } = await PostModel.get(postKey.id).populate('user', ['email', 'private']);
+                const { entityData } = await PostModel.get(postKey.name).populate('user', ['email', 'private']);
                 assert.isDefined(entityData.user.email);
                 assert.isUndefined(entityData.user.name);
                 assert.isDefined(entityData.user.private); // force get private fields
@@ -185,7 +188,7 @@ describe('Model (Integration Tests)', () => {
                 const { entityKey: postKey } = await addPost(userKey);
 
                 try {
-                    await PostModel.get(postKey.id).populate(['user', 'publication'], ['email', 'private']);
+                    await PostModel.get(postKey.name).populate(['user', 'publication'], ['email', 'private']);
                     throw new Error('Shoud not get here.');
                 } catch (err) {
                     expect(err.message).equal('Only 1 property can be populated when fields to select are provided');
@@ -198,10 +201,10 @@ describe('Model (Integration Tests)', () => {
                 const { entityKey: postKey1 } = await addPost(userKey1);
                 const { entityKey: postKey2 } = await addPost(userKey2);
 
-                const [post1, post2] = await PostModel.get([postKey1.id, postKey2.id]).populate('user');
-                expect(post1.entityData.user.id).equal(userKey1.id);
+                const [post1, post2] = await PostModel.get([postKey1.name, postKey2.name]).populate('user');
+                expect(post1.entityData.user.id).equal(userKey1.name);
                 expect(post1.entityData.user.name).equal(userName1);
-                expect(post2.entityData.user.id).equal(userKey2.id);
+                expect(post2.entityData.user.id).equal(userKey2.name);
                 expect(post2.entityData.user.name).equal(userName2);
             });
 
@@ -211,14 +214,14 @@ describe('Model (Integration Tests)', () => {
                 const { title: publicationTitle, entityKey: publicationKey } = await addPublication(userKey);
                 const { entityKey: postKey } = await addPost(userKey, publicationKey);
 
-                const { entityData } = await PostModel.get(postKey.id)
+                const { entityData } = await PostModel.get(postKey.name)
                     .populate(['user', 'user.company'])
                     .populate('publication')
                     .populate('publication.user')
                     .populate('publication.user.company')
                     .populate('path.that.does.not.exist');
 
-                expect(entityData.user.id).equal(userKey.id);
+                expect(entityData.user.id).equal(userKey.name);
                 expect(entityData.user.name).equal(userName);
                 expect(entityData.user.company.name).equal(companyName);
                 expect(entityData.publication.title).equal(publicationTitle);
@@ -230,7 +233,7 @@ describe('Model (Integration Tests)', () => {
                 const { name: userName, entityKey: userKey } = await addUser();
                 const { entityKey: postKey } = await addPost(userKey);
 
-                const { entityData } = await PostModel.get(postKey.id).populate();
+                const { entityData } = await PostModel.get(postKey.name).populate();
                 expect(entityData.user.name).equal(userName);
                 expect(entityData.publication).equal(null);
                 assert.isUndefined(entityData.user.private); // make sure "read: false" is not leaked
@@ -244,7 +247,7 @@ describe('Model (Integration Tests)', () => {
                 const { entityKey: postKey } = await addPost(userKey);
 
                 await transaction.run();
-                const { entityData } = await PostModel.get(postKey.id, null, null, transaction).populate('user');
+                const { entityData } = await PostModel.get(postKey.name, null, null, transaction).populate('user');
                 await transaction.commit();
                 expect(transaction.get.called).equal(true);
                 expect(transaction.get.callCount).equal(2);
@@ -274,19 +277,19 @@ describe('Model (Integration Tests)', () => {
                         const transaction = gstore.transaction();
                         return transaction.run()
                             .then(async () => {
-                                await User.update(fromUser.entityKey.id, {
+                                await User.update(fromUser.entityKey.name, {
                                     coins: fromUser.coins - amount,
                                 }, null, null, transaction);
 
-                                await User.update(toUser.entityKey.id, {
+                                await User.update(toUser.entityKey.name, {
                                     coins: toUser.coins + amount,
                                 }, null, null, transaction);
 
                                 transaction.commit()
                                     .then(async () => {
                                         const [user1, user2] = await User.get([
-                                            fromUser.entityKey.id,
-                                            toUser.entityKey.id,
+                                            fromUser.entityKey.name,
+                                            toUser.entityKey.name,
                                         ], null, null, null, { preserveOrder: true });
                                         expect(user1.name).equal('User1');
                                         expect(user1.coins).equal(0);
@@ -303,8 +306,8 @@ describe('Model (Integration Tests)', () => {
                     });
                 }
 
-                const fromUser = new User({ name: 'User1', coins: 1000 });
-                const toUser = new User({ name: 'User2', coins: 50 });
+                const fromUser = new User({ name: 'User1', coins: 1000 }, randomName());
+                const toUser = new User({ name: 'User2', coins: 50 }, randomName());
 
                 return fromUser.save()
                     .then(({ entityKey }) => {
@@ -319,7 +322,6 @@ describe('Model (Integration Tests)', () => {
         });
     });
 
-
     describe('hooks', () => {
         it('post delete hook should set scope on entity instance', () => {
             const schema = new Schema({ name: { type: 'string' } });
diff --git a/test/model-test.js b/test/model-test.js
