getServiceURL break url #21

Closed
melnikaite opened this Issue Jan 11, 2017 · 4 comments

Comments

Projects
None yet
2 participants
@melnikaite

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=true

mongodb://admin:XXX@sl-us-dal-9-portal.4.dblayer.com:18394,sl-us-dal-9-portal.1.dblayer.com:18394/admin?ssl=true

@pmuellr

This comment has been minimized.

Show comment
Hide comment
@pmuellr

pmuellr Jan 13, 2017

Member

Ah, yikes! smells like url.parse() shenanigans, since I just ran into something very similar in a different set of code.

Thanks for reporting!

Member

pmuellr commented Jan 13, 2017

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 added a commit that referenced this issue Jan 13, 2017

don't parse url for getServiceURL unless required
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 added a commit that referenced this issue Jan 13, 2017

don't parse url for getServiceURL unless required
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 pmuellr closed this in #22 Jan 13, 2017

@melnikaite

This comment has been minimized.

Show comment
Hide comment
@melnikaite

melnikaite Feb 1, 2017

It'd be nice to have this fix in 1.0.4.
Are there any plans to make new release?

It'd be nice to have this fix in 1.0.4.
Are there any plans to make new release?

@pmuellr

This comment has been minimized.

Show comment
Hide comment
@pmuellr

pmuellr Feb 1, 2017

Member

@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.

Member

pmuellr commented Feb 1, 2017

@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.

@pmuellr

This comment has been minimized.

Show comment
Hide comment
@pmuellr

pmuellr Feb 2, 2017

Member

@melnikaite there's now a fresh 1.0.4 up on npm

Member

pmuellr commented Feb 2, 2017

@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