Add compliance#155
Conversation
Includes `.golangci.yaml` for linting configuration, `DCO.txt`, and SPDX license files for compliance. Introduces GitHub workflow for REUSE compliance and DCO enforcement. Signed-off-by: Alexander Adam <alphaone23@gmail.com>
Signed-off-by: Alexander Adam <alphaone23@gmail.com>
…omments. - Added package-level documentation for clarity. - Standardized comment formatting. - Renamed and relocated test packages to align with conventions. - Replaced hardcoded request methods with constants. - Enhanced HTTP request initialization with context awareness. Signed-off-by: Alexander Adam <alphaone23@gmail.com>
…n LocalLimit. Signed-off-by: Alexander Adam <alphaone23@gmail.com>
Signed-off-by: Alexander Adam <alphaone23@gmail.com>
- Introduced `t.Run` for subtests to improve test isolation and parallelism. - Standardized test case naming with `fmt.Sprintf`. - Added missing context-aware functions for requests. - Reorganized declarations for improved readability. Signed-off-by: Alexander Adam <alphaone23@gmail.com>
…ype-checking. - Renamed test packages for alignment with Go conventions (e.g., `*_test` naming). - Replaced unqualified type usage with fully-qualified imports. - Added validation for type assertions to prevent runtime panics. - Improved error handling and logging in test cases. Signed-off-by: Alexander Adam <alphaone23@gmail.com>
Signed-off-by: Alexander Adam <alphaone23@gmail.com>
…bility. - Replaced ambiguous variable names with clear, descriptive names (`v` -> `test`). - Standardized subtest names using `t.Run` and `fmt.Sprintf` with index formatting. - Improved usage of constants in request methods for better readability. - Updated handler constructors for cleaner parameter assignment and naming alignment. - Consolidated duplicate code in rate limiter functionality. Signed-off-by: Alexander Adam <alphaone23@gmail.com>
- Renamed packages and files for clearer separation (`rate_limit` -> `ratelimit`, `basic_auth` -> `basicauth`). - Updated imports across all usages to reflect new package names. - Added error constants for better error management and maintainability. - Replaced `util` with `helper` for utility functions. - Improved test assertions and standardized test names. - Enhanced middleware configurations for uniformity. Signed-off-by: Alexander Adam <alphaone23@gmail.com>
WalkthroughPR adds SPDX headers and licensing/governance files, introduces new CI workflows (compliance, release, dependency review), renames many packages (util→helper, access_log→accesslog, etc.), adds sentinel errors and GetMWBase accessors, refactors tests/examples, updates go.mod and tooling, and moves/rewrites many handler implementations and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant GH as GitHub
participant WF as Compliance Workflow
note over GH,WF: push or pull_request
GH->>WF: trigger (push / pull_request)
rect rgb(245,248,255)
WF->>WF: checkout repo
WF->>WF: run REUSE/fsfe action
end
rect rgb(255,245,240)
WF->>WF: list commits (push) / gh api list PR commits (PR)
WF->>WF: verify "Signed-off-by" per commit
alt missing Signed-off-by
WF-->>GH: fail (DCO check failed)
else
WF-->>GH: pass
end
end
sequenceDiagram
autonumber
actor Rel as Release Manager
participant GH as GitHub
participant RL as Release Workflow
participant SLSA as Provenance Builder
Rel->>GH: publish release
GH->>RL: trigger
RL->>RL: build source tar.gz
RL->>GH: upload release asset
RL->>RL: download assets, compute sha256, base64 encode
RL->>SLSA: generate provenance subjects
SLSA-->>GH: attach provenance to release
sequenceDiagram
autonumber
participant Client
participant MW as Middleware
participant Help as helper (IntroCheck/WriteState)
participant Next as next http.Handler
Client->>MW: HTTP request
MW->>Help: IntroCheck(ctx, mw, req)
alt intro error
MW->>Help: WriteState(error)
MW-->>Client: 500 or error response
else
MW->>Next: ServeHTTP
Next-->>MW: response
MW-->>Client: response (possibly mutated)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
Codecov Report❌ Patch coverage is
|
Signed-off-by: Alexander Adam <alphaone23@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
.github/workflows/security.yml (1)
72-79: Critical shell script errors will cause workflow failure.The PrintSarif step contains multiple critical issues:
- Typo in filename: Line 74 uses
govulncheck-results.serifbut should begovulncheck-results.sarif- Invalid conditional syntax:
if [ grep results ... ]doesn't execute grep; it tests if the string "grep" is non-empty (always true)- Missing step ID: Line 82 references
steps.PrintSarif.outputs.hasResults, but the PrintSarif step (line 71-79) lacks anid: PrintSariffield- Incorrect output assignment:
echo "..." >> $GITHUB_OUTPUTon lines 76, 78 will fail because$GITHUB_OUTPUTis only available within a step that has outputs definedApply this diff to fix all issues:
- - name: PrintSarif - run: | + - name: PrintSarif + id: PrintSarif + run: | cat govulncheck-results.sarif - if [ grep results govulncheck-results.serif ] - then - echo "hasResults=true" >> $GITHUB_OUTPUT - else - echo "hasResults=false" >> $GITHUB_OUTPUT - fi + if grep -q '"results"' govulncheck-results.sarif; then + echo "hasResults=true" >> $GITHUB_OUTPUT + else + echo "hasResults=false" >> $GITHUB_OUTPUT + fihandler/correlation/correlation_test.go (2)
15-40: Fix header name casing inconsistency.Line 37 uses
"X-Correlation-iD"(lowercase 'i', uppercase 'D') which is inconsistent with the correct casing"X-Correlation-ID"used elsewhere (lines 21, 48, 56).Apply this diff to fix the casing:
- if rec.Header().Get("X-Correlation-iD") == "" { + if rec.Header().Get("X-Correlation-ID") == "" {
42-68: Fix header name casing inconsistency.Line 65 uses
"X-Correlation-iD"(lowercase 'i', uppercase 'D') which is inconsistent with the correct casing"X-Correlation-ID".Apply this diff to fix the casing:
- if rec.Header().Get("X-Correlation-iD") != "setOutside" { + if rec.Header().Get("X-Correlation-ID") != "setOutside" {handler/accesslog/base_test.go (1)
100-108: Fix typo in error message.Line 106 contains a typo: "expted" should be "expected".
- t.Errorf("expted error on configuring nil handler") + t.Errorf("expected error on configuring nil handler")
🧹 Nitpick comments (11)
GOVERNANCE.md (2)
24-25: Consider clarifying core contributor recognition criteria.The definition relies on GitHub "Member" badges, which requires organization membership management. Consider documenting:
- Who has authority to grant core contributor/member status
- Specific criteria for repeated and significant contributions (e.g., number of merged PRs, time period, types of contributions)
This would make the recognition process more transparent and actionable.
35-43: Consider clarifying timing expectations.The decision-making process is well-structured, but two timing-related aspects could be more explicit:
- What constitutes "reasonable time" for reaching consensus before escalating to the BDFL decision process?
- The 48-hour waiting period is practical, though you may want to note whether this is business hours or calendar hours, especially for a distributed contributor base.
These clarifications would help contributors understand when and how the escalation process should be initiated.
helper/helper_internal_test.go (2)
7-7: Consider a more idiomatic naming convention.The variable name
TexitFuncwith the "T" prefix is unconventional. Consider renaming to something more descriptive and idiomatic, such asExitFuncForTestingorTestableExitFunc, to better communicate its purpose.Apply this diff to improve the naming:
-var TexitFunc = &exitFunc //nolint:gochecknoglobals +var ExitFuncForTesting = &exitFunc //nolint:gochecknoglobals
7-7: Rename TexitFunc to ExitFuncForTesting
TheexitFuncglobal is correctly defined in helper/helper.go:20; in helper/helper_internal_test.go, expose it asExitFuncForTestingfor clearer intent..github/workflows/release.yml (2)
28-32: Consider reducing fetch-depth for efficiency.The
fetch-depth: 0fetches full repository history, but since you're checking out a specific tag and usinggit archive, only that tag's tree is needed.Consider this optimization:
- with: - fetch-depth: 0 - ref: ${{ github.event.release.tag_name }} + with: + fetch-depth: 1 + ref: ${{ github.event.release.tag_name }}This reduces clone time and bandwidth without impacting the archive generation.
104-104: Fix spacing for YAML linting consistency.YAMLlint detected extra spaces after the colon. While functionally correct, fixing this improves consistency with YAML style guides.
Apply this diff:
- actions: read # Needed for detection of GitHub Actions environment. + actions: read # Needed for detection of GitHub Actions environment.helper/helper_test.go (2)
80-80: Simplify the boolean expression.The expression
!v.wantNew == trueis confusing and redundant. It's equivalent to!v.wantNewor more clearlyv.wantNew == false.Apply this diff to improve clarity:
- if !v.wantNew == true && got != v.in { + if !v.wantNew && got != v.in {
146-185: Consider isolating test cases with individual recorders.The test reuses a single
httptest.NewRecorder()across all iterations. While this works sequentially, each test case should have its own recorder for better isolation and to prevent state accumulation between test cases.Apply this diff to create a recorder per test case:
- rec := httptest.NewRecorder() - for k, v := range tests { + rec := httptest.NewRecorder() got := helper.IntroCheck(v.h, rec, v.req) if got != v.want {Alternatively, convert to subtests with
t.Runfor better test isolation and clearer output:- rec := httptest.NewRecorder() - for k, v := range tests { - got := helper.IntroCheck(v.h, rec, v.req) - - if got != v.want { - t.Errorf("%v: got %v but wanted %v", k, got, v.want) - } + t.Run(fmt.Sprintf("TestIntroCheck-%d", k), func(t *testing.T) { + t.Parallel() + rec := httptest.NewRecorder() + got := helper.IntroCheck(v.h, rec, v.req) + + if got != v.want { + t.Errorf("got %v but wanted %v", got, v.want) + } + }) }.golangci.yaml (1)
42-44: Consider lowering the cyclomatic complexity threshold.A maximum complexity of 25 is relatively high—most projects target 10-15 to encourage decomposition. While middleware logic can have legitimate branching, this threshold may allow overly complex functions to pass review.
Consider reducing to 15-20 if feasible, or document why 25 is necessary for this codebase.
.github/workflows/compliance.yml (2)
101-153: LGTM: DCO check for pull requests with bypass option.The PR-level DCO check correctly:
- Uses GitHub API with pagination to fetch all PR commits
- Provides a
bypass-dcolabel escape hatch- Validates each commit's Signed-off-by line
The same shell modernization note from the push job applies here (line 139: use
$()instead of backticks).Consider extracting the Signed-off-by verification logic into a reusable composite action or shell function, since lines 76-99 and lines 131-153 are nearly identical.
38-99: Verify edge cases in DCO check for push events.
- The zero-SHA branch check (
git rev-list --no-merges "$AFTER") lists the entire commit history. If you only intend to check new commits, constrain the list (e.g. add--max-countor adjust the range).- Use modern command substitution:
msg=$(git log --format=%B -n 1 "$sha")instead of backticks.- Note the regex (
^[[:space:]]*Signed[- ]off[- ]by:) correctly matches “Signed-off-by” with hyphens or spaces but won’t match “Signedoffby” without separators—update it if you need to catch that.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.summidgard_logo.svgis excluded by!**/*.svg
📒 Files selected for processing (84)
.github/CODEOWNERS(1 hunks).github/FUNDING.yml(1 hunks).github/ISSUE_TEMPLATE/bug_report.md(2 hunks).github/ISSUE_TEMPLATE/feature_request.md(1 hunks).github/PULL_REQUEST_TEMPLATE.md(2 hunks).github/dependabot.yml(1 hunks).github/workflows/codeql.yml(2 hunks).github/workflows/compliance.yml(1 hunks).github/workflows/dependency-review.yml(2 hunks).github/workflows/release.yml(1 hunks).github/workflows/scorecard.yml(1 hunks).github/workflows/security.yml(1 hunks).github/workflows/test.yml(7 hunks).gitignore(1 hunks).golangci.yaml(1 hunks).markdownlint.json.license(1 hunks)AUTHORS.md(1 hunks)CHANGELOG.md(2 hunks)CODE_OF_CONDUCT.md(2 hunks)CONTRIBUTING.md(3 hunks)DCO.txt(1 hunks)GOVERNANCE.md(1 hunks)LICENSES/CC-BY-4.0.txt(1 hunks)LICENSES/LicenseRef-DCO.txt(1 hunks)LICENSES/MPL-2.0.txt(1 hunks)README.md(4 hunks)SECURITY.md(1 hunks)defs/middleware.go(1 hunks)defs/mwbase.go(7 hunks)defs/mwbase_test.go(9 hunks)example/example_test.go(3 hunks)example/hello.html(1 hunks)example/main.go(3 hunks)go.mod(2 hunks)go.sum.license(1 hunks)handler/accesslog/README.md(3 hunks)handler/accesslog/access_log.go(4 hunks)handler/accesslog/access_log_test.go(4 hunks)handler/accesslog/base_test.go(5 hunks)handler/add_header/add_headers_test.go(0 hunks)handler/add_header/base_test.go(0 hunks)handler/addheader/README.md(2 hunks)handler/addheader/add_header.go(3 hunks)handler/addheader/add_headers_test.go(1 hunks)handler/addheader/base_test.go(5 hunks)handler/basic_auth/map_auth/README.md(0 hunks)handler/basic_auth/map_auth/map_auth_test.go(0 hunks)handler/basicauth/README.md(3 hunks)handler/basicauth/base_test.go(1 hunks)handler/basicauth/basic_auth.go(9 hunks)handler/basicauth/basic_auth_test.go(5 hunks)handler/basicauth/htpasswdauth/README.md(2 hunks)handler/basicauth/htpasswdauth/htpasswd_auth.go(3 hunks)handler/basicauth/htpasswdauth/htpasswd_auth_test.go(3 hunks)handler/basicauth/htpasswdauth/testwd.license(1 hunks)handler/basicauth/mapauth/README.md(1 hunks)handler/basicauth/mapauth/map_auth.go(4 hunks)handler/basicauth/mapauth/map_auth_test.go(1 hunks)handler/correlation/README.md(1 hunks)handler/correlation/base_test.go(5 hunks)handler/correlation/correlation.go(4 hunks)handler/correlation/correlation_test.go(2 hunks)handler/cors/README.md(2 hunks)handler/cors/base_test.go(5 hunks)handler/cors/cors.go(8 hunks)handler/cors/cors_test.go(2 hunks)handler/methodfilter/README.md(2 hunks)handler/methodfilter/base_test.go(5 hunks)handler/methodfilter/methods_filter.go(3 hunks)handler/methodfilter/methods_filter_test.go(3 hunks)handler/rate_limit/README.md(0 hunks)handler/rate_limit/base_test.go(0 hunks)handler/ratelimit/README.md(1 hunks)handler/ratelimit/base_test.go(1 hunks)handler/ratelimit/locallimit/README.md(2 hunks)handler/ratelimit/locallimit/local_limit.go(7 hunks)handler/ratelimit/locallimit/local_limit_test.go(9 hunks)handler/ratelimit/rate_limit.go(5 hunks)handler/ratelimit/rate_limit_test.go(3 hunks)helper/helper.go(4 hunks)helper/helper_internal_test.go(1 hunks)helper/helper_test.go(10 hunks)midgard.go(1 hunks)midgard_test.go(3 hunks)
💤 Files with no reviewable changes (6)
- handler/basic_auth/map_auth/README.md
- handler/rate_limit/README.md
- handler/add_header/base_test.go
- handler/add_header/add_headers_test.go
- handler/rate_limit/base_test.go
- handler/basic_auth/map_auth/map_auth_test.go
🧰 Additional context used
🧬 Code graph analysis (41)
handler/basicauth/mapauth/README.md (1)
handler/basic_auth/map_auth/map_auth.go (4)
auths(9-11)New(50-64)WithAuths(31-47)a(15-28)
handler/ratelimit/base_test.go (4)
handler/ratelimit/rate_limit.go (5)
Handler(29-33)New(83-107)WithLogLevel(78-80)WithLimiter(60-70)WithLogger(73-75)handler/ratelimit/locallimit/local_limit.go (1)
New(173-194)helper/helper.go (2)
Must(24-33)DummyHandler(114-116)defs/mwbase.go (2)
WithLogLevel(118-126)WithLogger(106-114)
handler/basicauth/basic_auth.go (3)
handler/accesslog/access_log.go (3)
New(78-98)Handler(21-23)ErrNilOption(18-18)helper/helper.go (2)
IntroCheck(94-110)WriteState(74-86)handler/basic_auth/basic_auth_test.go (2)
TestBasicAuthNoAuthenticator(121-128)TestBasicAuth(28-93)
handler/methodfilter/README.md (2)
handler/method_filter/methods_filter.go (2)
WithMethods(51-63)MWBase(16-20)handler/method_filter/methods_filter_test.go (4)
FuzzMethodFilter(84-107)TestMethodFilter(15-67)filter(16-20)TestMethodFilterUninitialized(69-82)
example/main.go (5)
helper/helper.go (1)
Must(24-33)handler/accesslog/access_log.go (3)
New(78-98)WithLogLevel(73-75)Handler(21-23)handler/cors/cors.go (5)
New(203-229)WithLogLevel(195-197)WithHeaders(146-159)MinimumAllowHeaders(56-67)Handler(28-43)handler/methodfilter/methods_filter.go (3)
New(83-103)WithLogLevel(78-80)Handler(20-25)defs/mwbase.go (1)
WithLogLevel(118-126)
defs/mwbase.go (3)
handler/accesslog/access_log.go (1)
New(78-98)handler/addheader/add_header.go (1)
New(68-88)handler/correlation/correlation.go (1)
New(67-87)
handler/addheader/README.md (2)
handler/add_header/add_header.go (4)
New(61-80)MWBase(16-19)WithHeaders(53-58)h(30-40)handler/add_header/add_headers_test.go (1)
TestAddHeaders(14-27)
handler/methodfilter/methods_filter_test.go (2)
helper/helper.go (3)
Must(24-33)DummyHandler(114-116)MapKeys(38-54)handler/methodfilter/methods_filter.go (2)
New(83-103)WithMethods(58-70)
handler/accesslog/base_test.go (3)
helper/helper.go (2)
Must(24-33)DummyHandler(114-116)defs/mwbase.go (2)
WithLogLevel(118-126)WithLogger(106-114)handler/access_log/access_log_test.go (3)
TestAccessLogging(18-59)TestAccessLoggingUser(85-107)TestAccessLoggingCorrelationID(61-83)
handler/methodfilter/base_test.go (3)
handler/methodfilter/methods_filter.go (4)
Handler(20-25)New(83-103)WithLogLevel(78-80)WithLogger(73-75)helper/helper.go (2)
Must(24-33)DummyHandler(114-116)defs/mwbase.go (2)
WithLogLevel(118-126)WithLogger(106-114)
handler/basicauth/htpasswdauth/htpasswd_auth.go (1)
handler/basic_auth/htpasswd_auth/htpasswd_auth_test.go (5)
TestHtpasswdEmptyFilename(82-88)TestHtpasswdWrongReader(74-80)TestHtpasswdNoOptions(66-72)TestHtpasswdNil(50-56)TestHtpasswdNonExistingFile(58-64)
handler/methodfilter/methods_filter.go (3)
handler/accesslog/access_log.go (3)
New(78-98)Handler(21-23)ErrNilOption(18-18)defs/mwbase.go (1)
MWBase(24-28)helper/helper.go (2)
IntroCheck(94-110)WriteState(74-86)
handler/basicauth/mapauth/map_auth.go (2)
handler/basicauth/htpasswdauth/htpasswd_auth.go (2)
New(79-93)ErrNotInitialized(21-21)defs/mwbase.go (1)
ErrNotInitialized(21-21)
README.md (1)
handler/access_log/access_log_test.go (1)
TestAccessLogging(18-59)
handler/cors/base_test.go (3)
handler/cors/cors.go (4)
Handler(28-43)New(203-229)WithLogLevel(195-197)WithLogger(190-192)helper/helper.go (2)
Must(24-33)DummyHandler(114-116)defs/mwbase.go (2)
WithLogLevel(118-126)WithLogger(106-114)
midgard_test.go (5)
midgard.go (2)
StackMiddlewareHandler(30-36)StackMiddleware(16-27)defs/middleware.go (1)
Middleware(15-15)helper/helper.go (2)
Must(24-33)DummyHandler(114-116)handler/correlation/correlation.go (1)
New(67-87)handler/methodfilter/methods_filter.go (2)
New(83-103)WithMethods(58-70)
handler/ratelimit/locallimit/local_limit_test.go (2)
helper/helper.go (1)
Must(24-33)handler/ratelimit/locallimit/local_limit.go (6)
New(173-194)WithTargetRate(160-170)WithSleepInterval(147-157)WithMaxDropsAbsolute(123-129)WithMaxDropsInterval(136-142)WithDropTimeout(112-118)
handler/correlation/base_test.go (3)
handler/correlation/correlation.go (4)
Handler(20-22)New(67-87)WithLogLevel(62-64)WithLogger(57-59)helper/helper.go (2)
Must(24-33)DummyHandler(114-116)defs/mwbase.go (2)
WithLogLevel(118-126)WithLogger(106-114)
handler/basicauth/htpasswdauth/htpasswd_auth_test.go (2)
helper/helper.go (1)
Must(24-33)handler/basicauth/htpasswdauth/htpasswd_auth.go (4)
New(79-93)WithAuthFile(60-76)HTPassWDAuth(24-26)WithAuthInput(44-56)
handler/addheader/add_header.go (3)
handler/accesslog/access_log.go (3)
New(78-98)Handler(21-23)ErrNilOption(18-18)defs/mwbase.go (1)
MWBase(24-28)helper/helper.go (1)
IntroCheck(94-110)
handler/basicauth/htpasswdauth/README.md (2)
handler/basic_auth/htpasswd_auth/htpasswd_auth.go (4)
auth(15-17)WithAuthFile(50-66)WithAuthInput(35-46)New(69-83)handler/basic_auth/basic_auth.go (3)
MWBase(28-34)h(83-116)WithAuthenticator(119-125)
handler/addheader/base_test.go (2)
helper/helper.go (2)
Must(24-33)DummyHandler(114-116)defs/mwbase.go (2)
WithLogLevel(118-126)WithLogger(106-114)
handler/cors/cors.go (3)
defs/mwbase.go (1)
MWBase(24-28)helper/helper.go (2)
IntroCheck(94-110)WriteState(74-86)defs/middleware.go (1)
Middleware(15-15)
handler/cors/cors_test.go (2)
helper/helper.go (2)
Must(24-33)DummyHandler(114-116)handler/cors/cors.go (5)
New(203-229)WithMethods(163-176)WithHeaders(146-159)MinimumAllowHeaders(56-67)WithOrigins(181-187)
handler/basicauth/mapauth/map_auth_test.go (1)
handler/basicauth/mapauth/map_auth.go (2)
New(57-71)WithAuths(38-54)
handler/basicauth/base_test.go (4)
handler/basicauth/basic_auth.go (5)
Handler(35-42)New(167-197)WithLogLevel(162-164)WithAuthenticator(130-136)WithLogger(157-159)handler/basicauth/mapauth/map_auth.go (2)
New(57-71)WithAuths(38-54)helper/helper.go (2)
Must(24-33)DummyHandler(114-116)defs/mwbase.go (2)
WithLogLevel(118-126)WithLogger(106-114)
handler/addheader/add_headers_test.go (2)
helper/helper.go (2)
Must(24-33)DummyHandler(114-116)handler/addheader/add_header.go (2)
New(68-88)WithHeaders(59-65)
handler/accesslog/access_log_test.go (1)
helper/helper.go (2)
Must(24-33)DummyHandler(114-116)
handler/ratelimit/README.md (3)
handler/rate_limit/rate_limit.go (4)
Limit(17-19)MWBase(22-25)New(73-96)h(36-47)handler/rate_limit/local_limit/local_limit.go (2)
TargetRate(16-45)l(80-82)handler/rate_limit/local_limit/local_limit_test.go (2)
TargetRate(14-19)TestLocalLimitRate(13-61)
defs/mwbase_test.go (2)
defs/mwbase.go (4)
MWBase(24-28)WithLogger(106-114)MWBaser(31-34)WithLogLevel(118-126)handler/correlation/correlation.go (3)
New(67-87)WithLogger(57-59)WithLogLevel(62-64)
handler/ratelimit/rate_limit.go (4)
handler/accesslog/access_log.go (3)
New(78-98)Handler(21-23)ErrNilOption(18-18)helper/helper.go (2)
IntroCheck(94-110)WriteState(74-86)handler/rate_limit/rate_limit_test.go (1)
TestNilLimiter(56-62)handler/rate_limit/base_test.go (2)
TestOptionNil(55-61)TestHandlerNil(22-37)
handler/ratelimit/rate_limit_test.go (5)
midgard.go (1)
StackMiddlewareHandler(30-36)defs/middleware.go (1)
Middleware(15-15)helper/helper.go (2)
Must(24-33)DummyHandler(114-116)handler/ratelimit/locallimit/local_limit.go (4)
New(173-194)WithTargetRate(160-170)WithDropTimeout(112-118)WithSleepInterval(147-157)handler/ratelimit/rate_limit.go (2)
New(83-107)WithLimiter(60-70)
handler/ratelimit/locallimit/README.md (1)
handler/rate_limit/local_limit/local_limit.go (6)
TargetRate(16-45)WithSleepInterval(138-148)l(80-82)l(48-77)New(164-181)WithTargetRate(151-161)
helper/helper_test.go (2)
helper/helper.go (6)
Must(24-33)GetOrCreateID(57-69)DummyHandler(114-116)WriteState(74-86)IntroCheck(94-110)MapKeys(38-54)helper/helper_internal_test.go (1)
TexitFunc(7-7)
handler/basicauth/README.md (2)
handler/basic_auth/map_auth/map_auth.go (3)
WithAuths(31-47)auths(9-11)New(50-64)handler/basic_auth/basic_auth.go (2)
MWBase(28-34)WithAuthenticator(119-125)
handler/correlation/correlation_test.go (2)
helper/helper.go (1)
Must(24-33)handler/correlation/correlation.go (1)
New(67-87)
helper/helper.go (2)
defs/mwbase.go (1)
MWBaser(31-34)util/util_test.go (1)
TestMapKeys(170-206)
handler/basicauth/basic_auth_test.go (4)
helper/helper.go (2)
Must(24-33)DummyHandler(114-116)handler/basicauth/basic_auth.go (4)
New(167-197)WithAuthenticator(130-136)WithRealm(139-145)WithRedirect(148-154)midgard.go (1)
StackMiddlewareHandler(30-36)defs/middleware.go (1)
Middleware(15-15)
handler/accesslog/README.md (1)
handler/access_log/access_log_test.go (1)
TestAccessLogging(18-59)
handler/accesslog/access_log.go (3)
handler/basicauth/basic_auth.go (2)
Handler(35-42)ExtractUserPass(56-79)defs/mwbase.go (1)
MWBase(24-28)helper/helper.go (1)
IntroCheck(94-110)
handler/correlation/correlation.go (4)
handler/accesslog/access_log.go (3)
New(78-98)Handler(21-23)ErrNilOption(18-18)handler/addheader/add_header.go (3)
New(68-88)Handler(20-24)ErrNilOption(17-17)defs/mwbase.go (1)
MWBase(24-28)helper/helper.go (2)
IntroCheck(94-110)GetOrCreateID(57-69)
🪛 GitHub Actions: Security
defs/middleware.go
[error] 4-4: package requires newer Go version go1.24 (application built with go1.23)
🪛 GitHub Check: VulnerabilityCheck (1.23)
handler/basicauth/basic_auth.go
[failure] 5-5:
package requires newer Go version go1.24 (application built with go1.23)
handler/methodfilter/methods_filter.go
[failure] 5-5:
package requires newer Go version go1.24 (application built with go1.23)
handler/addheader/add_header.go
[failure] 5-5:
package requires newer Go version go1.24 (application built with go1.23)
handler/cors/cors.go
[failure] 5-5:
package requires newer Go version go1.24 (application built with go1.23)
helper/helper.go
[failure] 5-5:
package requires newer Go version go1.24 (application built with go1.23)
handler/accesslog/access_log.go
[failure] 5-5:
package requires newer Go version go1.24 (application built with go1.23)
🪛 YAMLlint (1.37.1)
.github/workflows/release.yml
[warning] 104-104: too many spaces after colon
(colons)
Signed-off-by: Alexander Adam <alphaone23@gmail.com>
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 (1)
.github/workflows/security.yml (1)
71-81: Fix PrintSarif results detection.
grep "'results'"never matches because SARIF JSON encodes the requiredresultsproperty with double quotes, sohasResultsstays false and the upload step is skipped even when vulnerabilities exist.(docs.oasis-open.org)Apply this diff to restore correct behavior:
- if grep "'results'" govulncheck-results.sarif + if grep '"results"' govulncheck-results.sarif
♻️ Duplicate comments (9)
handler/ratelimit/base_test.go (2)
107-115: Fix typo in error message (already flagged).Line 113 contains the typo "expted" which should be "expected". This was previously identified.
Apply this diff:
- t.Errorf("expted error on configuring nil handler") + t.Errorf("expected error on configuring nil handler")
141-149: Fix typo in error message (already flagged).Line 147 contains the same typo "expted" which should be "expected". This was previously identified.
Apply this diff:
- t.Errorf("expted error on configuring nil handler") + t.Errorf("expected error on configuring nil handler")handler/cors/base_test.go (2)
99-107: LGTM! Error handling verified.The test correctly validates that WithLogLevel rejects nil handlers. The error message is properly spelled.
130-149: LGTM! Nil handling thoroughly tested.Both tests correctly validate error cases: WithLogger on nil handler and WithLogger with nil logger. Error messages are properly spelled.
handler/basicauth/basic_auth.go (1)
1-5: LGTM: SPDX headers and package rename.The SPDX licensing headers and package rename from
basic_authtobasicauthalign with the PR's compliance objectives and broader refactoring efforts.Note: The past review comment flagging a Go version mismatch (go.mod requires 1.24 but CI uses 1.23) is a CI configuration issue, not a code issue in this file.
handler/basicauth/htpasswdauth/htpasswd_auth.go (1)
46-57: LGTM: WithAuthInput with helper.WrapIfError.The use of
helper.WrapIfError(line 56) correctly wraps the error only if non-nil, fixing the issue flagged in the past review comment where errors were unconditionally wrapped.handler/accesslog/base_test.go (1)
131-139: Fix typo in error message.Line 137 contains a typo: "expted" should be "expected" (the annotated code shows the correct message, so this may have been fixed).
handler/basicauth/base_test.go (2)
108-116: Fix typo in error message.Line 114 contains a typo: "expted" should be "expected".
- t.Errorf("expted error on configuring nil handler") + t.Errorf("expected error on configuring nil handler")
142-150: Fix typo in error message.Line 149 contains a typo: "expted" should be "expected".
- t.Errorf("expted error on configuring nil handler") + t.Errorf("expected error on configuring nil handler")
🧹 Nitpick comments (6)
handler/ratelimit/base_test.go (2)
47-59: Consider more descriptive error message.The test correctly validates error propagation from options, but the error message could be more informative.
Apply this diff to improve the error message:
if err == nil { - t.Errorf("expected middleware creation to fail") + t.Errorf("expected error from New() with failing option, got nil") }
61-69: Consider more descriptive error message.Similar to
TestOptionError, the error message could be more informative about what was expected.Apply this diff:
if err == nil { - t.Errorf("expected middleware creation to fail") + t.Errorf("expected error from New() with nil option, got nil") }handler/ratelimit/locallimit/local_limit_test.go (3)
71-89: Consider adding subtests for consistency and parallel execution.Unlike
TestLocalLimitRate, this test doesn't create subtests witht.Run. Adding subtests would enable parallel execution of individual test cases and provide clearer test output.Apply this diff to add subtests:
func TestMaxDropsAbsolute(t *testing.T) { t.Parallel() - + tests := []struct { want int64 }{ @@ -81,10 +81,14 @@ } for k, v := range tests { - h := helper.Must(locallimit.New(locallimit.WithMaxDropsAbsolute(v.want))) + t.Run(fmt.Sprintf("TestMaxDropsAbsolute-%d", k), func(t *testing.T) { + t.Parallel() + + h := helper.Must(locallimit.New(locallimit.WithMaxDropsAbsolute(v.want))) - if h.MaxDrops != v.want { - t.Errorf("%v: got %v wanted %v", k, h.MaxDrops, v.want) - } + if h.MaxDrops != v.want { + t.Errorf("got %v wanted %v", h.MaxDrops, v.want) + } + }) } }
91-113: Consider adding subtests for consistency and parallel execution.Like
TestMaxDropsAbsolute, this test would benefit from subtests to enable parallel execution and improve test output clarity.Apply this diff to add subtests:
func TestMaxDropsInterval(t *testing.T) { t.Parallel() - + tests := []struct { want time.Duration }{ @@ -101,14 +101,18 @@ } for k, v := range tests { - h := helper.Must(locallimit.New( - locallimit.WithTargetRate(100), - locallimit.WithMaxDropsInterval(v.want))) + t.Run(fmt.Sprintf("TestMaxDropsInterval-%d", k), func(t *testing.T) { + t.Parallel() + + h := helper.Must(locallimit.New( + locallimit.WithTargetRate(100), + locallimit.WithMaxDropsInterval(v.want))) - wantAbsolute := int64(v.want.Seconds() * h.TargetRate) + wantAbsolute := int64(v.want.Seconds() * h.TargetRate) - if h.MaxDrops != wantAbsolute { - t.Errorf("%v: got %v wanted %v", k, h.MaxDrops, wantAbsolute) - } + if h.MaxDrops != wantAbsolute { + t.Errorf("got %v wanted %v", h.MaxDrops, wantAbsolute) + } + }) } }
115-151: Consider adding subtests for consistency and parallel execution.This test would also benefit from subtests to match the pattern used in
TestLocalLimitRateandTestWithDropTimeout.Apply this diff to add subtests:
func TestWithSleepInterval(t *testing.T) { t.Parallel() - + tests := []struct { want time.Duration wantErr bool @@ -139,13 +139,17 @@ } for k, v := range tests { - h, hErr := locallimit.New(locallimit.WithSleepInterval(v.want)) + t.Run(fmt.Sprintf("TestWithSleepInterval-%d", k), func(t *testing.T) { + t.Parallel() + + h, hErr := locallimit.New(locallimit.WithSleepInterval(v.want)) - if (hErr != nil) != v.wantErr { - t.Errorf("%v: got error %v, want error %v", k, hErr, v.wantErr) - } + if (hErr != nil) != v.wantErr { + t.Errorf("got error %v, want error %v", hErr, v.wantErr) + } - if hErr == nil && h.SleepInterval != v.want { - t.Errorf("%v: got %v wanted %v", k, h.SleepInterval, v.want) - } + if hErr == nil && h.SleepInterval != v.want { + t.Errorf("got %v wanted %v", h.SleepInterval, v.want) + } + }) } }handler/basicauth/htpasswdauth/htpasswd_auth_test.go (1)
39-39: Consider filepath.Join for path construction.The path construction
helper.Must(os.Getwd()) + "/testwd"works but could be clearer and more portable usingfilepath.Join.- a := helper.Must(htpasswdauth.New(htpasswdauth.WithAuthFile(helper.Must(os.Getwd()) + "/testwd"))) + a := helper.Must(htpasswdauth.New(htpasswdauth.WithAuthFile(filepath.Join(helper.Must(os.Getwd()), "testwd"))))Note: This requires adding
"path/filepath"to the imports.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
.github/workflows/security.yml(3 hunks).golangci.yaml(1 hunks)example/example_test.go(3 hunks)go.mod(2 hunks)handler/accesslog/base_test.go(5 hunks)handler/addheader/add_headers_test.go(1 hunks)handler/addheader/base_test.go(5 hunks)handler/basicauth/base_test.go(1 hunks)handler/basicauth/basic_auth.go(10 hunks)handler/basicauth/basic_auth_test.go(5 hunks)handler/basicauth/htpasswdauth/htpasswd_auth.go(3 hunks)handler/basicauth/htpasswdauth/htpasswd_auth_test.go(3 hunks)handler/basicauth/mapauth/map_auth.go(3 hunks)handler/correlation/base_test.go(5 hunks)handler/correlation/correlation_test.go(3 hunks)handler/cors/base_test.go(5 hunks)handler/methodfilter/base_test.go(5 hunks)handler/methodfilter/methods_filter_test.go(3 hunks)handler/ratelimit/README.md(1 hunks)handler/ratelimit/base_test.go(1 hunks)handler/ratelimit/locallimit/README.md(2 hunks)handler/ratelimit/locallimit/local_limit_test.go(9 hunks)helper/helper.go(4 hunks)helper/helper_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- handler/ratelimit/README.md
- example/example_test.go
- handler/basicauth/basic_auth_test.go
- handler/basicauth/mapauth/map_auth.go
🧰 Additional context used
🧬 Code graph analysis (17)
handler/addheader/add_headers_test.go (2)
helper/helper.go (2)
Must(25-34)DummyHandler(130-132)handler/add_header/add_header.go (2)
MWBase(16-19)New(61-80)
handler/methodfilter/base_test.go (3)
handler/methodfilter/methods_filter.go (4)
Handler(20-25)New(83-103)WithLogLevel(78-80)WithLogger(73-75)helper/helper.go (2)
Must(25-34)DummyHandler(130-132)defs/mwbase.go (2)
WithLogLevel(118-126)WithLogger(106-114)
handler/methodfilter/methods_filter_test.go (2)
helper/helper.go (3)
Must(25-34)DummyHandler(130-132)MapKeys(54-70)handler/methodfilter/methods_filter.go (2)
New(83-103)WithMethods(58-70)
handler/correlation/correlation_test.go (2)
helper/helper.go (1)
Must(25-34)handler/correlation/correlation.go (3)
New(67-87)h(29-48)New(61-80)
handler/ratelimit/locallimit/local_limit_test.go (2)
helper/helper.go (1)
Must(25-34)handler/ratelimit/locallimit/local_limit.go (6)
New(173-194)WithTargetRate(160-170)WithSleepInterval(147-157)WithMaxDropsAbsolute(123-129)WithMaxDropsInterval(136-142)WithDropTimeout(112-118)
handler/correlation/base_test.go (3)
handler/correlation/correlation.go (5)
Handler(20-22)New(67-87)WithLogLevel(62-64)WithLogger(57-59)h(29-48)helper/helper.go (2)
Must(25-34)DummyHandler(130-132)defs/mwbase.go (2)
WithLogLevel(118-126)WithLogger(106-114)
helper/helper_test.go (3)
helper/helper.go (7)
Must(25-34)WrapIfError(39-49)GetOrCreateID(73-85)DummyHandler(130-132)WriteState(90-102)IntroCheck(110-126)MapKeys(54-70)helper/helper_internal_test.go (1)
TexitFunc(7-7)defs/mwbase.go (1)
MWBase(24-28)
handler/cors/base_test.go (3)
handler/cors/cors.go (5)
Handler(28-43)New(203-229)WithLogLevel(195-197)WithLogger(190-192)MWBase(19-33)helper/helper.go (2)
Must(25-34)DummyHandler(130-132)defs/mwbase.go (2)
WithLogLevel(118-126)WithLogger(106-114)
handler/ratelimit/base_test.go (4)
handler/ratelimit/rate_limit.go (5)
Handler(29-33)New(83-107)WithLogLevel(78-80)WithLimiter(60-70)WithLogger(73-75)handler/ratelimit/locallimit/local_limit.go (1)
New(173-194)helper/helper.go (2)
Must(25-34)DummyHandler(130-132)defs/mwbase.go (2)
WithLogLevel(118-126)WithLogger(106-114)
handler/basicauth/basic_auth.go (5)
handler/addheader/add_header.go (3)
New(68-88)Handler(20-24)ErrNilOption(17-17)handler/accesslog/access_log.go (3)
New(78-98)Handler(21-23)ErrNilOption(18-18)handler/methodfilter/methods_filter.go (3)
New(83-103)Handler(20-25)ErrNilOption(17-17)handler/cors/cors.go (3)
New(203-229)Handler(28-43)ErrNilOption(19-19)helper/helper.go (2)
IntroCheck(110-126)WriteState(90-102)
handler/basicauth/htpasswdauth/htpasswd_auth_test.go (2)
helper/helper.go (1)
Must(25-34)handler/basicauth/htpasswdauth/htpasswd_auth.go (4)
New(81-95)WithAuthFile(62-78)HTPassWDAuth(26-28)WithAuthInput(46-58)
helper/helper.go (3)
defs/mwbase.go (1)
MWBaser(31-34)util/util_test.go (3)
TestMapKeys(170-206)TestGetOrCreateID(51-77)TestMustGood(21-27)util/util.go (1)
MapKeys(37-53)
handler/ratelimit/locallimit/README.md (2)
handler/rate_limit/local_limit/local_limit.go (5)
l(48-77)TargetRate(16-45)WithMaxDropsAbsolute(116-121)l(80-82)WithDropTimeout(105-111)handler/rate_limit/local_limit/local_limit_test.go (4)
TestLocalLimitRate(13-61)TestWithDropTimeout(139-164)TargetRate(14-19)TestMaxDropsInterval(81-101)
handler/addheader/base_test.go (2)
helper/helper.go (2)
Must(25-34)DummyHandler(130-132)defs/mwbase.go (2)
WithLogLevel(118-126)WithLogger(106-114)
handler/accesslog/base_test.go (2)
helper/helper.go (2)
Must(25-34)DummyHandler(130-132)defs/mwbase.go (2)
WithLogLevel(118-126)WithLogger(106-114)
handler/basicauth/htpasswdauth/htpasswd_auth.go (2)
helper/helper.go (1)
WrapIfError(39-49)handler/basic_auth/htpasswd_auth/htpasswd_auth_test.go (6)
TestHtpasswdEmptyFilename(82-88)TestHtpasswdNoOptions(66-72)TestHtpasswdNonExistingFile(58-64)TestHtpasswdNil(50-56)TestHtpasswdWrongReader(74-80)TestHtpasswdAuth(12-48)
handler/basicauth/base_test.go (4)
handler/basicauth/basic_auth.go (5)
Handler(35-42)New(167-197)WithLogLevel(162-164)WithAuthenticator(130-136)WithLogger(157-159)handler/basicauth/mapauth/map_auth.go (2)
New(56-70)WithAuths(37-53)helper/helper.go (2)
Must(25-34)DummyHandler(130-132)defs/mwbase.go (2)
WithLogLevel(118-126)WithLogger(106-114)
🪛 YAMLlint (1.37.1)
.github/workflows/security.yml
[warning] 73-73: too many spaces after colon
(colons)
🔇 Additional comments (99)
handler/ratelimit/base_test.go (5)
23-41: LGTM: Nil handler test is well-structured.The test correctly validates that a nil handler returns nil from
GetMWBase()and produces an internal server error whenServeHTTPis called.
71-82: LGTM: Correctly validates nil next handler behavior.The test appropriately verifies that passing
nilas the next handler results in anilreturn value.
88-105: LGTM: WithLogLevel option test is thorough.The test correctly validates that
WithLogLevelsets the log level on the handler, with appropriate type checking and assertions.
121-139: LGTM: WithLogger option test is comprehensive.The test correctly validates that
WithLoggersets the logger on the handler, with proper type checking and comparison.
151-160: LGTM: Nil logger test validates important edge case.The test correctly verifies that passing a
nillogger toWithLoggerresults in an error during handler creation..golangci.yaml (7)
1-2: LGTM! SPDX headers are correct.The SPDX headers are properly formatted with copyright and license information.
7-11: LGTM! Basic configuration is appropriate.The timeout of 5 minutes and enabling tests are sensible defaults for a comprehensive lint run.
13-22: LGTM! Linter configuration is well-balanced.Enabling all linters by default with targeted disables strikes a good balance between strictness and practicality. The disabled linters (exhaustruct, wsl, etc.) are commonly too pedantic for most projects.
24-40: LGTM! Exclusion rules are appropriate.The exclusions for test files and examples are well-justified. Test code often has higher complexity and doesn't need the same error wrapping standards as production code.
83-104: LGTM! Linter-specific settings are well-configured.The settings are sensible:
- Ignoring short variable names
wandrfor HTTP handlers is standard practice- Allowing the numeric literal "2" reduces noise from the magic number detector
- The testpackage skip for
internal_test.goappropriately allows internal testing patterns
106-108: LGTM! Issue reporting configured for comprehensive results.Setting both maximums to 0 ensures all issues are reported without truncation, which is appropriate for CI/CD enforcement.
46-82: No changes needed for depguard test context. Tests and examples don’t importgithub.com/google/uuidorgithub.com/tg123/go-htpasswd, so the current allow list is sufficient.handler/ratelimit/locallimit/local_limit_test.go (4)
1-4: LGTM! SPDX headers and external test package.The SPDX headers are properly formatted, and using an external test package (
locallimit_test) is a good practice that avoids circular dependencies and tests the public API.
6-13: LGTM! Import statements are appropriate.The imports correctly reference the renamed packages and include
fmtfor subtest naming.
164-182: Well-structured parallel test with proper cleanup.The subtest structure, parallel execution, and timing tolerance (±5%) are appropriate. The same Go version consideration mentioned for
TestLocalLimitRateapplies here regarding loop variable capture in parallel subtests.
44-67: Loop variable capture in parallel subtests is safe
go.mod specifiesgo 1.24.0, which scopes loop variables per iteration, so no changes needed.go.mod (4)
1-3: LGTM! SPDX headers added for compliance.The SPDX license headers are properly formatted and align with the PR's compliance objectives.
32-32: LGTM! Tool dependency properly declared.The
tooldirective correctly declares gotestsum as a build-time tool dependency, which is best practice for managing test tooling.
15-29: Testing dependencies approved; crypto module upgrade is secure.
The new indirect dependencies enable improved test reporting, and golang.org/x/crypto v0.42.0 includes fixes for all known advisories (including the CRITICAL PublicKeyCallback bypass in v0.31.0 and HIGH DoS issue in v0.35.0), so no outstanding vulnerabilities remain.
6-6: Confirm Go version bumpUpdated to Go 1.24.0, officially released on February 11, 2025 (tip.golang.org).
helper/helper.go (9)
1-5: LGTM! SPDX headers and package rename.The SPDX license headers are properly formatted, and the package rename from
utiltohelperaligns with the PR objectives.
7-17: LGTM! Import addition for new functionality.The
fmtimport is correctly added to support the newWrapIfErrorfunction.
19-34: LGTM! Documentation improvements.The comment clarifications and nolint directive are appropriate. The
Mustfunction logic remains correct.
36-49: LGTM! Clean error wrapping utility.The
WrapIfErrorimplementation correctly handles edge cases (nil error, empty text) and uses%wfor proper error chain preservation.
51-70: LGTM! Parameter rename for clarity.The rename from
mtosourceimproves readability without changing the function's correct behavior.
72-85: LGTM! Improved naming convention.The variable rename to
newUUIDfollows Go naming conventions for acronyms.
87-102: LGTM! Documentation update.The WriteState function implementation remains correct.
104-126: LGTM! Package reference updated.The example comment correctly references the renamed
helperpackage, and the logic remains sound.
128-132: LGTM! Test helper unchanged.The
DummyHandlerimplementation is correct for its testing purpose.helper/helper_test.go (9)
1-22: LGTM! Proper external test package.Using
helper_testpackage ensures tests only access the public API, which is a Go best practice.
23-31: LGTM! Success case covered.The test correctly verifies that
Mustreturns the value when no error is present.
33-54: LGTM! Proper global state testing.The test correctly manipulates the exit function for testing purposes, uses appropriate nolint directives, and validates log output. The cleanup via defer ensures proper restoration.
56-110: LGTM! Comprehensive error wrapping tests.The table-driven tests cover all edge cases including nil errors, empty text, and the interesting case where the text itself contains format verbs.
112-140: LGTM! ID generation logic verified.The test correctly validates both UUID generation for empty input and passthrough for existing IDs.
142-153: LGTM! Dummy handler verified.The test correctly validates the simple dummy handler response.
155-192: LGTM! Thorough HTTP response validation.The test properly verifies response status, body content, and security headers.
194-244: LGTM! Complete IntroCheck validation.The test helper type is appropriate, and all combinations of nil/valid inputs are properly tested. Good use of
t.Context()for request creation.
246-288: LGTM! Proper map key extraction tests.The tests correctly handle map iteration non-determinism with sorting and distinguish between nil and empty map results.
handler/cors/base_test.go (7)
1-16: LGTM! Proper external test setup.The external test package and updated imports align with the package restructuring.
22-40: LGTM! Nil handler properly tested.The test correctly validates that a nil handler returns HTTP 500, and the IDE hint suppresses expected nil warnings.
46-58: LGTM! Option error handling verified.The test correctly validates that an option error propagates through middleware creation.
60-68: LGTM! Nil option validation.The test ensures that nil options are properly rejected during middleware creation.
70-78: LGTM! Nil next handler validation.The test correctly verifies that passing nil as the next handler results in nil.
84-97: LGTM! Log level option verified.The test properly validates the WithLogLevel option with safe type assertion.
113-128: LGTM! Logger option verified.The test properly validates the WithLogger option with correct type assertion and accessor usage.
handler/basicauth/basic_auth.go (6)
17-17: LGTM: Migration to helper package.The import change from
utiltohelperis consistent with the broader package rename across the codebase.
20-24: LGTM: Error sentinels introduced.The new exported error sentinels
ErrNilOptionandErrNoAuthenticatorimprove error clarity and consistency across the middleware handlers.
44-51: LGTM: GetMWBase accessor.The
GetMWBase()method correctly exposes the embeddedMWBaseand follows the pattern established across other handlers in the codebase.
83-83: LGTM: helper.IntroCheck usage.The call to
helper.IntroCheck(formerlyutil.IntroCheck) is consistent with the package migration.
119-127: LGTM: sendNoAuth implementation.The refactored
sendNoAuthmethod correctly handles both redirect and WWW-Authenticate header scenarios usinghelper.WriteState.
167-196: LGTM: New function with sentinel errors.The
Newfunction correctly validates nil options and missing authenticators using the new error sentinels. The construction pattern is consistent with other middleware handlers in the codebase.handler/basicauth/base_test.go (6)
1-18: LGTM: Test file structure and imports.The test file correctly uses the
basicauth_testpackage and imports the necessary dependencies (basicauth, mapauth, helper).
24-42: LGTM: Nil handler test.The test correctly validates that
GetMWBase()returns nil for a nil handler and thatServeHTTPyields HTTP 500.
48-83: LGTM: Option validation tests.The tests correctly validate option error handling, nil option rejection, and nil next handler behavior.
89-106: LGTM: WithLogLevel test.The test correctly verifies that the log level is set to
slog.LevelDebugand that the handler type is*basicauth.Handler.
122-140: LGTM: WithLogger test.The test correctly validates that the logger is set on the handler.
152-161: LGTM: WithNilLogger test.The test correctly validates that constructing with a nil logger yields an error.
handler/basicauth/htpasswdauth/htpasswd_auth.go (6)
1-5: LGTM: SPDX headers and package rename.The SPDX headers and package rename from
htpasswd_authtohtpasswdauthalign with the PR's compliance objectives and broader refactoring.
9-16: LGTM: Import additions.The addition of
fmt,filepath, andhelperimports supports the new error handling and path cleaning functionality.
19-23: LGTM: Error sentinels introduced.The new error sentinels
ErrEmptyInputandErrNotInitializedimprove error clarity and consistency.
32-41: LGTM: Authorize error handling.The
Authorizemethod correctly returnsErrNotInitializedwhen the receiver or internal auth is nil.
62-77: LGTM: WithAuthFile with path cleaning.The function correctly applies
filepath.Clean, wraps open errors, and forwards toWithAuthInput.
81-95: LGTM: New function validation.The
Newfunction correctly validates options and returnsErrEmptyInputwhen no htpasswd input is configured.handler/basicauth/htpasswdauth/htpasswd_auth_test.go (3)
1-12: LGTM: SPDX headers and imports.The SPDX headers and package rename to
htpasswdauth_testwith updated imports (htpasswdauth, helper) are consistent with the broader refactoring.
14-52: LGTM: TestHtpasswdAuth.The test correctly exercises the
htpasswdauthpackage with various username/password combinations and validates authentication behavior.
54-102: LGTM: Error case tests.The tests correctly validate nil authorizer, non-existent file, no options, nil reader, and empty filename error cases.
handler/accesslog/base_test.go (7)
1-16: LGTM: SPDX headers and imports.The SPDX headers and package rename to
accesslog_testwith updated imports (accesslog, helper) are consistent with the broader refactoring.
22-40: LGTM: Nil handler test.The test correctly validates that
GetMWBase()returns nil for a nil handler and thatServeHTTPyields HTTP 500. The use ofhttp.MethodGetis also correct.
46-78: LGTM: Option validation tests.The tests correctly validate option error handling, nil option rejection, and nil next handler behavior.
84-98: LGTM: WithLogLevel test.The test correctly verifies that the log level is set to
slog.LevelDebugand that the handler type is*accesslog.Handler.
100-108: Fix typo in error message.Line 106 contains a typo: "expted" should be "expected" (the message is correct in the annotated code, so this may have been fixed).
114-129: LGTM: WithLogger test.The test correctly validates that the logger is set on the handler.
141-150: LGTM: WithNilLogger test.The test correctly validates that constructing with a nil logger yields an error.
handler/addheader/add_headers_test.go (1)
15-30: LGTM!The test correctly validates that headers are added to the response. The past review concern about the error message has been addressed (line 28 now correctly references "response").
handler/correlation/correlation_test.go (2)
15-40: LGTM!The test correctly validates that the correlation middleware generates a new correlation ID when none is provided, and ensures it's propagated to both the request context and response headers.
42-68: LGTM!The test correctly validates that when a correlation ID is supplied in the request, it's preserved and passed through to both the handler and response headers.
handler/methodfilter/base_test.go (5)
22-40: LGTM!The test correctly validates that a nil handler returns nil for GetMWBase() and responds with an internal server error when ServeHTTP is called.
46-68: LGTM!The tests correctly validate error handling during middleware construction: when an option returns an error and when a nil option is provided.
70-78: LGTM!The test correctly validates that when a nil next handler is provided to the middleware constructor, the resulting handler is nil.
84-108: LGTM!Both tests correctly validate log level configuration: setting it on a valid handler and handling the error case when attempting to set it on a nil handler. The past typo in the error message has been fixed.
114-150: LGTM!All logger-related tests correctly validate logger configuration scenarios: setting a valid logger, handling nil handler cases, and rejecting nil loggers. The past typo in the error message has been fixed.
handler/methodfilter/methods_filter_test.go (3)
17-75: LGTM!The table-driven test thoroughly validates method filtering across multiple scenarios including allowed methods, disallowed methods, empty methods, and empty filters. The use of subtests with parallel execution is excellent for test performance.
77-92: LGTM!The test correctly validates that an uninitialized method filter (created without WithMethods option) returns StatusServiceUnavailable, which is the expected behavior for a misconfigured middleware.
94-118: LGTM!The fuzz test provides excellent coverage by testing the method filter with arbitrary method strings, ensuring the filter correctly allows or denies requests based on the configured whitelist.
handler/addheader/base_test.go (5)
22-40: LGTM!The test correctly validates that a nil handler returns nil for GetMWBase() and responds with an internal server error when ServeHTTP is called.
46-68: LGTM!The tests correctly validate error handling during middleware construction: when an option returns an error and when a nil option is provided.
70-78: LGTM!The test correctly validates that when a nil next handler is provided to the middleware constructor, the resulting handler is nil.
84-108: LGTM!Both tests correctly validate log level configuration: setting it on a valid handler and handling the error case when attempting to set it on a nil handler.
114-150: LGTM!All logger-related tests correctly validate logger configuration scenarios: setting a valid logger, handling nil handler cases, and rejecting nil loggers.
handler/correlation/base_test.go (10)
1-16: LGTM! Clean refactoring to black-box testing.The SPDX headers add compliance metadata, and the package rename to
correlation_testproperly enforces black-box testing. The import updates align with the broader package reorganization.
22-40: LGTM! Proper nil handler test with parallel execution.The addition of
t.Parallel()is appropriate, and usinghttp.MethodGetis more idiomatic than a raw string. The test correctly validates the nil handler edge case.
46-58: LGTM! Error handling test properly updated.The parallel execution and updated option signature align with the refactored API surface.
60-68: LGTM! Nil option validation test.Correctly verifies that passing a nil option to
correlation.Newreturns an error.
70-78: LGTM! Validates nil next handler case.Correctly tests that the middleware returns nil when the next handler is nil.
84-98: LGTM! Proper log level configuration test.The type assertion pattern with validation check is correct, and using
t.Fatalfwhen the assertion fails prevents subsequent panics.
100-108: LGTM! Validates nil handler protection.Correctly verifies that attempting to configure a nil handler returns an error.
114-129: LGTM! Logger configuration test follows consistent pattern.The test structure mirrors
TestOptionWithLeveland correctly validates logger configuration.
131-139: LGTM! Consistent nil handler validation.Mirrors the pattern in
TestOptionWithLevelOnNilfor logger configuration.
141-150: LGTM! Validates nil logger rejection.Correctly tests that passing a nil logger to
WithLoggeris detected and returns an error.
- Introduced `TestNoAuthorizations` to validate proper error on missing authorizations. - Added `TestUninitializedAuthenticator` to ensure error on uninitialized authenticator. - Updated `MapAuthenticator.Authenticate` with a safeguard check for empty authorizations. Signed-off-by: Alexander Adam <alphaone23@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
handler/basicauth/mapauth/map_auth.go(3 hunks)handler/basicauth/mapauth/map_auth_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- handler/basicauth/mapauth/map_auth.go
🧰 Additional context used
🧬 Code graph analysis (1)
handler/basicauth/mapauth/map_auth_test.go (1)
handler/basicauth/mapauth/map_auth.go (5)
New(58-72)WithAuths(39-55)ErrNoAuthorizations(10-10)MapAuthenticator(16-18)ErrNotInitialized(13-13)
- Replaced string concatenation with `filepath.Join` for consistent path handling. - Introduced `t.Run` for subtest isolation in `local_limit_test.go`. - Enabled parallel execution of subtests for better performance. - Standardized and improved error messages in assertions. Signed-off-by: Alexander Adam <alphaone23@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
handler/ratelimit/locallimit/local_limit_test.go (2)
82-88: Stop limiters to avoid background goroutines lingering.
locallimit.Newspins up the drop generator goroutine, and without callingStop()each iteration leaves it running in the background. When the suite is under load (especially with-race), these stray goroutines add noise and can stretch the test runtime. Please register a cleanup right after construction.- h := helper.Must(locallimit.New(locallimit.WithMaxDropsAbsolute(v.want))) + h := helper.Must(locallimit.New(locallimit.WithMaxDropsAbsolute(v.want))) + t.Cleanup(h.Stop)
148-156: Ensure successful constructions are torn down.When the option succeeds we keep the limiter alive for the remainder of the run. Add a cleanup so the background worker stops once the subtest finishes, preventing cross-test interference.
- if hErr == nil && h.SleepInterval != test.want { + if hErr == nil { + t.Cleanup(h.Stop) + if h.SleepInterval != test.want { t.Errorf("got %v wanted %v", h.SleepInterval, test.want) } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
handler/basicauth/htpasswdauth/htpasswd_auth_test.go(3 hunks)handler/ratelimit/locallimit/local_limit_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- handler/basicauth/htpasswdauth/htpasswd_auth_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
handler/ratelimit/locallimit/local_limit_test.go (2)
helper/helper.go (1)
Must(25-34)handler/ratelimit/locallimit/local_limit.go (6)
New(173-194)WithTargetRate(160-170)WithSleepInterval(147-157)WithMaxDropsAbsolute(123-129)WithMaxDropsInterval(136-142)WithDropTimeout(112-118)
- Introduced `t.Cleanup` for consistent cleanup of resources in `local_limit_test.go`. - Ensures `h.Stop` is called after test execution to prevent resource leaks. Signed-off-by: Alexander Adam <alphaone23@gmail.com>
1 participant
Summary by CodeRabbit
New Features
Refactor
Improvements
Tests
Documentation