From 2a5904b0c247683e6d8454ff911643461ce33cdb Mon Sep 17 00:00:00 2001 From: Drew Gross Date: Thu, 14 Apr 2016 10:16:18 -0700 Subject: [PATCH 1/2] Move "No two geopoints" logic into mongo adapter --- spec/ParseGeoPoint.spec.js | 4 +- .../Storage/Mongo/MongoSchemaCollection.js | 52 ++++++++++++++----- src/Adapters/Storage/Mongo/MongoTransform.js | 21 +++----- src/Schema.js | 13 ++--- 4 files changed, 51 insertions(+), 39 deletions(-) diff --git a/spec/ParseGeoPoint.spec.js b/spec/ParseGeoPoint.spec.js index 7d6a829190..c24ae1ac1f 100644 --- a/spec/ParseGeoPoint.spec.js +++ b/spec/ParseGeoPoint.spec.js @@ -85,8 +85,8 @@ describe('Parse.GeoPoint testing', () => { equal(results.length, 3); done(); }, (err) => { - console.log(err); - fail(); + fail("Couldn't query GeoPoint"); + fail(err) }); }); diff --git a/src/Adapters/Storage/Mongo/MongoSchemaCollection.js b/src/Adapters/Storage/Mongo/MongoSchemaCollection.js index 28fe615d84..1b640caf00 100644 --- a/src/Adapters/Storage/Mongo/MongoSchemaCollection.js +++ b/src/Adapters/Storage/Mongo/MongoSchemaCollection.js @@ -185,21 +185,45 @@ class MongoSchemaCollection { return this._collection.upsertOne(_mongoSchemaQueryFromNameQuery(name, query), update); } - updateField(className: string, fieldName: string, type: string) { - // We don't have this field. Update the schema. - // Note that we use the $exists guard and $set to avoid race - // conditions in the database. This is important! - let query = {}; - query[fieldName] = { '$exists': false }; - let update = {}; - if (typeof type === 'string') { - type = { - type: type + // Add a field to the schema. If database does not support the field + // type (e.g. mongo doesn't support more than one GeoPoint in a class) reject with an "Incorrect Type" + // Parse error with a desciptive message. If the field already exists, this function must + // not modify the schema, and must reject with an error. Exact error format is TBD. If this function + // is called for a class that doesn't exist, this function must create that class. + + // TODO: throw an error if an unsupported field type is passed. Deciding whether a type is supported + // should be the job of the adapter. Some adapters may not support GeoPoint at all. Others may + // Support additional types that Mongo doesn't, like Money, or something. + + // TODO: don't spend an extra query on finding the schema if the type we are trying to add isn't a GeoPoint. + addFieldIfNotExists(className: string, fieldName: string, type: string) { + return this.findSchema(className) + .then(schema => { + // The schema exists. Check for existing GeoPoints. + if (type.type === 'GeoPoint') { + // Make sure there are not other geopoint fields + if (Object.keys(schema.fields).some(existingField => schema.fields[existingField].type === 'GeoPoint')) { + return Promise.reject(new Parse.Error(Parse.Error.INCORRECT_TYPE, 'MongoDB only supports one GeoPoint field in a class.')); + } } - } - update[fieldName] = parseFieldTypeToMongoFieldType(type); - update = {'$set': update}; - return this.upsertSchema(className, query, update); + return Promise.resolve(); + }, error => { + // If error is undefined, the schema doesn't exist, and we can create the schema with the field. + // If some other error, reject with it. + if (error === undefined) { + return Promise.resolve() + } + throw Promise.reject(error); + }) + .then(() => { + // We use $exists and $set to avoid overwriting the field type if it + // already exists. (it could have added inbetween the last query and the update) + return this.upsertSchema( + className, + { [fieldName]: { '$exists': false } }, + { '$set' : { [fieldName]: parseFieldTypeToMongoFieldType(type) } } + ); + }); } get transform() { diff --git a/src/Adapters/Storage/Mongo/MongoTransform.js b/src/Adapters/Storage/Mongo/MongoTransform.js index 6ca8aba59c..bb686fe5e2 100644 --- a/src/Adapters/Storage/Mongo/MongoTransform.js +++ b/src/Adapters/Storage/Mongo/MongoTransform.js @@ -348,23 +348,20 @@ function CannotTransform() {} // Raises an error if this cannot possibly be valid REST format. // Returns CannotTransform if it's just not an atom, or if force is // true, throws an error. -function transformAtom(atom, force, options) { - options = options || {}; - var inArray = options.inArray; - var inObject = options.inObject; +function transformAtom(atom, force, { + inArray, + inObject, +} = {}) { switch(typeof atom) { case 'string': case 'number': case 'boolean': return atom; - case 'undefined': return atom; case 'symbol': case 'function': - throw new Parse.Error(Parse.Error.INVALID_JSON, - 'cannot transform value: ' + atom); - + throw new Parse.Error(Parse.Error.INVALID_JSON, `cannot transform value: ${atom}`); case 'object': if (atom instanceof Date) { // Technically dates are not rest format, but, it seems pretty @@ -379,7 +376,7 @@ function transformAtom(atom, force, options) { // TODO: check validity harder for the __type-defined types if (atom.__type == 'Pointer') { if (!inArray && !inObject) { - return atom.className + '$' + atom.objectId; + return `${atom.className}$${atom.objectId}`; } return { __type: 'Pointer', @@ -404,15 +401,13 @@ function transformAtom(atom, force, options) { } if (force) { - throw new Parse.Error(Parse.Error.INVALID_JSON, - 'bad atom: ' + atom); + throw new Parse.Error(Parse.Error.INVALID_JSON, `bad atom: ${atom}`); } return CannotTransform; default: // I don't think typeof can ever let us get here - throw new Parse.Error(Parse.Error.INTERNAL_SERVER_ERROR, - 'really did not expect value: ' + atom); + throw new Parse.Error(Parse.Error.INTERNAL_SERVER_ERROR, `really did not expect value: ${atom}`); } } diff --git a/src/Schema.js b/src/Schema.js index 880a224f86..e0c99516cb 100644 --- a/src/Schema.js +++ b/src/Schema.js @@ -463,18 +463,11 @@ class Schema { return Promise.resolve(this); } - if (type === 'GeoPoint') { - // Make sure there are not other geopoint fields - for (let otherKey in this.data[className]) { - if (this.data[className][otherKey].type === 'GeoPoint') { - throw new Parse.Error( - Parse.Error.INCORRECT_TYPE, - 'there can only be one geopoint field in a class'); - } - } + if (typeof type === 'string') { + type = { type }; } - return this._collection.updateField(className, fieldName, type).then(() => { + return this._collection.addFieldIfNotExists(className, fieldName, type).then(() => { // The update succeeded. Reload the schema return this.reloadData(); }, () => { From 0fa8c0be7c7a63a3c027058f6c71219af705c1ec Mon Sep 17 00:00:00 2001 From: Drew Gross Date: Mon, 18 Apr 2016 16:51:03 -0700 Subject: [PATCH 2/2] Semicolon --- src/Adapters/Storage/Mongo/MongoSchemaCollection.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Adapters/Storage/Mongo/MongoSchemaCollection.js b/src/Adapters/Storage/Mongo/MongoSchemaCollection.js index 1b640caf00..fa0db6e3c9 100644 --- a/src/Adapters/Storage/Mongo/MongoSchemaCollection.js +++ b/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] === '*') { @@ -211,7 +211,7 @@ class MongoSchemaCollection { // If error is undefined, the schema doesn't exist, and we can create the schema with the field. // If some other error, reject with it. if (error === undefined) { - return Promise.resolve() + return Promise.resolve(); } throw Promise.reject(error); })