Skip to content

Add MongoCollection wrapper and move few basic uses of collection to it. #752

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 2, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions src/Adapters/Storage/Mongo/MongoCollection.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@

let mongodb = require('mongodb');
let Collection = mongodb.Collection;

export default class MongoCollection {
_mongoCollection:Collection;

constructor(mongoCollection:Collection) {
this._mongoCollection = mongoCollection;
}

// Does a find with "smart indexing".
// Currently this just means, if it needs a geoindex and there is
// none, then build the geoindex.
// This could be improved a lot but it's not clear if that's a good
// idea. Or even if this behavior is a good idea.
find(query, { skip, limit, sort } = {}) {
return this._rawFind(query, { skip, limit, sort })
.catch(error => {
// Check for "no geoindex" error
if (error.code != 17007 ||
!error.message.match(/unable to find index for .geoNear/)) {
throw error;
}
// Figure out what key needs an index
let key = error.message.match(/field=([A-Za-z_0-9]+) /)[1];
if (!key) {
throw error;
}

var index = {};
index[key] = '2d';
//TODO: condiser moving index creation logic into Schema.js
return this._mongoCollection.createIndex(index)
// Retry, but just once.
.then(() => this._rawFind(query, { skip, limit, sort }));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Everything going to go through a private method now, which will retry find just once.

});
}

_rawFind(query, { skip, limit, sort } = {}) {
return this._mongoCollection
.find(query, { skip, limit, sort })
.toArray();
}

count(query, { skip, limit, sort } = {}) {
return this._mongoCollection.count(query, { skip, limit, sort });
}

drop() {
return this._mongoCollection.drop();
}
}
8 changes: 8 additions & 0 deletions src/Adapters/Storage/Mongo/MongoStorageAdapter.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@

import MongoCollection from './MongoCollection';

let mongodb = require('mongodb');
let MongoClient = mongodb.MongoClient;

Expand Down Expand Up @@ -30,6 +32,12 @@ export class MongoStorageAdapter {
});
}

adaptiveCollection(name: string) {
return this.connect()
.then(() => this.database.collection(name))
.then(rawCollection => new MongoCollection(rawCollection));
}

collectionExists(name: string) {
return this.connect().then(() => {
return this.database.listCollections({ name: name }).toArray();
Expand Down
71 changes: 20 additions & 51 deletions src/Controllers/DatabaseController.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ DatabaseController.prototype.collection = function(className) {
return this.rawCollection(className);
};

DatabaseController.prototype.adaptiveCollection = function(className) {
return this.adapter.adaptiveCollection(this.collectionPrefix + className);
};

DatabaseController.prototype.collectionExists = function(className) {
return this.adapter.collectionExists(this.collectionPrefix + className);
};
Expand Down Expand Up @@ -340,9 +344,8 @@ DatabaseController.prototype.create = function(className, object, options) {
// to avoid Mongo-format dependencies.
// Returns a promise that resolves to a list of items.
DatabaseController.prototype.mongoFind = function(className, query, options = {}) {
return this.collection(className).then((coll) => {
return coll.find(query, options).toArray();
});
return this.adaptiveCollection(className)
.then(collection => collection.find(query, options));
};

// Deletes everything in the database matching the current collectionPrefix
Expand Down Expand Up @@ -378,23 +381,17 @@ function keysForQuery(query) {
// Returns a promise for a list of related ids given an owning id.
// className here is the owning className.
DatabaseController.prototype.relatedIds = function(className, key, owningId) {
var joinTable = '_Join:' + key + ':' + className;
return this.collection(joinTable).then((coll) => {
return coll.find({owningId: owningId}).toArray();
}).then((results) => {
return results.map(r => r.relatedId);
});
return this.adaptiveCollection(joinTableName(className, key))
.then(coll => coll.find({owningId : owningId}))
.then(results => results.map(r => r.relatedId));
};

// Returns a promise for a list of owning ids given some related ids.
// className here is the owning className.
DatabaseController.prototype.owningIds = function(className, key, relatedIds) {
var joinTable = '_Join:' + key + ':' + className;
return this.collection(joinTable).then((coll) => {
return coll.find({relatedId: {'$in': relatedIds}}).toArray();
}).then((results) => {
return results.map(r => r.owningId);
});
return this.adaptiveCollection(joinTableName(className, key))
.then(coll => coll.find({ relatedId: { '$in': relatedIds } }))
.then(results => results.map(r => r.owningId));
};

// Modifies query so that it no longer has $in on relation fields, or
Expand Down Expand Up @@ -443,38 +440,6 @@ DatabaseController.prototype.reduceRelationKeys = function(className, query) {
}
};

// Does a find with "smart indexing".
// Currently this just means, if it needs a geoindex and there is
// none, then build the geoindex.
// This could be improved a lot but it's not clear if that's a good
// idea. Or even if this behavior is a good idea.
DatabaseController.prototype.smartFind = function(coll, where, options) {
return coll.find(where, options).toArray()
.then((result) => {
return result;
}, (error) => {
// Check for "no geoindex" error
if (!error.message.match(/unable to find index for .geoNear/) ||
error.code != 17007) {
throw error;
}

// Figure out what key needs an index
var key = error.message.match(/field=([A-Za-z_0-9]+) /)[1];
if (!key) {
throw error;
}

var index = {};
index[key] = '2d';
//TODO: condiser moving index creation logic into Schema.js
return coll.createIndex(index).then(() => {
// Retry, but just once.
return coll.find(where, options).toArray();
});
});
};

// Runs a query on the database.
// Returns a promise that resolves to a list of items.
// Options:
Expand Down Expand Up @@ -528,8 +493,8 @@ DatabaseController.prototype.find = function(className, query, options = {}) {
}).then(() => {
return this.reduceInRelation(className, query, schema);
}).then(() => {
return this.collection(className);
}).then((coll) => {
return this.adaptiveCollection(className);
}).then(collection => {
var mongoWhere = transform.transformWhere(schema, className, query);
if (!isMaster) {
var orParts = [
Expand All @@ -542,9 +507,9 @@ DatabaseController.prototype.find = function(className, query, options = {}) {
mongoWhere = {'$and': [mongoWhere, {'$or': orParts}]};
}
if (options.count) {
return coll.count(mongoWhere, mongoOptions);
return collection.count(mongoWhere, mongoOptions);
} else {
return this.smartFind(coll, mongoWhere, mongoOptions)
return collection.find(mongoWhere, mongoOptions)
.then((mongoResults) => {
return mongoResults.map((r) => {
return this.untransformObject(
Expand All @@ -555,4 +520,8 @@ DatabaseController.prototype.find = function(className, query, options = {}) {
});
};

function joinTableName(className, key) {
return `_Join:${key}:${className}`;
}

module.exports = DatabaseController;
37 changes: 18 additions & 19 deletions src/Routers/SchemasRouter.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,10 @@ function mongoSchemaToSchemaAPIResponse(schema) {
}

function getAllSchemas(req) {
return req.config.database.collection('_SCHEMA')
.then(coll => coll.find({}).toArray())
.then(schemas => ({response: {
results: schemas.map(mongoSchemaToSchemaAPIResponse)
}}));
return req.config.database.adaptiveCollection('_SCHEMA')
.then(collection => collection.find({}))
.then(schemas => schemas.map(mongoSchemaToSchemaAPIResponse))
.then(schemas => ({ response: { results: schemas }}));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting this in a separate then is actually marginally slower than not doing so, because each then callback happens in a separate turn of the event loop. Very minor difference, but I thought you might want to know (if you end up contributing to the dashboard at all, avoiding extraneous thens is a bigger deal because doing too many can cause a re-render on each turn of the event loop)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(y) That makes sense.
Do you want me to change it here? It is soo beautiful with an extra then :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it is a very small perf impact, so I don't think it really matters. In the dashboard I would ask you to change it :p

}

function getOneSchema(req) {
Expand Down Expand Up @@ -152,7 +151,7 @@ function deleteSchema(req) {
throw new Parse.Error(Parse.Error.INVALID_CLASS_NAME, Schema.invalidClassNameMessage(req.params.className));
}

return req.config.database.collection(req.params.className)
return req.config.database.adaptiveCollection(req.params.className)
.then(collection => {
return collection.count()
.then(count => {
Expand All @@ -161,19 +160,19 @@ function deleteSchema(req) {
}
return collection.drop();
})
.then(() => {
// We've dropped the collection now, so delete the item from _SCHEMA
// and clear the _Join collections
return req.config.database.collection('_SCHEMA')
.then(coll => coll.findAndRemove({_id: req.params.className}, []))
.then(doc => {
if (doc.value === null) {
//tried to delete non-existent class
return Promise.resolve();
}
return removeJoinTables(req.config.database, doc.value);
});
})
})
.then(() => {
// We've dropped the collection now, so delete the item from _SCHEMA
// and clear the _Join collections
return req.config.database.collection('_SCHEMA')
.then(coll => coll.findAndRemove({_id: req.params.className}, []))
.then(doc => {
if (doc.value === null) {
//tried to delete non-existent class
return Promise.resolve();
}
return removeJoinTables(req.config.database, doc.value);
});
})
.then(() => {
// Success
Expand Down