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

don't parse url for getServiceURL unless required #22

Merged
merged 1 commit into from
Jan 13, 2017
Merged

Conversation

pmuellr
Copy link
Member

@pmuellr pmuellr commented Jan 13, 2017

fixes #21

Turns out the following is not always true:

  url.format(url.parse(SomeURL)) == SomeURL

At least when the protocol isn't http: or https:. cfenv expects this
to be true in getServiceURL(), when using the replacements argument. It
seems to break for some example mongodb urls, like the one added to the
test in this commit.

Fix for now is that if there aren't any replacement values, then return early
with the currently calculated URL. So it would still break if you passed
one of these not-parsed-correctly URLs and replacements, but the odds of that
seem pretty slim.

Also noticed that url.format() must be standardizing URLs to return them with
a trailing / if there was no other path. So now you don't always get that
trailing /. Seems survivable.

@pmuellr
Copy link
Member Author

pmuellr commented Jan 13, 2017

@melnikaite do you want to give this a try before I create the new version on npm?

To install from the branch with the fix, you'd specify the version in your package.json like this:

  "dependencies": {
    "cfenv": "github:cloudfoundry-community/node-cfenv#issue-21"
  }

@melnikaite
Copy link

Thanks for quick fix, now getServiceURL returns correct url.

fixes #21

Turns out the following is not always true:

```js
  url.format(url.parse(SomeURL)) == SomeURL
```

At least when the protocol isn't `http:` or `https:`.  cfenv expects this
to be true in `getServiceURL()`, when using the replacements argument.  It
seems to break for some example mongodb urls, like the one added to the
test in this commit.

Fix for now is that if there aren't any replacement values, then return early
with the currently calculated URL.  So it would still break if you passed
one of these not-parsed-correctly URLs and replacements, but the odds of that
seem pretty slim.

Also noticed that `url.format()`` must be standardizing URLs to return them with
a trailing `/` if there was no other path.  So now you don't always get that
trailing `/`. Seems survivable.
@srl295
Copy link

srl295 commented Feb 2, 2017

after the fact LGTM

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.

getServiceURL break url
3 participants