Schema Cache Improvements (#5612)

* Cache Improvements

* improve tests

* more tests

* clean-up

* test with singlecache

* ensure indexes exists

* remove ALL_KEYS

* Add Insert Test

* enableSingleSchemaCache default true

* Revert "enableSingleSchemaCache default true"

This reverts commit 323e7130fb8f695e3ca44ebf9b3b1d38905353da.

* further optimization

* refactor enforceFieldExists

* coverage improvements

* improve tests

* remove flaky test

* cleanup

* Learned something new

spec/.eslintrc.json

@@ -13,6 +13,7 @@
       "Item": true,
       "Container": true,
       "equal": true,
+      "expectAsync": true,
       "notEqual": true,
       "it_only_db": true,
       "it_exclude_dbs": true,

spec/MongoStorageAdapter.spec.js

@@ -286,4 +286,27 @@ describe_only_db('mongo')('MongoStorageAdapter', () => {
       done();
     });
   });
+
+  it('getClass if exists', async () => {
+    const adapter = new MongoStorageAdapter({ uri: databaseURI });
+
+    const schema = {
+      fields: {
+        array: { type: 'Array' },
+        object: { type: 'Object' },
+        date: { type: 'Date' },
+      },
+    };
+
+    await adapter.createClass('MyClass', schema);
+    const myClassSchema = await adapter.getClass('MyClass');
+    expect(myClassSchema).toBeDefined();
+  });
+
+  it('getClass if not exists', async () => {
+    const adapter = new MongoStorageAdapter({ uri: databaseURI });
+    await expectAsync(adapter.getClass('UnknownClass')).toBeRejectedWith(
+      undefined
+    );
+  });
 });

spec/PostgresStorageAdapter.spec.js

@@ -114,6 +114,33 @@ describe_only_db('postgres')('PostgresStorageAdapter', () => {
       })
       .catch(done);
   });
+
+  it('getClass if exists', async () => {
+    const schema = {
+      fields: {
+        array: { type: 'Array' },
+        object: { type: 'Object' },
+        date: { type: 'Date' },
+      },
+    };
+    await adapter.createClass('MyClass', schema);
+    const myClassSchema = await adapter.getClass('MyClass');
+    expect(myClassSchema).toBeDefined();
+  });
+
+  it('getClass if not exists', async () => {
+    const schema = {
+      fields: {
+        array: { type: 'Array' },
+        object: { type: 'Object' },
+        date: { type: 'Date' },
+      },
+    };
+    await adapter.createClass('MyClass', schema);
+    await expectAsync(adapter.getClass('UnknownClass')).toBeRejectedWith(
+      undefined
+    );
+  });
 });
 
 describe_only_db('postgres')('PostgresStorageAdapter shutdown', () => {

spec/RedisCacheAdapter.spec.js

@@ -1,5 +1,7 @@
 const RedisCacheAdapter = require('../lib/Adapters/Cache/RedisCacheAdapter')
   .default;
+const Config = require('../lib/Config');
+
 /*
 To run this test part of the complete suite
 set PARSE_SERVER_TEST_CACHE='redis'
@@ -163,3 +165,168 @@ describe_only(() => {
       .then(done);
   });
 });
+
+describe_only(() => {
+  return process.env.PARSE_SERVER_TEST_CACHE === 'redis';
+})('Redis Performance', function() {
+  const cacheAdapter = new RedisCacheAdapter();
+  let getSpy;
+  let putSpy;
+
+  beforeEach(async () => {
+    await cacheAdapter.clear();
+    getSpy = spyOn(cacheAdapter, 'get').and.callThrough();
+    putSpy = spyOn(cacheAdapter, 'put').and.callThrough();
+    await reconfigureServer({
+      cacheAdapter,
+      enableSingleSchemaCache: true,
+    });
+  });
+
+  it('test new object', async () => {
+    const object = new TestObject();
+    object.set('foo', 'bar');
+    await object.save();
+    expect(getSpy.calls.count()).toBe(4);
+    expect(putSpy.calls.count()).toBe(3);
+  });
+
+  it('test new object multiple fields', async () => {
+    const container = new Container({
+      dateField: new Date(),
+      arrayField: [],
+      numberField: 1,
+      stringField: 'hello',
+      booleanField: true,
+    });
+    await container.save();
+    expect(getSpy.calls.count()).toBe(4);
+    expect(putSpy.calls.count()).toBe(3);
+  });
+
+  it('test update existing fields', async () => {
+    const object = new TestObject();
+    object.set('foo', 'bar');
+    await object.save();
+
+    getSpy.calls.reset();
+    putSpy.calls.reset();
+
+    object.set('foo', 'barz');
+    await object.save();
+    expect(getSpy.calls.count()).toBe(3);
+    expect(putSpy.calls.count()).toBe(0);
+  });
+
+  it('test add new field to existing object', async () => {
+    const object = new TestObject();
+    object.set('foo', 'bar');
+    await object.save();
+
+    getSpy.calls.reset();
+    putSpy.calls.reset();
+
+    object.set('new', 'barz');
+    await object.save();
+    expect(getSpy.calls.count()).toBe(3);
+    expect(putSpy.calls.count()).toBe(1);
+  });
+
+  it('test add multiple fields to existing object', async () => {
+    const object = new TestObject();
+    object.set('foo', 'bar');
+    await object.save();
+
+    getSpy.calls.reset();
+    putSpy.calls.reset();
+
+    object.set({
+      dateField: new Date(),
+      arrayField: [],
+      numberField: 1,
+      stringField: 'hello',
+      booleanField: true,
+    });
+    await object.save();
+    expect(getSpy.calls.count()).toBe(3);
+    expect(putSpy.calls.count()).toBe(1);
+  });
+
+  it('test query', async () => {
+    const object = new TestObject();
+    object.set('foo', 'bar');
+    await object.save();
+
+    getSpy.calls.reset();
+    putSpy.calls.reset();
+
+    const query = new Parse.Query(TestObject);
+    await query.get(object.id);
+    expect(getSpy.calls.count()).toBe(2);
+    expect(putSpy.calls.count()).toBe(0);
+  });
+
+  it('test delete object', async () => {
+    const object = new TestObject();
+    object.set('foo', 'bar');
+    await object.save();
+
+    getSpy.calls.reset();
+    putSpy.calls.reset();
+
+    await object.destroy();
+    expect(getSpy.calls.count()).toBe(3);
+    expect(putSpy.calls.count()).toBe(0);
+  });
+
+  it('test schema update class', async () => {
+    const container = new Container();
+    await container.save();
+
+    getSpy.calls.reset();
+    putSpy.calls.reset();
+
+    const config = Config.get('test');
+    const schema = await config.database.loadSchema();
+    await schema.reloadData();
+
+    const levelPermissions = {
+      find: { '*': true },
+      get: { '*': true },
+      create: { '*': true },
+      update: { '*': true },
+      delete: { '*': true },
+      addField: { '*': true },
+      protectedFields: { '*': [] },
+    };
+
+    await schema.updateClass(
+      'Container',
+      {
+        fooOne: { type: 'Number' },
+        fooTwo: { type: 'Array' },
+        fooThree: { type: 'Date' },
+        fooFour: { type: 'Object' },
+        fooFive: { type: 'Relation', targetClass: '_User' },
+        fooSix: { type: 'String' },
+        fooSeven: { type: 'Object' },
+        fooEight: { type: 'String' },
+        fooNine: { type: 'String' },
+        fooTeen: { type: 'Number' },
+        fooEleven: { type: 'String' },
+        fooTwelve: { type: 'String' },
+        fooThirteen: { type: 'String' },
+        fooFourteen: { type: 'String' },
+        fooFifteen: { type: 'String' },
+        fooSixteen: { type: 'String' },
+        fooEighteen: { type: 'String' },
+        fooNineteen: { type: 'String' },
+      },
+      levelPermissions,
+      {},
+      config.database
+    );
+    expect(getSpy.calls.count()).toBe(3);
+    expect(putSpy.calls.count()).toBe(2);
+  });
+});

spec/Schema.spec.js

@@ -1363,6 +1363,47 @@ describe('SchemaController', () => {
       .then(done)
       .catch(done.fail);
   });
+
+  it('setAllClasses return classes if cache fails', async () => {
+    const schema = await config.database.loadSchema();
+
+    spyOn(schema._cache, 'setAllClasses').and.callFake(() =>
+      Promise.reject('Oops!')
+    );
+    const errorSpy = spyOn(console, 'error').and.callFake(() => {});
+    const allSchema = await schema.setAllClasses();
+
+    expect(allSchema).toBeDefined();
+    expect(errorSpy).toHaveBeenCalledWith(
+      'Error saving schema to cache:',
+      'Oops!'
+    );
+  });
+
+  it('should not throw on null field types', async () => {
+    const schema = await config.database.loadSchema();
+    const result = await schema.enforceFieldExists(
+      'NewClass',
+      'fieldName',
+      null
+    );
+    expect(result).toBeUndefined();
+  });
+
+  it('ensureFields should throw when schema is not set', async () => {
+    const schema = await config.database.loadSchema();
+    try {
+      schema.ensureFields([
+        {
+          className: 'NewClass',
+          fieldName: 'fieldName',
+          type: 'String',
+        },
+      ]);
+    } catch (e) {
+      expect(e.message).toBe('Could not add field fieldName');
+    }
+  });
 });
 
 describe('Class Level Permissions for requiredAuth', () => {

spec/SchemaCache.spec.js

@@ -33,28 +33,12 @@ describe('SchemaCache', () => {
       });
   });
 
-  it('does not return all schemas after a single schema is stored', done => {
-    const schemaCache = new SchemaCache(cacheController);
-    const schema = {
-      className: 'Class1',
-    };
-    schemaCache
-      .setOneSchema(schema.className, schema)
-      .then(() => {
-        return schemaCache.getAllClasses();
-      })
-      .then(allSchemas => {
-        expect(allSchemas).toBeNull();
-        done();
-      });
-  });
-
   it("doesn't persist cached data by default", done => {
     const schemaCache = new SchemaCache(cacheController);
     const schema = {
       className: 'Class1',
     };
-    schemaCache.setOneSchema(schema.className, schema).then(() => {
+    schemaCache.setAllClasses([schema]).then(() => {
       const anotherSchemaCache = new SchemaCache(cacheController);
       return anotherSchemaCache.getOneSchema(schema.className).then(schema => {
         expect(schema).toBeNull();
@@ -68,7 +52,7 @@ describe('SchemaCache', () => {
     const schema = {
       className: 'Class1',
     };
-    schemaCache.setOneSchema(schema.className, schema).then(() => {
+    schemaCache.setAllClasses([schema]).then(() => {
       const anotherSchemaCache = new SchemaCache(cacheController, 5000, true);
       return anotherSchemaCache.getOneSchema(schema.className).then(schema => {
         expect(schema).not.toBeNull();
@@ -76,4 +60,18 @@ describe('SchemaCache', () => {
       });
     });
   });
+
+  it('should not store if ttl is null', async () => {
+    const ttl = null;
+    const schemaCache = new SchemaCache(cacheController, ttl);
+    expect(await schemaCache.getAllClasses()).toBeNull();
+    expect(await schemaCache.setAllClasses()).toBeNull();
+    expect(await schemaCache.getOneSchema()).toBeNull();
+  });
+
+  it('should convert string ttl to number', async () => {
+    const ttl = '5000';
+    const schemaCache = new SchemaCache(cacheController, ttl);
+    expect(schemaCache.ttl).toBe(5000);
+  });
 });

src/Controllers/DatabaseController.js

@@ -844,7 +844,6 @@ class DatabaseController {
           : schemaController.validatePermission(className, aclGroup, 'create')
         )
           .then(() => schemaController.enforceClassExists(className))
-          .then(() => schemaController.reloadData())
           .then(() => schemaController.getOneSchema(className, true))
           .then(schema => {
             transformAuthData(className, object, schema);

src/Controllers/SchemaCache.js

@@ -1,6 +1,5 @@
 const MAIN_SCHEMA = '__MAIN_SCHEMA';
 const SCHEMA_CACHE_PREFIX = '__SCHEMA';
-const ALL_KEYS = '__ALL_KEYS';
 
 import { randomString } from '../cryptoUtils';
 import defaults from '../defaults';
@@ -24,17 +23,6 @@ export default class SchemaCache {
     }
   }
 
-  put(key, value) {
-    return this.cache.get(this.prefix + ALL_KEYS).then(allKeys => {
-      allKeys = allKeys || {};
-      allKeys[key] = true;
-      return Promise.all([
-        this.cache.put(this.prefix + ALL_KEYS, allKeys, this.ttl),
-        this.cache.put(key, value, this.ttl),
-      ]);
-    });
-  }
-
   getAllClasses() {
     if (!this.ttl) {
       return Promise.resolve(null);
@@ -46,27 +34,16 @@ export default class SchemaCache {
     if (!this.ttl) {
       return Promise.resolve(null);
     }
-    return this.put(this.prefix + MAIN_SCHEMA, schema);
+    return this.cache.put(this.prefix + MAIN_SCHEMA, schema);
-  }
-
-  setOneSchema(className, schema) {
-    if (!this.ttl) {
-      return Promise.resolve(null);
-    }
-    return this.put(this.prefix + className, schema);
   }
 
   getOneSchema(className) {
     if (!this.ttl) {
       return Promise.resolve(null);
     }
-    return this.cache.get(this.prefix + className).then(schema => {
-      if (schema) {
-        return Promise.resolve(schema);
-      }
     return this.cache.get(this.prefix + MAIN_SCHEMA).then(cachedSchemas => {
       cachedSchemas = cachedSchemas || [];
-        schema = cachedSchemas.find(cachedSchema => {
+      const schema = cachedSchemas.find(cachedSchema => {
         return cachedSchema.className === className;
       });
       if (schema) {
@@ -74,19 +51,9 @@ export default class SchemaCache {
       }
       return Promise.resolve(null);
     });
-    });
   }
 
   clear() {
-    // That clears all caches...
+    return this.cache.del(this.prefix + MAIN_SCHEMA);
-    return this.cache.get(this.prefix + ALL_KEYS).then(allKeys => {
-      if (!allKeys) {
-        return;
-      }
-      const promises = Object.keys(allKeys).map(key => {
-        return this.cache.del(key);
-      });
-      return Promise.all(promises);
-    });
   }
 }

src/Controllers/SchemaController.js

@@ -549,18 +549,11 @@ export default class SchemaController {
   }
 
   reloadData(options: LoadSchemaOptions = { clearCache: false }): Promise<any> {
-    let promise = Promise.resolve();
-    if (options.clearCache) {
-      promise = promise.then(() => {
-        return this._cache.clear();
-      });
-    }
     if (this.reloadDataPromise && !options.clearCache) {
       return this.reloadDataPromise;
     }
-    this.reloadDataPromise = promise
+    this.reloadDataPromise = this.getAllClasses(options)
-      .then(() => {
+      .then(
-        return this.getAllClasses(options).then(
         allSchemas => {
           this.schemaData = new SchemaData(allSchemas, this.protectedFields);
           delete this.reloadDataPromise;
@@ -570,8 +563,7 @@ export default class SchemaController {
           delete this.reloadDataPromise;
           throw err;
         }
-        );
+      )
-      })
       .then(() => {});
     return this.reloadDataPromise;
   }
@@ -579,27 +571,31 @@ export default class SchemaController {
   getAllClasses(
     options: LoadSchemaOptions = { clearCache: false }
   ): Promise<Array<Schema>> {
-    let promise = Promise.resolve();
     if (options.clearCache) {
-      promise = this._cache.clear();
+      return this.setAllClasses();
     }
-    return promise
+    return this._cache.getAllClasses().then(allClasses => {
-      .then(() => {
+      if (allClasses && allClasses.length) {
-        return this._cache.getAllClasses();
-      })
-      .then(allClasses => {
-        if (allClasses && allClasses.length && !options.clearCache) {
         return Promise.resolve(allClasses);
       }
+      return this.setAllClasses();
+    });
+  }
+
+  setAllClasses(): Promise<Array<Schema>> {
     return this._dbAdapter
       .getAllClasses()
       .then(allSchemas => allSchemas.map(injectDefaultSchema))
       .then(allSchemas => {
-            return this._cache.setAllClasses(allSchemas).then(() => {
+        /* eslint-disable no-console */
+        this._cache
+          .setAllClasses(allSchemas)
+          .catch(error =>
+            console.error('Error saving schema to cache:', error)
+          );
+        /* eslint-enable no-console */
         return allSchemas;
       });
-          });
-      });
   }
 
   getOneSchema(
@@ -625,13 +621,14 @@ export default class SchemaController {
         if (cached && !options.clearCache) {
           return Promise.resolve(cached);
         }
-        return this._dbAdapter
+        return this.setAllClasses().then(allSchemas => {
-          .getClass(className)
+          const oneSchema = allSchemas.find(
-          .then(injectDefaultSchema)
+            schema => schema.className === className
-          .then(result => {
+          );
-            return this._cache.setOneSchema(className, result).then(() => {
+          if (!oneSchema) {
-              return result;
+            return Promise.reject(undefined);
-            });
+          }
+          return oneSchema;
         });
       });
     });
@@ -745,6 +742,7 @@ export default class SchemaController {
         if (deletedFields.length > 0) {
           deletePromise = this.deleteFields(deletedFields, className, database);
         }
+        let enforceFields = [];
         return (
           deletePromise // Delete Everything
             .then(() => this.reloadData({ clearCache: true })) // Reload our Schema, so we have all the new values
@@ -755,9 +753,10 @@ export default class SchemaController {
               });
               return Promise.all(promises);
             })
-            .then(() =>
+            .then(results => {
-              this.setPermissions(className, classLevelPermissions, newSchema)
+              enforceFields = results.filter(result => !!result);
-            )
+              this.setPermissions(className, classLevelPermissions, newSchema);
+            })
             .then(() =>
               this._dbAdapter.setIndexesWithSchemaFormat(
                 className,
@@ -769,6 +768,7 @@ export default class SchemaController {
             .then(() => this.reloadData({ clearCache: true }))
             //TODO: Move this logic into the database adapter
             .then(() => {
+              this.ensureFields(enforceFields);
               const schema = this.schemaData[className];
               const reloadedSchema: Schema = {
                 className: className,
@@ -936,10 +936,9 @@ export default class SchemaController {
 
     // If someone tries to create a new field with null/undefined as the value, return;
     if (!type) {
-      return Promise.resolve(this);
+      return undefined;
     }
 
-    return this.reloadData().then(() => {
     const expectedType = this.getExpectedType(className, fieldName);
     if (typeof type === 'string') {
       type = { type };
@@ -954,17 +953,12 @@ export default class SchemaController {
           )} but got ${typeToString(type)}`
         );
       }
-        return this;
+      return undefined;
     }
 
     return this._dbAdapter
       .addFieldIfNotExists(className, fieldName, type)
-        .then(
+      .catch(error => {
-          () => {
-            // The update succeeded. Reload the schema
-            return this.reloadData({ clearCache: true });
-          },
-          error => {
         if (error.code == Parse.Error.INCORRECT_TYPE) {
           // Make sure that we throw errors when it is appropriate to do so.
           throw error;
@@ -972,14 +966,24 @@ export default class SchemaController {
         // The update failed. This can be okay - it might have been a race
         // condition where another client updated the schema in the same
         // way that we wanted to. So, just reload the schema
-            return this.reloadData({ clearCache: true });
+        return Promise.resolve();
-          }
+      })
-        )
       .then(() => {
-          // Ensure that the schema now validates
+        return {
+          className,
+          fieldName,
+          type,
+        };
+      });
+  }
+
+  ensureFields(fields: any) {
+    for (let i = 0; i < fields.length; i += 1) {
+      const { className, fieldName } = fields[i];
+      let { type } = fields[i];
       const expectedType = this.getExpectedType(className, fieldName);
       if (typeof type === 'string') {
-            type = { type };
+        type = { type: type };
       }
       if (!expectedType || !dbTypeMatchesObjectType(expectedType, type)) {
         throw new Parse.Error(
@@ -987,11 +991,7 @@ export default class SchemaController {
           `Could not add field ${fieldName}`
         );
       }
-          // Remove the cached schema
+    }
-          this._cache.clear();
-          return this;
-        });
-    });
   }
 
   // maintain compatibility
@@ -1074,17 +1074,17 @@ export default class SchemaController {
             );
           });
       })
-      .then(() => {
+      .then(() => this._cache.clear());
-        this._cache.clear();
-      });
   }
 
   // Validates an object provided in REST format.
   // Returns a promise that resolves to the new schema if this object is
   // valid.
-  validateObject(className: string, object: any, query: any) {
+  async validateObject(className: string, object: any, query: any) {
     let geocount = 0;
-    let promise = this.enforceClassExists(className);
+    const schema = await this.enforceClassExists(className);
+    const promises = [];
+
     for (const fieldName in object) {
       if (object[fieldName] === undefined) {
         continue;
@@ -1096,14 +1096,12 @@ export default class SchemaController {
       if (geocount > 1) {
         // Make sure all field validation operations run before we return.
         // If not - we are continuing to run logic, but already provided response from the server.
-        return promise.then(() => {
         return Promise.reject(
           new Parse.Error(
             Parse.Error.INCORRECT_TYPE,
             'there can only be one geopoint field in a class'
           )
         );
-        });
       }
       if (!expected) {
         continue;
@@ -1112,13 +1110,18 @@ export default class SchemaController {
         // Every object has ACL implicitly.
         continue;
       }
-
+      promises.push(schema.enforceFieldExists(className, fieldName, expected));
-      promise = promise.then(schema =>
-        schema.enforceFieldExists(className, fieldName, expected)
-      );
     }
-    promise = thenValidateRequiredColumns(promise, className, object, query);
+    const results = await Promise.all(promises);
-    return promise;
+    const enforceFields = results.filter(result => !!result);
+
+    if (enforceFields.length !== 0) {
+      await this.reloadData({ clearCache: true });
+    }
+    this.ensureFields(enforceFields);
+
+    const promise = Promise.resolve(schema);
+    return thenValidateRequiredColumns(promise, className, object, query);
   }
 
   // Validates that all the properties are set for the object