feat: post with custom params - #75
Conversation
ci: add standard CI workflows to optimum-common
got rid of redundant GetLastCommitHash
go 1.22+ redefines the range variable on each iteration
added versioning package
chore: repo cleanup
* new field in confgi * version bump * version bump * rename field
feat: add gateway utilities to common library
feat: migrate proxy utilities to common library
* identity code added
* hash missed field * tests fix
* slice util helpers * Update pkg/utils/sliceutil_test.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* dynamic config helpers * validate chainid * test fix
* ttl upsert method * review fixes
* pin runners to hash
* added DoAndApply
feat: add generic RotatingFileWriter utility
fix: make TestRenewConfig skip on timeout instead of failing
fix: address all golangci-lint issues
📝 WalkthroughWalkthroughThis change introduces a configurable decoding mechanism for HTTP responses in the request utility package using the options pattern. A new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 📜 Recent review detailsConfiguration used: Repository: getoptimum/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Lite 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.go📄 CodeRabbit inference engine (Custom checks)
Files:
⚙️ CodeRabbit configuration file
Files:
**/*⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (2)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
pkg/utils/request.go (4)
25-31: Add options support to align with PostCurl and GetCurl.PatchCurl does not support custom decoders via options, while PostCurl (line 33) and GetCurl (line 67) both do. This API inconsistency limits functionality and creates an uneven developer experience.
🔧 Proposed fix to add options support
-func PatchCurl[T any](ctx context.Context, targetURL string, payload any, headers map[string]string) (res *T, statusCode int, err error) { +func PatchCurl[T any](ctx context.Context, targetURL string, payload any, headers map[string]string, opts ...CurlOpts[T]) (res *T, statusCode int, err error) { + config := &CurlConf[T]{} + for _, opt := range opts { + opt(config) + } payloadJSON, err := json.Marshal(payload) if err != nil { return res, 0, fmt.Errorf("unable to marshal payload: %w", err) } - return CurlWithBody[T](ctx, http.MethodPatch, targetURL, payloadJSON, headers, nil) + return CurlWithBody[T](ctx, http.MethodPatch, targetURL, payloadJSON, headers, config) }
45-60: LGTM! Decoder configuration properly threaded through.The conditional logic correctly handles both nil and non-nil config cases. Context propagation is proper.
Optional micro-optimization: The two branches (lines 56-59) could be unified since both may pass a nil decoder:
decoder := (*func(io.Reader) error)(nil) if cfg != nil { decoder = cfg.Decoder } return executeWithDefaultClient[T](req, decoder)However, the current approach is equally clear.
90-92: Decoder usage returns nil result, breaking the function contract.When a custom decoder is provided, the function returns
nilfor the result (line 91) while the signature promises*T. Callers may expect to receive both a decoded result and use the decoder for side effects, leading to nil pointer dereferences.Consider returning a zero value or documenting that decoders are responsible for all result handling and that the returned
*Tmust be ignored.
82-88: Address the timeout TODO for production reliability.The TODO (line 82) correctly identifies that
http.DefaultClientlacks timeout configuration, allowing requests to hang indefinitely. This impacts all exported functions: PostCurl, PatchCurl, GetCurl, and CurlWithBody.Consider creating a package-level client with sensible defaults:
var defaultClient = &http.Client{ Timeout: 30 * time.Second, }Then use
defaultClient.Do(req)instead ofhttp.DefaultClient.Do(req)at line 85.
📜 Review details
Configuration used: Repository: getoptimum/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (1)
pkg/utils/request.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (Custom checks)
Watch for goroutine leaks, unbounded channels, and racy access; ensure contexts are plumbed and respected
Files:
pkg/utils/request.go
⚙️ CodeRabbit configuration file
Review Go code for: idiomatic patterns, error handling (check all errors), concurrency safety (context propagation, goroutine cleanup), test coverage for changed logic, exported API stability, memory efficiency (avoid unnecessary allocations), and prefer standard library over third-party when reasonable.
Files:
pkg/utils/request.go
**/*
⚙️ CodeRabbit configuration file
**/*: When you find the same underlying issue in multiple locations (same pattern, same fix):
- Do NOT post separate comments for each occurrence.
- Post a single primary comment on the first occurrence.
- In that comment, include a short list of “Also at: file:line” references
for the other locations (e.g. 'Also at: foo.go:42, bar.go:17').
Prioritize signal over volume: one strong comment that references multiple
locations is preferred over many near-identical comments.
Files:
pkg/utils/request.go
🔇 Additional comments (2)
pkg/utils/request.go (2)
13-23: LGTM! Clean options pattern implementation.The functional options pattern is idiomatic and allows for future extensibility. The generic types are properly constrained.
33-43: LGTM! Options pattern correctly implemented.The variadic options parameter is backwards-compatible, and the config building logic is clean and idiomatic.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.