REST API Prototype#4259
Conversation
|
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughIntroduces a new REST API service for Companion, gated by a ChangesCompanion REST API v1
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (4)
companion/test/Service/RestApi/ConnectionsRouter.test.ts (1)
154-182: ⚡ Quick winAdd one positive
execute-scope test to lock auth semanticsNice coverage on denied restart with read-only tokens. I’d also add a positive
cpn_executetest forPOST /connections/v1/:id/restart(and optionallyGET /connections/v1) so regressions inexecute => readscope implication are caught early.Suggested test shape
+ test('execute token can restart connections', async () => { + const { app, instanceController } = createService() + const executeToken = 'cpn_execute' + + instanceController.getConnectionClientJson.mockReturnValue(createConnectionConfigs()) + instanceController.restartConnection.mockReturnValue(true) + + const res = await supertest(app) + .post('/api/connections/v1/conn-1/restart') + .set('Authorization', `Bearer ${executeToken}`) + .send() + + expect(res.status).toBe(200) + })Also applies to: 1201-1231
companion/test/Service/RestApi/OpenApiSpec.test.ts (2)
55-59: ⚡ Quick winInclude the
config-fieldsendpoint in expected OpenAPI path coverageThe
expectedPathslist currently skips/connections/v1/{connectionId}/config-fields, so OpenAPI regressions for that implemented endpoint could slip through.Suggested update
const expectedPaths = [ '/connections/v1', '/connections/v1/{connectionId}', + '/connections/v1/{connectionId}/config-fields', '/connections/v1/{connectionId}/restart', ]
225-228: ⚡ Quick winAssert
secretsin PATCH request schema fieldsLine 225’s expected field list omits
secrets, even though PATCH supports secrets updates. Adding it keeps spec tests aligned with runtime behavior and auth-scope coverage.Suggested update
- const expectedFields = ['label', 'disabled', 'config', 'updatePolicy', 'versionId'] + const expectedFields = ['label', 'disabled', 'config', 'secrets', 'updatePolicy', 'versionId']tools/generate_rest_api_docs.mts (1)
317-317: ⚡ Quick winSanitize tag slugs before using them as filenames.
At Line 317, only whitespace is normalized. If a tag contains
/or other filename-unsafe chars, docs generation can fail when writing Line 384.Suggested patch
+function toSafeSlug(tag: string): string { + const slug = tag + .toLowerCase() + .replace(/[^a-z0-9]+/g, '-') + .replace(/^-+|-+$/g, '') + return slug || 'general' +} + // Generate one markdown file per tag let tagPosition = 1 for (const [tag, operations] of tagGroups) { - const slug = tag.toLowerCase().replace(/\s+/g, '-') + const slug = toSafeSlug(tag) const lines: string[] = []Also applies to: 384-384
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d31e6a31-53ee-4880-a884-4c02620f52fa
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (25)
companion/lib/Data/UserConfig.tscompanion/lib/Instance/Controller.tscompanion/lib/Service/Controller.tscompanion/lib/Service/RestApi/RestApiAuth.tscompanion/lib/Service/RestApi/RestApiRouter.tscompanion/lib/Service/RestApi/RestApiService.tscompanion/lib/Service/RestApi/RestApiTokenStore.tscompanion/lib/Service/RestApi/errors.tscompanion/lib/Service/RestApi/middleware/errorHandler.tscompanion/lib/Service/RestApi/openapi.tscompanion/lib/Service/RestApi/registry.tscompanion/lib/Service/RestApi/routes/ConnectionsRouter.tscompanion/lib/Service/RestApi/schemas/common.tscompanion/lib/Service/RestApi/schemas/connections.tscompanion/lib/UI/Express.tscompanion/package.jsoncompanion/test/Service/RestApi/ConnectionsRouter.test.tscompanion/test/Service/RestApi/OpenApiSpec.test.tscompanion/test/UI/Express.test.tsdocs/.gitignorepackage.jsonshared-lib/lib/Model/UserConfigModel.tstools/generate_rest_api_docs.mtstools/rest-api-docs/_category-intro.mdtools/rest-api-docs/authentication.md
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
2 participants
This is the first prototype for #4101. The idea of this PR is to implement one resource to finalize the open discussion points.
Implementation Scope
connectionscpn_admin,cpn_write,cpn_read,cpn_secrets)yarn run build:rest-api-docs. It is not integrated into the CI build, so that the docs are not published yet, but they can be viewed locallyHow to test
Open http://localhost:8000/api/docs/ where API calls can be executed interactively
Kown Open Discussion Points
How should we handle versioning
Option A:
By resource, e.g.
connections/v1,buttons/v1. Whenever there is a breaking change in the API of a resource, we would bump the version of this resource only. New resources always start with V1. Note that this is related to the API only. So when we introduceconnections/v2, we could keepconnections/v1` for compatibility.A similar approach could be achieved with
Accept: application/vnd.companion.v2+jsonheaders, but making it less transparent I think.Option B
Include the Companion major version in the api path, e.g.
api/v4/connections.Personally I'm not a fan of this option, as every major companion release would break API paths, despite most endpoints probably stay the same
Option C
Global API versioning, so start with
api/v1, so that when one day we want to change the structure of the api completly (imaginge in a few years REST is not "best practice" anymore and we want to completly change the API structure).I guess in this case we would imply that API endpoints with the same path can be breaking across major companion version
Option D
Completely omit versioning for now.
Format of "one" resource
In the discussions there were voices to not have endpoints for an individual resources (e.g.
connections/:id), instead havingconnectionsendpoint which can be filtered by id, always returning a collection. Personally I don't like that, I feels wrong, not how HTTP works, but eventually it is just a matter of taste. The Companion Core team should reach a decision in this matter and I will implement it accordingly.Summary by CodeRabbit
/api/docs/and an OpenAPI spec at/api/openapi.json./api/connections/v1endpoints to list, create, retrieve, patch, delete, and restart connections (with optional config/secrets access).