Schema.js cleanup (#1540)

* Tidy up schemas router

* de-duplicate logic for injecting default schemas

* Remove no-op promise

* use hasClass

* Make getAllSchemas part of SchemaController

* Move getOneSchema logic onto schema controller

* remove log

Author: drew-gross

src/Adapters/Storage/Mongo/MongoSchemaCollection.js

@@ -1,6 +1,6 @@
 
 import MongoCollection from './MongoCollection';
-import * as transform from './MongoTransform';
+import * as transform  from './MongoTransform';
 
 function mongoFieldToParseSchemaField(type) {
   if (type[0] === '*') {
@@ -128,18 +128,12 @@ class MongoSchemaCollection {
     this._collection = collection;
   }
 
-  // Return a promise for all schemas known to this adapter, in Parse format. In case the
-  // schemas cannot be retrieved, returns a promise that rejects. Requirements fot the
-  // rejection reason are TBD.
-  getAllSchemas() {
+  _fetchAllSchemasFrom_SCHEMA() {
     return this._collection._rawFind({})
     .then(schemas => schemas.map(mongoSchemaToParseSchema));
   }
 
-  // Return a promise for the schema with the given name, in Parse format. If
-  // this adapter doesn't know about the schema, return a promise that rejects with
-  // undefined as the reason.
-  findSchema(name: string) {
+  _fechOneSchemaFrom_SCHEMA(name: string) {
     return this._collection._rawFind(_mongoSchemaQueryFromNameQuery(name), { limit: 1 }).then(results => {
       if (results.length === 1) {
         return mongoSchemaToParseSchema(results[0]);

src/Adapters/Storage/Mongo/MongoStorageAdapter.js

@@ -127,6 +127,20 @@ export class MongoStorageAdapter {
     .then(schemaCollection => schemaCollection.updateSchema(className, schemaUpdate));
   }
 
+  // Return a promise for all schemas known to this adapter, in Parse format. In case the
+  // schemas cannot be retrieved, returns a promise that rejects. Requirements for the
+  // rejection reason are TBD.
+  getAllSchemas() {
+    return this.schemaCollection().then(schemasCollection => schemasCollection._fetchAllSchemasFrom_SCHEMA());
+  }
+
+  // Return a promise for the schema with the given name, in Parse format. If
+  // this adapter doesn't know about the schema, return a promise that rejects with
+  // undefined as the reason.
+  getOneSchema(className) {
+    return this.schemaCollection().then(schemasCollection => schemasCollection._fechOneSchemaFrom_SCHEMA(className));
+  }
+
   // TODO: As yet not particularly well specified. Creates an object. Does it really need the schema?
   // or can it fetch the schema itself? Also the schema is not currently a Parse format schema, and it
   // should be, if we are passing it at all.

src/Controllers/DatabaseController.js

@@ -67,7 +67,7 @@ DatabaseController.prototype.loadSchema = function(acceptor = () => true) {
   if (!this.schemaPromise) {
     this.schemaPromise = this.schemaCollection().then(collection => {
       delete this.schemaPromise;
-      return Schema.load(collection);
+      return Schema.load(collection, this.adapter);
     });
     return this.schemaPromise;
   }
@@ -78,7 +78,7 @@ DatabaseController.prototype.loadSchema = function(acceptor = () => true) {
     }
     this.schemaPromise = this.schemaCollection().then(collection => {
       delete this.schemaPromise;
-      return Schema.load(collection);
+      return Schema.load(collection, this.adapter);
     });
     return this.schemaPromise;
   });

src/Routers/SchemasRouter.js

@@ -14,36 +14,24 @@ function classNameMismatchResponse(bodyClass, pathClass) {
   );
 }
 
-function injectDefaultSchema(schema) {
-  let defaultSchema = Schema.defaultColumns[schema.className];
-  if (defaultSchema) {
-    Object.keys(defaultSchema).forEach((key) => {
-      schema.fields[key] = defaultSchema[key];
-    });
-  }
-  return schema;
-}
-
 function getAllSchemas(req) {
-  return req.config.database.schemaCollection()
-    .then(collection => collection.getAllSchemas())
-    .then(schemas => schemas.map(injectDefaultSchema))
-    .then(schemas => ({ response: { results: schemas } }));
+  return req.config.database.loadSchema()
+  .then(schemaController => schemaController.getAllSchemas())
+  .then(schemas => ({ response: { results: schemas } }));
 }
 
 function getOneSchema(req) {
   const className = req.params.className;
-  return req.config.database.schemaCollection()
-    .then(collection => collection.findSchema(className))
-    .then(injectDefaultSchema)
-    .then(schema => ({ response: schema }))
-    .catch(error => {
-      if (error === undefined) {
-        throw new Parse.Error(Parse.Error.INVALID_CLASS_NAME, `Class ${className} does not exist.`);
-      } else {
-        throw new Parse.Error(Parse.Error.INTERNAL_SERVER_ERROR, 'Database adapter error.');
-      }
-    });
+  return req.config.database.loadSchema()
+  .then(schemaController => schemaController.getOneSchema(className))
+  .then(schema => ({ response: schema }))
+  .catch(error => {
+    if (error === undefined) {
+      throw new Parse.Error(Parse.Error.INVALID_CLASS_NAME, `Class ${className} does not exist.`);
+    } else {
+      throw new Parse.Error(Parse.Error.INTERNAL_SERVER_ERROR, 'Database adapter error.');
+    }
+  });
 }
 
 function createSchema(req) {
@@ -72,19 +60,8 @@ function modifySchema(req) {
   let className = req.params.className;
 
   return req.config.database.loadSchema()
-    .then(schema => {
-      return schema.updateClass(className, submittedFields, req.body.classLevelPermissions, req.config.database);
-    }).then((result) => {
-        return Promise.resolve({response: result});
-    });
-}
-
-function getSchemaPermissions(req) {
-  var className = req.params.className;
-  return req.config.database.loadSchema()
-    .then(schema => {
-      return Promise.resolve({response: schema.perms[className]});
-  });
+  .then(schema => schema.updateClass(className, submittedFields, req.body.classLevelPermissions, req.config.database))
+  .then(result => ({response: result}));
 }
 
 // A helper function that removes all join tables for a schema. Returns a promise.

src/Schema.js

@@ -201,15 +201,27 @@ const fieldTypeIsInvalid = ({ type, targetClass }) => {
   return undefined;
 }
 
+const injectDefaultSchema = schema => ({
+  className: schema.className,
+  fields: {
+    ...defaultColumns._Default,
+    ...(defaultColumns[schema.className] || {}),
+    ...schema.fields,
+  },
+  classLevelPermissions: schema.classLevelPermissions,
+})
+
 // Stores the entire schema of the app in a weird hybrid format somewhere between
 // the mongo format and the Parse format. Soon, this will all be Parse format.
-class Schema {
+class SchemaController {
   _collection;
+  _dbAdapter;
   data;
   perms;
 
-  constructor(collection) {
+  constructor(collection, databaseAdapter) {
     this._collection = collection;
+    this._dbAdapter = databaseAdapter;
 
     // this.data[className][fieldName] tells you the type of that field, in mongo format
     this.data = {};
@@ -220,19 +232,25 @@ class Schema {
   reloadData() {
     this.data = {};
     this.perms = {};
-    return this._collection.getAllSchemas().then(allSchemas => {
+    return this.getAllSchemas()
+    .then(allSchemas => {
       allSchemas.forEach(schema => {
-        const parseFormatSchema = {
-          ...defaultColumns._Default,
-          ...(defaultColumns[schema.className] || {}),
-          ...schema.fields,
-        }
-        this.data[schema.className] = parseFormatSchema;
+        this.data[schema.className] = schema.fields;
         this.perms[schema.className] = schema.classLevelPermissions;
       });
     });
   }
 
+  getAllSchemas() {
+    return this._dbAdapter.getAllSchemas()
+    .then(allSchemas => allSchemas.map(injectDefaultSchema));
+  }
+
+  getOneSchema(className) {
+    return this._dbAdapter.getOneSchema(className)
+    .then(injectDefaultSchema);
+  }
+
   // Create a new class that includes the three default fields.
   // ACL is an implicit column that does not get an entry in the
   // _SCHEMAS database. Returns a promise that resolves with the
@@ -247,9 +265,6 @@ class Schema {
     }
 
     return this._collection.addSchema(className, fields, classLevelPermissions)
-    .then(res => {
-      return Promise.resolve(res);
-    })
     .catch(error => {
       if (error === undefined) {
         throw new Parse.Error(Parse.Error.INVALID_CLASS_NAME, `Class ${className} already exists.`);
@@ -260,40 +275,42 @@ class Schema {
   }
 
   updateClass(className, submittedFields, classLevelPermissions, database) {
-    if (!this.data[className]) {
-      throw new Parse.Error(Parse.Error.INVALID_CLASS_NAME, `Class ${className} does not exist.`);
-    }
-    let existingFields = Object.assign(this.data[className], {_id: className});
-    Object.keys(submittedFields).forEach(name => {
-      let field = submittedFields[name];
-      if (existingFields[name] && field.__op !== 'Delete') {
-        throw new Parse.Error(255, `Field ${name} exists, cannot update.`);
-      }
-      if (!existingFields[name] && field.__op === 'Delete') {
-        throw new Parse.Error(255, `Field ${name} does not exist, cannot delete.`);
+    return this.hasClass(className)
+    .then(hasClass => {
+      if (!hasClass) {
+        throw new Parse.Error(Parse.Error.INVALID_CLASS_NAME, `Class ${className} does not exist.`);
       }
-    });
-
-    let newSchema = buildMergedSchemaObject(existingFields, submittedFields);
-    let validationError = this.validateSchemaData(className, newSchema, classLevelPermissions);
-    if (validationError) {
-      throw new Parse.Error(validationError.code, validationError.error);
-    }
+      let existingFields = Object.assign(this.data[className], {_id: className});
+      Object.keys(submittedFields).forEach(name => {
+        let field = submittedFields[name];
+        if (existingFields[name] && field.__op !== 'Delete') {
+          throw new Parse.Error(255, `Field ${name} exists, cannot update.`);
+        }
+        if (!existingFields[name] && field.__op === 'Delete') {
+          throw new Parse.Error(255, `Field ${name} does not exist, cannot delete.`);
+        }
+      });
 
-    // Finally we have checked to make sure the request is valid and we can start deleting fields.
-    // Do all deletions first, then a single save to _SCHEMA collection to handle all additions.
-    let deletePromises = [];
-    let insertedFields = [];
-    Object.keys(submittedFields).forEach(fieldName => {
-      if (submittedFields[fieldName].__op === 'Delete') {
-        const promise = this.deleteField(fieldName, className, database);
-        deletePromises.push(promise);
-      } else {
-        insertedFields.push(fieldName);
+      let newSchema = buildMergedSchemaObject(existingFields, submittedFields);
+      let validationError = this.validateSchemaData(className, newSchema, classLevelPermissions);
+      if (validationError) {
+        throw new Parse.Error(validationError.code, validationError.error);
       }
-    });
 
-    return Promise.all(deletePromises) // Delete Everything
+      // Finally we have checked to make sure the request is valid and we can start deleting fields.
+      // Do all deletions first, then a single save to _SCHEMA collection to handle all additions.
+      let deletePromises = [];
+      let insertedFields = [];
+      Object.keys(submittedFields).forEach(fieldName => {
+        if (submittedFields[fieldName].__op === 'Delete') {
+          const promise = this.deleteField(fieldName, className, database);
+          deletePromises.push(promise);
+        } else {
+          insertedFields.push(fieldName);
+        }
+      });
+
+      return Promise.all(deletePromises) // Delete Everything
       .then(() => this.reloadData()) // Reload our Schema, so we have all the new values
       .then(() => {
         let promises = insertedFields.map(fieldName => {
@@ -302,15 +319,14 @@ class Schema {
         });
         return Promise.all(promises);
       })
-      .then(() => {
-        return this.setPermissions(className, classLevelPermissions)
-      })
+      .then(() => this.setPermissions(className, classLevelPermissions))
       //TODO: Move this logic into the database adapter
-      .then(() => {
-        return { className: className,
-              fields: this.data[className],
-              classLevelPermissions: this.perms[className] }
-      });
+      .then(() => ({
+        className: className,
+        fields: this.data[className],
+        classLevelPermissions: this.perms[className]
+      }));
+    })
   }
 
 
@@ -637,9 +653,7 @@ class Schema {
     return undefined;
   };
 
-  // Checks if a given class is in the schema. Needs to load the
-  // schema first, which is kinda janky. Hopefully we can refactor
-  // and make this be a regular value.
+  // Checks if a given class is in the schema.
   hasClass(className) {
     return this.reloadData().then(() => !!(this.data[className]));
   }
@@ -672,8 +686,8 @@ class Schema {
 }
 
 // Returns a promise for a new Schema.
-function load(collection) {
-  let schema = new Schema(collection);
+function load(collection, dbAdapter) {
+  let schema = new SchemaController(collection, dbAdapter);
   return schema.reloadData().then(() => schema);
 }