fix: resolve golangci-lint errors#6
Conversation
- S1000: Simplify for { select {} } to for range ticker.C in rate_limit.go
- S1039: Remove unnecessary fmt.Sprintf calls in auth_service.go
- ineffassign: Fix ineffectual assignment to limit in GetUpcomingHolidays
ilramdhan
added
bug
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| if limiter.TokensAt(time.Now()) == float64(rl.burst) { | ||
| delete(rl.visitors, ip) | ||
| } | ||
| for range ticker.C { |
There was a problem hiding this comment.
WARNING: Performance issue - The mutex lock (rl.mu.Lock()) is now held during the entire iteration over all visitors (lines 57-63). This can cause significant blocking of other goroutines that need to check/update rate limits.
Previous code acquired the lock inside the select block for each tick, but still held it during iteration. The current simplification using for range ticker.C doesn't change the locking behavior, but the code is now clearer. However, consider copying the map keys first, then releasing the lock before processing:
for range ticker.C {
rl.mu.Lock()
ips := make([]string, 0, len(rl.visitors))
for ip := range rl.visitors {
ips = append(ips, ip)
}
rl.mu.Unlock()
for _, ip := range ips {
rl.mu.Lock()
limiter, exists := rl.visitors[ip]
rl.mu.Unlock()
if exists && limiter.TokensAt(time.Now()) == float64(rl.burst) {
rl.mu.Lock()
delete(rl.visitors, ip)
rl.mu.Unlock()
}
}
}This allows other operations to proceed while cleanup is happening.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Positive Changes
Files Reviewed (3 files)
Language: Go (Golang) Note: The changes to |
There was a problem hiding this comment.
Pull request overview
This PR aims to address golangci-lint findings by simplifying control flow in the rate limiter, removing redundant string formatting in auth auditing, and fixing an ineffectual assignment by actually applying the limit parameter in upcoming-holidays retrieval.
Changes:
- Apply
limitinGetUpcomingHolidaysby slicing the results. - Remove an unnecessary
fmt.Sprintfcall when logging a successful login audit entry. - Simplify the rate-limiter cleanup loop from
for { select { ... } }tofor range ticker.C.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
internal/services/holiday_service.go |
Fetch upcoming holidays and apply limit in the service before returning results. |
internal/services/auth_service.go |
Replace a redundant fmt.Sprintf call with a string literal in audit logging. |
internal/middleware/rate_limit.go |
Simplify periodic cleanup loop to a for range over the ticker channel. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Log successful login | ||
| s.logAudit(&user.ID, user.Username, models.ActionLogin, models.ResourceAuth, | ||
| fmt.Sprintf("User logged in successfully"), ipAddress, userAgent, true) | ||
| "User logged in successfully", ipAddress, userAgent, true) |
There was a problem hiding this comment.
This removes one unnecessary fmt.Sprintf, but there are still several fmt.Sprintf calls in Login that pass only a constant string (e.g., the audit logs for "user not found", "invalid password", "account deactivated", and "token generation error"). If golangci-lint S1039 is enabled, those remaining calls will still be flagged—replace them with plain string literals (or add formatting args if intended).
| holidays, err := s.repo.GetByDateRange(today, endDate, nil) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Apply limit | ||
| if len(holidays) > limit { | ||
| holidays = holidays[:limit] | ||
| } |
There was a problem hiding this comment.
Applying the limit here happens before the handler’s optional type filter (which is applied after calling the service). This means /holidays/upcoming?limit=N&type=... can return fewer than N items even when more matching holidays exist. Consider moving the type filtering into the service (e.g., accept an optional HolidayType and pass it to GetByDateRange) or re-applying the limit after filtering at the handler level so limit constrains the final result set.
| // Apply limit | ||
| if len(holidays) > limit { | ||
| holidays = holidays[:limit] | ||
| } | ||
|
|
||
| return holidays, nil |
There was a problem hiding this comment.
GetUpcomingHolidays now enforces limit by slicing the returned list. There are existing unit tests for this service, but none cover this new limiting behavior (including edge cases like fewer results than the limit). Adding a focused test for this method would help prevent regressions.
Description
This PR fixes all golangci-lint errors to ensure CI pipeline passes. Changes include simplifying the ticker loop pattern, removing unnecessary fmt.Sprintf calls, and fixing the ineffectual assignment to limit variable in GetUpcomingHolidays.
Fixes # (issue number)
Type of Change
Changes Made
for { select { case <-ticker.C: ... } }tofor range ticker.C { ... }in rate_limit.go cleanupVisitors()fmt.Sprintf()calls with string literals in auth_service.go Login methodlimitvariable in holiday_service.go GetUpcomingHolidays() by adding limit logic to slice resultsHow Has This Been Tested?
Test Configuration:
Checklist
Screenshots (if applicable)
Additional Notes
These changes resolve the following golangci-lint errors:
for rangeinstead offor { select {} }fmt.SprintfcallsRelated PRs