refactor(core): hoist redirect following out of getInputStream - #6
Merged
Merged
Conversation
Clears three maintainability findings, but the tests come first: follow's redirect-failure exits were 1-of-4 covered, with only "unsafe redirect target" asserted. Refactoring uncovered branches is how they change behaviour quietly. Adds the three missing cases — redirect budget exhausted, missing or blank Location, malformed Location — each asserting the message and that the response body was closed. They pass before the refactor and after. The two cognitive-complexity findings on RuntimeHttp (22 at getInputStream, 17 at follow) were the same code counted twice: follow was a nested @tailrec def, so its branching aggregated into the enclosing method on top of a nesting surcharge. Hoisting it to object level clears both. Extracting helpers while leaving it nested would have left the outer method flickering around the threshold. redirectTarget and provenanceFor come out as siblings. redirectTarget deliberately leaves the response open on every path, success and failure alike — the body belongs to the caller, which releases it through failClosing, and closing it here would double-close. provenanceFor's two arms draw on different sources on purpose: a chain this loop followed is recorded in `redirects`, while a response redirected by the transport carries its history only on the response. The redirect target stays a match rather than a for-comprehension: moving the recursive call inside a flatMap lambda takes it out of tail position and fails compilation. getInputStream's public signature is unchanged, so its three callers are untouched. Separately, StatefulApplyRunner.runWithState drops its fileSystem parameter, which duplicated installer.fileSystem, taking it from 8 parameters to 7. This is redundant state, not a latent bug — the object is private[core], the method is private, and the single caller passed installer.fileSystem literally, so the divergence was not constructible. scalafmt, compile under -Werror, and the full suite pass: 514 tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AaPvsFt1xYoXErXaZ4Kk8A
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 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 |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Clears three maintainability findings —
scala:S3776×2 onRuntimeHttp.scalaandscala:S107onStatefulApplyRunner.scala. These are the only open Sonar findings in production Scala; the other 79 are test code.Tests first, because the branches being refactored were uncovered
follow's redirect-failure exits were 1-of-4 covered — only"unsafe redirect target"was asserted. Refactoring uncovered branches is how they change behaviour quietly, so this PR adds the three missing cases before touching production code:LocationLocationEach asserts the error message and that the response body was closed. All three pass before the refactor and after.
The two S3776 findings were the same code counted twice
Sonar reports complexity 22 at
getInputStream:38and 17 atfollow:46.followwas a nested@tailrec def, so its branching aggregated into the enclosing method on top of a nesting surcharge — one method scored twice, once through its child.Hoisting
followto object level clears both. Extracting helpers while leaving it nested would have left the outer method around 14–15 and flickering across the threshold on the next edit.redirectTargetandprovenanceForcome out as siblings.Three things that are load-bearing, not incidental
match, not a for-comprehension. Moving the recursive call inside aflatMaplambda takes it out of tail position and@tailrecfails compilation.redirectTargetleaves the response open on every path, success and failure alike. The body belongs to the caller, which releases it throughfailClosing; closing it here would double-close.provenanceFor's two arms draw on different sources on purpose. A chain this loop followed itself is recorded inredirects; a response that never redirected here may still have been redirected by the transport, and only the response carries that history. They are not collapsible.getInputStream's public signature is unchanged, so its three callers are untouched.StatefulApplyRunner
runWithStatedrops itsfileSystemparameter, which duplicatedinstaller.fileSystem, taking it from 8 parameters to 7. The single caller inResolvingBinaryInstallerServiceis updated.This is redundant state, not a latent bug. The object is
private[core], the method isprivate, and the caller passedinstaller.fileSystemliterally — the divergence was not constructible. Removing it for tidiness, not for safety.Verification
scalafmt/checkFormatAll,__.compileunder-Werror -Wunused:all -Wvalue-discard -Wnonunit-statement, and__.testall pass — 514 tests, 0 failures. The newHttpClientTestcases were also run against the pre-refactor code to confirm they pass on both sides of the change.🤖 Generated with Claude Code
https://claude.ai/code/session_01AaPvsFt1xYoXErXaZ4Kk8A