Skip to content

Conversation

@codeaucafe
Copy link
Contributor

@codeaucafe codeaucafe commented Aug 17, 2021

This PR fixes Issue #2190 by creating a handler to return invalid error for routes downstream of the BaseURL.

This PR fixes the issue by creating a handler that ensures that only routes on the mounted route (i.e. /api/v1) will be successful. Other routes are returned with ErrInvalidAPIEndpoint with the error string being invalid API endpoint.

This handler can be used with future/other routes by chi Mount'ing the handler to the specific route you want so that users are returned with the invalid endpoint error when trying to access any downstream route from the route/pattern the handler is chi Mount'ed to.

Tests were successful for the tested for this fix (test created by @nopcoder - thank you!), but I still had issues with getting TestLocalLoad to pass successfully. I wanted to put this PR up for review and then we can talk about any issues it has AND how to resolve the testing issue for TestLocalLoad.

Thank you.

@CLAassistant
Copy link

CLAassistant commented Aug 17, 2021

CLA assistant check
All committers have signed the CLA.

This commit works to close issue treeverse#2190 by creating handler that returns
ErrInvalidAPIEndpoint when called. This will be used by chi Mount'ing
the handler to a specified route so that any calls to a route downstream
the Mount'd pattern will respond with the ErrInvalidAPIEndpoint error.

Also add corresponding test, TestInvalidRoute, to test
InvalidAPIEndpointHandler.

Create middleware to catch downstream routes from BaseURL

This commit works to close issue treeverse#2190 by creating middleware to ensure
users that call routes downstream of the BaseURL (i.e. /api/v1) are
return an invalid endpoint error and internal error status.

Remove BaseURLHandler
@codeaucafe codeaucafe force-pushed the fix/api-unknown-path-return-error-2190 branch from 846a775 to c96a22d Compare August 19, 2021 10:13
@codeaucafe
Copy link
Contributor Author

Hey @nopcoder thank you so much for the great suggestions/info (i.e. basically doing the work for me 🤣). Anyways, I made the changes needed. I tried to rebase my history so I don't have multiple commits just adding and removing stuff, but since my rebase experience is lacking I had a hard time.

FYI I think I got it right, but please let me know if its an issue. To be clear, so long as I rebase my forked commits, rebasing should be fine before I merge my PR branch, correct?

Thanks again for all your help.

best,
dd

@codeaucafe
Copy link
Contributor Author

Hey @nopcoder FYI I forgot to mention that the tests were still successful after the change, but I still had the TestLocalLoad failing :(

@nopcoder
Copy link
Contributor

nopcoder commented Aug 20, 2021

@DataDavD all you - will squash and merge it after the tests will complete green on GitHub.
If you like me to help with debugging why the test doesn't run locally - you are welcome to jump on our slack and post the error. The team and myself would be happy to help.

@nopcoder nopcoder linked an issue Aug 20, 2021 that may be closed by this pull request
@nopcoder nopcoder merged commit d509894 into treeverse:master Aug 20, 2021
@codeaucafe
Copy link
Contributor Author

codeaucafe commented Aug 20, 2021

Ok yes, I will definitely do that, especially since I want to help contribute more.

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.

API with an unknown path should return an error

3 participants