Add API v2 with structured responses (chi-based rebuild)#42
Add API v2 with structured responses (chi-based rebuild)#42renecannao merged 1 commit intomasterfrom
Conversation
Implement a new v2 REST API mounted at /api/v2 that provides: - Consistent JSON response envelope (V2APIResponse with status/data/error) - Proper HTTP status codes (404, 400, 500, 503) instead of always 200 - RESTful URL structure with nested resources - Machine-readable error codes alongside human-readable messages Endpoints: clusters, cluster info, cluster instances, topology, instance details, recoveries, active recoveries, status, and ProxySQL servers. Closes #33
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances Orchestrator's API by introducing a new version 2. This new API focuses on improving consistency, predictability, and usability through structured JSON responses, appropriate HTTP status codes, and a more RESTful URL design. It provides a modern interface for interacting with Orchestrator's core functionalities without altering the underlying business logic, making it easier for clients to integrate and consume data. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new /api/v2 with a more structured and consistent JSON response format, which is a great improvement over the v1 API. The code is well-organized, with clear separation of concerns using response helpers and chi router for routing. The new endpoints are logical wrappers around existing functionality, minimizing the introduction of new business logic. The inclusion of unit tests for the response helpers and comprehensive documentation for the new API is also commendable.
I have a couple of suggestions to improve the clarity of the new API response structure by removing an unused field from the response envelope and updating the documentation accordingly.
|
|
||
| The v2 API wraps the same underlying functions as v1 but provides: | ||
|
|
||
| 1. **Consistent response envelope**: All responses use the `V2APIResponse` struct with `status`, `data`, `error`, and `message` fields. |
There was a problem hiding this comment.
The documentation states that the response envelope includes a top-level message field. However, this field is unused in the API responses, and the error message is contained within the error object. This description is misleading and should be corrected to reflect the actual response structure.
| 1. **Consistent response envelope**: All responses use the `V2APIResponse` struct with `status`, `data`, `error`, and `message` fields. | |
| 1. **Consistent response envelope**: All responses use the `V2APIResponse` struct with `status`, `data`, and `error` fields. |
| type V2APIResponse struct { | ||
| Status string `json:"status"` | ||
| Data interface{} `json:"data,omitempty"` | ||
| Error *V2APIError `json:"error,omitempty"` | ||
| Message string `json:"message,omitempty"` | ||
| } |
There was a problem hiding this comment.
The Message field in V2APIResponse is declared but never used in any of the response helper functions (respondOK, respondError, respondNotFound). This can be confusing for developers maintaining this code and for API consumers, as the error message is already part of the V2APIError struct. To improve clarity and remove dead code, this field should be removed.
Note that this change will require updating the TestV2APIResponseJSONOmitempty test in go/http/apiv2_test.go.
type V2APIResponse struct {
Status string `json:"status"`
Data interface{} `json:"data,omitempty"`
Error *V2APIError `json:"error,omitempty"`
}There was a problem hiding this comment.
Pull request overview
Introduces a new chi-mounted /api/v2 surface with a consistent JSON envelope (V2APIResponse) and several read-only endpoints, intended to modernize the API without adding new business logic.
Changes:
- Added
/api/v2router registration and handler implementations with structured success/error envelopes. - Added unit tests for v2 response helpers/envelope marshaling behavior.
- Added API v2 endpoint documentation in
docs/api-v2.md.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| go/http/apiv2.go | Adds v2 response types, helpers, route registration, and 9 v2 handlers |
| go/http/apiv2_test.go | Adds unit tests for v2 response helpers and JSON omitempty behavior |
| go/app/http.go | Mounts v2 routes during HTTP server setup |
| docs/api-v2.md | Documents v2 endpoints and response format |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if p, err := strconv.Atoi(pageStr); err == nil { | ||
| page = p | ||
| } |
There was a problem hiding this comment.
page query parsing allows negative values (and ignores parse errors by silently defaulting). logic.ReadRecentRecoveries computes offset as page*config.AuditPageSize, so a negative page yields a negative SQL offset and can break the query. Clamp page to >= 0 (matching v1) and consider returning a 400 for non-integer/negative values instead of silently accepting them.
| if p, err := strconv.Atoi(pageStr); err == nil { | |
| page = p | |
| } | |
| p, err := strconv.Atoi(pageStr) | |
| if err != nil || p < 0 { | |
| respondError(w, http.StatusBadRequest, "INVALID_PAGE", "Invalid 'page' parameter; must be a non-negative integer") | |
| return | |
| } | |
| page = p |
| // RegisterV2Routes mounts all v2 API routes under /api/v2 on the given router. | ||
| func RegisterV2Routes(r chi.Router) { | ||
| prefix := config.Config.URLPrefix | ||
| r.Route(fmt.Sprintf("%s/api/v2", prefix), func(r chi.Router) { | ||
| // Cluster endpoints | ||
| r.Get("/clusters", V2Clusters) | ||
| r.Get("/clusters/{name}", V2ClusterInfo) | ||
| r.Get("/clusters/{name}/instances", V2ClusterInstances) | ||
| r.Get("/clusters/{name}/topology", V2Topology) | ||
|
|
||
| // Instance endpoints | ||
| r.Get("/instances/{host}/{port}", V2Instance) | ||
|
|
||
| // Recovery endpoints | ||
| r.Get("/recoveries", V2Recoveries) | ||
| r.Get("/recoveries/active", V2ActiveRecoveries) | ||
|
|
||
| // Status endpoints | ||
| r.Get("/status", V2Status) | ||
|
|
||
| // ProxySQL endpoints | ||
| r.Get("/proxysql/servers", V2ProxySQLServers) | ||
| }) |
There was a problem hiding this comment.
API v1 routes are registered with raftReverseProxyMiddleware when raft is enabled (see HttpAPI.registerSingleAPIRequest), ensuring followers proxy requests to the leader. The v2 routes are mounted without this middleware, so in raft mode requests hitting a follower won’t be proxied and may behave inconsistently vs v1. Consider wrapping the entire /api/v2 route group with r.With(raftReverseProxyMiddleware) (the middleware is a no-op when raft is disabled).
| /* | ||
| Copyright 2024 Orchestrator Authors | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ |
There was a problem hiding this comment.
File header copyright differs from the standard used across existing go/http/* sources (most use Copyright 2014 Outbrain Inc.). If this repo expects consistent license headers, please align this new file’s header with the existing convention to avoid future legal/attribution inconsistencies.
Summary
/api/v2REST API with consistent JSON response envelope (V2APIResponse), proper HTTP status codes, and RESTful URL structureinst.*,logic.*, andprocess.*functions -- no new business logic, just a cleaner API surfacedocs/api-v2.mdThis is a clean rebuild of the original PR #40 on top of the current chi-based router (post-martini migration).
Closes #33
Test plan
go buildsucceedsgo test ./go/http/... -run TestV2\|TestRespondOK\|TestRespondError\|TestRespondNotFound)/api/v2/clustersand/api/v2/statusreturn structured JSON