feat(robot-types): add config template API#81
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| // @Summary Get robot type config | ||
| // @Description Returns an unrendered robot type config template by robot type and filename. | ||
| // @Tags robot_type_config_templates | ||
| // @Produce plain |
There was a problem hiding this comment.
SUGGESTION: Swagger @Produce: plain annotation does not match actual response type
The handler at line 115 returns c.Data(http.StatusOK, "text/yaml; charset=utf-8", ...), but the gin-swagger annotation at this line declares @Produce: plain. For a YAML-specific response, update the annotation to @Produce: text/yaml so generated API docs accurately expose the response format to consumers.
| FROM robot_type_config_templates | ||
| WHERE robot_type_id = ? AND filename = ? AND deleted_at IS NULL | ||
| LIMIT 1 | ||
| `, robotTypeID, filename).Scan(&existingID) |
There was a problem hiding this comment.
HIGH: Upsert has a time-of-check-to-time-of-use race under concurrent PUT requests
UpsertRobotTypeConfigTemplate runs a pre-check SELECT id FROM robot_type_config_templates WHERE robot_type_id=? AND filename=? AND deleted_at IS NULL LIMIT 1 at line 264, then decides between INSERT and UPDATE based on the SELECT result. When two concurrent PUTs arrive simultaneously, both goroutines can receive sql.ErrNoRows from that SELECT and both will enter the tx.Exec(INSERT INTO ...) branch, causing the second transaction to fail with a unique-constraint violation that is swallowed by the generic 500 Internal Server Error at lines 285-290 — callers see a hard error instead of last-write-wins.
Fix: replace the two-step (SELECT + INSERT/UPDATE) with a single INSERT INTO robot_type_config_templates (robot_type_id, filename, content, created_at, updated_at) VALUES (?, ?, ?, ?, ?) ON DUPLICATE KEY UPDATE content=VALUES(content), updated_at=VALUES(updated_at). The stored generated column _active_unique is NULL for soft-deleted rows, so only the single active row (deleted_at IS NULL) participates in the uniqueness constraint — ON DUPLICATE KEY UPDATE will therefore fire correctly for the active row and never for a soft-deleted one.
| s.robotType.RegisterRoutes(v1Tasks) | ||
| s.robotType.RegisterConfigTemplatePublicRoutes(v1Tasks) | ||
| adminRobotTypes := v1Routes.Group("", middleware.JWTAuth(&s.cfg.Auth), middleware.RequireRole("admin")) | ||
| s.robotType.RegisterConfigTemplateAdminRoutes(adminRobotTypes) |
There was a problem hiding this comment.
SUGGESTION: Consider separating route registration for public and admin endpoints with a comment for reviewers
Line 259 registers RegisterConfigTemplatePublicRoutes under v1Tasks (no middleware), and line 261 registers RegisterConfigTemplateAdminRoutes under adminRobotTypes (JWT + admin role). The two registrations sit side-by-side here with no separating comment, which makes the intent and the middleware boundary easy to misread during a future change. The design and the function names are clear, so this is low urgency.
| // @Summary Get robot type config | ||
| // @Description Returns an unrendered robot type config template by robot type and filename. | ||
| // @Tags robot_type_config_templates | ||
| // @Produce plain |
There was a problem hiding this comment.
SUGGESTION: Swagger @Produce: plain annotation does not match actual response type
The handler at line 115 returns c.Data(http.StatusOK, "text/yaml; charset=utf-8", ...), but the gin-swagger annotation at this line declares @Produce: plain. For a YAML-specific response, update the annotation to @Produce: text/yaml so generated API docs accurately expose the response format to consumers.
| FROM robot_type_config_templates | ||
| WHERE robot_type_id = ? AND filename = ? AND deleted_at IS NULL | ||
| LIMIT 1 | ||
| `, robotTypeID, filename).Scan(&existingID) |
There was a problem hiding this comment.
HIGH: Upsert has a TOCTOU race under concurrent PUT requests
UpsertRobotTypeConfigTemplate runs a pre-check SELECT id ... WHERE robot_type_id=? AND filename=? AND deleted_at IS NULL LIMIT 1 at line 264, then decides between INSERT and UPDATE based on the SELECT result. When two concurrent PUTs arrive simultaneously, both can see sql.ErrNoRows and both will attempt the INSERT INTO ... branch. The second hits the _active_unique unique-constraint violation and falls into the generic 500 / JS error branch at lines 285-290 — callers receive an opaque error instead of the expected last-write-wins result.
Fix: replace the two-step SELECT + INSERT/UPDATE with a single INSERT INTO robot_type_config_templates (...) VALUES (?, ?, ?, ?, ?) ON DUPLICATE KEY UPDATE content=VALUES(content), updated_at=VALUES(updated_at). The stored generated column _active_unique is NULL for soft-deleted rows, so the DUPLICATE KEY branch fires only for the single active row (deleted_at IS NULL), which is the exact behavior the current code intends.
| s.robotType.RegisterRoutes(v1Tasks) | ||
| s.robotType.RegisterConfigTemplatePublicRoutes(v1Tasks) | ||
| adminRobotTypes := v1Routes.Group("", middleware.JWTAuth(&s.cfg.Auth), middleware.RequireRole("admin")) | ||
| s.robotType.RegisterConfigTemplateAdminRoutes(adminRobotTypes) |
There was a problem hiding this comment.
SUGGESTION: Separate the public and admin route registrations with an explicit comment
Line 259 registers RegisterConfigTemplatePublicRoutes under v1Tasks (no middleware — deliberate design per the API spec) and line 261 registers RegisterConfigTemplateAdminRoutes under adminRobotTypes (JWT + admin). The adjacent lines differ in critical ways — one is unauthenticated by design, the other enforces admin auth — but there is no comment separating them. Future maintainers can easily miss the distinction when extending the handler.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Approve with changes Overview
Issue Details (click to expand)HIGH
SUGGESTION
Other Observations (not in diff, cannot receive inline comments)Test coverage gaps
Design note
Files Reviewed (6 files)
Reviewed by step-3.5-flash · 3,769,541 tokens |
1 participant
Pull Request Checklist
Please ensure your PR meets the following requirements:
Summary
This PR adds database-backed robot type config templates in Keystone and exposes the canonical API for retrieving raw Axon config templates by
robot_type_idand filename.Motivation
recorder.yamlandtransfer.yamltemplates per robot type.Changes
Modified Files
[internal/server/server.go](internal/server/server.go)- Registers public config template GET routes and admin-only management routes.Added Files
[internal/api/handlers/robot_type_config_template.go](internal/api/handlers/robot_type_config_template.go)- Adds public raw YAML retrieval plus admin list/get/upsert/delete handlers.[internal/api/handlers/robot_type_config_template_test.go](internal/api/handlers/robot_type_config_template_test.go)- Covers successful public fetches, validation errors, fixed-slot listing, PUT validation, upsert, delete, and route registration.[internal/storage/database/migrations/000003_robot_type_config_templates.up.sql](internal/storage/database/migrations/000003_robot_type_config_templates.up.sql)- Creates therobot_type_config_templatestable with soft-delete-aware active uniqueness.[internal/storage/database/migrations/000003_robot_type_config_templates.down.sql](internal/storage/database/migrations/000003_robot_type_config_templates.down.sql)- Drops the new table.[docs/designs/robot-type-config-templates.html](docs/designs/robot-type-config-templates.html)- Documents the agreed API, validation rules, storage model, Synapse scope, and future Axon integration notes.Deleted Files
Type of Change
Impact Analysis
Breaking Changes
None
Backward Compatibility
Fully backward compatible. This adds new endpoints and does not change existing robot type or device registration behavior.
Testing
Test Environment
/tmp/go-build-cacheTest Cases
Manual Testing Steps
GOCACHE=/tmp/go-build-cache go test ./internal/api/handlers -run RobotTypeConfigTemplate -vGOCACHE=/tmp/go-build-cache go test ./internal/server ./internal/api/handlersGOCACHE=/tmp/go-build-cache go test ./...swag init -g internal/server/server.go -o docsrobot_type_id=1:GET /api/v1/robot_types/1/configs/recorder.yamlreturned200withtext/yaml; charset=utf-8GET /api/v1/robot_types/1/configs/transfer.yamlreturned200withtext/yaml; charset=utf-8Test Coverage
Screenshots / Recordings
Not applicable for Keystone backend changes.
Performance Impact
Documentation
Related Issues
Additional Notes
recorder.yamlandtransfer.yamlare accepted in this first version.docs/docs.go,docs/swagger.json, anddocs/swagger.yamlare ignored by this repository.Reviewers
@reviewer1 @reviewer2
Notes for Reviewers
Checklist for Reviewers