From 4c07ea41c377abf6e70a736eef474687a8d99e9d Mon Sep 17 00:00:00 2001 From: hkfires <10558748+hkfires@users.noreply.github.com> Date: Mon, 15 Dec 2025 16:18:44 +0800 Subject: [PATCH 1/4] feat(api): return structured JSON error responses The API error handling is updated to return a structured JSON payload instead of a plain text message. This provides more context and allows clients to programmatically handle different error types. The new error response has the following structure: { "error": { "message": "...", "type": "..." } } The `type` field is determined by the HTTP status code, such as `authentication_error`, `rate_limit_error`, or `server_error`. If the underlying error message from an upstream service is already a valid JSON string, it will be preserved and returned directly. BREAKING CHANGE: API error responses are now in a structured JSON format instead of plain text. Clients expecting plain text error messages will need to be updated to parse the new JSON body. --- sdk/api/handlers/handlers.go | 47 +++++++++++++++++++++++++++++++++--- 1 file changed, 43 insertions(+), 4 deletions(-) diff --git a/sdk/api/handlers/handlers.go b/sdk/api/handlers/handlers.go index 8893e5d6..14ba83ce 100644 --- a/sdk/api/handlers/handlers.go +++ b/sdk/api/handlers/handlers.go @@ -5,6 +5,7 @@ package handlers import ( "bytes" + "encoding/json" "fmt" "net/http" "strings" @@ -437,12 +438,50 @@ func (h *BaseAPIHandler) WriteErrorResponse(c *gin.Context, msg *interfaces.Erro } } } - c.Status(status) + + errText := http.StatusText(status) if msg != nil && msg.Error != nil { - _, _ = c.Writer.Write([]byte(msg.Error.Error())) - } else { - _, _ = c.Writer.Write([]byte(http.StatusText(status))) + if v := strings.TrimSpace(msg.Error.Error()); v != "" { + errText = v + } } + + // Prefer preserving upstream JSON error bodies when possible. + buildJSONBody := func() []byte { + trimmed := strings.TrimSpace(errText) + if trimmed != "" && json.Valid([]byte(trimmed)) { + return []byte(trimmed) + } + errType := "invalid_request_error" + switch status { + case http.StatusUnauthorized: + errType = "authentication_error" + case http.StatusForbidden: + errType = "permission_error" + case http.StatusTooManyRequests: + errType = "rate_limit_error" + default: + if status >= http.StatusInternalServerError { + errType = "server_error" + } + } + payload, err := json.Marshal(ErrorResponse{ + Error: ErrorDetail{ + Message: errText, + Type: errType, + }, + }) + if err != nil { + return []byte(fmt.Sprintf(`{"error":{"message":%q,"type":"server_error"}}`, errText)) + } + return payload + } + + if !c.Writer.Written() { + c.Writer.Header().Set("Content-Type", "application/json") + } + c.Status(status) + _, _ = c.Writer.Write(buildJSONBody()) } func (h *BaseAPIHandler) LoggingAPIResponseError(ctx context.Context, err *interfaces.ErrorMessage) { From 3bc489254b96e24a3f8d0d723a0a755e729744ee Mon Sep 17 00:00:00 2001 From: hkfires <10558748+hkfires@users.noreply.github.com> Date: Mon, 15 Dec 2025 16:36:01 +0800 Subject: [PATCH 2/4] fix(api): prevent double logging for error responses The WriteErrorResponse function now caches the error response body in the gin context. The deferred request logger checks for this cached response. If an error response is found, it bypasses the standard response logging. This prevents scenarios where an error is logged twice or an empty payload log overwrites the original, more detailed error log. --- sdk/api/handlers/handlers.go | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/sdk/api/handlers/handlers.go b/sdk/api/handlers/handlers.go index 14ba83ce..a17e54aa 100644 --- a/sdk/api/handlers/handlers.go +++ b/sdk/api/handlers/handlers.go @@ -118,6 +118,16 @@ func (h *BaseAPIHandler) GetContextWithCancel(handler interfaces.APIHandler, c * newCtx = context.WithValue(newCtx, "handler", handler) return newCtx, func(params ...interface{}) { if h.Cfg.RequestLog && len(params) == 1 { + if existing, exists := c.Get("API_RESPONSE"); exists { + if existingBytes, ok := existing.([]byte); ok && len(bytes.TrimSpace(existingBytes)) > 0 { + switch params[0].(type) { + case error, string: + cancel() + return + } + } + } + var payload []byte switch data := params[0].(type) { case []byte: @@ -477,11 +487,14 @@ func (h *BaseAPIHandler) WriteErrorResponse(c *gin.Context, msg *interfaces.Erro return payload } + body := buildJSONBody() + c.Set("API_RESPONSE", bytes.Clone(body)) + if !c.Writer.Written() { c.Writer.Header().Set("Content-Type", "application/json") } c.Status(status) - _, _ = c.Writer.Write(buildJSONBody()) + _, _ = c.Writer.Write(body) } func (h *BaseAPIHandler) LoggingAPIResponseError(ctx context.Context, err *interfaces.ErrorMessage) { From 14aa6cc7e809525ffb6b40b55c9b40b94bceb296 Mon Sep 17 00:00:00 2001 From: hkfires <10558748+hkfires@users.noreply.github.com> Date: Mon, 15 Dec 2025 17:45:16 +0800 Subject: [PATCH 3/4] fix(api): ensure all response writes are captured for logging The response writer wrapper has been refactored to more reliably capture response bodies for logging, fixing several edge cases. - Implements `WriteString` to capture writes from `io.StringWriter`, which were previously missed by the `Write` method override. - A new `shouldBufferResponseBody` helper centralizes the logic to ensure the body is buffered only when logging is active or for errors when `logOnErrorOnly` is enabled. - Streaming detection is now more robust. It correctly handles non-streaming error responses (e.g., `application/json`) that are generated for a request that was intended to be streaming. BREAKING CHANGE: The public methods `Status()`, `Size()`, and `Written()` have been removed from the `ResponseWriterWrapper` as they are no longer required by the new implementation. --- internal/api/middleware/response_writer.go | 95 ++++++++++++++-------- 1 file changed, 59 insertions(+), 36 deletions(-) diff --git a/internal/api/middleware/response_writer.go b/internal/api/middleware/response_writer.go index b7259bc6..e70556d0 100644 --- a/internal/api/middleware/response_writer.go +++ b/internal/api/middleware/response_writer.go @@ -71,22 +71,64 @@ func (w *ResponseWriterWrapper) Write(data []byte) (int, error) { n, err := w.ResponseWriter.Write(data) // THEN: Handle logging based on response type - if w.isStreaming { + if w.isStreaming && w.chunkChannel != nil { // For streaming responses: Send to async logging channel (non-blocking) - if w.chunkChannel != nil { - select { - case w.chunkChannel <- append([]byte(nil), data...): // Non-blocking send with copy - default: // Channel full, skip logging to avoid blocking - } + select { + case w.chunkChannel <- append([]byte(nil), data...): // Non-blocking send with copy + default: // Channel full, skip logging to avoid blocking } - } else { - // For non-streaming responses: Buffer complete response + return n, err + } + + if w.shouldBufferResponseBody() { w.body.Write(data) } return n, err } +func (w *ResponseWriterWrapper) shouldBufferResponseBody() bool { + if w.logger != nil && w.logger.IsEnabled() { + return true + } + if !w.logOnErrorOnly { + return false + } + status := w.statusCode + if status == 0 { + if statusWriter, ok := w.ResponseWriter.(interface{ Status() int }); ok && statusWriter != nil { + status = statusWriter.Status() + } else { + status = http.StatusOK + } + } + return status >= http.StatusBadRequest +} + +// WriteString wraps the underlying ResponseWriter's WriteString method to capture response data. +// Some handlers (and fmt/io helpers) write via io.StringWriter; without this override, those writes +// bypass Write() and would be missing from request logs. +func (w *ResponseWriterWrapper) WriteString(data string) (int, error) { + w.ensureHeadersCaptured() + + // CRITICAL: Write to client first (zero latency) + n, err := w.ResponseWriter.WriteString(data) + + // THEN: Capture for logging + if w.isStreaming && w.chunkChannel != nil { + select { + case w.chunkChannel <- []byte(data): + default: + } + return n, err + } + + if w.shouldBufferResponseBody() { + w.body.WriteString(data) + } + return n, err +} + // WriteHeader wraps the underlying ResponseWriter's WriteHeader method. // It captures the status code, detects if the response is streaming based on the Content-Type header, // and initializes the appropriate logging mechanism (standard or streaming). @@ -160,12 +202,16 @@ func (w *ResponseWriterWrapper) detectStreaming(contentType string) bool { return true } - // Check request body for streaming indicators - if w.requestInfo.Body != nil { + // If a concrete Content-Type is already set (e.g., application/json for error responses), + // treat it as non-streaming instead of inferring from the request payload. + if strings.TrimSpace(contentType) != "" { + return false + } + + // Only fall back to request payload hints when Content-Type is not set yet. + if w.requestInfo != nil && len(w.requestInfo.Body) > 0 { bodyStr := string(w.requestInfo.Body) - if strings.Contains(bodyStr, `"stream": true`) || strings.Contains(bodyStr, `"stream":true`) { - return true - } + return strings.Contains(bodyStr, `"stream": true`) || strings.Contains(bodyStr, `"stream":true`) } return false @@ -335,26 +381,3 @@ func (w *ResponseWriterWrapper) logRequest(statusCode int, headers map[string][] apiResponseErrors, ) } - -// Status returns the HTTP response status code captured by the wrapper. -// It defaults to 200 if WriteHeader has not been called. -func (w *ResponseWriterWrapper) Status() int { - if w.statusCode == 0 { - return 200 // Default status code - } - return w.statusCode -} - -// Size returns the size of the response body in bytes for non-streaming responses. -// For streaming responses, it returns -1, as the total size is unknown. -func (w *ResponseWriterWrapper) Size() int { - if w.isStreaming { - return -1 // Unknown size for streaming responses - } - return w.body.Len() -} - -// Written returns true if the response header has been written (i.e., a status code has been set). -func (w *ResponseWriterWrapper) Written() bool { - return w.statusCode != 0 -} From 97ab623d429e6e620f8f6af5a51a7b7e0a2cec53 Mon Sep 17 00:00:00 2001 From: hkfires <10558748+hkfires@users.noreply.github.com> Date: Mon, 15 Dec 2025 18:00:32 +0800 Subject: [PATCH 4/4] fix(api): prevent double logging for streaming responses --- internal/api/middleware/response_writer.go | 29 +++++++++------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/internal/api/middleware/response_writer.go b/internal/api/middleware/response_writer.go index e70556d0..8005df23 100644 --- a/internal/api/middleware/response_writer.go +++ b/internal/api/middleware/response_writer.go @@ -267,7 +267,7 @@ func (w *ResponseWriterWrapper) Finalize(c *gin.Context) error { return nil } - if w.isStreaming { + if w.isStreaming && w.streamWriter != nil { if w.chunkChannel != nil { close(w.chunkChannel) w.chunkChannel = nil @@ -279,24 +279,19 @@ func (w *ResponseWriterWrapper) Finalize(c *gin.Context) error { } // Write API Request and Response to the streaming log before closing - if w.streamWriter != nil { - apiRequest := w.extractAPIRequest(c) - if len(apiRequest) > 0 { - _ = w.streamWriter.WriteAPIRequest(apiRequest) - } - apiResponse := w.extractAPIResponse(c) - if len(apiResponse) > 0 { - _ = w.streamWriter.WriteAPIResponse(apiResponse) - } - if err := w.streamWriter.Close(); err != nil { - w.streamWriter = nil - return err - } + apiRequest := w.extractAPIRequest(c) + if len(apiRequest) > 0 { + _ = w.streamWriter.WriteAPIRequest(apiRequest) + } + apiResponse := w.extractAPIResponse(c) + if len(apiResponse) > 0 { + _ = w.streamWriter.WriteAPIResponse(apiResponse) + } + if err := w.streamWriter.Close(); err != nil { w.streamWriter = nil + return err } - if forceLog { - return w.logRequest(finalStatusCode, w.cloneHeaders(), w.body.Bytes(), w.extractAPIRequest(c), w.extractAPIResponse(c), slicesAPIResponseError, forceLog) - } + w.streamWriter = nil return nil }