search

cloudfoundry-community / node-cfenv

easy access to your Cloud Foundry application environment for node
Apache License 2.0
73 stars 20 forks source link

don't parse url for getServiceURL unless required #22

Closed pmuellr closed 7 years ago

pmuellr commented 7 years ago

fixes https://github.com/cloudfoundry-community/node-cfenv/issues/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 commented 7 years ago

@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 commented 7 years ago

Thanks for quick fix, now getServiceURL returns correct url.

srl295 commented 7 years ago

after the fact LGTM