fix: Request Id was empty whehn audit logs were disabled +remove redundant request ID logic from audit log middleware #101
Conversation
The request ID middleware in http.go now always runs before audit logging, making the ID generation in middleware.go dead code. Also moves the response header echo outside the `if` block so client-provided IDs are echoed back for correlation — matching the previous audit log behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe changes centralize request ID management by moving X-Request-ID header generation from the audit log middleware to a new global HTTP middleware layer. The audit log middleware now only reads the request ID header, assuming it's already set by the earlier middleware. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/auditlog/middleware.go (1)
27-30:⚠️ Potential issue | 🟡 MinorUpdate the
MiddlewareGo doc comment to document the new precondition.The function's behavior changed: it no longer generates a fallback request ID and now silently produces a log entry with an empty
RequestIDif called without the request ID middleware preceding it. The exported doc comment must reflect this per coding guidelines.📝 Proposed doc update
-// Middleware creates an Echo middleware for audit logging. -// It captures request metadata at the start and response metadata at the end, -// then writes the log entry asynchronously. +// Middleware creates an Echo middleware for audit logging. +// It captures request metadata at the start and response metadata at the end, +// then writes the log entry asynchronously. +// +// Middleware assumes the X-Request-ID header is already set on the incoming +// request (e.g. by the request ID middleware registered in server.New). +// If the header is absent, RequestID in the log entry will be empty. func Middleware(logger LoggerInterface) echo.MiddlewareFunc {As per coding guidelines: "Maintain Go doc comments on all exported types, functions, and interfaces; update when changing public APIs or function signatures/behavior."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auditlog/middleware.go` around lines 27 - 30, Update the exported doc comment for the Middleware function to document the new precondition: state that Middleware requires the request ID middleware to have run earlier in the chain (it no longer generates a fallback RequestID), and that if called without that middleware the produced audit entry may have an empty RequestID; reference the function name Middleware and keep the comment concise and on the exported function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/server/http_test.go`:
- Around line 29-32: The current check only verifies len(got)==36 which allows
non-UUID strings; replace it by parsing the value in variable got using a UUID
parser (e.g., uuid.Parse from github.com/google/uuid or uuid.Parse from
github.com/gofrs/uuid) and assert the parse returns no error (and optionally
assert the parsed UUID's Version()==4 if you want to enforce v4). Update the
test around the existing validation (the block that checks len(got) and
references got) to call uuid.Parse(got), fail the test on parse error, and
remove or keep a shorter length check only as a fallback.
---
Outside diff comments:
In `@internal/auditlog/middleware.go`:
- Around line 27-30: Update the exported doc comment for the Middleware function
to document the new precondition: state that Middleware requires the request ID
middleware to have run earlier in the chain (it no longer generates a fallback
RequestID), and that if called without that middleware the produced audit entry
may have an empty RequestID; reference the function name Middleware and keep the
comment concise and on the exported function.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
internal/auditlog/middleware.gointernal/server/http.gointernal/server/http_test.go
| // Validate UUID format (8-4-4-4-12 hex digits) | ||
| if len(got) != 36 { | ||
| t.Errorf("expected UUID (36 chars), got %q (%d chars)", got, len(got)) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
UUID format validation is weaker than the comment implies.
The comment on line 29 states "Validate UUID format (8-4-4-4-12 hex digits)" but the check only validates the string length (36). Any 36-character string would pass — e.g., a non-UUID. Consider using uuid.Parse to assert a well-formed UUID v4.
♻️ Proposed stronger validation
+import "github.com/google/uuid"
- // Validate UUID format (8-4-4-4-12 hex digits)
- if len(got) != 36 {
- t.Errorf("expected UUID (36 chars), got %q (%d chars)", got, len(got))
- }
+ // Validate well-formed UUID
+ if _, err := uuid.Parse(got); err != nil {
+ t.Errorf("expected valid UUID, got %q: %v", got, err)
+ }📝 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.
| // Validate UUID format (8-4-4-4-12 hex digits) | |
| if len(got) != 36 { | |
| t.Errorf("expected UUID (36 chars), got %q (%d chars)", got, len(got)) | |
| } | |
| // Validate well-formed UUID | |
| if _, err := uuid.Parse(got); err != nil { | |
| t.Errorf("expected valid UUID, got %q: %v", got, err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/http_test.go` around lines 29 - 32, The current check only
verifies len(got)==36 which allows non-UUID strings; replace it by parsing the
value in variable got using a UUID parser (e.g., uuid.Parse from
github.com/google/uuid or uuid.Parse from github.com/gofrs/uuid) and assert the
parse returns no error (and optionally assert the parsed UUID's Version()==4 if
you want to enforce v4). Update the test around the existing validation (the
block that checks len(got) and references got) to call uuid.Parse(got), fail the
test on parse error, and remove or keep a shorter length check only as a
fallback.
1 participant
The request ID middleware in http.go now always runs before audit logging, making the ID generation in middleware.go dead code. Also moves the response header echo outside the
ifblock so client-provided IDs are echoed back for correlation — matching the previous audit log behavior.Summary by CodeRabbit
New Features
Tests