Skip to content

fix: trailing slash in request url & remove whitespace when parsing external config properties#77

Merged
jorge-ibm merged 1 commit intomasterfrom
fix-trailing-slash
May 1, 2020
Merged

fix: trailing slash in request url & remove whitespace when parsing external config properties#77
jorge-ibm merged 1 commit intomasterfrom
fix-trailing-slash

Conversation

@jorge-ibm
Copy link
Copy Markdown
Contributor

ref: https://github.ibm.com/arf/planning-sdk-squad/issues/1738, https://github.ibm.com/arf/planning-sdk-squad/issues/1742

Changes/fixes:

  • Empty path segments "" are no longer added to the request url to prevent the java request url builder library from adding a / in place of the empty path segment
  • When parsing a credentials file (vcap or environment file), the credential's key/value pairs are trimmed of whitespace

*/
private static void addToMap(Map<String, String> map, String key, String value) {
if (StringUtils.isNotEmpty(value)) {
value = value.trim();
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@padamstx - I went ahead and added this when parsing vcap creds as well.

@jorge-ibm jorge-ibm requested review from mkistler and padamstx April 30, 2020 21:26
@jorge-ibm jorge-ibm added the ready-for-review PR is ready for a review label Apr 30, 2020
Copy link
Copy Markdown
Contributor

@padamstx padamstx left a comment

Choose a reason for hiding this comment

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

Changes look good, I just have one question about the path segment stuff.

String[] pathParameters = { "param1", "param2" };
HttpUrl url = RequestBuilder.constructHttpUrl("https://myserver.com/testservice/api", pathSegments, pathParameters);
assertNotNull(url);
assertEquals("https://myserver.com/testservice/api/param1", url.toString());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you check to see the result of this test BEFORE your fix was applied?
I'm sort of wondering whether we'd end up with a url of https://myserver.com/testservice/api/param1/param2.
I can imagine a path like "/{param1}/{param2}" which would result in "" within the pathSegments list, but {"param1", "param2" in the pathParameters list (I think). And in that case I think we'd expect the url to be https://myserver.com/testservice/api/param1/param2, wouldn't we?
I realize this might be a corner case, but I can't help myself from always bringing those up :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I just tested this without my changes and the result is still the same https://myserver.com/testservice/api/param1. The / most likely got added by the empty path segment, then the library saw that param1 didn't need a a / in front of it

Copy link
Copy Markdown
Contributor

@mkistler mkistler left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@jorge-ibm jorge-ibm merged commit 57382f1 into master May 1, 2020
ibm-devx-automation pushed a commit that referenced this pull request May 1, 2020
## [8.1.4](8.1.3...8.1.4) (2020-05-01)

### Bug Fixes

* trailing slash when building request urls, remove whitespace when parsing external config properties ([#77](#77)) ([57382f1](57382f1))
@ibm-devx-automation
Copy link
Copy Markdown

🎉 This PR is included in version 8.1.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@jorge-ibm jorge-ibm deleted the fix-trailing-slash branch September 14, 2020 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-review PR is ready for a review released

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants