resolved name vs. id issue for family.js (googleapis#186)

* resolved name vs. id issue for family.js * added code review updates
feywind · Jun 13, 2018 · 4b89949 · 4b89949
1 parent fc17372
commit 4b89949
Show file tree

Hide file tree

Showing 5 changed files with 53 additions and 48 deletions.
diff --git a/src/family.js b/src/family.js
@@ -34,7 +34,7 @@ class FamilyError extends Error {
  *
  * @class
  * @param {Table} table
- * @param {string} name
+ * @param {string} id
  *
  * @example
  * const Bigtable = require('@google-cloud/bigtable');
@@ -44,17 +44,14 @@ class FamilyError extends Error {
  * const family = table.family('follows');
  */
 class Family {
-  constructor(table, name) {
+  constructor(table, id) {
     this.bigtable = table.bigtable;
     this.table = table;
 
-    this.id = Family.formatName_(table.name, name);
+    const name = Family.formatName_(table.name, id);
 
-    /**
-     * @name Family#familyName
-     * @type {string}
-     */
-    this.familyName = name.split('/').pop();
+    this.id = name.split('/').pop();
+    this.name = name;
   }
 
   /**
@@ -63,7 +60,7 @@ class Family {
    * @private
    *
    * @param {string} tableName The full formatted table name.
-   * @param {string} name The column family name.
+   * @param {string} id The column family unique-identifier.
    * @returns {string}
    *
    * @example
@@ -73,12 +70,12 @@ class Family {
    * );
    * // 'projects/p/zones/z/clusters/c/tables/t/columnFamilies/my-family'
    */
-  static formatName_(tableName, name) {
-    if (name.includes('/')) {
-      return name;
+  static formatName_(tableName, id) {
+    if (id.includes('/')) {
+      return id;
     }
 
-    return `${tableName}/columnFamilies/${name}`;
+    return `${tableName}/columnFamilies/${id}`;
   }
 
   /**
@@ -184,7 +181,7 @@ class Family {
       options = {};
     }
 
-    this.table.createFamily(this.familyName, options, callback);
+    this.table.createFamily(this.id, options, callback);
   }
 
   /**
@@ -221,7 +218,7 @@ class Family {
           name: this.table.name,
           modifications: [
             {
-              id: this.familyName,
+              id: this.id,
               drop: true,
             },
           ],
@@ -361,7 +358,7 @@ class Family {
       }
 
       for (let i = 0, l = families.length; i < l; i++) {
-        if (families[i].id === this.id) {
+        if (families[i].name === this.name) {
           this.metadata = families[i].metadata;
           callback(null, this.metadata);
           return;
@@ -414,7 +411,7 @@ class Family {
     }
 
     const mod = {
-      id: this.familyName,
+      id: this.id,
       update: {},
     };
 
@@ -436,7 +433,7 @@ class Family {
       },
       (...args) => {
         if (args[1]) {
-          this.metadata = args[1].columnFamilies[this.familyName];
+          this.metadata = args[1].columnFamilies[this.id];
           args.splice(1, 0, this.metadata);
         }
 

diff --git a/src/table.js b/src/table.js
@@ -166,7 +166,7 @@ class Table {
    *
    * @throws {error} If a name is not provided.
    *
-   * @param {string} name The name of column family.
+   * @param {string} id The unique identifier of column family.
    * @param {object} [options] Configuration object.
    * @param {object} [options.gaxOptions] Request configuration options, outlined
    *     here: https://googleapis.github.io/gax-nodejs/global.html#CallOptions.
@@ -228,18 +228,18 @@ class Table {
    *   },
    * };
    */
-  createFamily(name, options, callback) {
+  createFamily(id, options, callback) {
     if (is.function(options)) {
       callback = options;
       options = {};
     }
 
-    if (!name) {
-      throw new Error('A name is required to create a family.');
+    if (!id) {
+      throw new Error('An id is required to create a family.');
     }
 
     const mod = {
-      id: name,
+      id: id,
       create: {},
     };
 
@@ -695,18 +695,18 @@ class Table {
    *
    * @throws {error} If a name is not provided.
    *
-   * @param {string} name The family name.
+   * @param {string} id The family unique identifier.
    * @returns {Family}
    *
    * @example
    * const family = table.family('my-family');
    */
-  family(name) {
-    if (!name) {
-      throw new Error('A family name must be provided.');
+  family(id) {
+    if (!id) {
+      throw new Error('A family id must be provided.');
     }
 
-    return new Family(this, name);
+    return new Family(this, id);
   }
 
   /**

diff --git a/system-test/bigtable.js b/system-test/bigtable.js
@@ -375,11 +375,11 @@ describe('Bigtable', function() {
   });
 
   describe('column families', function() {
-    var FAMILY_NAME = 'presidents';
+    var FAMILY_ID = 'presidents';
     var FAMILY;
 
     before(function(done) {
-      FAMILY = TABLE.family(FAMILY_NAME);
+      FAMILY = TABLE.family(FAMILY_ID);
       FAMILY.create(done);
     });
 
@@ -388,18 +388,26 @@ describe('Bigtable', function() {
         assert.ifError(err);
         assert.strictEqual(families.length, 3);
         assert(families[0] instanceof Family);
-        assert.strictEqual(families[0].name, FAMILY.name);
+        assert.notEqual(
+          -1,
+          families
+            .map(f => {
+              return f.id;
+            })
+            .indexOf(FAMILY.id)
+        );
         done();
       });
     });
 
     it('should get a family', function(done) {
-      var family = TABLE.family(FAMILY_NAME);
+      var family = TABLE.family(FAMILY_ID);
 
       family.get(function(err, family) {
         assert.ifError(err);
         assert(family instanceof Family);
         assert.strictEqual(family.name, FAMILY.name);
+        assert.strictEqual(family.id, FAMILY.id);
         done();
       });
     });

diff --git a/test/family.js b/test/family.js
@@ -32,7 +32,7 @@ var fakeUtil = extend({}, util, {
 });
 
 describe('Bigtable/Family', function() {
-  var FAMILY_NAME = 'family-test';
+  var FAMILY_ID = 'family-test';
   var TABLE = {
     bigtable: {},
     id: 'my-table',
@@ -41,9 +41,9 @@ describe('Bigtable/Family', function() {
     createFamily: util.noop,
   };
 
-  var FAMILY_ID = format('{t}/columnFamilies/{f}', {
+  var FAMILY_NAME = format('{t}/columnFamilies/{f}', {
     t: TABLE.name,
-    f: FAMILY_NAME,
+    f: FAMILY_ID,
   });
 
   var Family;
@@ -83,21 +83,21 @@ describe('Bigtable/Family', function() {
 
     it('should extract the family name', function() {
       var family = new Family(TABLE, FAMILY_ID);
-      assert.strictEqual(family.familyName, FAMILY_NAME);
+      assert.strictEqual(family.name, FAMILY_NAME);
     });
   });
 
   describe('formatName_', function() {
     it('should format the column family name', function() {
-      var formatted = Family.formatName_(TABLE.name, FAMILY_NAME);
+      var formatted = Family.formatName_(TABLE.name, FAMILY_ID);
 
-      assert.strictEqual(formatted, FAMILY_ID);
+      assert.strictEqual(formatted, FAMILY_NAME);
     });
 
     it('should not re-format the name', function() {
       var formatted = Family.formatName_(TABLE.name, FAMILY_ID);
 
-      assert.strictEqual(formatted, FAMILY_ID);
+      assert.strictEqual(formatted, FAMILY_NAME);
     });
   });
 
@@ -214,8 +214,8 @@ describe('Bigtable/Family', function() {
     it('should call createFamily from table', function(done) {
       var options = {};
 
-      family.table.createFamily = function(name, options_, callback) {
-        assert.strictEqual(name, family.familyName);
+      family.table.createFamily = function(id, options_, callback) {
+        assert.strictEqual(id, family.id);
         assert.strictEqual(options_, options);
         callback(); // done()
       };
@@ -243,7 +243,7 @@ describe('Bigtable/Family', function() {
           name: family.table.name,
           modifications: [
             {
-              id: family.familyName,
+              id: family.id,
               drop: true,
             },
           ],
@@ -528,7 +528,7 @@ describe('Bigtable/Family', function() {
         assert.strictEqual(config.reqOpts.name, TABLE.name);
         assert.deepEqual(config.reqOpts.modifications, [
           {
-            id: FAMILY_NAME,
+            id: FAMILY_ID,
             update: {},
           },
         ]);
@@ -564,7 +564,7 @@ describe('Bigtable/Family', function() {
           name: TABLE.name,
           modifications: [
             {
-              id: family.familyName,
+              id: family.id,
               update: {
                 gcRule: formattedRule,
               },

diff --git a/test/table.js b/test/table.js
@@ -270,10 +270,10 @@ describe('Bigtable/Table', function() {
   describe('createFamily', function() {
     var COLUMN_ID = 'my-column';
 
-    it('should throw if a name is not provided', function() {
+    it('should throw if a id is not provided', function() {
       assert.throws(function() {
         table.createFamily();
-      }, /A name is required to create a family\./);
+      }, /An id is required to create a family\./);
     });
 
     it('should provide the proper request options', function(done) {
@@ -1188,10 +1188,10 @@ describe('Bigtable/Table', function() {
   describe('family', function() {
     var FAMILY_ID = 'test-family';
 
-    it('should throw if a name is not provided', function() {
+    it('should throw if an id is not provided', function() {
       assert.throws(function() {
         table.family();
-      }, /A family name must be provided\./);
+      }, /A family id must be provided\./);
     });
 
     it('should create a family with the proper arguments', function() {