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: routes precedence #2820 #2821

Merged
merged 2 commits into from
Mar 29, 2024
Merged

fix: routes precedence #2820 #2821

merged 2 commits into from
Mar 29, 2024

Conversation

dnmeid
Copy link
Member

@dnmeid dnmeid commented Mar 29, 2024

So far the express routes of the new http API have been pushed to the end of the routes of express app. This results in bug #2820 where the default catch all get requests and return index is served before the get of the api.
This PR changes the routing a little bit:
The api router is invoked with a base url from the app, so all urls are relative to the base url there. The router itself does not define a base url any more.
Legacy API routes and the new API routes are set up in one shared router. The router is added dynamically to the express app and can be swapped at runtime to make the logic consistent with the controller.
The new api routes are now parsed before the legacy routes.
The catch all 404 route for the api is not a part of the legacy or the new api routes any more but always at the end of all api routes.

@dnmeid dnmeid added BUG Something isn't working area/backend Something in the core of companion labels Mar 29, 2024
@dnmeid
Copy link
Member Author

dnmeid commented Mar 29, 2024

Ah, I see the tests are failing. Didn't remember that there are unit tests for the api and they are broken now.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@dnmeid dnmeid linked an issue Mar 29, 2024 that may be closed by this pull request
2 tasks
@dnmeid dnmeid merged commit 1f4d701 into main Mar 29, 2024
11 of 13 checks passed
@dnmeid dnmeid deleted the fix/routes-precedence-2820 branch March 29, 2024 23:59
@Julusian
Copy link
Member

Julusian commented Apr 2, 2024

@dnmeid This has broken the legacy/old http api, it has moved those old urls to be under /api instead.

Ah, I see the tests are failing. Didn't remember that there are unit tests for the api and they are broken now.

Yeah, I wrote some unit tests for the new api while writing it, as part of a gentle nudge into getting better test coverage. :)

@dnmeid
Copy link
Member Author

dnmeid commented Apr 2, 2024

Wait, our old api was not under /api. Crazy. Ok, I'll fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/backend Something in the core of companion BUG Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GET value of a custom-variable via new HTTP API
3 participants