refactor: extract readRequestBody and parseBoolQueryParam helpers

[ericcurtin]

Deduplicate the MaxBytesReader+ReadAll error-handling block (repeated 4x in scheduling/http_handler.go) into a readRequestBody helper, and the bool query-param parsing block (repeated 2x in models/http_handler.go) into a parseBoolQueryParam helper.

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• In Configure, the change from backend, err = h.scheduler.ConfigureRunner(...) to backend, err := ... introduces variable shadowing, so the outer backend value will not be updated after the call; this should be reverted to an assignment (=) to preserve the original behavior.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `Configure`, the change from `backend, err = h.scheduler.ConfigureRunner(...)` to `backend, err := ...` introduces variable shadowing, so the outer `backend` value will not be updated after the call; this should be reverted to an assignment (`=`) to preserve the original behavior.

## Individual Comments

### Comment 1
<location path="pkg/inference/scheduling/http_handler.go" line_range="429" />
<code_context>
 	}

-	backend, err = h.scheduler.ConfigureRunner(r.Context(), backend, configureRequest, r.UserAgent())
+	backend, err := h.scheduler.ConfigureRunner(r.Context(), backend, configureRequest, r.UserAgent())
 	if err != nil {
 		if errors.Is(err, errRunnerAlreadyActive) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid shadowing the outer `backend`/`err` here to prevent subtle bugs

This line used to reassign the existing `backend`/`err`; switching to `:=` creates new inner-scope variables and leaves the outer ones unchanged. Any code after this block will still see the old values, which changes the previous behavior and can introduce hard-to-spot bugs. Unless a new inner scope is intended, please use `backend, err = ...` to match the original semantics.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

issue (bug_risk): Avoid shadowing the outer backend/err here to prevent subtle bugs

This line used to reassign the existing backend/err; switching to := creates new inner-scope variables and leaves the outer ones unchanged. Any code after this block will still see the old values, which changes the previous behavior and can introduce hard-to-spot bugs. Unless a new inner scope is intended, please use backend, err = ... to match the original semantics.

[gemini-code-assist]

Code Review

This pull request effectively refactors duplicated code into two new helper functions, readRequestBody and parseBoolQueryParam, improving code maintainability. The refactoring is well-executed. However, I've identified a potential logic issue in the new parseBoolQueryParam function (which was also present in the original code) regarding the handling of boolean query parameters without explicit values. I've suggested a fix to make the API behavior more intuitive.

[gemini-code-assist]

The current implementation of parseBoolQueryParam treats a query parameter present without a value (e.g., ?force) as false because r.URL.Query().Get("force") returns an empty string, and strconv.ParseBool("") results in an error. This is likely not the expected behavior for users of the API, who would probably expect the presence of the flag to mean true.

I suggest modifying the logic to treat the presence of the key with an empty value as true. This would make the API more intuitive and align with common web API conventions. The updated implementation also avoids calling r.URL.Query() multiple times and improves the warning log message to include the unparseable value.

func parseBoolQueryParam(r *http.Request, log logging.Logger, name string) bool {
	q := r.URL.Query()
	if !q.Has(name) {
		return false
	}
	valStr := q.Get(name)
	// Treat presence of key with empty value as true (e.g. `?force`)
	if valStr == "" {
		return true
	}
	val, err := strconv.ParseBool(valStr)
	if err != nil {
		log.Warn("error while parsing query parameter", "param", name, "value", valStr, "error", err)
		return false
	}
	return val
}