jcoglan / restore

Simple remoteStorage server written in Node.js
293 stars 27 forks source link

Add MongoDB adapter #7

Closed agrueneberg closed 10 years ago

agrueneberg commented 11 years ago

This pull request adds support for MongoDB :smile: The code passes the test suite, but has not been tested in the wild yet and is still missing locks. Thought I'd put this here for discussion and feedback.

jcoglan commented 11 years ago

Firstly: awesome, thank you, this is terrific. However, it should not be in core. What I'm shooting for here is an architecture where anyone can bring in storage adapters from outside and have them just work, and just supply a couple of baked in adapters myself. So:

I realize the second point requires a level of stability that we may not currently have arrived at, but I'd like your thoughts.

Basically, I want you to be able to build your driver, and have it be stable, without my explicit blessing. If it goes in core, then that turns me into a roadblock on updates, which sucks for all concerned.

agrueneberg commented 11 years ago

I fully agree with you: one project simply doesn't scale. On the one hand I felt bad when I added mongodb as a dependency to core, on the other hand I would have loved to pull in more dependencies such as a promises library.

The good thing is that I didn't have to make any modifications to core at all, except for adding my adapter to the test suite. First I was thinking about a registerStorageAdapter API, but the current solution is already pluggable and allows you to just provide your own adapter as part of the reStore constructor call.

The adapter interface is pretty simple, and all I had to do was to implement createUser, authenticate, authorize, revokeAccess, permissions, get, put, and delete. However, those functions are non-trivial, and providing access to the (excellent) test suite is therefore a must. I'm not sure yet what the best approach for this would be. More documentation about function and callback parameters would certainly help, too.

Anyway, I'll go ahead and create a separate project for this. Maybe we can find some convention and call it restore-mongodb on both GitHub and NPM?

agrueneberg commented 11 years ago

I started moving the adapter into a separate project and realized that I need access to the lib/stores/core module. I started looking for projects with an architecture similar to what we are planning to do and found https://github.com/e14n/databank and https://github.com/jaredhanson/passport. Will spend some time to see how they did it.

jcoglan commented 11 years ago

You shouldn't use lib/stores/core.js, that module is not considered part of the public API. It's just stuff that happens to be shared between the stores that ship with restore, not anything that's a fundamental shared concept.

jcoglan commented 11 years ago

For the test suite -- on projects where I've employed a similar architecture (e.g. Faye) I just include the project containing the tests as a submodule of my plugin library. That's how the Redis plugins for Faye are developed.

agrueneberg commented 11 years ago

Thanks for your feedback, James. Here's the first draft based on Faye's architecture: https://github.com/agrueneberg/restore-mongodb For now I just copied over lib/stores/core.js. No NPM release yet, there is still a lot of stuff to do.

jcoglan commented 10 years ago

Closing since there's no work to be done in this repo (correct me if I'm wrong). Thanks for the new adapter, it's great seeing pluggable architecture work for people :)