utils.query: surface unhandled 4xx/5xx and stop mutating caller's payload

[thodson-usgs]

Summary

Two related correctness fixes to the shared HTTP wrapper used by every module in the package.

• Unhandled 4xx/5xx silently passed through. query only formatted explicit messages for 400, 404, 414, and 500/502/503 — every other status (401, 403, 405, 408, 429, 501, 504, …) fell through and the response was returned as if it had succeeded. Callers then parsed an HTML error page as RDB or CSV and reported confusing downstream errors. Adds a response.raise_for_status()-style fallback after the explicit branches so every non-success surfaces, while keeping the friendlier messages for the codes we already format.

• query mutated the caller's payload dict. The body did for key, value in payload.items(): payload[key] = to_str(...) in place, so list values came back replaced with their stringified joins after the call returned. Build a fresh params dict in a comprehension and leave the input alone.

Minimal reproducible examples

Bug 1: pre-fix payload mutation

# Pre-fix utils.query body (excerpt):
def to_str(listlike, delim=","):
    if isinstance(listlike, list):
        return delim.join(str(x) for x in listlike)
    return str(listlike)

def buggy_normalize(payload):
    for key, value in payload.items():
        payload[key] = to_str(value)

payload = {"sites": "12345", "parameterCd": ["00060", "00010"]}
buggy_normalize(payload)
print(payload)
# {'sites': '12345', 'parameterCd': '00060,00010'}
#                                   ^ list silently replaced with comma-joined string

After this PR, the caller's dict is left untouched; only the request-time params dict carries the joined string.

Bug 2: pre-fix 4xx/5xx pass-through

import io
import pandas as pd
import requests

# Pre-fix `query` only handled 400, 404, 414, 500/502/503 explicitly.
# httpbin returns a real 405 Method Not Allowed that the pre-fix code
# would have returned as if successful:
r = requests.get("https://httpbin.org/status/405")
print(r.status_code, r.ok)
# 405 False

# A caller (e.g. nwis._read_rdb / wqp.get_results) would then do:
df = pd.read_csv(io.StringIO(r.text or "x"))
print(df)
# Empty DataFrame  <-- silently empty result for the user, no clue why

After this PR, any non-success status raises ValueError("HTTP 405 Method Not Allowed for ...") so the failure surfaces immediately.

Test plan

• New parametrized test covers seven previously-silent statuses (401, 403, 405, 408, 429, 501, 504) — each now raises.
• New test verifies the caller's payload dict is not mutated and list values still point at the same object.
• Existing test_url_too_long and test_header continue to pass.
• Full local test suite passes (227 tests, with API_USGS_PAT set).

[Copilot]

Pull request overview

Improves the shared dataretrieval.utils.query() HTTP wrapper to (1) avoid mutating the caller-provided query payload and (2) surface previously-silent non-success HTTP statuses so downstream parsers don’t attempt to parse HTML error bodies as data.

Changes:

• Build a new params dict for requests.get(..., params=...) instead of mutating the input payload.
• Add a response.raise_for_status() catch-all after existing friendly status-code branches.
• Add regression tests for payload immutability and for raising on additional 4xx/5xx statuses.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
dataretrieval/utils.py	Stops mutating the caller’s payload and ensures unhandled 4xx/5xx statuses raise instead of returning an error body as “data”.
tests/utils_test.py	Adds regression tests for payload immutability and for raising on additional previously-unhandled HTTP error statuses.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.