chore: simplify, modernize (Go 1.26), update deps#256
Conversation
- attributes: guard Get/Set type-assertion against non-attrs stored types (R-High) - handler/request: strings.NewReplacer for CRLF stripping; drop redundant ContainsRune pre-check (M) - handler/uploads: errors.Is(fs.ErrNotExist) instead of os.IsNotExist (M) - middleware/log: remove dead wrapper.data field; if/else over bool switch; reuse start time instead of two time.Now() calls; NewReplacer for CRLF (S+M) - middleware/maxRequest: assign r.Body directly instead of r.Clone (S) - plugin: Go 1.22 range-over-value for server loop; drop redundant r.Body.Close; call pool.Destroy via interface (S+M+R-Low) - servers/https/config: errors.Is(fs.ErrNotExist) in three stat checks (M) - tlsconf: remove dead TLS 1.3 cipher IDs from CipherSuites (silently ignored by Go) (R-Med+S) - acme: drop explicit zero-value struct fields (M)
|
Warning Review limit reached
More reviews will be available in 12 minutes and 30 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
Refactor pass aligning the HTTP plugin with Go 1.26 idioms and dependency updates: simplifies redundant code, modernizes stdlib usage, hardens a couple of unchecked type assertions, and drops cipher-suite entries that the TLS stack ignores.
Changes:
- Simplification: remove dead fields, redundant clones/closes, redundant type switches; collapse
switch boolintoif; consolidate CRLF stripping viastrings.NewReplacer. - Modernization: prefer
errors.Is(err, fs.ErrNotExist), range-over-value for goroutine loop, reusestartinstead of re-callingtime.Now(), drop unneededContainsRune(':')pre-check. - Correctness: guard
attrstype assertions inattributes.Get/Set; remove TLS 1.3 cipher IDs fromCipherSuites(silently ignored by Go) and the incorrectcap=22.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| acme/acme.go | Drop explicit zero-value struct fields in certmagic configs. |
| attributes/attributes.go | Guard v.(attrs) assertions in Get/Set to avoid panics. |
| handler/request.go | Use shared crlfReplacer; drop redundant ContainsRune(':') check. |
| handler/uploads.go | Switch os.IsNotExist to errors.Is(err, fs.ErrNotExist). |
| middleware/log.go | Remove dead data field, replace switch bool with if, factor CRLF replacer, reuse start. |
| middleware/maxRequest.go | Assign to r.Body directly instead of cloning the request. |
| plugin.go | Drop redundant body close, range-over-value for server start, call Destroy via api.Pool interface. |
| servers/https/config.go | Alias roadrunner-server/errors as rrerrors; use errors.Is/fs.ErrNotExist. |
| tlsconf/default_tlsconf.go | Remove TLS 1.3 cipher entries and the incorrect cap=22 allocation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replace the two package-level strings.Replacer vars (+nolint:gochecknoglobals) with inline strings.ReplaceAll (request.go) and a small stripCRLF helper (middleware/log.go). No global state.
…error NewPool returns a concrete *static_pool.Pool, so assigning its result directly to the api.Pool interface field on the error path boxed a typed nil into p.pool. The simplified Stop guard (`if p.pool != nil`) then sees a non-nil interface and calls Destroy on the nil receiver, which panics (Destroy dereferences sp.log immediately). Assign to p.pool only when the returned pointer is non-nil so the interface never holds a typed nil and the existing guard stays correct. Also distinguish the wrong-type case in attributes.Set from the missing-key case with its own error message for easier diagnosis.
Applied fixes
S (simplify) — 5 applied:
middleware/log: remove deadwrapper.datafield;if/elseinstead ofswitch bool{case false/true}; CRLF stripping viastrings.NewReplacermiddleware/maxRequest: assignr.Bodydirectly instead ofr.Clone+ swapplugin: drop redundant_ = r.Body.Close()(server closes it); drop redundant type-switch to reachDestroy(interface already exposes it)M (modern-Go) — 6 applied:
handler/request: twoReplaceAll→strings.NewReplacer; drop redundantContainsRune(':')pre-check beforenet.SplitHostPorthandler/uploads:os.IsNotExist→errors.Is(err, fs.ErrNotExist)middleware/log: reusestartinstead of twotime.Now()calls in access-log pathplugin: Go 1.22 range-over-value for server-start loop (closes loop-variable capture)servers/https/config:os.IsNotExist→errors.Is(err, fs.ErrNotExist)in three stat checksacme: drop explicit zero-value struct fieldsR (review/bugs) — 3 applied:
attributes: guardGet/Setunchecked type-assertionv.(attrs)— panics if another type is stored under the same context key (R-High)tlsconf: remove TLS 1.3 cipher IDs fromCipherSuites— Go silently ignores them; also fixes wrong cap22(max 6) (R-Med)plugin: callpool.Destroythrough theapi.Poolinterface instead of unnecessary type-assert to*static_pool.Pool(R-Low)Not applied (needs manual review)
Stopmutex race (plugin.go:179): goroutine readspool/queue/serverswhile lock is released — requires rethinking the shutdown contractmiddleware/log1xxWriteHeaderlogic (log.go:39):wc=trueon 1xx blocks later real WriteHeader — logic change with tricky semanticsmiddleware/redirectchain (redirect.go:13):Redirectignoresnextand HSTS on plain-HTTP — redirect-chain rewriteapplyMiddleware/TLSAddrconsolidation (S-Med): cross-server dedup refactorsinit.go:59duplicate type-switch arms (S-Med): two identical arms inapplyBundledMiddlewareDeps
go get -u all && go mod tidyin root andtests/;go work sync. No version changes produced (already up to date).Verification