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

Improve API routing #1557

Merged
merged 12 commits into from
Sep 9, 2020
Merged

Improve API routing #1557

merged 12 commits into from
Sep 9, 2020

Conversation

Twixes
Copy link
Member

@Twixes Twixes commented Sep 3, 2020

Changes

Our API endpoints in this form: /api/events have confusingly been not working… the difference is just the lack of a trailing slash. That has been a longstanding issue, hard to diagnose for users and annoying when developing. This PR fixes that, along with lacking support for personal API keys in /decide (superseded by #1553), and also adds API-specific 404 handling (as to not confusingly 302 to the app login page when hitting a wrong endpoint).
Updated URLs in a few tests to make this covered by tests.

Checklist

  • Backend tests (if this PR affects the backend)

@timgl timgl temporarily deployed to posthog-apioptional-tra-uucoee September 3, 2020 10:15 Inactive
@yakkomajuri
Copy link
Contributor

@Twixes maybe take a look at #1553 to see the approach you'd wanna have to the /decide endpoint since I set up the same authentication mechanism on /decide as /capture

@Twixes Twixes temporarily deployed to posthog-apioptional-tra-uucoee September 3, 2020 12:41 Inactive
@Twixes Twixes temporarily deployed to posthog-apioptional-tra-uucoee September 4, 2020 09:05 Inactive
@Twixes Twixes requested a review from timgl September 4, 2020 21:21
@yakkomajuri
Copy link
Contributor

yakkomajuri commented Sep 9, 2020

@Twixes when you get a chance take a look at the conflicts and then I could pick up the review on this, unless there's any objection to that

@Twixes Twixes requested a review from yakkomajuri September 9, 2020 14:17
Copy link
Contributor

@yakkomajuri yakkomajuri left a comment

Choose a reason for hiding this comment

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

This looks good to me. I made some changes to also make the internal use endpoints flexible, as well as made sure the tests were testing endpoints both with and without a trailing slash. Added comments so that people can make sense of the "inconsistency".

I made changes regarding endpoints that weren't in the scope of this PR but that I thought made sense. @Twixes see if the changes make sense and otherwise this should be good to go.

@Twixes
Copy link
Member Author

Twixes commented Sep 9, 2020

Makes sense. 👍

@Twixes
Copy link
Member Author

Twixes commented Sep 9, 2020

Will have to take a look at these Cypress tests finally… But this should be OK for the moment.

@Twixes Twixes merged commit fd9df64 into master Sep 9, 2020
@Twixes Twixes deleted the apioptional-trailing-slash branch September 9, 2020 14:54
@yakkomajuri
Copy link
Contributor

Yeah I was thinking of doing that and then we got the debug bar PR merged, all new PRs pass, but old PRs that get merged with master don't?

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