From 263a719d3a555dc9d25ad6f4e1b110848764fe88 Mon Sep 17 00:00:00 2001 From: Diamond Lewis Date: Fri, 19 Mar 2021 17:10:07 -0500 Subject: [PATCH 1/6] Reduce schema calls on object creation --- spec/SchemaPerformance.spec.js | 10 +++++----- src/Adapters/Cache/SchemaCache.js | 11 +++++++++++ src/Controllers/SchemaController.js | 19 ++++++++++++------- 3 files changed, 28 insertions(+), 12 deletions(-) diff --git a/spec/SchemaPerformance.spec.js b/spec/SchemaPerformance.spec.js index 21e97b0d43..72cd93938c 100644 --- a/spec/SchemaPerformance.spec.js +++ b/spec/SchemaPerformance.spec.js @@ -18,7 +18,7 @@ describe_only_db('mongo')('Schema Performance', function () { const object = new TestObject(); object.set('foo', 'bar'); await object.save(); - expect(getAllSpy.calls.count()).toBe(2); + expect(getAllSpy.calls.count()).toBe(0); }); it('test new object multiple fields', async () => { @@ -30,7 +30,7 @@ describe_only_db('mongo')('Schema Performance', function () { booleanField: true, }); await container.save(); - expect(getAllSpy.calls.count()).toBe(2); + expect(getAllSpy.calls.count()).toBe(0); }); it('test update existing fields', async () => { @@ -83,7 +83,7 @@ describe_only_db('mongo')('Schema Performance', function () { object.set('new', 'barz'); await object.save(); - expect(getAllSpy.calls.count()).toBe(1); + expect(getAllSpy.calls.count()).toBe(0); }); it('test add multiple fields to existing object', async () => { @@ -101,7 +101,7 @@ describe_only_db('mongo')('Schema Performance', function () { booleanField: true, }); await object.save(); - expect(getAllSpy.calls.count()).toBe(1); + expect(getAllSpy.calls.count()).toBe(0); }); it('test user', async () => { @@ -110,7 +110,7 @@ describe_only_db('mongo')('Schema Performance', function () { user.setPassword('testing'); await user.signUp(); - expect(getAllSpy.calls.count()).toBe(1); + expect(getAllSpy.calls.count()).toBe(0); }); it('test query include', async () => { diff --git a/src/Adapters/Cache/SchemaCache.js b/src/Adapters/Cache/SchemaCache.js index f55edf0635..43f1f2e397 100644 --- a/src/Adapters/Cache/SchemaCache.js +++ b/src/Adapters/Cache/SchemaCache.js @@ -9,6 +9,17 @@ export default { return this.all().find(cached => cached.className === className); }, + set(className, schema) { + const cache = this.all(); + const index = cache.findIndex(cached => cached.className === className); + if (index >= 0) { + cache[index] = schema; + } else { + cache.push(schema); + } + this.put(cache); + }, + put(allSchema) { SchemaCache.allClasses = allSchema; }, diff --git a/src/Controllers/SchemaController.js b/src/Controllers/SchemaController.js index 90f32b0b16..8d38a7e784 100644 --- a/src/Controllers/SchemaController.js +++ b/src/Controllers/SchemaController.js @@ -807,9 +807,8 @@ export default class SchemaController { className, }) ); - // TODO: Remove by updating schema cache directly - await this.reloadData({ clearCache: true }); const parseSchema = convertAdapterSchemaToParseSchema(adapterSchema); + SchemaCache.set(className, parseSchema); return parseSchema; } catch (error) { if (error && error.code === Parse.Error.DUPLICATE_VALUE) { @@ -933,6 +932,7 @@ export default class SchemaController { return ( // The schema update succeeded. Reload the schema this.addClassIfNotExists(className) + .then(() => this.reloadData()) .catch(() => { // The schema update failed. This can be okay - it might // have failed because there's a race condition and a different @@ -1118,6 +1118,13 @@ export default class SchemaController { return Promise.resolve(); }) .then(() => { + const cached = SchemaCache.get(className); + if (cached) { + if (cached && !cached.fields[fieldName]) { + cached.fields[fieldName] = type; + SchemaCache.set(className, cached); + } + } return { className, fieldName, @@ -1210,7 +1217,7 @@ export default class SchemaController { async validateObject(className: string, object: any, query: any) { let geocount = 0; const schema = await this.enforceClassExists(className); - const promises = []; + const results = []; for (const fieldName in object) { if (object[fieldName] && getType(object[fieldName]) === 'GeoPoint') { @@ -1237,14 +1244,12 @@ export default class SchemaController { // Every object has ACL implicitly. continue; } - promises.push(schema.enforceFieldExists(className, fieldName, expected)); + results.push(await schema.enforceFieldExists(className, fieldName, expected)); } - const results = await Promise.all(promises); const enforceFields = results.filter(result => !!result); if (enforceFields.length !== 0) { - // TODO: Remove by updating schema cache directly - await this.reloadData({ clearCache: true }); + await this.reloadData(); } this.ensureFields(enforceFields); From e1519b6b6c9f2fe97d750bbf39f6007e1f0455ec Mon Sep 17 00:00:00 2001 From: Diamond Lewis Date: Mon, 26 Apr 2021 14:48:24 -0500 Subject: [PATCH 2/6] Properly handle transactions --- .../Postgres/PostgresStorageAdapter.js | 74 +++++++++---------- src/Controllers/DatabaseController.js | 2 +- src/Controllers/SchemaController.js | 15 +++- 3 files changed, 48 insertions(+), 43 deletions(-) diff --git a/src/Adapters/Storage/Postgres/PostgresStorageAdapter.js b/src/Adapters/Storage/Postgres/PostgresStorageAdapter.js index b653ab4806..81d8337e44 100644 --- a/src/Adapters/Storage/Postgres/PostgresStorageAdapter.js +++ b/src/Adapters/Storage/Postgres/PostgresStorageAdapter.js @@ -1075,50 +1075,48 @@ export class PostgresStorageAdapter implements StorageAdapter { async addFieldIfNotExists(className: string, fieldName: string, type: any, conn: any) { // TODO: Must be revised for invalid logic... debug('addFieldIfNotExists'); - conn = conn || this._client; + const t = (conn && conn.t ? conn.t : conn) || this._client; const self = this; - await conn.tx('add-field-if-not-exists', async t => { - if (type.type !== 'Relation') { - try { - await t.none( - 'ALTER TABLE $ ADD COLUMN IF NOT EXISTS $ $', - { - className, - fieldName, - postgresType: parseTypeToPostgresType(type), - } - ); - } catch (error) { - if (error.code === PostgresRelationDoesNotExistError) { - return self.createClass(className, { fields: { [fieldName]: type } }, t); - } - if (error.code !== PostgresDuplicateColumnError) { - throw error; - } - // Column already exists, created by other request. Carry on to see if it's the right type. - } - } else { + if (type.type !== 'Relation') { + try { await t.none( - 'CREATE TABLE IF NOT EXISTS $ ("relatedId" varChar(120), "owningId" varChar(120), PRIMARY KEY("relatedId", "owningId") )', - { joinTable: `_Join:${fieldName}:${className}` } + 'ALTER TABLE $ ADD COLUMN IF NOT EXISTS $ $', + { + className, + fieldName, + postgresType: parseTypeToPostgresType(type), + } ); + } catch (error) { + if (error.code === PostgresRelationDoesNotExistError) { + return self.createClass(className, { fields: { [fieldName]: type } }, t); + } + if (error.code !== PostgresDuplicateColumnError) { + throw error; + } + // Column already exists, created by other request. Carry on to see if it's the right type. } - - const result = await t.any( - 'SELECT "schema" FROM "_SCHEMA" WHERE "className" = $ and ("schema"::json->\'fields\'->$) is not null', - { className, fieldName } + } else { + await t.none( + 'CREATE TABLE IF NOT EXISTS $ ("relatedId" varChar(120), "owningId" varChar(120), PRIMARY KEY("relatedId", "owningId") )', + { joinTable: `_Join:${fieldName}:${className}` } ); + } - if (result[0]) { - throw 'Attempted to add a field that already exists'; - } else { - const path = `{fields,${fieldName}}`; - await t.none( - 'UPDATE "_SCHEMA" SET "schema"=jsonb_set("schema", $, $) WHERE "className"=$', - { path, type, className } - ); - } - }); + const result = await t.any( + 'SELECT "schema" FROM "_SCHEMA" WHERE "className" = $ and ("schema"::json->\'fields\'->$) is not null', + { className, fieldName } + ); + + if (result[0]) { + throw 'Attempted to add a field that already exists'; + } else { + const path = `{fields,${fieldName}}`; + await t.none( + 'UPDATE "_SCHEMA" SET "schema"=jsonb_set("schema", $, $) WHERE "className"=$', + { path, type, className } + ); + } this._notifySchemaChange(); } diff --git a/src/Controllers/DatabaseController.js b/src/Controllers/DatabaseController.js index be2e61ab42..19487c3151 100644 --- a/src/Controllers/DatabaseController.js +++ b/src/Controllers/DatabaseController.js @@ -484,7 +484,7 @@ class DatabaseController { return this.canAddField(schema, className, object, aclGroup, runOptions); }) .then(() => { - return schema.validateObject(className, object, query); + return schema.validateObject(className, object, query, this._transactionalSession); }); } diff --git a/src/Controllers/SchemaController.js b/src/Controllers/SchemaController.js index 8d38a7e784..a5b0a87ccb 100644 --- a/src/Controllers/SchemaController.js +++ b/src/Controllers/SchemaController.js @@ -1058,7 +1058,12 @@ export default class SchemaController { // object if the provided className-fieldName-type tuple is valid. // The className must already be validated. // If 'freeze' is true, refuse to update the schema for this field. - enforceFieldExists(className: string, fieldName: string, type: string | SchemaField) { + enforceFieldExists( + className: string, + fieldName: string, + type: string | SchemaField, + transactionalSession: ?any + ) { if (fieldName.indexOf('.') > 0) { // subdocument key (x.y) => ok if x is of type 'object' fieldName = fieldName.split('.')[0]; @@ -1106,7 +1111,7 @@ export default class SchemaController { } return this._dbAdapter - .addFieldIfNotExists(className, fieldName, type) + .addFieldIfNotExists(className, fieldName, type, transactionalSession) .catch(error => { if (error.code == Parse.Error.INCORRECT_TYPE) { // Make sure that we throw errors when it is appropriate to do so. @@ -1214,7 +1219,7 @@ export default class SchemaController { // Validates an object provided in REST format. // Returns a promise that resolves to the new schema if this object is // valid. - async validateObject(className: string, object: any, query: any) { + async validateObject(className: string, object: any, query: any, transactionalSession: ?any) { let geocount = 0; const schema = await this.enforceClassExists(className); const results = []; @@ -1244,7 +1249,9 @@ export default class SchemaController { // Every object has ACL implicitly. continue; } - results.push(await schema.enforceFieldExists(className, fieldName, expected)); + results.push( + await schema.enforceFieldExists(className, fieldName, expected, transactionalSession) + ); } const enforceFields = results.filter(result => !!result); From fbf12ee2833e8223c4e0fe18558fd4fd7336af13 Mon Sep 17 00:00:00 2001 From: Diamond Lewis Date: Mon, 26 Apr 2021 14:50:45 -0500 Subject: [PATCH 3/6] lint --- src/Adapters/Storage/StorageAdapter.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/Adapters/Storage/StorageAdapter.js b/src/Adapters/Storage/StorageAdapter.js index d46265f64f..fc1444b967 100644 --- a/src/Adapters/Storage/StorageAdapter.js +++ b/src/Adapters/Storage/StorageAdapter.js @@ -34,7 +34,12 @@ export interface StorageAdapter { classExists(className: string): Promise; setClassLevelPermissions(className: string, clps: any): Promise; createClass(className: string, schema: SchemaType): Promise; - addFieldIfNotExists(className: string, fieldName: string, type: any): Promise; + addFieldIfNotExists( + className: string, + fieldName: string, + type: any, + transactionalSession: ?any + ): Promise; deleteClass(className: string): Promise; deleteAllClasses(fast: boolean): Promise; deleteFields(className: string, schema: SchemaType, fieldNames: Array): Promise; From f034211140614d13b12e926524f02f18bfc71030 Mon Sep 17 00:00:00 2001 From: Diamond Lewis Date: Mon, 26 Apr 2021 17:08:54 -0500 Subject: [PATCH 4/6] ensure fields exists in schema --- src/Controllers/SchemaController.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/Controllers/SchemaController.js b/src/Controllers/SchemaController.js index a5b0a87ccb..a8e14f7802 100644 --- a/src/Controllers/SchemaController.js +++ b/src/Controllers/SchemaController.js @@ -874,12 +874,14 @@ export default class SchemaController { return ( deletePromise // Delete Everything .then(() => this.reloadData({ clearCache: true })) // Reload our Schema, so we have all the new values - .then(() => { - const promises = insertedFields.map(fieldName => { + .then(async () => { + const results = []; + for (let i = 0; i < insertedFields.length; i += 1) { + const fieldName = insertedFields[i]; const type = submittedFields[fieldName]; - return this.enforceFieldExists(className, fieldName, type); - }); - return Promise.all(promises); + results.push(await this.enforceFieldExists(className, fieldName, type)); + } + return results; }) .then(results => { enforceFields = results.filter(result => !!result); From 50f7118c691b8a272860d29745e11c6546125575 Mon Sep 17 00:00:00 2001 From: Diamond Lewis Date: Wed, 28 Apr 2021 12:25:32 -0500 Subject: [PATCH 5/6] remove schema cleanup --- src/Controllers/SchemaController.js | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/Controllers/SchemaController.js b/src/Controllers/SchemaController.js index a8e14f7802..8c677ae6f4 100644 --- a/src/Controllers/SchemaController.js +++ b/src/Controllers/SchemaController.js @@ -807,8 +807,9 @@ export default class SchemaController { className, }) ); + // TODO: Remove by updating schema cache directly + await this.reloadData({ clearCache: true }); const parseSchema = convertAdapterSchemaToParseSchema(adapterSchema); - SchemaCache.set(className, parseSchema); return parseSchema; } catch (error) { if (error && error.code === Parse.Error.DUPLICATE_VALUE) { @@ -934,7 +935,6 @@ export default class SchemaController { return ( // The schema update succeeded. Reload the schema this.addClassIfNotExists(className) - .then(() => this.reloadData()) .catch(() => { // The schema update failed. This can be okay - it might // have failed because there's a race condition and a different @@ -1125,13 +1125,6 @@ export default class SchemaController { return Promise.resolve(); }) .then(() => { - const cached = SchemaCache.get(className); - if (cached) { - if (cached && !cached.fields[fieldName]) { - cached.fields[fieldName] = type; - SchemaCache.set(className, cached); - } - } return { className, fieldName, @@ -1258,7 +1251,8 @@ export default class SchemaController { const enforceFields = results.filter(result => !!result); if (enforceFields.length !== 0) { - await this.reloadData(); + // TODO: Remove by updating schema cache directly + await this.reloadData({ clearCache: true }); } this.ensureFields(enforceFields); From e199ee4d1f972d48c94a36c508e4663129721516 Mon Sep 17 00:00:00 2001 From: Diamond Lewis Date: Mon, 10 May 2021 13:09:03 -0500 Subject: [PATCH 6/6] fix conflicts --- .../Postgres/PostgresStorageAdapter.js | 49 ++++++------------- src/Controllers/DatabaseController.js | 2 +- src/Controllers/SchemaController.js | 29 ++++++----- 3 files changed, 31 insertions(+), 49 deletions(-) diff --git a/src/Adapters/Storage/Postgres/PostgresStorageAdapter.js b/src/Adapters/Storage/Postgres/PostgresStorageAdapter.js index 619e951882..5d0e211ab4 100644 --- a/src/Adapters/Storage/Postgres/PostgresStorageAdapter.js +++ b/src/Adapters/Storage/Postgres/PostgresStorageAdapter.js @@ -1096,43 +1096,26 @@ export class PostgresStorageAdapter implements StorageAdapter { } } else { await t.none( - 'ALTER TABLE $ ADD COLUMN IF NOT EXISTS $ $', - { - className, - fieldName, - postgresType: parseTypeToPostgresType(type), - } + 'CREATE TABLE IF NOT EXISTS $ ("relatedId" varChar(120), "owningId" varChar(120), PRIMARY KEY("relatedId", "owningId") )', + { joinTable: `_Join:${fieldName}:${className}` } ); - } catch (error) { - if (error.code === PostgresRelationDoesNotExistError) { - return self.createClass(className, { fields: { [fieldName]: type } }, t); - } - if (error.code !== PostgresDuplicateColumnError) { - throw error; - } - // Column already exists, created by other request. Carry on to see if it's the right type. } - } else { - await t.none( - 'CREATE TABLE IF NOT EXISTS $ ("relatedId" varChar(120), "owningId" varChar(120), PRIMARY KEY("relatedId", "owningId") )', - { joinTable: `_Join:${fieldName}:${className}` } - ); - } - const result = await t.any( - 'SELECT "schema" FROM "_SCHEMA" WHERE "className" = $ and ("schema"::json->\'fields\'->$) is not null', - { className, fieldName } - ); - - if (result[0]) { - throw 'Attempted to add a field that already exists'; - } else { - const path = `{fields,${fieldName}}`; - await t.none( - 'UPDATE "_SCHEMA" SET "schema"=jsonb_set("schema", $, $) WHERE "className"=$', - { path, type, className } + const result = await t.any( + 'SELECT "schema" FROM "_SCHEMA" WHERE "className" = $ and ("schema"::json->\'fields\'->$) is not null', + { className, fieldName } ); - } + + if (result[0]) { + throw 'Attempted to add a field that already exists'; + } else { + const path = `{fields,${fieldName}}`; + await t.none( + 'UPDATE "_SCHEMA" SET "schema"=jsonb_set("schema", $, $) WHERE "className"=$', + { path, type, className } + ); + } + }); this._notifySchemaChange(); } diff --git a/src/Controllers/DatabaseController.js b/src/Controllers/DatabaseController.js index 19487c3151..be2e61ab42 100644 --- a/src/Controllers/DatabaseController.js +++ b/src/Controllers/DatabaseController.js @@ -484,7 +484,7 @@ class DatabaseController { return this.canAddField(schema, className, object, aclGroup, runOptions); }) .then(() => { - return schema.validateObject(className, object, query, this._transactionalSession); + return schema.validateObject(className, object, query); }); } diff --git a/src/Controllers/SchemaController.js b/src/Controllers/SchemaController.js index 8c677ae6f4..f2663f18cc 100644 --- a/src/Controllers/SchemaController.js +++ b/src/Controllers/SchemaController.js @@ -807,9 +807,8 @@ export default class SchemaController { className, }) ); - // TODO: Remove by updating schema cache directly - await this.reloadData({ clearCache: true }); const parseSchema = convertAdapterSchemaToParseSchema(adapterSchema); + SchemaCache.set(className, parseSchema); return parseSchema; } catch (error) { if (error && error.code === Parse.Error.DUPLICATE_VALUE) { @@ -935,6 +934,7 @@ export default class SchemaController { return ( // The schema update succeeded. Reload the schema this.addClassIfNotExists(className) + .then(() => this.reloadData()) .catch(() => { // The schema update failed. This can be okay - it might // have failed because there's a race condition and a different @@ -1060,12 +1060,7 @@ export default class SchemaController { // object if the provided className-fieldName-type tuple is valid. // The className must already be validated. // If 'freeze' is true, refuse to update the schema for this field. - enforceFieldExists( - className: string, - fieldName: string, - type: string | SchemaField, - transactionalSession: ?any - ) { + enforceFieldExists(className: string, fieldName: string, type: string | SchemaField) { if (fieldName.indexOf('.') > 0) { // subdocument key (x.y) => ok if x is of type 'object' fieldName = fieldName.split('.')[0]; @@ -1113,7 +1108,7 @@ export default class SchemaController { } return this._dbAdapter - .addFieldIfNotExists(className, fieldName, type, transactionalSession) + .addFieldIfNotExists(className, fieldName, type) .catch(error => { if (error.code == Parse.Error.INCORRECT_TYPE) { // Make sure that we throw errors when it is appropriate to do so. @@ -1125,6 +1120,13 @@ export default class SchemaController { return Promise.resolve(); }) .then(() => { + const cached = SchemaCache.get(className); + if (cached) { + if (cached && !cached.fields[fieldName]) { + cached.fields[fieldName] = type; + SchemaCache.set(className, cached); + } + } return { className, fieldName, @@ -1214,7 +1216,7 @@ export default class SchemaController { // Validates an object provided in REST format. // Returns a promise that resolves to the new schema if this object is // valid. - async validateObject(className: string, object: any, query: any, transactionalSession: ?any) { + async validateObject(className: string, object: any, query: any) { let geocount = 0; const schema = await this.enforceClassExists(className); const results = []; @@ -1244,15 +1246,12 @@ export default class SchemaController { // Every object has ACL implicitly. continue; } - results.push( - await schema.enforceFieldExists(className, fieldName, expected, transactionalSession) - ); + results.push(await schema.enforceFieldExists(className, fieldName, expected)); } const enforceFields = results.filter(result => !!result); if (enforceFields.length !== 0) { - // TODO: Remove by updating schema cache directly - await this.reloadData({ clearCache: true }); + await this.reloadData(); } this.ensureFields(enforceFields);