Skip to content

RTDB accepts multi region URL #3050

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.

Sign up for GitHub

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 17 commits into from
May 13, 2020
Merged

RTDB accepts multi region URL #3050

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
9b8355e
test
fredzqm May 11, 2020
434ad87
Merge remote-tracking branch 'origin/master' into fz/multi-region-url
fredzqm May 11, 2020
fde052e
pass
fredzqm May 11, 2020
e223614
pass
fredzqm May 11, 2020
1494388
generalize
fredzqm May 11, 2020
1488424
pass
fredzqm May 11, 2020
2e8e3d7
check localhost first
fredzqm May 11, 2020
b7146d5
m
fredzqm May 12, 2020
343b1f4
m
fredzqm May 12, 2020
e5bf5ea
m
fredzqm May 12, 2020
5a368ac
m
fredzqm May 12, 2020
92dc48b
x
fredzqm May 12, 2020
86e50b9
at least 2
fredzqm May 12, 2020
756b239
more tests
fredzqm May 12, 2020
0240e17
more
fredzqm May 12, 2020
02c5c8b
Update CHANGELOG.md
fredzqm May 12, 2020
46b6d50
Update CHANGELOG.md
fredzqm May 12, 2020
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
22 changes: 11 additions & 11 deletions packages/database/src/core/util/libs/parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2017 Google Inc.
* Copyright 2017 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -72,7 +72,7 @@ export const parseRepoInfo = function(
const parsedUrl = parseDatabaseURL(dataURL),
namespace = parsedUrl.namespace;

if (parsedUrl.domain === 'firebase') {
if (parsedUrl.domain === 'firebase.com') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this a breaking change for users that use firebase.com?ns=foo?

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind. This is only breaking for firebase/ns?foo which shouldn't affect anyone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe in the past. *.firebase.* will hit this.

My guess is that RTDB was served in firebase.com instead of firebaseio.com in the past.

Added a test for this.

fatal(
parsedUrl.host +
' is no longer supported. ' +
Expand Down Expand Up @@ -174,17 +174,17 @@ export const parseDatabaseURL = function(
colonInd = dataURL.length;
}

const parts = host.split('.');
if (parts.length === 3) {
const hostWithoutPort = host.slice(0, colonInd);
Copy link
Contributor

Choose a reason for hiding this comment

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

From the slice docs:

"A negative index can be used, indicating an offset from the end of the sequence. slice(2,-1) extracts the third element through the second-to-last element in the sequence."

So, in this case, if no port is specified it seems like we would drop the last character, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If no port is specified, colonInd = host.length above, so host.slice(0, host.length) ==> host

if (hostWithoutPort.toLowerCase() === 'localhost') {
domain = 'localhost';
} else if (hostWithoutPort.split('.').length <= 2) {
domain = hostWithoutPort; // Domain is at least 2 parts.
} else {
const dotInd = host.indexOf('.');
domain = host.substring(dotInd + 1);
subdomain = host.substring(0, dotInd).toLowerCase();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Flip two lines above to parse string consecutively.

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

// Normalize namespaces to lowercase to share storage / connection.
domain = parts[1];
subdomain = parts[0].toLowerCase();
// We interpret the subdomain of a 3 component URL as the namespace name.
namespace = subdomain;
} else if (parts.length === 2) {
domain = parts[0];
} else if (parts[0].slice(0, colonInd).toLowerCase() === 'localhost') {
domain = 'localhost';
}
// Always treat the value of the `ns` as the namespace name if it is present.
if ('ns' in queryParams) {
Expand Down
11 changes: 10 additions & 1 deletion packages/database/test/database.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2017 Google Inc.
* Copyright 2017 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -62,6 +62,15 @@ describe('Database Tests', () => {
expect(db.ref().toString()).to.equal('https://foo.bar.com/');
});

it('Can get database with multi-region URL', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to add a test with a non-lowercase namespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds like a good idea.

const db = defaultApp.database('http://foo.euw1.firebasedatabase.app');
expect(db).to.be.ok;
expect(db.repo_.repoInfo_.namespace).to.equal('foo');
expect(db.ref().toString()).to.equal(
'https://foo.euw1.firebasedatabase.app/'
);
});

it('Can get database with localhost URL and port', () => {
const db = defaultApp.database('http://localhost:80');
expect(db).to.be.ok;
Expand Down