Skip to content
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

db can now be switched dynamically on the adapter #89

Merged
merged 9 commits into from
Jan 16, 2016

Conversation

olivierchatry
Copy link
Contributor

This patch allows to switch dynamically a pouch DB on the adapter. This can be very useful if you have one model that can represent different databases.

@rsutphin
Copy link
Collaborator

rsutphin commented Sep 2, 2015

Thanks for putting this together, @olivierchatry. Can you give some more details on why you want to do this? I don't understand the use case.

@olivierchatry
Copy link
Contributor Author

Hey ! Sure :)
So I'm making a software that have many different clients. All these clients have many different projects. All of these projects needs to be very well separated ( even on a user basis ).

So what I have done is that I have a private DB that can only be accessed from a backend. The backend then create a couchdb for each project, and set the right roles for a given user. ( my users are actually glorified couchdb users).

When the user browse the projects, he do not actually open any project, but just browse through what the backend return from the private db.

When a user open a project, I actually dynamically change the database (database URL is given by the backend ) object inside the project adapter. This is working quite well right now ( as long as you are sure that their will be only one active project at a time, which is my case, and I would guess the most common case).

This allows me to leverage couch security system, as well as having offline possibility with pouch, and the user only have on the local computer what he really needs to have access to.

Note that, on the store side, I have also to clear the already loaded project with :

this.store.unloadAll('project')

@olivierchatry
Copy link
Contributor Author

So ? What do you think ? Is it good enough or should we close the request ?

@@ -81,7 +87,8 @@ export default DS.RESTAdapter.extend({
_init: function (store, type) {
var self = this,
recordTypeName = this.getRecordTypeName(type);
if (!this.get('db') || typeof this.get('db') !== 'object') {
var db = this.get('db');
if (!db || typeof db !== 'object') {
Copy link
Member

Choose a reason for hiding this comment

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

^ makes sense

@nolanlawson
Copy link
Member

@olivierchatry This sounds like a great change to me, although if it's an API-level change, then it would also be good to add a section to the README explaining how to use it.

The other thing I'd say is that it seems there are a few unrelated changes in here (e.g. code cleanup, ember-data beta 19 fixes), so maybe it would be helpful to break this up into a few smaller PRs? That way, we can just deal with the proposed new feature (swapping out databases), while considering the other changes separately. Does that make sense?

for (var i = 0, len = this._schema.length; i < len; i++) {
var currentSchemaDef = this._schema[i];
if (currentSchemaDef.singular === singular) {
return;
isSchemaNew = false;
break;
Copy link
Member

Choose a reason for hiding this comment

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

^ this code doesn't look equivalent to me. can you explain it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the main problem is that the model name is the same, ( for me 'project' ) which means that the schema does not need to be recreated BUT needs to be applied to the new DB instance ( or else 'rel' is not defined ). That the goal of this code and the code below.

@rsutphin
Copy link
Collaborator

rsutphin commented Sep 5, 2015

I appreciate you providing some more context for this feature.

The tricky thing I think is the interaction with the store, as you alluded to at the end of your description. With this change alone, the store will still reflect any records that were loaded from the previous database. Updates to those records will be saved into the new database, which seems like it would probably surprise the user. Since saves are async, it's possible you'd still get records winding up in the wrong place, even if you had some awareness of the potential for this problem.

What if, instead of just observing the db property being set, this feature was activated by a method on the adapter? (changeDb?) This would allow for cleaning up the old change listener and for automatically unloading all the records the adapter knows (about along with the other necessary behavior).

@olivierchatry
Copy link
Contributor Author

Yeah that would make more sense I think. I will wrap that inside a function and see if it works.

@broerse
Copy link
Collaborator

broerse commented Sep 5, 2015

Are open and close functions not more what we want. We can build a changeDB on top of them. If we have more projects (opening more Excel sheets) this will also work. Anyway I like this already.

@olivierchatry
Copy link
Contributor Author

  • refactored the code so that the pull request is more contained.
  • changing database is now done using changeDb function, which take cares of cleaning up what needs to be cleaned.

@rsutphin
Copy link
Collaborator

@olivierchatry, this stalled because of the test failures after your last few changes. I think the test failures were caused when you fixed those deprecation warnings — this broke backwards compatibility with some of the versions of ember-data we supported in version 2.x of the adapter.

Good news is that we dropped compatibility with those old versions for version 3.0 of the adapter. If you could rebase and squash on top of current master, we could pick this up again. Thanks for your efforts so far.

# Conflicts:
#	addon/adapters/pouch.js
@olivierchatry
Copy link
Contributor Author

Well, not sure what to do ( seems like the build fails because of a dependency that was not fetched ). Should I fake commit to relauch ? or we can close the PR and open it up again ?

@backspace
Copy link
Collaborator

You could git commit --allow-empty, push the branch, see the results, and then remove that commit and force push.

@rsutphin
Copy link
Collaborator

I manually retried the build. Let's see what happens with that.

@backspace
Copy link
Collaborator

🎉

@@ -183,6 +183,31 @@ The value for `documentType` is the camelCase version of the primary model name.

For best results, only create/update records using the full model definition. Treat the others as read-only.

## Multiple databases for the same model

In some cases it might diserable (security related, where you want a given user to only have some informations stored on his computer) to have multiple databases for the same model of data.
Copy link
Member

Choose a reason for hiding this comment

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

typo: desirable

@nolanlawson
Copy link
Member

And it's green! :) 👍

@nolanlawson
Copy link
Member

Ah, unfortunately there's a merge conflict, and I'm not really sure what to do in this case:

++<<<<<<< d09cb4c3ed0fab63eca17233efd442f50b989c7b
 +    var store = this.store;
++=======
+     var store = this.container.lookup('service:store');
++>>>>>>> fixes livesync for ember data 2.x

@olivierchatry
Copy link
Contributor Author

Set to this.store I would guess.
On Dec 19, 2015 19:20, "Nolan Lawson" notifications@github.com wrote:

Ah, unfortunately there's a merge conflict, and I'm not really sure what
to do in this case:

++<<<<<<< d09cb4c3ed0fab63eca17233efd442f50b989c7b

  • var store = this.store;++=======+ var store = this.container.lookup('service:store');++>>>>>>> fixes livesync for ember data 2.x


Reply to this email directly or view it on GitHub
#89 (comment)
.

@tamebadger
Copy link

This is really awesome! What needs to happen to get this merged ?

@nolanlawson
Copy link
Member

I'm gonna rebase to double-check that the tests are still passing, but then I'm happy to merge.

@nolanlawson nolanlawson mentioned this pull request Jan 16, 2016
@nolanlawson
Copy link
Member

If #107 passes, then we're good to go.

@nolanlawson
Copy link
Member

And it's green! :)

nolanlawson added a commit that referenced this pull request Jan 16, 2016
db can now be switched dynamically on the adapter
@nolanlawson nolanlawson merged commit 461f90b into pouchdb-community:master Jan 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants