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: remove trailing slash for invalid URL #455

Merged
merged 3 commits into from
Oct 27, 2023

Conversation

maemaemae3
Copy link
Contributor

Resolves #395


Before the change?

  • remove trailing slashes from URL except expanded endpoint is /

After the change?

  • if you specify url like endpoint("https://github.acme-inc/api/v3/"), it will cause to trailing slash removed.

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

  • if you specify url like endpoint("https://github.acme-inc/api/v3/"), it will cause to trailing slash removed.

Please see our docs on breaking changes to help!

  • Yes
  • No

@wolfy1339 wolfy1339 marked this pull request as ready for review October 25, 2023 17:21
@maemaemae3
Copy link
Contributor Author

considering for effects about breaking changes.
It seems there's no issue with omitting the trailing slash, but I'm considering whether there could be any unexpected effects.

@wolfy1339 wolfy1339 added the Type: Bug Something isn't working as documented label Oct 25, 2023
Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

I like the change, I've seen lots of people reporting issues that turned out to be a trailing slash. But we have to be careful to not accidentally break use cases. For example, can you please test with https://docs.github.com/en/free-pro-team@latest/rest/git/refs?apiVersion=2022-11-28#list-matching-references and https://docs.github.com/en/rest/repos/contents?apiVersion=2022-11-28#get-repository-content (loading a folder)

I think in either case the / should be URL encoded, but I'm not sure

@maemaemae3
Copy link
Contributor Author

maemaemae3 commented Oct 26, 2023

Thanks for the excellent advice!
I added tests for trailing slash in expressions, and checked its encoded properly.
However trailing slash in https://docs.github.com/en/rest/repos/contents?apiVersion=2022-11-28#get-repository-content (loading a folder) returns Bad request, which means you must omit trailing slash.
(I mean API like http://api.github.com/repos/my-org/my-repo/contents/mydir%2Fnested-dir%2F returns Bad request.)
Should we also consider cases like this?

test/endpoint.test.ts Outdated Show resolved Hide resolved
@gr2m
Copy link
Contributor

gr2m commented Oct 26, 2023

However trailing slash in https://docs.github.com/en/rest/repos/contents?apiVersion=2022-11-28#get-repository-content (loading a folder) returns Bad request, which means you must omit trailing slash

That is something we should fix on the API side, so folks not using Octokit will benefit as well. I'll raise that internally.

Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

I am a bit worried about unforeseen consequences. I'm sure there are users that use octokit to send requests to non-github endpoints and this change might break their code. But I would not take that into account as well, it will bloat @octokit/endpoint even more, which is our lowest-level @octokit library for JavaScript.

My vote is to merge, release, and see if problems pop up. It's easy enough to reverse

@kfcampbell
Copy link
Member

👍 I'll merge this now and standby to rollback if necessary.

@kfcampbell kfcampbell merged commit 1578ba5 into octokit:main Oct 27, 2023
7 checks passed
@github-actions
Copy link

🎉 This PR is included in version 9.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@maemaemae3 maemaemae3 deleted the fix-invalid-trailing-slash branch October 28, 2023 02:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Type: Bug Something isn't working as documented
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[BUG]: Undefined slash-parameters likely expanded incorrectly
4 participants