getServiceURL break url #21
Comments
|
Ah, yikes! smells like url.parse() shenanigans, since I just ran into something very similar in a different set of code. Thanks for reporting! |
pmuellr
pushed a commit
that referenced
this issue
Jan 13, 2017
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.
pmuellr
pushed a commit
that referenced
this issue
Jan 13, 2017
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.
|
It'd be nice to have this fix in |
|
@melnikaite sorry, was waiting for a review in the 1.0.4 PR #23 . Ping'd reviewer again, will update tomorrow if I haven't heard back. |
|
@melnikaite there's now a fresh 1.0.4 up on npm |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
console.log(appEnv.getServiceURL('mongodb-instance'));console.log(appEnv.getService('mongodb-instance').credentials.uri);mongodb://admin:XXX@sl-us-dal-9-portal.4.dblayer.com:18394/:18394,sl-us-dal-9-portal.1.dblayer.com/admin?ssl=truemongodb://admin:XXX@sl-us-dal-9-portal.4.dblayer.com:18394,sl-us-dal-9-portal.1.dblayer.com:18394/admin?ssl=trueThe text was updated successfully, but these errors were encountered: