Skip to content

Commit 897492b

Browse files
JoesHageclaude
authored andcommitted
fix(httpgen): preserve structure of proto.Message errors returned from handlers
Previously, when a handler returned a custom proto error message (e.g., NotFoundError), it was wrapped in sebufhttp.Error{Message: err.Error()}, which converted the structured proto to a string and lost all its fields. This fix: - Adds proto.Message type check in genericHandler before wrapping errors - Adds proto.Message type check in defaultErrorResponse as fallback - Preserves full proto structure in error responses Example: Before: {"message":"{\"resourceType\":\"user\",\"resourceId\":\"123\"}"} After: {"resourceType":"user","resourceId":"123"} Fixes #81 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 0d84a24 commit 897492b

File tree

6 files changed

+149
-102
lines changed

6 files changed

+149
-102
lines changed

CLAUDE.md

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -259,6 +259,7 @@ service UserService {
259259
### Error Handling
260260
- **Structured Error Responses**: All errors use protobuf messages for consistent API responses
261261
- **Automatic Go Error Interface**: Any protobuf message ending with "Error" automatically implements Go's error interface for `errors.As()` and `errors.Is()` support
262+
- **Proto Message Error Preservation**: Custom proto error messages returned from handlers are serialized directly, preserving their structure (not wrapped in a generic Error message)
262263
- **Validation Errors (HTTP 400)**: ValidationError with field-level violations for body and header validation failures
263264
- **Handler Errors (HTTP 500)**: Error messages for service implementation failures with custom messages
264265
- **Content-Type Aware**: Error responses serialized as JSON or protobuf based on request Content-Type
@@ -267,6 +268,31 @@ service UserService {
267268
- **Header validation errors**: Clear messages indicating which header failed validation and why
268269
- **Fail-fast**: Validation stops request processing immediately on failure (headers validated before body)
269270

271+
**Custom Proto Error Example:**
272+
```protobuf
273+
// Define a custom error message
274+
message NotFoundError {
275+
string resource_type = 1;
276+
string resource_id = 2;
277+
}
278+
```
279+
280+
```go
281+
// Return it from your handler - it will be serialized directly
282+
func (s *Server) GetUser(ctx context.Context, req *GetUserRequest) (*User, error) {
283+
user, err := s.db.FindUser(req.Id)
284+
if err != nil {
285+
return nil, &NotFoundError{
286+
ResourceType: "user",
287+
ResourceId: req.Id,
288+
}
289+
}
290+
return user, nil
291+
}
292+
// Response: {"resourceType":"user","resourceId":"123"}
293+
// NOT: {"message":"{\"resourceType\":\"user\",\"resourceId\":\"123\"}"}
294+
```
295+
270296
## Type System
271297

272298
The plugin handles comprehensive protobuf-to-Go type mapping in `getFieldType()` (generator.go:118):

internal/httpgen/error_handler_test.go

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -381,3 +381,87 @@ func TestErrorHandlerBackwardCompatibility(t *testing.T) {
381381
}
382382
})
383383
}
384+
385+
// TestProtoMessageErrorPreservation tests that proto.Message errors are preserved without wrapping.
386+
func TestProtoMessageErrorPreservation(t *testing.T) {
387+
files := generateTestFiles(t, "http_verbs_comprehensive.proto")
388+
389+
t.Run("genericHandler checks for proto.Message errors", func(t *testing.T) {
390+
// The genericHandler should check if the error is already a proto.Message
391+
if !strings.Contains(files.binding, "if protoErr, ok := err.(proto.Message); ok {") {
392+
t.Error("genericHandler should check if error is a proto.Message")
393+
}
394+
})
395+
396+
t.Run("genericHandler passes proto.Message errors directly to writeErrorWithHandler", func(t *testing.T) {
397+
// When error is a proto.Message, it should be passed directly without wrapping
398+
if !strings.Contains(files.binding, "writeErrorWithHandler(w, r, protoErr, errorHandler)") {
399+
t.Error("genericHandler should pass proto.Message errors directly to writeErrorWithHandler")
400+
}
401+
})
402+
403+
t.Run("genericHandler has comment explaining proto.Message check", func(t *testing.T) {
404+
if !strings.Contains(files.binding, "Check if error is already a proto.Message") {
405+
t.Error("genericHandler should have comment explaining proto.Message check")
406+
}
407+
})
408+
409+
t.Run("defaultErrorResponse checks for proto.Message errors", func(t *testing.T) {
410+
// defaultErrorResponse should also check for proto.Message errors as fallback
411+
if !strings.Contains(files.binding, "if protoErr, ok := err.(proto.Message); ok {") {
412+
t.Error("defaultErrorResponse should check if error is a proto.Message")
413+
}
414+
})
415+
416+
t.Run("defaultErrorResponse returns proto.Message errors directly", func(t *testing.T) {
417+
// The function should return the proto.Message directly without wrapping
418+
if !strings.Contains(files.binding, "return protoErr") {
419+
t.Error("defaultErrorResponse should return proto.Message errors directly")
420+
}
421+
})
422+
423+
t.Run("proto.Message check comes before sebufhttp.Error wrapping", func(t *testing.T) {
424+
// In genericHandler, the proto.Message check should come before the sebufhttp.Error wrapping
425+
binding := files.binding
426+
protoMsgCheckPos := strings.Index(binding, "if protoErr, ok := err.(proto.Message)")
427+
sebufErrorPos := strings.Index(binding, "errorMsg := &sebufhttp.Error{")
428+
429+
if protoMsgCheckPos == -1 || sebufErrorPos == -1 {
430+
t.Error("Both proto.Message check and sebufhttp.Error creation should exist")
431+
return
432+
}
433+
434+
if protoMsgCheckPos > sebufErrorPos {
435+
t.Error("proto.Message check should come before sebufhttp.Error wrapping in genericHandler")
436+
}
437+
})
438+
439+
t.Run("defaultErrorResponse proto.Message check comes after known error types", func(t *testing.T) {
440+
// In defaultErrorResponse, proto.Message check should be after ValidationError and Error checks
441+
binding := files.binding
442+
443+
// Find the defaultErrorResponse function
444+
funcStart := strings.Index(binding, "func defaultErrorResponse(err error) proto.Message {")
445+
if funcStart == -1 {
446+
t.Error("defaultErrorResponse function not found")
447+
return
448+
}
449+
450+
funcBody := binding[funcStart:]
451+
452+
// Check order: ValidationError -> Error -> proto.Message -> fallback
453+
valErrPos := strings.Index(funcBody, "var valErr *sebufhttp.ValidationError")
454+
handlerErrPos := strings.Index(funcBody, "var handlerErr *sebufhttp.Error")
455+
protoMsgPos := strings.Index(funcBody, "if protoErr, ok := err.(proto.Message)")
456+
fallbackPos := strings.Index(funcBody, "return &sebufhttp.Error{Message: err.Error()}")
457+
458+
if valErrPos == -1 || handlerErrPos == -1 || protoMsgPos == -1 || fallbackPos == -1 {
459+
t.Error("defaultErrorResponse should have all four error handling cases")
460+
return
461+
}
462+
463+
if !(valErrPos < handlerErrPos && handlerErrPos < protoMsgPos && protoMsgPos < fallbackPos) {
464+
t.Error("defaultErrorResponse should check errors in order: ValidationError, Error, proto.Message, fallback")
465+
}
466+
})
467+
}

internal/httpgen/generator.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -565,6 +565,11 @@ func (g *Generator) generateBindingFile(file *protogen.File) error {
565565
gf.P()
566566
gf.P("response, err := serve(r.Context(), request)")
567567
gf.P("if err != nil {")
568+
gf.P("// Check if error is already a proto.Message (e.g., custom proto error types)")
569+
gf.P("if protoErr, ok := err.(proto.Message); ok {")
570+
gf.P("writeErrorWithHandler(w, r, protoErr, errorHandler)")
571+
gf.P("return")
572+
gf.P("}")
568573
gf.P("errorMsg := &sebufhttp.Error{")
569574
gf.P("Message: err.Error(),")
570575
gf.P("}")
@@ -961,6 +966,10 @@ func (g *Generator) generateDefaultErrorResponseFunc(gf *protogen.GeneratedFile)
961966
gf.P("if errors.As(err, &handlerErr) {")
962967
gf.P("return handlerErr")
963968
gf.P("}")
969+
gf.P("// Check if error is already a proto.Message (e.g., custom proto error types)")
970+
gf.P("if protoErr, ok := err.(proto.Message); ok {")
971+
gf.P("return protoErr")
972+
gf.P("}")
964973
gf.P("return &sebufhttp.Error{Message: err.Error()}")
965974
gf.P("}")
966975
gf.P()

internal/httpgen/testdata/golden/backward_compat_http_binding.pb.go

Lines changed: 10 additions & 34 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

internal/httpgen/testdata/golden/http_verbs_comprehensive_http_binding.pb.go

Lines changed: 10 additions & 34 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

internal/httpgen/testdata/golden/query_params_http_binding.pb.go

Lines changed: 10 additions & 34 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)