From f07836e33f973a385c25bbe5de301623cf6250d2 Mon Sep 17 00:00:00 2001 From: Drew Gross Date: Tue, 9 Feb 2016 16:23:46 -0800 Subject: [PATCH 1/5] Add validation of deleteField function --- spec/Schema.spec.js | 57 ++++++++++++++++++++++++++++++++++++++++++++- src/Schema.js | 32 +++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 1 deletion(-) diff --git a/spec/Schema.spec.js b/spec/Schema.spec.js index 636311a640..d6310ec885 100644 --- a/spec/Schema.spec.js +++ b/spec/Schema.spec.js @@ -1,4 +1,3 @@ -// These tests check that the Schema operates correctly. var Config = require('../src/Config'); var Schema = require('../src/Schema'); var dd = require('deep-diff'); @@ -406,4 +405,60 @@ describe('Schema', () => { done(); }); }); + + it('can check if a class exists', done => { + config.database.loadSchema() + .then(schema => { + return schema.addClassIfNotExists('NewClass', {}) + .then(() => { + console.log(Object.getPrototypeOf(schema)); + schema.hasClass('NewClass') + .then(hasClass => { + expect(hasClass).toEqual(true); + done(); + }) + .catch(fail); + + schema.hasClass('NonexistantClass') + .then(hasClass => { + expect(hasClass).toEqual(false); + done(); + }) + .catch(fail); + }) + .catch(error => { + fail('Couldn\'t create class'); + fail(error); + }); + }) + .catch(error => fail('Couldn\'t load schema')); + }); + + it('refuses to delete fields from invalid class names', done => { + config.database.loadSchema() + .then(schema => schema.deleteField('fieldName', 'invalid class name')) + .catch(error => { + expect(error.code).toEqual(Parse.Error.INVALID_CLASS_NAME); + done(); + }); + }); + + it('refuses to delete invalid fields', done => { + config.database.loadSchema() + .then(schema => schema.deleteField('invalid field name', 'ValidClassName')) + .catch(error => { + expect(error.code).toEqual(Parse.Error.INVALID_KEY_NAME); + done(); + }); + }); + + it('refuses to delete the default fields', done => { + config.database.loadSchema() + .then(schema => schema.deleteField('installationId', '_Installation')) + .catch(error => { + expect(error.code).toEqual(136); + expect(error.error).toEqual('field installationId cannot be changed'); + done(); + }); + }); }); diff --git a/src/Schema.js b/src/Schema.js index 3656507a7c..9477bd3969 100644 --- a/src/Schema.js +++ b/src/Schema.js @@ -409,6 +409,38 @@ Schema.prototype.validateField = function(className, key, type, freeze) { }); }; +// Delete a field, and remove that data from all objects. This is intended +// to remove unused fields, if other writers are writing objects that include +// this field, the field may reappear. Returns a Promise that resolves with +// no object on success, or rejects with { code, error } on failure. +Schema.prototype.deleteField = function(fieldName, className) { + if (!classNameIsValid(className)) { + return Promise.reject({ + code: Parse.Error.INVALID_CLASS_NAME, + error: invalidClassNameMessage(className), + }); + } + + if (!fieldNameIsValid(fieldName)) { + return Promise.reject({ + code: Parse.Error.INVALID_KEY_NAME, + error: 'invalid field name: ' + fieldName, + }); + } + + //Don't allow deleting the default fields. + if (!fieldNameIsValidForClass(fieldName, className)) { + return Promise.reject({ + code: 136, + error: 'field ' + fieldName + ' cannot be changed', + }); + } + + return this.reload() + .then(schema => { + }); +} + // Given a schema promise, construct another schema promise that // validates this field once the schema loads. function thenValidateField(schemaPromise, className, key, type) { From b0c4b8f6cec32eb8f3d5966ac7840b44c9ec00ce Mon Sep 17 00:00:00 2001 From: Drew Gross Date: Tue, 9 Feb 2016 20:16:53 -0800 Subject: [PATCH 2/5] Drop _Join collection when deleting a relation field --- spec/Schema.spec.js | 61 +++++++++++++++++++++++++++++++++++++++++++- src/ExportAdapter.js | 2 -- src/Schema.js | 44 +++++++++++++++++++++++++++++++- 3 files changed, 103 insertions(+), 4 deletions(-) diff --git a/spec/Schema.spec.js b/spec/Schema.spec.js index d6310ec885..9524ea1b1a 100644 --- a/spec/Schema.spec.js +++ b/spec/Schema.spec.js @@ -4,6 +4,22 @@ var dd = require('deep-diff'); var config = new Config('test'); +var hasAllPODobject = () => { + var obj = new Parse.Object('HasAllPOD'); + obj.set('aNumber', 5); + obj.set('aString', 'string'); + obj.set('aBool', true); + obj.set('aDate', new Date()); + obj.set('aObject', {k1: 'value', k2: true, k3: 5}); + obj.set('aArray', ['contents', true, 5]); + obj.set('aGeoPoint', new Parse.GeoPoint({latitude: 0, longitude: 0})); + obj.set('aFile', new Parse.File('f.txt', { base64: 'V29ya2luZyBhdCBQYXJzZSBpcyBncmVhdCE=' })); + var objACL = new Parse.ACL(); + objACL.setPublicWriteAccess(false); + obj.setACL(objACL); + return obj; +}; + describe('Schema', () => { it('can validate one object', (done) => { config.database.loadSchema().then((schema) => { @@ -411,7 +427,6 @@ describe('Schema', () => { .then(schema => { return schema.addClassIfNotExists('NewClass', {}) .then(() => { - console.log(Object.getPrototypeOf(schema)); schema.hasClass('NewClass') .then(hasClass => { expect(hasClass).toEqual(true); @@ -461,4 +476,48 @@ describe('Schema', () => { done(); }); }); + + it('refuses to delete fields from nonexistant classes', done => { + config.database.loadSchema() + .then(schema => schema.deleteField('field', 'NoClass')) + .catch(error => { + expect(error.code).toEqual(Parse.Error.INVALID_CLASS_NAME); + expect(error.error).toEqual('class NoClass does not exist'); + done(); + }); + }); + + it('refuses to delete fields that dont exist', done => { + hasAllPODobject().save() + .then(() => config.database.loadSchema()) + .then(schema => schema.deleteField('missingField', 'HasAllPOD')) + .fail(error => { + expect(error.code).toEqual(255); + expect(error.error).toEqual('field missingField does not exist, cannot delete'); + done(); + }); + }); + + it('drops related collection when deleting relation field', done => { + var obj1 = hasAllPODobject(); + obj1.save() + .then(savedObj1 => { + var obj2 = new Parse.Object('HasPointersAndRelations'); + obj2.set('aPointer', savedObj1); + var relation = obj2.relation('aRelation'); + relation.add(obj1); + return obj2.save(); + }) + .then(() => { + config.database.db.collection('test__Join:aRelation:HasPointersAndRelations', { strict: true }, (err, coll) => { + expect(err).toEqual(null); + config.database.loadSchema() + .then(schema => schema.deleteField('aRelation', 'HasPointersAndRelations', config.database.db, 'test_')) + .then(() => config.database.db.collection('test__Join:aRelation:HasPointersAndRelations', { strict: true }, (err, coll) => { + expect(err).not.toEqual(null); + done(); + })) + }); + }) + }); }); diff --git a/src/ExportAdapter.js b/src/ExportAdapter.js index 1676ccfb42..7c2bd67437 100644 --- a/src/ExportAdapter.js +++ b/src/ExportAdapter.js @@ -43,8 +43,6 @@ ExportAdapter.prototype.connect = function() { // Returns a promise for a Mongo collection. // Generally just for internal use. -var joinRegex = /^_Join:[A-Za-z0-9_]+:[A-Za-z0-9_]+/; -var otherRegex = /^[A-Za-z][A-Za-z0-9_]*$/; ExportAdapter.prototype.collection = function(className) { if (!Schema.classNameIsValid(className)) { throw new Parse.Error(Parse.Error.INVALID_CLASS_NAME, diff --git a/src/Schema.js b/src/Schema.js index 9477bd3969..33dedb9922 100644 --- a/src/Schema.js +++ b/src/Schema.js @@ -413,7 +413,11 @@ Schema.prototype.validateField = function(className, key, type, freeze) { // to remove unused fields, if other writers are writing objects that include // this field, the field may reappear. Returns a Promise that resolves with // no object on success, or rejects with { code, error } on failure. -Schema.prototype.deleteField = function(fieldName, className) { + +// Passing the database and prefix is necessary in order to drop relation collections +// and remove fields from objects. Ideally the database would belong to +// a database adapter and this fuction would close over it or access it via member. +Schema.prototype.deleteField = function(fieldName, className, database, prefix) { if (!classNameIsValid(className)) { return Promise.reject({ code: Parse.Error.INVALID_CLASS_NAME, @@ -438,6 +442,37 @@ Schema.prototype.deleteField = function(fieldName, className) { return this.reload() .then(schema => { + return schema.hasClass(className) + .then(hasClass => { + if (!hasClass) { + return Promise.reject({ + code: Parse.Error.INVALID_CLASS_NAME, + error: 'class ' + className + ' does not exist', + }); + } + + if (!schema.data[className][fieldName]) { + return Promise.reject({ + code: 255, + error: 'field ' + fieldName + ' does not exist, cannot delete', + }); + } + + let p = null; + if (schema.data[className][fieldName].startsWith('relation')) { + //For relations, drop the _Join table + + p = database.dropCollection(prefix + '_Join:' + fieldName + ':' + className); + } else { + //for non-relations, remove all the data. This is necessary to ensure that the data is still gone + //if they add the same field. + p = Promise.resolve(); + } + return p.then(() => { + //Save the _SCHEMA object + return Promise.resolve(); + }); + }); }); } @@ -509,6 +544,13 @@ Schema.prototype.getExpectedType = function(className, key) { 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. Parse would probably +Schema.prototype.hasClass = function(className) { + return this.reload().then(newSchema => !!newSchema.data[className]); +} + // Helper function to check if a field is a pointer, returns true or false. Schema.prototype.isPointer = function(className, key) { var expected = this.getExpectedType(className, key); From 0a0d4f65efe86ae14fa0a401bacdf83f9cb53868 Mon Sep 17 00:00:00 2001 From: Drew Gross Date: Wed, 10 Feb 2016 14:11:42 -0800 Subject: [PATCH 3/5] Finish implementation of delete field from schema --- spec/Schema.spec.js | 46 +++++++++++++++++++++++++++++++++++++++++++++ src/Schema.js | 28 +++++++++++++++++++-------- 2 files changed, 66 insertions(+), 8 deletions(-) diff --git a/spec/Schema.spec.js b/spec/Schema.spec.js index 9524ea1b1a..b8222e5fc3 100644 --- a/spec/Schema.spec.js +++ b/spec/Schema.spec.js @@ -520,4 +520,50 @@ describe('Schema', () => { }); }) }); + + it('can delete string fields and resave as number field', done => { + var obj1 = hasAllPODobject(); + var obj2 = hasAllPODobject(); + var p = Parse.Object.saveAll([obj1, obj2]) + .then(() => config.database.loadSchema()) + .then(schema => schema.deleteField('aString', 'HasAllPOD', config.database.db, 'test_')) + .then(() => new Parse.Query('HasAllPOD').get(obj1.id)) + .then(obj1 => { + expect(obj1.get('aString')).toEqual(undefined); + obj1.set('aString', ['not a string', 'this time']); + obj1.save() + .then(obj1 => { + expect(obj1.get('aString')).toEqual(['not a string', 'this time']); + return new Parse.Query('HasAllPOD').get(obj2.id); + }) + .then(obj2 => { + expect(obj2.get('aString')).toEqual(undefined); + done(); + }); + }) + }); + + it('can delete pointer fields and resave as string', done => { + var obj1 = new Parse.Object('NewClass'); + obj1.save() + .then(() => { + obj1.set('aPointer', obj1); + return obj1.save(); + }) + .then(obj1 => { + expect(obj1.get('aPointer').id).toEqual(obj1.id); + }) + .then(() => config.database.loadSchema()) + .then(schema => schema.deleteField('aPointer', 'NewClass', config.database.db, 'test_')) + .then(() => new Parse.Query('NewClass').get(obj1.id)) + .then(obj1 => { + expect(obj1.get('aPointer')).toEqual(undefined); + obj1.set('aPointer', 'Now a string'); + return obj1.save(); + }) + .then(obj1 => { + expect(obj1.get('aPointer')).toEqual('Now a string'); + done(); + }); + }); }); diff --git a/src/Schema.js b/src/Schema.js index 33dedb9922..ecf1a5b38c 100644 --- a/src/Schema.js +++ b/src/Schema.js @@ -458,20 +458,32 @@ Schema.prototype.deleteField = function(fieldName, className, database, prefix) }); } - let p = null; if (schema.data[className][fieldName].startsWith('relation')) { //For relations, drop the _Join table - - p = database.dropCollection(prefix + '_Join:' + fieldName + ':' + className); + return database.dropCollection(prefix + '_Join:' + fieldName + ':' + className) + //Save the _SCHEMA object + .then(() => this.collection.update({ _id: className }, { $unset: {[fieldName]: null }})); } else { //for non-relations, remove all the data. This is necessary to ensure that the data is still gone //if they add the same field. - p = Promise.resolve(); + return new Promise((resolve, reject) => { + database.collection(prefix + className, (err, coll) => { + if (err) { + reject(err); + } else { + return coll.update({}, { + "$unset": { [fieldName] : null }, + }, { + multi: true, + }) + //Save the _SCHEMA object + .then(() => this.collection.update({ _id: className }, { $unset: {[fieldName]: null }})) + .then(resolve) + .catch(reject); + } + }); + }); } - return p.then(() => { - //Save the _SCHEMA object - return Promise.resolve(); - }); }); }); } From 747f278f2acc3f7ca3c7d7efa2d399892de51a06 Mon Sep 17 00:00:00 2001 From: Drew Gross Date: Wed, 10 Feb 2016 16:25:28 -0800 Subject: [PATCH 4/5] Enable deleting pointer fields, fix tests --- spec/Schema.spec.js | 20 ++++++++++++-------- src/Schema.js | 5 ++++- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/spec/Schema.spec.js b/spec/Schema.spec.js index b8222e5fc3..7f59fec2a2 100644 --- a/spec/Schema.spec.js +++ b/spec/Schema.spec.js @@ -522,28 +522,31 @@ describe('Schema', () => { }); it('can delete string fields and resave as number field', done => { + Parse.Object.disableSingleInstance(); var obj1 = hasAllPODobject(); var obj2 = hasAllPODobject(); var p = Parse.Object.saveAll([obj1, obj2]) .then(() => config.database.loadSchema()) .then(schema => schema.deleteField('aString', 'HasAllPOD', config.database.db, 'test_')) .then(() => new Parse.Query('HasAllPOD').get(obj1.id)) - .then(obj1 => { - expect(obj1.get('aString')).toEqual(undefined); - obj1.set('aString', ['not a string', 'this time']); - obj1.save() - .then(obj1 => { - expect(obj1.get('aString')).toEqual(['not a string', 'this time']); + .then(obj1Reloaded => { + expect(obj1Reloaded.get('aString')).toEqual(undefined); + obj1Reloaded.set('aString', ['not a string', 'this time']); + obj1Reloaded.save() + .then(obj1reloadedAgain => { + expect(obj1reloadedAgain.get('aString')).toEqual(['not a string', 'this time']); return new Parse.Query('HasAllPOD').get(obj2.id); }) - .then(obj2 => { - expect(obj2.get('aString')).toEqual(undefined); + .then(obj2reloaded => { + expect(obj2reloaded.get('aString')).toEqual(undefined); done(); + Parse.Object.enableSingleInstance(); }); }) }); it('can delete pointer fields and resave as string', done => { + Parse.Object.disableSingleInstance(); var obj1 = new Parse.Object('NewClass'); obj1.save() .then(() => { @@ -564,6 +567,7 @@ describe('Schema', () => { .then(obj1 => { expect(obj1.get('aPointer')).toEqual('Now a string'); done(); + Parse.Object.enableSingleInstance(); }); }); }); diff --git a/src/Schema.js b/src/Schema.js index ecf1a5b38c..859ad804d9 100644 --- a/src/Schema.js +++ b/src/Schema.js @@ -471,8 +471,11 @@ Schema.prototype.deleteField = function(fieldName, className, database, prefix) if (err) { reject(err); } else { + var mongoFieldName = schema.data[className][fieldName].startsWith('*') ? + '_p_' + fieldName : + fieldName; return coll.update({}, { - "$unset": { [fieldName] : null }, + "$unset": { [mongoFieldName] : null }, }, { multi: true, }) From 92e9db90649a158be7600b73813d632d2002c1ce Mon Sep 17 00:00:00 2001 From: Drew Gross Date: Wed, 10 Feb 2016 16:31:07 -0800 Subject: [PATCH 5/5] Fix comment --- src/Schema.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Schema.js b/src/Schema.js index 859ad804d9..d8f38499f6 100644 --- a/src/Schema.js +++ b/src/Schema.js @@ -561,7 +561,7 @@ Schema.prototype.getExpectedType = function(className, key) { // 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. Parse would probably +// and make this be a regular value. Schema.prototype.hasClass = function(className) { return this.reload().then(newSchema => !!newSchema.data[className]); }