Skip to content

Commit 5cfe77d

Browse files
committed
refactor: use params for all other URL constructions
Motivation: Consistently use the 'params' argument for URL query parameters across the codebase to avoid manual string manipulation and ensure proper encoding. Design Choices: - Refactored `get_json` and `get_json_list` in `openqabot/loader/gitea.py` to support `params` and updated callers. - Updated `get_aggregate_settings_data` in `openqabot/loader/qem.py` to pass parameters via `params`. - Refactored `get_older_jobs` in `openqabot/openqa.py` to use positional arguments for params as expected by `openqa_client`. - Updated relevant test mocks in `tests/test_giteasync.py` and `tests/test_loader_qem.py`. Benefits: - Code consistency and readability. - Robust handling of query parameters across different modules. Related issue: #480
1 parent f2d9742 commit 5cfe77d

File tree

5 files changed

+32
-13
lines changed

5 files changed

+32
-13
lines changed

openqabot/loader/gitea.py

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -66,17 +66,28 @@ def make_token_header(token: str) -> dict[str, str]:
6666
return {} if token is None else {"Authorization": "token " + token}
6767

6868

69-
def get_json(query: str, token: dict[str, str], host: str | None = None) -> JsonType:
69+
def get_json(
70+
query: str,
71+
token: dict[str, str],
72+
host: str | None = None,
73+
params: dict[str, Any] | None = None,
74+
) -> JsonType:
7075
"""Fetch JSON data from Gitea API."""
7176
host = host or config.settings.gitea_url
72-
response = retried_requests.get(host + "/api/v1/" + query, verify=not config.settings.insecure, headers=token)
77+
url = f"{host}/api/v1/{query}"
78+
response = retried_requests.get(url, verify=not config.settings.insecure, headers=token, params=params)
7379
response.raise_for_status()
7480
return response.json()
7581

7682

77-
def get_json_list(query: str, token: dict[str, str], host: str | None = None) -> list[Any]:
83+
def get_json_list(
84+
query: str,
85+
token: dict[str, str],
86+
host: str | None = None,
87+
params: dict[str, Any] | None = None,
88+
) -> list[Any]:
7889
"""Fetch a list of JSON data from Gitea API."""
79-
res = get_json(query, token, host)
90+
res = get_json(query, token, host, params=params)
8091
if not isinstance(res, list):
8192
msg = f"Gitea API returned {type(res).__name__} instead of list for query: {query}"
8293
raise TypeError(msg)
@@ -199,7 +210,7 @@ def iter_pr_pages() -> Iterator[list[dict[str, Any]]]:
199210
page = 1
200211
while True:
201212
# https://docs.gitea.com/api/1.20/#tag/repository/operation/repolistPullRequests
202-
prs_on_page = get_json(f"repos/{repo}/pulls?state=open&page={page}", token)
213+
prs_on_page = get_json(f"repos/{repo}/pulls", token, params={"state": "open", "page": page})
203214
if not isinstance(prs_on_page, list):
204215
msg = f"Gitea API returned {type(prs_on_page).__name__} instead of list for PR pages"
205216
raise TypeError(msg)
@@ -330,10 +341,10 @@ def _extract_version(name: str, prefix: str) -> str:
330341
def get_product_version_from_repo_listing(project: str, product_name: str, repository: str) -> str:
331342
"""Determine the product version by inspecting an OBS repository listing."""
332343
project_path = project.replace(":", ":/")
333-
url = f"{config.settings.obs_download_url}/{project_path}/{repository}/repo?jsontable"
344+
url = f"{config.settings.obs_download_url}/{project_path}/{repository}/repo"
334345
start = f"{product_name}-"
335346
try:
336-
r = retried_requests.get(url)
347+
r = retried_requests.get(url, params={"jsontable": 1})
337348
r.raise_for_status()
338349
data = r.json()["data"]
339350
except requests.exceptions.HTTPError as e:

openqabot/loader/qem.py

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -226,8 +226,11 @@ def get_aggregate_settings(sub: int, submission_type: str | None = None) -> list
226226

227227
def get_aggregate_settings_data(data: Data) -> Sequence[Data]:
228228
"""Fetch aggregate job settings data for a product and architecture."""
229-
url = "api/update_settings" + f"?product={data.product}&arch={data.arch}"
230-
settings = dashboard.get_json(url, headers=config_module.settings.dashboard_token_dict)
229+
settings = dashboard.get_json(
230+
"api/update_settings",
231+
headers=config_module.settings.dashboard_token_dict,
232+
params={"product": data.product, "arch": data.arch},
233+
)
231234
if not settings:
232235
log.info("No aggregate settings found for product %s on arch %s", data.product, data.arch)
233236
return []

openqabot/openqa.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,10 @@ def get_older_jobs(self, job_id: int, limit: int) -> dict:
141141
"""Fetch older jobs for a specific job."""
142142
try:
143143
return self.openqa.openqa_request(
144-
"GET", f"/tests/{job_id}/ajax?previous_limit={limit}&next_limit=0", retries=self.retries
144+
"GET",
145+
f"/tests/{job_id}/ajax",
146+
{"previous_limit": limit, "next_limit": 0},
147+
retries=self.retries,
145148
)
146149
except RequestError:
147150
log.exception("openQA API error when fetching older jobs for job %s", job_id)

tests/test_giteasync.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,9 +77,9 @@ def reply_callback(request: Any) -> tuple[int, dict[str, str], Any]:
7777

7878
@pytest.fixture
7979
def fake_repo() -> None:
80-
url = f"{settings.obs_download_url}/SUSE:/SLFO:/1.1.99:/PullRequest:/124:/SLES/standard/repo?jsontable"
80+
url = f"{settings.obs_download_url}/SUSE:/SLFO:/1.1.99:/PullRequest:/124:/SLES/standard/repo"
8181
listing = Path("tests/fixtures/responses/test-product-repo.json").read_bytes()
82-
responses.add(GET, url, body=listing)
82+
responses.add(GET, url, body=listing, match=[matchers.query_param_matcher({"jsontable": "1"})])
8383

8484

8585
def fake_osc_http_get(url: str) -> etree.ElementTree:

tests/test_loader_qem.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -301,7 +301,9 @@ def test_get_aggregate_settings_data(mock_get_json: MagicMock) -> None:
301301
assert len(res) == 1
302302
assert res[0].settings_id == 1
303303
mock_get_json.assert_called_once_with(
304-
"api/update_settings?product=product&arch=arch", headers=settings.dashboard_token_dict
304+
"api/update_settings",
305+
headers=settings.dashboard_token_dict,
306+
params={"product": "product", "arch": "arch"},
305307
)
306308

307309

0 commit comments

Comments
 (0)