Add a REST/JSON gateway for the RPC framework#978
Conversation
📝 WalkthroughWalkthroughThe change adds HTTP route metadata to RPC definitions and generated method adapters, including verbs, paths, request bodies, path parameters, and typed query parameters. It introduces public HTTP binding descriptors and a REST gateway that dispatches annotated methods, decodes request data, serializes JSON responses, and maps errors to HTTP statuses. Application and example RPC interfaces gain REST routes. New widget fixtures and tests validate generation, route mounting, JSON responses, error handling, method restrictions, and typed query parsing. Comment |
![miren-code-agent[bot]](https://avatars.githubusercontent.com/u/174767976?s=60&v=4){kind=small}
There was a problem hiding this comment.
🍪 biscuit:
This is a draft PR adding a REST/JSON gateway on top of the existing RPC framework. The design is sound: IDL annotations flow through the generator into HTTPBinding structs embedded in each Method, and RegisterREST uses those structs to mount routes on a plain http.ServeMux. I read through all of the new code carefully.
What works well
The Args precedence order (body → query → path) is the right call — path params canonically win over anything else. The coerceQueryValue switch is clear and covers the expected scalar kinds. The two-step round-trip (json.Marshal(fields) → json.Unmarshal(data, v)) is a little roundabout, but it's correct and lets the generated UnmarshalJSON on args structs do its own work without a custom parser. The generator changes are clean — httpBinding, httpPathParams, and httpQueryParams are small, focused, and easy to follow, and the test that compares generated output against the golden testdata/rest.go gives confidence the codegen is stable.
Concrete concerns
-
No auth enforcement in
restHandler(rest.go, lines 41–60). The existing RPC transport refuses unauthenticated calls to non-Publicmethods.restHandlerdispatches directly tom.Handlerwithout checkingm.Publicorcall.IsAuthenticated(). If a method is non-public and no auth middleware wraps the mux, the REST gateway will answer calls that the QUIC transport would reject. The doc comment says "a future auth middleware" will handle this, but that's an open security gap at the point of mounting. At minimum the code should enforcem.Publichere, and ideally return HTTP 401/403 for non-public methods missing an identity, so callers get the same semantics over both transports. -
Path prefix joining can produce double slashes (
generator.go,httpBindingfunction):bare = strings.TrimRight(i.HTTP.Prefix, "/") + "/" + strings.TrimLeft(bare, "/")
When the interface prefix is
/api/v1and the method path already starts with/, you get/api/v1/apps. Fine. But if the method path has no leading slash (which the IDL files here don't), the logic is still correct. The real risk is the opposite: if someone writes a prefix without a trailing slash and a method path without a leading slash, the middle"/"handles it. This is robust. Not a bug, just worth knowing. -
restCall.Resultscaptures only the last assignment (rest.go, line 187–189). If a handler stores intermediate results before the final state (unusual but possible), earlier writes are silently dropped. This matches the QUIC path's semantics so it's consistent, but worth documenting if this ever supports streaming results. -
json.Marshal(call.results)afterw.Header().Setbut beforew.WriteHeader(rest.go, lines 46–60). TheContent-Typeheader is set at line 46, then the marshal happens. Ifjson.Marshalfails at line 54,http.Erroris called — buthttp.Errorsets its ownContent-Typeand writes a status. That works fine in practice becauseWriteHeaderhasn't been called yet at that point, so the secondContent-Typewill win. However, if marshal succeeds,w.WriteHeader(http.StatusOK)is explicit at line 59. Callingw.Header().Setat line 46 beforew.WriteHeaderis correct, but a reader has to trace the two paths carefully to confirm it. Moving theContent-Typeassignment to just before the write would make the success path unambiguous. -
Silent body-decode error (
rest.go, lines 82–86). Ignoring JSON decode errors for a body-binding (Body == "*") is intentional and the comment explains it. That's a reasonable choice for an "all fields optional" schema. But a malformedContent-Type: application/jsonbody that sends{"name": }(invalid JSON) is silently treated as "no body", and the args will be empty rather than rejected. If the IDL has any mandatory-feeling fields, callers get a confusing empty-args invocation rather than a 400. Consider returning 400 when the body is non-empty and fails to parse, at least when theContent-Typeisapplication/json.
The security point (item 1) is the only thing I'd call a merge blocker for a non-draft. Since this is still a draft, it's appropriate to raise these now rather than after the PR graduates. Everything else is refinement.
🍪 full review note · comment /biscuit review to run biscuit again.
|
|
||
| call := &restCall{r: r, binding: m.HTTP} | ||
|
|
||
| if err := m.Handler(r.Context(), call); err != nil { |
There was a problem hiding this comment.
Auth gap: restHandler dispatches directly to m.Handler without checking m.Public or whether the caller is authenticated. The RPC transport enforces auth on non-public methods; the REST gateway currently does not. If this mux is mounted without an external auth middleware, non-public methods will be callable unauthenticated. At minimum, add a check like if !m.Public && !call.IsAuthenticated() { writeRESTError(w, ErrUnauthorized); return } before invoking the handler.
🤖 Prompt for AI Agents
In pkg/rpc/rest.go inside the restHandler closure,
before the call to m.Handler (around line 41), add
an authentication check. If the method is not
public (m.Public == false) and the caller is not
authenticated (call.IsAuthenticated() == false),
call writeRESTError(w, ErrUnauthorized) and return
early. This mirrors the auth enforcement the QUIC
transport already does for non-public methods.
| fields := map[string]json.RawMessage{} | ||
|
|
||
| if c.binding.Body == "*" && c.r.Body != nil { | ||
| // Ignore decode errors: an empty or absent body just leaves fields |
There was a problem hiding this comment.
Silent body parse failure: when Body == "*", a malformed JSON body (e.g. {"name": }) is silently ignored and args are left empty. For callers sending Content-Type: application/json, consider returning HTTP 400 when the body is non-empty and fails to decode, so they learn about the bad payload rather than receiving a confusing empty-args response.
🤖 Prompt for AI Agents
In pkg/rpc/rest.go in the restCall.Args method
around line 82-86, when c.binding.Body == "*" and
c.r.Body is non-nil, the JSON decode error is
currently silently discarded. Change this so that
if the request Content-Type is application/json
and the body is non-empty but fails to decode into
the fields map, the function signals an error to
the caller. One approach: check
r.Header.Get("Content-Type") and r.ContentLength
!= 0, and if the decode fails, write an HTTP 400
error via the response writer. Because Args
doesn't have access to the ResponseWriter
directly, consider having Args return an error or
having restHandler check the decode result before
proceeding.
Our RPC framework speaks CBOR over QUIC with a capability model, which is great for internal service-to-service calls but gives us nothing to hand a user who just wants a plain HTTP/JSON API. This lays the foundation for a simple Miren Runtime API: annotate methods in the IDL with an HTTP verb and path, and get a real http.Server that exposes a full REST surface backed by the exact same handler implementations. No second implementation, no hand-written HTTP glue. The approach mirrors grpc-gateway but stays in-process. A method opts in with an http: annotation, either the compact "POST /apps" form (also backward compatible with the pre-existing convention in the IDL) or a mapping when it needs an explicit body binding. The generator threads that binding into the generated rpc.Method, and RegisterREST mounts routes on a stdlib ServeMux. Each request runs through the existing Method.Handler via a restCall that sources args from the request (JSON body, path wildcards, query string) and captures results as JSON. The generated arg/result structs already carry json tags, so round-tripping is free. Query params are type-aware: the generator emits each query param's kind so the runtime can coerce bools, numbers, and RFC3339 timestamps into their typed fields rather than binding everything as a string. Errors map onto HTTP status via the existing ErrorCode taxonomy. The app API (Crud, AppStatus, Disks, Addons, and the non-streaming Logs reads) is annotated as the first real consumer. Streaming and capability-returning methods are intentionally left RPC-only.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/rpc/generator.go`:
- Around line 2021-2046: Add generation-time HTTP annotation validation before
code generation, covering each method’s `DescHTTPMethod`: reject multiple
non-empty verb fields, reject `Body` values other than empty or `"*"`, and
ensure every wildcard from `verbPath()`/`httpPathParams()` matches a declared
`m.Parameters` entry. Propagate these errors through the existing
`Generate()`/`generateInterfaces()` error paths with clear messages, while
leaving the HTTP binding generation unchanged for valid annotations.
- Around line 2404-2422: Update httpBinding to reject annotated
capability/streaming methods before returning a binding, using the method shape
information available on DescMethods. Return ok=false for unsupported methods so
generation skips them and the REST gateway never attempts to create a
capability/client; preserve existing verb, path, and interface-prefix handling
for supported methods.
In `@pkg/rpc/rest.go`:
- Around line 219-240: Update writeRESTError to use errors.As when extracting
ErrorMessage, ErrorCode, and ErrorCategory, so wrapped errors are traversed and
their metadata populates restErrorBody before restStatusFor is called. Preserve
the existing direct-error behavior and ensure the errors package is imported.
- Around line 35-117: Update restHandler to decode the request body before
invoking m.Handler when the binding body is "*", treating io.EOF as an empty
body and returning HTTP 400 for malformed JSON. Pass the parsed fields into
restCall so restCall.Args no longer decodes the body or silently discards body
errors; also propagate final unmarshal/type-mismatch errors through restCall for
a 400 response, and bound the body size before decoding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f884afd-e048-4dd8-beb7-736135289993
📒 Files selected for processing (11)
api/app/app_v1alpha/rpc.gen.goapi/app/rpc.ymlpkg/rpc/example/rpc.gopkg/rpc/example/rpc.ymlpkg/rpc/generator.gopkg/rpc/generator_test.gopkg/rpc/rest.gopkg/rpc/rest_test.gopkg/rpc/server.gopkg/rpc/testdata/rest.gopkg/rpc/testdata/rest.yml
| if verb, bare, ok := httpBinding(i, m); ok { | ||
| params := httpPathParams(bare) | ||
| query := httpQueryParams(m, params) | ||
| g.Line().Id("HTTP").Op(":").Op("&").Qual(rpc, "HTTPBinding").ValuesFunc(func(g *j.Group) { | ||
| g.Line().Id("Verb").Op(":").Lit(verb) | ||
| g.Line().Id("Path").Op(":").Lit(bare) | ||
| g.Line().Id("Body").Op(":").Lit(m.HTTP.Body) | ||
| g.Line().Id("PathParams").Op(":").Index().String().ValuesFunc(func(g *j.Group) { | ||
| for _, p := range params { | ||
| g.Lit(p) | ||
| } | ||
| }) | ||
| if len(query) > 0 { | ||
| g.Line().Id("Query").Op(":").Index().Qual(rpc, "HTTPParam").ValuesFunc(func(g *j.Group) { | ||
| for _, q := range query { | ||
| g.Line().Values( | ||
| j.Id("Name").Op(":").Lit(q.Name), | ||
| j.Id("Kind").Op(":").Lit(q.Kind), | ||
| ) | ||
| } | ||
| g.Line() | ||
| }) | ||
| } | ||
| g.Line() | ||
| }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add generation-time validation for HTTP annotation configs (verb ambiguity, body value, path-param names).
Three related gaps in the new HTTP-annotation parsing, none of which produce a generation-time error:
verbPath()(2387-2402) silently prefers the first non-empty verb field (GetbeforePost/Put/... ) if a method's IDL mistakenly sets two of them — the second is dropped with no warning.httpQueryParams()(2434-2452) treats any non-emptyBodyas "has a body" (m.HTTP.Body != ""), butpkg/rpc/rest.go'srestCall.Argsonly decodes the request body whenBodyis exactly"*"(c.binding.Body == "*"). A typo such asbody: "yes"compiles fine, suppresses query-param generation here, yet is never decoded inrest.go— the resulting endpoint silently loses both its query and body binding.httpPathParams()(2475-2491) extracts{name}wildcards purely from the path template text, with no check that each name matches a declared parameter inm.Parameters. A typo in the path template (e.g.{App}vs parameterapp) produces aPathParamsentry with no corresponding JSON field on the generated Args struct; at runtime the path value is silently dropped byjson.Unmarshal's unknown-field handling instead of erroring.
Since Generate()/generateInterfaces() already return error, an upfront validation pass over g.Interfaces/i.Method (before codegen starts) could reject all three cases with a clear message.
♻️ Sketch of validation checks to add
func (m *DescHTTPMethod) validate(params []*DescParamater) error {
verbs := 0
for _, v := range []string{m.Get, m.Post, m.Put, m.Delete, m.Patch} {
if v != "" {
verbs++
}
}
if verbs > 1 {
return errors.New("http: only one verb field may be set")
}
if m.Body != "" && m.Body != "*" {
return fmt.Errorf("http: body must be \"\" or \"*\", got %q", m.Body)
}
_, bare, _ := m.verbPath()
declared := make(map[string]bool, len(params))
for _, p := range params {
declared[p.Name] = true
}
for _, name := range httpPathParams(bare) {
if !declared[name] {
return fmt.Errorf("http: path param %q has no matching parameter", name)
}
}
return nil
}Also applies to: 2371-2402, 2434-2452, 2475-2491
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/rpc/generator.go` around lines 2021 - 2046, Add generation-time HTTP
annotation validation before code generation, covering each method’s
`DescHTTPMethod`: reject multiple non-empty verb fields, reject `Body` values
other than empty or `"*"`, and ensure every wildcard from
`verbPath()`/`httpPathParams()` matches a declared `m.Parameters` entry.
Propagate these errors through the existing `Generate()`/`generateInterfaces()`
error paths with clear messages, while leaving the HTTP binding generation
unchanged for valid annotations.
| // httpBinding resolves a method's REST binding, returning the HTTP verb and the | ||
| // full path template with the interface prefix applied. ok is false when the | ||
| // method carries no http: annotation. | ||
| func httpBinding(i *DescInterface, m *DescMethods) (verb, path string, ok bool) { | ||
| if m.HTTP == nil { | ||
| return "", "", false | ||
| } | ||
|
|
||
| verb, bare, ok := m.HTTP.verbPath() | ||
| if !ok { | ||
| return "", "", false | ||
| } | ||
|
|
||
| if i.HTTP != nil && i.HTTP.Prefix != "" { | ||
| bare = strings.TrimRight(i.HTTP.Prefix, "/") + "/" + strings.TrimLeft(bare, "/") | ||
| } | ||
|
|
||
| return verb, bare, true | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'Capability|stream\.' pkg/rpc/generator.go
rg -n 'IsCapability|isStream|Stream\b' pkg/rpc/*.goRepository: mirendev/runtime
Length of output: 2416
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== httpBinding and surrounding generator code ==\n'
rg -n -C 4 'func httpBinding|verbPath\(\)|HTTP != nil|NewCapability|NewClient' pkg/rpc/generator.go pkg/rpc/rest.go
printf '\n== method-shape helpers in generator ==\n'
rg -n -C 3 'Capability|Stream|isCapability|isStream|Result|Param|HTTP' pkg/rpc/generator.go
printf '\n== rest call constructors ==\n'
rg -n -C 6 'func \(.*NewCapability|func \(.*NewClient|panic\(' pkg/rpc/rest.goRepository: mirendev/runtime
Length of output: 47630
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 'httpBinding\(' pkg/rpc/generator.go
rg -n -C 4 'NewCapability|NewClient|Capability|Stream' pkg/rpc/generator.go | sed -n '1,220p'Repository: mirendev/runtime
Length of output: 5691
Reject http: on capability/streaming methods. httpBinding() applies to every annotated method, and the REST gateway panics if it has to create a capability/client. Add a shape check before emitting the binding so these methods fail at generation time instead of mid-request.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/rpc/generator.go` around lines 2404 - 2422, Update httpBinding to reject
annotated capability/streaming methods before returning a binding, using the
method shape information available on DescMethods. Return ok=false for
unsupported methods so generation skips them and the REST gateway never attempts
to create a capability/client; preserve existing verb, path, and
interface-prefix handling for supported methods.
| func restHandler(m Method) http.HandlerFunc { | ||
| return func(w http.ResponseWriter, r *http.Request) { | ||
| defer r.Body.Close() | ||
|
|
||
| call := &restCall{r: r, binding: m.HTTP} | ||
|
|
||
| if err := m.Handler(r.Context(), call); err != nil { | ||
| writeRESTError(w, err) | ||
| return | ||
| } | ||
|
|
||
| w.Header().Set("Content-Type", "application/json") | ||
|
|
||
| if call.results == nil { | ||
| w.WriteHeader(http.StatusOK) | ||
| return | ||
| } | ||
|
|
||
| body, err := json.Marshal(call.results) | ||
| if err != nil { | ||
| http.Error(w, err.Error(), http.StatusInternalServerError) | ||
| return | ||
| } | ||
|
|
||
| w.WriteHeader(http.StatusOK) | ||
| _, _ = w.Write(body) | ||
| } | ||
| } | ||
|
|
||
| // restCall adapts an HTTP request to the rpc.Call interface. It supports the | ||
| // data-plane of a call (arguments in, results out); the capability operations | ||
| // panic because capabilities cannot be expressed over plain REST. | ||
| type restCall struct { | ||
| r *http.Request | ||
| binding *HTTPBinding | ||
| results any | ||
| } | ||
|
|
||
| var _ Call = (*restCall)(nil) | ||
|
|
||
| // Args assembles a JSON object from the request body, path wildcards, and query | ||
| // string, then unmarshals it into v (a generated *XxxArgs, which implements | ||
| // json.Unmarshaler over its json-tagged data struct). Precedence, lowest to | ||
| // highest: request body, query string, path parameters. | ||
| func (c *restCall) Args(v any) { | ||
| fields := map[string]json.RawMessage{} | ||
|
|
||
| if c.binding.Body == "*" && c.r.Body != nil { | ||
| // Ignore decode errors: an empty or absent body just leaves fields | ||
| // unset, matching the "all optional" shape of generated args. | ||
| _ = json.NewDecoder(c.r.Body).Decode(&fields) | ||
| } | ||
|
|
||
| // Query params carry the non-path inputs of a bodyless verb (GET/DELETE). | ||
| // Each is coerced from its raw string into typed JSON per its declared kind, | ||
| // so bools, numbers, and timestamps land in their typed fields rather than | ||
| // as strings. A value that fails to parse for its kind is simply omitted. | ||
| query := c.r.URL.Query() | ||
| for _, p := range c.binding.Query { | ||
| raw := query.Get(p.Name) | ||
| if raw == "" { | ||
| continue | ||
| } | ||
| if val, ok := coerceQueryValue(p.Kind, raw); ok { | ||
| fields[p.Name] = val | ||
| } | ||
| } | ||
|
|
||
| // Path parameters win: they are the canonical addressing of the resource. | ||
| // Path segments are always strings. | ||
| for _, name := range c.binding.PathParams { | ||
| if val := c.r.PathValue(name); val != "" { | ||
| fields[name] = jsonString(val) | ||
| } | ||
| } | ||
|
|
||
| data, err := json.Marshal(fields) | ||
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
| _ = json.Unmarshal(data, v) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Args() silently swallows malformed-body and type-mismatch errors, turning bad requests into silent no-ops.
Both the body decode (line 85) and the final json.Unmarshal(data, v) (line 116) discard all errors unconditionally. The doc comment justifies this for an empty/absent body, but it also swallows genuine JSON syntax errors — a malformed POST body to e.g. /api/v1/widgets would silently run the handler with default/empty args and return 200, instead of a 400. Since rpc.Call.Args(v any) has no error return, the cleanest fix is to parse the body in restHandler (which can still write an HTTP response) before invoking m.Handler, distinguishing io.EOF (fine) from real parse errors (400).
Also, once this gateway is wired to a real listener, consider bounding the body size (e.g. http.MaxBytesReader) before decoding into map[string]json.RawMessage to avoid unbounded memory use from an oversized request.
🐛 Proposed fix: parse the body before dispatching, reject malformed JSON with 400
+import (
+ "io"
+)
+
func restHandler(m Method) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
defer r.Body.Close()
- call := &restCall{r: r, binding: m.HTTP}
+ var body map[string]json.RawMessage
+ if m.HTTP.Body == "*" {
+ if err := json.NewDecoder(r.Body).Decode(&body); err != nil && !errors.Is(err, io.EOF) {
+ http.Error(w, "invalid request body: "+err.Error(), http.StatusBadRequest)
+ return
+ }
+ }
+
+ call := &restCall{r: r, binding: m.HTTP, body: body}
...
type restCall struct {
r *http.Request
binding *HTTPBinding
+ body map[string]json.RawMessage
results any
}
...
func (c *restCall) Args(v any) {
fields := map[string]json.RawMessage{}
- if c.binding.Body == "*" && c.r.Body != nil {
- // Ignore decode errors: an empty or absent body just leaves fields
- // unset, matching the "all optional" shape of generated args.
- _ = json.NewDecoder(c.r.Body).Decode(&fields)
- }
+ for k, v := range c.body {
+ fields[k] = v
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/rpc/rest.go` around lines 35 - 117, Update restHandler to decode the
request body before invoking m.Handler when the binding body is "*", treating
io.EOF as an empty body and returning HTTP 400 for malformed JSON. Pass the
parsed fields into restCall so restCall.Args no longer decodes the body or
silently discards body errors; also propagate final unmarshal/type-mismatch
errors through restCall for a 400 response, and bound the body size before
decoding.
| func writeRESTError(w http.ResponseWriter, err error) { | ||
| body := restErrorBody{Error: err.Error()} | ||
|
|
||
| if em, ok := err.(ErrorMessage); ok { | ||
| body.Error = em.ErrorMessage() | ||
| } | ||
| if ec, ok := err.(ErrorCode); ok { | ||
| body.Code = ec.ErrorCode() | ||
| } | ||
| if ec, ok := err.(ErrorCategory); ok { | ||
| body.Category = ec.ErrorCategory() | ||
| } | ||
|
|
||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(restStatusFor(err, body.Code)) | ||
|
|
||
| data, mErr := json.Marshal(body) | ||
| if mErr != nil { | ||
| return | ||
| } | ||
| _, _ = w.Write(data) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use errors.As instead of raw type assertions to see through wrapped errors.
errors.Is(err, ErrUnauthorized) at line 254 correctly unwraps, but the ErrorMessage/ErrorCode/ErrorCategory checks here use direct type assertions on err. If any handler wraps a cond.* error with additional context (e.g. fmt.Errorf("...: %w", cond.NotFound(...)) — a common Go pattern), the assertion fails silently, body.Code stays empty, and restStatusFor falls back to 500 instead of the correct 404/409/400. This directly undercuts the PR's goal of mapping ErrorCode values onto HTTP statuses.
🐛 Proposed fix
- if em, ok := err.(ErrorMessage); ok {
- body.Error = em.ErrorMessage()
- }
- if ec, ok := err.(ErrorCode); ok {
- body.Code = ec.ErrorCode()
- }
- if ec, ok := err.(ErrorCategory); ok {
- body.Category = ec.ErrorCategory()
- }
+ var em ErrorMessage
+ if errors.As(err, &em) {
+ body.Error = em.ErrorMessage()
+ }
+ var ec ErrorCode
+ if errors.As(err, &ec) {
+ body.Code = ec.ErrorCode()
+ }
+ var ecat ErrorCategory
+ if errors.As(err, &ecat) {
+ body.Category = ecat.ErrorCategory()
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func writeRESTError(w http.ResponseWriter, err error) { | |
| body := restErrorBody{Error: err.Error()} | |
| if em, ok := err.(ErrorMessage); ok { | |
| body.Error = em.ErrorMessage() | |
| } | |
| if ec, ok := err.(ErrorCode); ok { | |
| body.Code = ec.ErrorCode() | |
| } | |
| if ec, ok := err.(ErrorCategory); ok { | |
| body.Category = ec.ErrorCategory() | |
| } | |
| w.Header().Set("Content-Type", "application/json") | |
| w.WriteHeader(restStatusFor(err, body.Code)) | |
| data, mErr := json.Marshal(body) | |
| if mErr != nil { | |
| return | |
| } | |
| _, _ = w.Write(data) | |
| } | |
| func writeRESTError(w http.ResponseWriter, err error) { | |
| body := restErrorBody{Error: err.Error()} | |
| var em ErrorMessage | |
| if errors.As(err, &em) { | |
| body.Error = em.ErrorMessage() | |
| } | |
| var ec ErrorCode | |
| if errors.As(err, &ec) { | |
| body.Code = ec.ErrorCode() | |
| } | |
| var ecat ErrorCategory | |
| if errors.As(err, &ecat) { | |
| body.Category = ecat.ErrorCategory() | |
| } | |
| w.Header().Set("Content-Type", "application/json") | |
| w.WriteHeader(restStatusFor(err, body.Code)) | |
| data, mErr := json.Marshal(body) | |
| if mErr != nil { | |
| return | |
| } | |
| _, _ = w.Write(data) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/rpc/rest.go` around lines 219 - 240, Update writeRESTError to use
errors.As when extracting ErrorMessage, ErrorCode, and ErrorCategory, so wrapped
errors are traversed and their metadata populates restErrorBody before
restStatusFor is called. Preserve the existing direct-error behavior and ensure
the errors package is imported.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/rpc/rest.go`:
- Around line 93-100: Update the query-binding loop in the REST request handler
to distinguish absent parameters from explicitly provided empty values using the
query map, and return HTTP 400 when a declared parameter is present but
coerceQueryValue fails. Preserve omission only for parameters that are truly
absent, while retaining successful coercion and field assignment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f04b266f-bf3d-45af-a09f-d424b9b0119a
📒 Files selected for processing (12)
api/app/app_v1alpha/rpc.gen.goapi/app/rpc.ymlapi/metric/metric_v1alpha/rpc.gen.gopkg/rpc/example/rpc.gopkg/rpc/example/rpc.ymlpkg/rpc/generator.gopkg/rpc/generator_test.gopkg/rpc/rest.gopkg/rpc/rest_test.gopkg/rpc/server.gopkg/rpc/testdata/rest.gopkg/rpc/testdata/rest.yml
🚧 Files skipped from review as they are similar to previous changes (9)
- pkg/rpc/generator_test.go
- pkg/rpc/testdata/rest.yml
- pkg/rpc/example/rpc.yml
- pkg/rpc/server.go
- pkg/rpc/generator.go
- api/app/app_v1alpha/rpc.gen.go
- api/app/rpc.yml
- pkg/rpc/example/rpc.go
- pkg/rpc/testdata/rest.go
| for _, p := range c.binding.Query { | ||
| raw := query.Get(p.Name) | ||
| if raw == "" { | ||
| continue | ||
| } | ||
| if val, ok := coerceQueryValue(p.Kind, raw); ok { | ||
| fields[p.Name] = val | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject invalid typed query parameters instead of silently dropping them.
For example, ?follow=tru is omitted and the handler runs with its default value. Return HTTP 400 when a declared query parameter is present but cannot be coerced; distinguish an absent parameter from an explicitly empty one via the query map.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/rpc/rest.go` around lines 93 - 100, Update the query-binding loop in the
REST request handler to distinguish absent parameters from explicitly provided
empty values using the query map, and return HTTP 400 when a declared parameter
is present but coerceQueryValue fails. Preserve omission only for parameters
that are truly absent, while retaining successful coercion and field assignment.
1 participant
Our RPC framework speaks CBOR over QUIC with a capability model. That's great for internal service-to-service calls, but it gives us nothing to hand a user who just wants a plain HTTP/JSON API. This is foundation work toward a simple Miren Runtime API we can expose to users: annotate methods in the IDL with an HTTP verb and path, and get a real
http.Serverserving a full REST surface backed by the exact same handler implementations. No second implementation, no hand-written HTTP glue.The approach mirrors grpc-gateway but stays in-process. A method opts in with an
http:block in itsrpc.yml; the generator threads that binding into the generatedrpc.Method, andRegisterRESTmounts routes on a stdlibServeMux. Each request runs through the existingMethod.Handlervia arestCallthat sources arguments from the request (JSON body, path wildcards, query string) and captures results as JSON. The generated arg/result structs already carryjsontags, so the round-trip is essentially free, and reusing the handler means REST and RPC can never drift apart.Query parameters are type-aware. The
Logsreads were the forcing function here:fromis a timestamp andfollowis a bool, neither of which survives being bound as a raw string. So the generator emits each query param's kind, and the runtime coerces bools, numbers, and RFC3339 timestamps into their typed fields. Errors map onto HTTP status codes through the existingErrorCodetaxonomy (not-foundto 404,conflictto 409, and so on).The app API is annotated as the first real consumer:
Crud,AppStatus,Disks,Addons, and the non-streamingLogsreads all get REST routes under/api/v1. Streaming and capability-returning methods are intentionally left RPC-only, since neither maps cleanly onto request/response HTTP.A couple of things deliberately left for follow-ups: nothing wires the gateway onto a listener in
miren serveryet, so the REST surface is exercised by tests but not yet served in production. And response timestamps still serialize as{seconds, nanoseconds}; making them RFC3339 on the way out (symmetric with query input) means teaching the generator to givestandard.Timestampa friendlier JSON codec, which touches every JSON response and felt like its own change.