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

Ignore trailing slash on API endpoints #443

Merged
merged 5 commits into from
Nov 22, 2022

Conversation

jcace
Copy link
Contributor

@jcace jcace commented Sep 20, 2022

Closes #438

This change will strip the trailing slash from any incoming request. The caller can either make a request with or without it (ex, http://localhost:3004/health or http://localhost:3004/health/), both will work now.

@en0ma
Copy link
Contributor

en0ma commented Sep 23, 2022

@jcace please change the destination branch to dev, and also apply this middle to this echo instance https://github.com/application-research/estuary/blob/master/cmd/estuary-shuttle/main.go#L1050

@jcace jcace changed the base branch from master to dev September 26, 2022 00:34
@jcace
Copy link
Contributor Author

jcace commented Sep 26, 2022

Thanks @en0ma , made those changes!

en0ma
en0ma previously approved these changes Sep 26, 2022
@alvin-reyes
Copy link
Contributor

alvin-reyes commented Sep 26, 2022

This impacts collections api. collections api uses a trailing slash to work. Without these, it'll return 404 and can impact the current users.

Copy link
Contributor

@alvin-reyes alvin-reyes left a comment

Choose a reason for hiding this comment

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

we need to discuss the impact of this before we merge.

@alvin-reyes alvin-reyes added the Question Further information is requested label Sep 26, 2022
@en0ma en0ma dismissed their stale review September 26, 2022 18:02

@alvin raised some nice points

@en0ma
Copy link
Contributor

en0ma commented Sep 26, 2022

This impacts collections api. collections api uses a trailing slash to work. Without these, it'll return 404 and can impact the current users.

why does it need a trailing slash?

@jcace
Copy link
Contributor Author

jcace commented Sep 26, 2022

This impacts collections api. collections api uses a trailing slash to work. Without these, it'll return 404 and can impact the current users.

I modified it here - removed the "/" at the root of the collections endpoint, and tested it out locally. It seemed to work with both a trailing / and without one

@alvin-reyes
Copy link
Contributor

It doesn’t need a trailing hash but as it stands today, it requires a trailing hash. We need to make it backward compatible and communicate these changes to the existing users.

gmelodie
gmelodie previously approved these changes Oct 10, 2022
Copy link
Contributor

@gmelodie gmelodie left a comment

Choose a reason for hiding this comment

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

LGTM

…/dev-to-master

merge v0.1.11 tag to master - 11182022
@neelvirdy
Copy link
Contributor

@alvin-reyes do you still have concerns around this? i dont think this is a break since the PR makes both options work (with or without trailing slash). can also ask @anjor to do a quick test since he's probably the collections API's biggest power user right now.

@jcace
Copy link
Contributor Author

jcace commented Nov 22, 2022

Fixed merge conflicts - let me know if any other concerns guys

Copy link
Contributor

@alvin-reyes alvin-reyes left a comment

Choose a reason for hiding this comment

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

LGTM

@jcace jcace merged commit 25fbaa3 into application-research:dev Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ignore trailing slash on API endpoints
5 participants