Skip to content

feat(database): enable database multi-resource support #159

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 2 commits into from
Sep 14, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
6 changes: 4 additions & 2 deletions src/database.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export function registerDatabase(instance: FirebaseNamespace) {
// Register the Database Service with the 'firebase' namespace.
const namespace = instance.INTERNAL.registerService(
'database',
app => RepoManager.getInstance().databaseFromApp(app),
(app, unused, url) => RepoManager.getInstance().databaseFromApp(app, url),
// firebase.database namespace properties
{
Reference,
Expand All @@ -39,7 +39,9 @@ export function registerDatabase(instance: FirebaseNamespace) {
INTERNAL,
ServerValue: Database.ServerValue,
TEST_ACCESS
}
},
null,
true
);

if (isNodeSdk()) {
Expand Down
2 changes: 1 addition & 1 deletion src/database/core/Repo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ export class Repo {
* @param {!FirebaseApp} app
*/
constructor(
private repoInfo_: RepoInfo,
public repoInfo_: RepoInfo,
forceRestClient: boolean,
public app: FirebaseApp
) {
Expand Down
46 changes: 32 additions & 14 deletions src/database/core/RepoManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,12 @@ let _staticInstance: RepoManager;
*/
export class RepoManager {
/**
* @private {!Object.<string, !Repo>}
* @private {!Object.<string, Object<string, !fb.core.Repo>>}
*/
private repos_: {
[name: string]: Repo;
[name: string]: {
[name: string]: Repo;
Copy link
Contributor

Choose a reason for hiding this comment

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

can you use something more specific than "name" for these? I think it's maybe appName and dbUrl or something ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, no problem.

};
} = {};

/**
Expand All @@ -55,14 +57,18 @@ export class RepoManager {

// TODO(koss): Remove these functions unless used in tests?
interrupt() {
for (const repo in this.repos_) {
this.repos_[repo].interrupt();
for (const instance in this.repos_) {
for (const repo in this.repos_[instance]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be:
"instance" => "appName"
"repo" => "dbUrl"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, changed this and the function below.

this.repos_[instance][repo].interrupt();
}
}
}

resume() {
for (const repo in this.repos_) {
this.repos_[repo].resume();
for (const instance in this.repos_) {
for (const repo in this.repos_[instance]) {
this.repos_[instance][repo].resume();
}
}
}

Expand All @@ -72,8 +78,8 @@ export class RepoManager {
* @param {!FirebaseApp} app
* @return {!Database}
*/
databaseFromApp(app: FirebaseApp): Database {
const dbUrl: string = app.options[DATABASE_URL_OPTION];
databaseFromApp(app: FirebaseApp, url?: string): Database {
const dbUrl: string = url || app.options[DATABASE_URL_OPTION];
if (dbUrl === undefined) {
fatal(
"Can't determine Firebase Database URL. Be sure to include " +
Expand Down Expand Up @@ -104,12 +110,15 @@ export class RepoManager {
* @param {!Repo} repo
*/
deleteRepo(repo: Repo) {
const appRepos = safeGet(this.repos_, repo.app.name);
// This should never happen...
if (safeGet(this.repos_, repo.app.name) !== repo) {
fatal('Database ' + repo.app.name + ' has already been deleted.');
if (!appRepos || safeGet(appRepos, repo.repoInfo_.toURLString()) !== repo) {
fatal(
`Database ${repo.app.name}(${repo.repoInfo_}) has already been deleted.`
);
}
repo.interrupt();
delete this.repos_[repo.app.name];
delete appRepos[repo.repoInfo_.toURLString()];
}

/**
Expand All @@ -121,12 +130,21 @@ export class RepoManager {
* @return {!Repo} The Repo object for the specified server / repoName.
*/
createRepo(repoInfo: RepoInfo, app: FirebaseApp): Repo {
let repo = safeGet(this.repos_, app.name);
let appRepos = safeGet(this.repos_, app.name);

if (!appRepos) {
appRepos = {};
this.repos_[app.name] = appRepos;
}

let repo = safeGet(appRepos, repoInfo.toURLString());
if (repo) {
fatal('FIREBASE INTERNAL ERROR: Database initialized multiple times.');
fatal(
'Database initialized multiple times. Please make sure the format of the database URL matches with each database() call.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively we could just return the existing instance. Would that cause other problems though? I guess app would end up with multiple references to it and might call INTERNAL.delete() multiple times, etc.?

I guess I don't really feel strongly. It's probably not too harmful to make them pass the exact same URL every time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I don't know that I'm terribly heartbroken making users pass the extra string around, especially considering that they can still just use the project config instead.

);
}
repo = new Repo(repoInfo, this.useRestClient_, app);
this.repos_[app.name] = repo;
appRepos[repoInfo.toURLString()] = repo;

return repo;
}
Expand Down
45 changes: 45 additions & 0 deletions tests/database/database.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,51 @@ describe('Database Tests', function() {
}).to.throw(/don't call new Database/i);
});

it('Can get database with custom URL', function() {
var db = defaultApp.database('http://foo.bar.com');
expect(db).to.be.ok;
// The URL is assumed to be secure if no port is specified.
expect(db.ref().toString()).to.equal('https://foo.bar.com/');
});

it('Can get database with custom URL and port', function() {
var db = defaultApp.database('http://foo.bar.com:80');
expect(db).to.be.ok;
expect(db.ref().toString()).to.equal('http://foo.bar.com:80/');
});

it('Can get database with https URL', function() {
var db = defaultApp.database('https://foo.bar.com');
expect(db).to.be.ok;
expect(db.ref().toString()).to.equal('https://foo.bar.com/');
});

it('Different instances for different URLs', function() {
var db1 = defaultApp.database('http://foo1.bar.com');
var db2 = defaultApp.database('http://foo2.bar.com');
expect(db1.ref().toString()).to.equal('https://foo1.bar.com/');
expect(db2.ref().toString()).to.equal('https://foo2.bar.com/');
});

it('Cannot use same URL twice', function() {
defaultApp.database('http://foo.bar.com');
expect(function() {
defaultApp.database('http://foo.bar.com/');
}).to.throw(/Database initialized multiple times/i);
});

it('Databases with invalid custom URLs', function() {
expect(function() {
defaultApp.database('not-a-url');
}).to.throw(/Cannot parse Firebase url/i);
expect(function() {
defaultApp.database('http://fblocal.com');
}).to.throw(/Cannot parse Firebase url/i);
expect(function() {
defaultApp.database('http://x.fblocal.com:9000/paths/are/bad');
}).to.throw(/Database URL must point to the root of a Firebase Database/i);
});

it('Can get app', function() {
const db = firebase.database();
expect(db.app).to.not.be.undefined;
Expand Down