Feature/database-manager : Allow sessions and users to be stored in different databases

[Aetherall]

No description provided.

[codecov-io]

Codecov Report

Merging #174 into master will increase coverage by 0.3%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #174     +/-   ##
=========================================
+ Coverage   93.56%   93.86%   +0.3%     
=========================================
  Files          48       50      +2     
  Lines        1274     1337     +63     
  Branches      176      173      -3     
=========================================
+ Hits         1192     1255     +63     
  Misses         71       71             
  Partials       11       11

Impacted Files	Coverage Δ
packages/database-manager/src/database-manager.ts	100% <100%> (ø)
packages/database-manager/src/index.ts	100% <100%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43ed826...ea833cc. Read the comment docs.

[TimMikeladze]

Code looks fine.

About the general design though: Instead of using a separate package to handle routing the function call to the correct database, why not make the distinction between user data and session data within the @accounts/core?

For example:

const accounts = new AccountsServer({
	db: {
		sessions: new Redis(),
		users: new Mongo()
	}
});

Then within AccountsServer modify all the calls to this.db to use either this.db.sessions or this.db.users.

This approach would eliminate the need for having to maintain another package.

What are your thoughts on this @Aetherall ?

[Aetherall]

I think it would be a good idea if we were not planning on extend accounts-js as we want to...

If we are going this direction, the AccountsServer will get quickly messy, and we will need to take care of the difference in all Authservices.

By having a separate package, maybe it will need to be changed too whenever we decide to change the DatabaseInterface related packages, but we will not need to change the authServices.

And in the current setup, it is easy to update that one, it took me 30s to make it compatible with TwoFactor ( copy paste the setService method and change setService to unsetService, then do the same in the tests )

Also, what about the case where we use a single database ? Handling this in AccountsServer would result on many additions

One goal I have ATM is to simplify the content of AccountsServer

[TimMikeladze]

I think it would be a good idea if we were not planning on extend accounts-js as we want to...

If we are going this direction, the AccountsServer will get quickly messy, and we will need to take care of the difference in all Authservices.

Can you provide me a real world example of where having the database manager would be beneficial? I'm failing to understand where exactly it improves things as opposed to handling the same logic within AccountsServer with far less code.

Also, what about the case where we use a single database ? Handling this in AccountsServer would result on many additions

Handling a single database would work like this:

const accounts = new AccountsServer({
	db: new Mongo
});

// inside AccountsServer constructor

constructor(options) {
    // improve logic and validation, just an example
	if (options.db instanceof DatabaseInterface) {
      this.db.sessions = options.db;
      this.db.users = options.db;
    } else if (options.db.sessions && options.db.users) {
       this.db.sessions = this.db.sessions;
       this.db.users = this.db.users;
    }
}

[Aetherall]

Both methods would work the same in the end, but I made this package so we don't have to handle those cases in all future packages

the setStore function would still work on the AuthServices, and no need to rewrite packages

With the database manager, the usage of two database is transparent for the AccountsServer as well as for the AuthenticationServices

It is a matter of point of view, I prefer the splitting to be transparent to the rest of the suite, but I guess we should ask for @davidyaha and @pradel opinions

[davidyaha]

I like the transparency aspect of the DBManager class.

[TimMikeladze]

Alright, I'm on board. @Aetherall good to merge?

[Aetherall]

Yep, good to me !