-
Notifications
You must be signed in to change notification settings - Fork 20
Update Wrap middleware arguments #35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- moved Config struct to internal/options/config.go - updated wrap middleware signature to pass config pointer
Reviewer's GuideExtracted configuration into a central options.Config and updated all Option builders, middleware signatures, invocation sites, and tests to work with a single config pointer rather than multiple flag arguments, simplifying the Wrap API and preparing for future flags without altering its signature. Class diagram for refactored Config struct and Option typeclassDiagram
class options.Config {
int MaxRequests
string DashboardPath
bool LogRequestBody
bool LogResponseBody
[]string IgnorePaths
bool EnableOpenTelemetry
string ServiceName
string ServiceVersion
string OTelEndpoint
store.StorageType StorageType
string ConnectionString
string TableName
int RedisTTL
sql.DB* ExistingDB
bool EnableProfiling
profiling.ProfileType ProfileType
time.Duration ProfileThreshold
int MaxProfileMetrics
ShouldIgnorePath(path string) bool
}
class Option {
<<function>>
Option(*options.Config)
}
Option --> options.Config : modifies
Class diagram for updated middleware signaturesclassDiagram
class middleware {
+Wrap(handler http.Handler, store store.Store, config *options.Config, pathMatcher PathMatcher) http.Handler
+WrapWithProfiling(handler http.Handler, store store.Store, config *options.Config, pathMatcher PathMatcher, profiler *profiling.Profiler) http.Handler
}
options.Config <.. middleware : used as parameter
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `internal/options/config.go:59` </location>
<code_context>
+}
+
+// ShouldIgnorePath checks if a path should be ignored based on the configured patterns
+func (c *Config) ShouldIgnorePath(path string) bool {
+ // First check if it's the dashboard path which should always be ignored to prevent recursive logging
+ if path == c.DashboardPath || strings.HasPrefix(path, c.DashboardPath+"/") {
</code_context>
<issue_to_address>
**suggestion:** ShouldIgnorePath uses filepath.Match for pattern matching.
filepath.Match may not handle URL paths correctly. Consider using a matcher better suited for HTTP paths, such as regex or prefix matching.
Suggested implementation:
```golang
for _, pattern := range c.IgnorePaths {
// If pattern starts with '^' or contains special regex characters, treat as regex
if strings.HasPrefix(pattern, "^") || strings.ContainsAny(pattern, ".*+?()[]{}|\\") {
matched, err := regexp.MatchString(pattern, path)
if err == nil && matched {
return true
}
} else {
// Otherwise, treat as simple prefix match
if strings.HasPrefix(path, pattern) {
return true
}
}
```
You will need to:
1. Import the "regexp" package at the top of the file: `import "regexp"`
2. Remove any import of "path/filepath" if it is only used for this matching.
3. Document in your config that ignore patterns can be either prefixes or regexes (starting with ^ or containing regex special characters).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 6 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="internal/middleware/middleware.go">
<violation number="1" location="internal/middleware/middleware.go:55">
Wrap now dereferences config without first guarding against nil, so a nil *options.Config will panic instead of using defaults. Please default or guard the config pointer before accessing its fields.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
Overview
Function signature of Wrap is too large and adding new flags further along the project will keep increasing it's size.
As such, function arguments to accommodate further config flags to be passed without a change in the function's signature.
Changes
Summary by cubic
Switched middleware wrappers to accept a config pointer and moved the Config struct to internal/options to shrink function signatures and make future flags easier to add. No change in behavior.
Refactors
Migration