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

Escape resource ID in API request path #62

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

shield-9
Copy link

PR Submission checklist

GitHub Issues

List the GitHub issues impacted by this PR:

GitHub Issue #

Common PR Checklist:

  • Have you made sure that the code compiles?
  • Have you commented your code, particularly in hard-to-understand areas
  • Did you run tests in a real Kubernetes cluster?
  • Have you maintained backward compatibility

Description of your changes:

It looks like client.DoAndGetResponseBody is not working as expected when id param contains /.

Example

  • Method: GET
  • URI: platform/15/protocols/nfs/aliases
  • ID: /example-nfs-alias

Expected

Request sent to /platform/15/protocols/nfs/aliases/%2Fexample-nfs-alias

Actual

Request sent to /platform/15/protocols/nfs/aliases//example-nfs-alias

It looks like client.DoAndGetResponseBody is not working as expected
when id param contains `/`.

Example:
* Method: GET
* URI: platform/15/protocols/nfs/aliases
* ID: /example-nfs-alias

* Expected: Request sent against `/platform/15/protocols/nfs/aliases/%2Fexample-nfs-alias`
* Actual: Request sent against `/platform/15/protocols/nfs/aliases//example-nfs-alias`
@donatwork
Copy link

Branch needs updating and PR info is incomplete.

@gallacher
Copy link
Contributor

@shield-9 please ensure unit tests are added for this code change

@donatwork
Copy link

Is there a real world use case driving this change?

@donatwork
Copy link

This PR cannot be considered for merging until the checks are successful. Either update or close this PR. The PR is also very old so it may be deleted by end of month if there is no activity.

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

Successfully merging this pull request may close these issues.

3 participants