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

fix(request_builder): encode unresolved path string prior to path parameter insertion #219

Merged
merged 1 commit into from
May 16, 2024

Conversation

padamstx
Copy link
Member

This commit modifies RequestBuilder.ResolveRequestURL() slightly so that the passed-in path string (unresolved, potentially containing special characters and path parameter references) is URL-encoded prior to replacing path param references with their corresponding encoded path param values. This ensures that the URL string passed to http.NewRequest() by RequestBuilder.Build() will be properly encoded (escaped) such that all special characters appearing in path segments are converted to their %nn equivalents, but yet any escaped characters within the encoded path param values are NOT decoded. This avoids a quirk in the url.Parse() method that is invoked by the http.NewRequest() method.

This change is needed in order to properly support a scenario where:

  1. a path parameter value contains one or more embedded slashes and is therefore escaped AND
  2. the unresolved path string ALSO contains other special characters needing to be escaped

@padamstx padamstx self-assigned this May 15, 2024
…ameter insertion

This commit modifies RequestBuilder.ResolveRequestURL() slightly so that
the passed-in path string (unresolved, potentially containing special characters
and path parameter references) is URL-encoded prior to replacing path param
references with their corresponding encoded path param values.  This ensures
that the URL string passed to http.NewRequest() by RequestBuilder.Build()
will be properly encoded (escaped) such that all special characters appearing in path
segments are converted to their %nn equivalents, but yet any escaped characters
within the encoded path param values are NOT decoded.  This avoids a quirk in the
url.Parse() method that is invoked by the http.NewRequest() method.

This change is needed in order to properly support a scenario where:
1. a path parameter value contains one or more embedded slashes and is therefore escaped
AND
2. the unresolved path string ALSO contains other special characters needing to be escaped

Signed-off-by: Phil Adams <phil_adams@us.ibm.com>
@@ -19,7 +19,7 @@ test:

lint:
${LINT} run --build-tags=all
DIFF=$$(${FORMATTER} -d core); if [[ -n "$$DIFF" ]]; then printf "\n$$DIFF" && exit 1; fi
DIFF=$$(${FORMATTER} -d core); if [ -n "$$DIFF" ]; then printf "\n$$DIFF\n" && exit 1; fi
Copy link
Member Author

Choose a reason for hiding this comment

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

It turns out that this statement was always failing on Travis but yet it did not cause the build to stop due to a difference between "bash" and "sh" (the default shell on a Travis build agent is "sh").

Example build hiccup:
https://app.travis-ci.com/github/IBM/go-sdk-core/jobs/621470281

$ make lint
golangci-lint run --build-tags=all
DIFF=$(goimports -d core); if [[ -n "$DIFF" ]]; then printf "\n$DIFF" && exit 1; fi
/bin/sh: 1: [[: not found
The command "make lint" exited with 0.

Copy link
Member Author

Choose a reason for hiding this comment

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

I verified that any formatting type differences will now cause the build to fail and "make" will return a non-zero exit status.

Copy link
Member

Choose a reason for hiding this comment

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

Wow, good catch! I think I was the one who wrote that line and... I never thought about the shell difference to be honest - since we use Ubuntu images that usually use bash by default.

@@ -239,7 +239,7 @@ func TestSDKErrorfNoSummary(t *testing.T) {
}

func TestSDKErrorfDoesntUseSDKCausedBy(t *testing.T) {
sdkProb := getPopulatedSDKProblem();
sdkProb := getPopulatedSDKProblem()
Copy link
Member Author

Choose a reason for hiding this comment

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

change by "make format"

@@ -108,13 +108,13 @@ func formatFrames(pcs []uintptr, componentName string) []sdkStackFrame {
}

type sparseSDKProblem struct {
ID string
ID string
Copy link
Member Author

Choose a reason for hiding this comment

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

changes by "make format"

Copy link
Member

@dpopp07 dpopp07 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! 👍

Copy link
Member

@pyrooka pyrooka left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -19,7 +19,7 @@ test:

lint:
${LINT} run --build-tags=all
DIFF=$$(${FORMATTER} -d core); if [[ -n "$$DIFF" ]]; then printf "\n$$DIFF" && exit 1; fi
DIFF=$$(${FORMATTER} -d core); if [ -n "$$DIFF" ]; then printf "\n$$DIFF\n" && exit 1; fi
Copy link
Member

Choose a reason for hiding this comment

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

Wow, good catch! I think I was the one who wrote that line and... I never thought about the shell difference to be honest - since we use Ubuntu images that usually use bash by default.

@padamstx padamstx merged commit 5f567da into main May 16, 2024
4 checks passed
@padamstx padamstx deleted the issue-3930 branch May 16, 2024 13:58
ibm-devx-sdk pushed a commit that referenced this pull request May 16, 2024
## [5.17.3](v5.17.2...v5.17.3) (2024-05-16)

### Bug Fixes

* **request_builder:** encode unresolved path string prior to path parameter insertion ([#219](#219)) ([5f567da](5f567da))
@ibm-devx-sdk
Copy link

🎉 This PR is included in version 5.17.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants