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

Allow localhost database URL #426

Merged
merged 5 commits into from Jan 22, 2018

Conversation

@rynobax
Copy link
Contributor

@rynobax rynobax commented Jan 11, 2018

This change was discussed in #116.

@googlebot
Copy link

@googlebot googlebot commented Jan 11, 2018

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

Loading

@rynobax
Copy link
Contributor Author

@rynobax rynobax commented Jan 11, 2018

I signed it!

Loading

@googlebot
Copy link

@googlebot googlebot commented Jan 11, 2018

CLAs look good, thanks!

Loading

@@ -131,6 +131,8 @@ export const parseURL = function(
subdomain = parts[0].toLowerCase();
} else if (parts.length === 2) {
domain = parts[0];
} else if (parts[0].split(':')[0].toLowerCase() === 'localhost') {
subdomain = 'localhost';
Copy link
Member

@schmidt-sebastian schmidt-sebastian Jan 15, 2018

Choose a reason for hiding this comment

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

Can you move the port extraction logic from line 136 up so that it happens before this if-statement? Then we won't have to double parse the port number.

Then I would also suggest that instead of assigning 'localhost' to subdomain you should assign it to domain. A subdomain would be something like 'foo' in 'foo.localhost'.

Loading

@rynobax
Copy link
Contributor Author

@rynobax rynobax commented Jan 18, 2018

I believe I made the change you requested about parsing the port number, let me know if that is not what you had in mind.

I also changed it to set the domain to localhost. However, the subdomain also needs to be set to something, as it is used for the 'namespace' variable, which cannot be empty. For now it also sets the subdomain to 'localhost', as I am not entirely sure of what the namespace is used for.

Loading

@schmidt-sebastian
Copy link
Member

@schmidt-sebastian schmidt-sebastian commented Jan 20, 2018

I also changed it to set the domain to localhost. However, the subdomain also needs to be set to something, as it is used for the 'namespace' variable, which cannot be empty. For now it also sets the subdomain to 'localhost', as I am not entirely sure of what the namespace is used for.

Thanks, the changes look mostly good. The namespace is used by our backend if you are not connecting the intended host. For mocking, this should not be needed. It seems like you changed the assert to no longer fire AND you set the subdomain to localhost? Can you explain why we need to do both - otherwise I would just remove the setting of the subdomain and we should be good to submit this PR.

Loading

@rynobax
Copy link
Contributor Author

@rynobax rynobax commented Jan 20, 2018

If the subdomain is not set, the test fails with the error Error: Invalid Firebase Database URL failed: First argument must be a valid firebase URL and the path can't contain ".", "#", "$", "[", or "]".

The error seems to be triggered by this line, because the namespace is an empty string:

!isValidKey(parsedUrl.repoInfo.namespace) ||

If I remove that line all the tests pass.

The two options that come to my mind are:

  1. Do not set the subdomain on localhost domains
  2. In validation.ts, skip the namepace validity check if the host is localhost

Or:

  1. Set the subdomain on localhost domains
  2. Remove the extra check I added to the assert

Loading

@schmidt-sebastian
Copy link
Member

@schmidt-sebastian schmidt-sebastian commented Jan 20, 2018

Do not set the subdomain on localhost domains
In validation.ts, skip the namespace validity check if the host is localhost

Please go ahead and do this. It seems cleaner.

Loading

@rynobax
Copy link
Contributor Author

@rynobax rynobax commented Jan 20, 2018

I made the changes.

Loading

@schmidt-sebastian
Copy link
Member

@schmidt-sebastian schmidt-sebastian commented Jan 22, 2018

LGTM. Note that this won't work for localhost URLs that don't have a port set. I will send you a follow up PR to add support for this.

Loading

@schmidt-sebastian schmidt-sebastian merged commit c653c56 into firebase:master Jan 22, 2018
2 checks passed
Loading
schmidt-sebastian added a commit that referenced this issue Feb 2, 2018
* Allow localhost database URL

* Setting domain on localhost server address

* [AUTOMATED]: Prettier Code Styling

* Allow namespace to be invalid if the host is localhost

* [AUTOMATED]: Prettier Code Styling
urish added a commit to urish/firebase-server that referenced this issue May 19, 2018
@firebase firebase locked and limited conversation to collaborators Oct 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants