Print the response's body on demand for debugging purposes #276
Conversation
|
Warning Rate limit exceeded@shackra has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 38 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
WalkthroughThe changes introduce a debugging facility for HTTP request responses in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (7)
pkg/uhttp/body_print.go (5)
3-7: Consider using more neutral documentation.The playful and suggestive wording may be fun in a testing or internal environment, but for production use, consider more neutral phrasing to maintain a professional tone.
20-21: Performance overhead from chunk-by-chunk logging.You're logging the response in each Read call, which might degrade performance for large or heavily chunked responses. Consider buffering the read data and printing it only once.
27-29: Nil-check the input reader.While it's uncommon, a
nilreader might cause unexpected panics. Consider adding a guard clause to ensurebodyis non-nil.func wrapBodyToUndress(body io.Reader) io.Reader { + if body == nil { + return nil + } return &printReader{reader: body} }
31-33: Naming clarity forundressOption.While the term "undress" appears playful, a name like
debugBodyOptionorprintBodyOptionmight be clearer for new or external contributors.
39-41: Add documentation forWithUndressBody.Briefly explain that enabling this option can log the response body for debugging purposes. This helps consumers fully understand the parameter's effect.
pkg/uhttp/wrapper.go (2)
97-97: Flag naming aligns with the code but lacks doc comments.Add a doc comment above
debugPrintBodyto clarify its purpose (to wrap and log the HTTP response body).
345-349: Consider partial logging to prevent large memory usage and PII risk.Even though
io.ReadAllis used to read the body into memory, be aware of potential large responses. Consider:
- Using a limited reader before wrapping the body.
- Or logging only the first N bytes to prevent excessive memory usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/uhttp/body_print.go(1 hunks)pkg/uhttp/wrapper.go(2 hunks)
🔇 Additional comments (1)
pkg/uhttp/body_print.go (1)
35-37: Simplify or validate the flag assignment.
The assignment is straightforward. Optionally, check if the client has a non-nil reference before setting fields.
|
it is a prank PR and I'll remove them eventually :)
…--
Jorge Araya
Contacto:
Telegram: t.me/shackra · Signal: Shackra.28
Enviado desde Proton Mail para Android
-------- Mensaje original --------
El 30/12/2024 11:28, Bjorn escribió:
@btipling commented on this pull request.
---------------------------------------------------------------
In [pkg/uhttp/body_print.go](#276 (comment)):
> @@ -0,0 +1,41 @@
+package uhttp
+
+// Implements a debugging facility for request responses. This changes
+// the behavior of `BaseHttpClient` with an unexported flag.
+//
+// If you always wanted to see the actual body of your response, now
+// you can 👁️👄👁️undress it🫦 to uncover all its... data!
Maybe the emoji stay contained to the PR?
—
Reply to this email directly, [view it on GitHub](#276 (review)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AAIBT4C4O5GXQPJJBHRN3FD2IF7CNAVCNFSM6AAAAABUJSQLYCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMRVHAZDONZRGY).
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
shackra
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/uhttp/body_print.go (3)
13-27: Add documentation and consider buffering output.The implementation looks good but could be improved:
- Add documentation for the type and Read method
- Consider using a buffered writer to prevent interleaved output in concurrent scenarios
+// printReader wraps an io.Reader and prints its contents to stdout while reading. +// This type is intended for development debugging only. type printReader struct { reader io.Reader } +// Read implements io.Reader interface, printing the read contents to stdout. +// If printing fails, the error is combined with any read error using errors.Join. func (pr *printReader) Read(p []byte) (int, error) {
29-31: Add documentation for the helper function.The implementation is correct, but needs documentation to explain its purpose and usage.
+// wrapPrintBody wraps the given io.Reader with a printReader that outputs +// the read contents to stdout. For development debugging only. func wrapPrintBody(body io.Reader) io.Reader {
33-43: Consider using environment variable for debug control.As suggested in previous reviews, using an environment variable instead of a WrapperOption might be safer to prevent accidentally committing debug code to production.
Example implementation:
-type printBodyOption struct { - debugPrintBody bool -} - -func (o printBodyOption) Apply(c *BaseHttpClient) { - c.debugPrintBody = o.debugPrintBody -} - -func WithPrintBody(shouldPrint bool) WrapperOption { - return printBodyOption{debugPrintBody: shouldPrint} -} +// Environment variable to control response body printing +const envDebugPrintBody = "BATON_DEBUG_PRINT_BODY" + +// isDebugPrintBodyEnabled returns true if response body printing is enabled via +// environment variable BATON_DEBUG_PRINT_BODY=1 +func isDebugPrintBodyEnabled() bool { + return os.Getenv(envDebugPrintBody) == "1" +}This approach:
- Prevents debug code from being accidentally committed
- Follows standard practice for debug flags
- Makes it clearer this is a development-only feature
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/uhttp/body_print.go(1 hunks)
🧰 Additional context used
📓 Learnings (1)
pkg/uhttp/body_print.go (1)
Learnt from: shackra
PR: ConductorOne/baton-sdk#276
File: pkg/uhttp/body_print.go:18-25
Timestamp: 2024-12-28T08:09:03.129Z
Learning: The response body printing functionality introduced in `body_print.go` is for development usage only and not intended for production, as explained by shackra in PR #276.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/sync/syncer.go (1)
Line range hint
972-1526: Consider splitting the grant expansion logic into a separate package.The grant expansion logic is complex and self-contained. Moving it to a dedicated package would:
- Improve code organization and maintainability
- Make the logic more testable
- Reduce the size of the syncer.go file
- Allow for better documentation of the grant expansion concepts
This refactoring could be done in a follow-up PR to keep the current changes focused on their original purpose.
🧰 Tools
🪛 GitHub Check: go-lint
[failure] 972-972:
Comment should end in a period (godot)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
.golangci.yml(1 hunks)pkg/sync/syncer.go(2 hunks)pkg/uhttp/body_print.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/uhttp/body_print.go
🧰 Additional context used
🪛 GitHub Check: go-lint
pkg/sync/syncer.go
[failure] 972-972:
Comment should end in a period (godot)
🔇 Additional comments (2)
pkg/sync/syncer.go (1)
36-36: LGTM! Clear and well-structured error declaration.The standalone error declaration improves code readability and follows Go's error naming conventions.
.golangci.yml (1)
39-39: LGTM! Addition of fmt.Fprint is consistent with existing error handling exemptions.The addition of
fmt.Fprintto the unhandled-error exemption list aligns with the existing pattern of exempting fmt package's printing functions, which rarely return meaningful errors in practice.Let's verify the usage of fmt.Fprint in the codebase to ensure it's being used appropriately:
✅ Verification successful
Verified: fmt.Fprint usage is appropriate and error handling is properly implemented
The codebase analysis shows that
fmt.Fprintis used appropriately in two main contexts:
- Debug/logging purposes (e.g., in
pkg/uhttp/body_print.gofor printing response bodies)- Standard output/error writing (e.g., in
pkg/cli/commands.go)In both cases where error handling is critical (writing to os.Stdout), proper error checking is implemented:
pkg/cli/commands.go: Checks and returns the errorpkg/uhttp/body_print.go: Joins with other errors usingerrors.JoinThe remaining usages are in vendor dependencies where the function is used for non-critical operations like debug output or string building with bytes.Buffer (which never returns errors).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze fmt.Fprint usage patterns # Expected: fmt.Fprint should primarily be used for debug/logging purposes # Search for fmt.Fprint usage echo "Analyzing fmt.Fprint usage patterns:" rg --type go "fmt\.Fprint\(" -A 2 -B 2 # Look for any error handling of fmt.Fprint to understand current practices echo -e "\nChecking for any existing error handling of fmt.Fprint:" ast-grep --pattern 'if err := fmt.Fprint($$$); err != nil { $$$ }'Length of output: 7764
December 28 has passed already
shackra
force-pushed
the
shackra/debug-print-body
branch
from
2bc8be3 to
267b1f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/uhttp/wrapper.go (1)
345-349: Add documentation for the debug feature.While the implementation is clean, it would be helpful to add documentation explaining:
- The purpose of
debugPrintBody- How it affects the response handling
- The relationship with
wrapPrintBodyConsider adding a comment block above this section:
+ // If debugPrintBody is enabled, wrap the response body with a reader that prints + // the contents as they are read. This is useful for debugging purposes. if c.debugPrintBody { resp.Body = io.NopCloser(wrapPrintBody(bytes.NewBuffer(body))) } else { resp.Body = io.NopCloser(bytes.NewBuffer(body)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.golangci.yml(1 hunks)pkg/sync/syncer.go(2 hunks)pkg/uhttp/body_print.go(1 hunks)pkg/uhttp/wrapper.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/sync/syncer.go
- pkg/uhttp/body_print.go
🔇 Additional comments (2)
.golangci.yml (1)
39-39: LGTM! Addition offmt.Fprintto unhandled-error exceptions is appropriate.The addition aligns with the PR's debugging feature and follows the existing pattern for fmt package functions.
pkg/uhttp/wrapper.go (1)
94-97: LGTM! Clean addition of the debug flag.The
debugPrintBodyfield is appropriately placed in theBaseHttpClientstruct and follows the existing field organization pattern.
- Adds `BATON_DEBUG_PRINT_RESPONSE_BODY`
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/uhttp/body_print.go (1)
3-13: Enhance documentation to include environment variable usage.The documentation should be updated to reflect both methods of enabling the debug printing:
- Environment variable
BATON_DEBUG_PRINT_RESPONSE_BODYWithPrintBodyoption// Implements a debugging facility for request responses. This changes // the behavior of `BaseHttpClient` with an unexported flag. // // IMPORTANT: This feature is intended for development and debugging purposes only. // Do not enable in production as it may expose sensitive information in logs. // -// Usage: +// Usage (Option 1 - Environment Variable): +// export BATON_DEBUG_PRINT_RESPONSE_BODY=true +// # Run your application +// +// Usage (Option 2 - Client Option): // client := uhttp.NewBaseHttpClient( // httpClient, // uhttp.WithPrintBody(true), // Enable response body printing
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/uhttp/body_print.go(1 hunks)pkg/uhttp/wrapper.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/uhttp/wrapper.go
🧰 Additional context used
📓 Learnings (1)
pkg/uhttp/body_print.go (1)
Learnt from: shackra
PR: ConductorOne/baton-sdk#276
File: pkg/uhttp/body_print.go:18-25
Timestamp: 2024-12-28T08:09:03.129Z
Learning: The response body printing functionality introduced in `body_print.go` is for development usage only and not intended for production, as explained by shackra in PR #276.
🔇 Additional comments (2)
pkg/uhttp/body_print.go (2)
22-24: LGTM! Clean and focused type definition.The
printReadertype follows Go conventions and properly encapsulates the underlying reader.
38-40: LGTM! Clean factory function implementation.The
wrapPrintBodyfunction follows Go conventions and provides a clean way to createprintReaderinstances.
This PR adds debugging functionality that displays the response's body as it arrives to us, not immediately, but when reading it.
Example:
The
uhttp.WithJSONResponse(&target)option on.Dowould trigger a read, and we would see what was read on the console.Summary by CodeRabbit
nolintdirectives.ErrSyncNotCompletevariable.SyncGrantExpansionmethod.