-
Notifications
You must be signed in to change notification settings - Fork 7.3k
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
Trimmed trailing slash in URL provided to create Client API #15513
Trimmed trailing slash in URL provided to create Client API #15513
Conversation
…ates a double-slash unwanted side effects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @shieldsjared! One comment below.
…eck api endpoint. Test case includes checks for expected (0), probable (1), and arbitrary (5) number of slashes to stress the implementation to ensure it wasnt built for a specific numbre of slashes
46d517a
to
2cab4ee
Compare
Changes implemented and ready for review again @lieut-data - Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet!
@prapti Kind reminder to review for v5.28. |
Thanks @amyblais! |
/update-branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Thank you @shieldsjared for this fix and thank you for noting that "this fix does not correct if a user attempts to use a URL with multiple trailing-slashes", that was helpful :)
Cherry pick is scheduled. |
…server-mattermost#15513-upstream-release-5.28
Summary
As described in this issue while attempting to utilize the MM Plugin Started Template, it was determined that including a trailing slash when calling
model.NewAPIv4Client
with a URL that includes a trailing slash, the resulting URL's become malformed (double-slash) and MM is unable to route the requests appropriately. Trimming a trailing slash prevents users from having to be too particular when specifying the URL of their target server.Note that this fix does not correct if a user attempts to use a URL with multiple trailing-slashes (however, this should be a bit more obvious of an issue to the end-user if they incorrectly set the MM url with multiple trailing-slashes).
Ticket Link
Fixes: mattermost/mattermost-plugin-starter-template#115
Also, please see PR on that repository for discussion and agreed-upon path forward for resolution (included in this PR)