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

src/Adapters/Storage/Mongo/MongoSchemaCollection.js

@@ -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
+  _fetchAllSchemasFrom_SCHEMA() {
-  // schemas cannot be retrieved, returns a promise that rejects. Requirements fot the
-  // rejection reason are TBD.
-  getAllSchemas() {
     return this._collection._rawFind({})
     .then(schemas => schemas.map(mongoSchemaToParseSchema));
   }
 
-  // Return a promise for the schema with the given name, in Parse format. If
+  _fechOneSchemaFrom_SCHEMA(name: string) {
-  // this adapter doesn't know about the schema, return a promise that rejects with
-  // undefined as the reason.
-  findSchema(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,28 +14,16 @@ 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()
+  return req.config.database.loadSchema()
-    .then(collection => collection.getAllSchemas())
+  .then(schemaController => schemaController.getAllSchemas())
-    .then(schemas => schemas.map(injectDefaultSchema))
   .then(schemas => ({ response: { results: schemas } }));
 }
 
 function getOneSchema(req) {
   const className = req.params.className;
-  return req.config.database.schemaCollection()
+  return req.config.database.loadSchema()
-    .then(collection => collection.findSchema(className))
+  .then(schemaController => schemaController.getOneSchema(className))
-    .then(injectDefaultSchema)
   .then(schema => ({ response: schema }))
   .catch(error => {
     if (error === undefined) {
@@ -72,19 +60,8 @@ function modifySchema(req) {
   let className = req.params.className;
 
   return req.config.database.loadSchema()
-    .then(schema => {
+  .then(schema => schema.updateClass(className, submittedFields, req.body.classLevelPermissions, req.config.database))
-      return schema.updateClass(className, submittedFields, req.body.classLevelPermissions, req.config.database);
+  .then(result => ({response: result}));
-    }).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]});
-  });
 }
 
 // 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 = {
+        this.data[schema.className] = schema.fields;
-          ...defaultColumns._Default,
-          ...(defaultColumns[schema.className] || {}),
-          ...schema.fields,
-        }
-        this.data[schema.className] = parseFormatSchema;
         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,7 +275,9 @@ class Schema {
   }
 
   updateClass(className, submittedFields, classLevelPermissions, database) {
-    if (!this.data[className]) {
+    return this.hasClass(className)
+    .then(hasClass => {
+      if (!hasClass) {
         throw new Parse.Error(Parse.Error.INVALID_CLASS_NAME, `Class ${className} does not exist.`);
       }
       let existingFields = Object.assign(this.data[className], {_id: className});
@@ -302,15 +319,14 @@ class Schema {
         });
         return Promise.all(promises);
       })
-      .then(() => {
+      .then(() => this.setPermissions(className, classLevelPermissions))
-        return this.setPermissions(className, classLevelPermissions)
-      })
       //TODO: Move this logic into the database adapter
-      .then(() => {
+      .then(() => ({
-        return { className: className,
+        className: className,
         fields: this.data[className],
-              classLevelPermissions: this.perms[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
+  // Checks if a given class is in the schema.
-  // schema first, which is kinda janky. Hopefully we can refactor
-  // and make this be a regular value.
   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) {
+function load(collection, dbAdapter) {
-  let schema = new Schema(collection);
+  let schema = new SchemaController(collection, dbAdapter);
   return schema.reloadData().then(() => schema);
 }