Skip to content

Commit f9b3915

Browse files
committed
refactor: strengthen configuration validation at startup
Motivation: Validating configuration at runtime is brittle and can lead to silent fail-open failures. Moving validation to the configuration layer ensures that the application starts with a known-good state. Design Choices: - Added a field_validator to Settings for obs_download_url to ensure it contains a valid hostname. - Introduced a repo_mirror_host property in Settings for centralized and safe hostname extraction. - Refactored RepoConfig and get_repo_url to use the pre-validated repo_mirror_host. - Removed redundant runtime checks and suppressed exceptions in verify_repo_exists. - Added comprehensive tests for the new validator and property in tests/test_config.py. User Benefits: - Faster failure on malformed configuration. - More robust and predictable repository verification logic.
1 parent 4a3bedf commit f9b3915

File tree

7 files changed

+47
-8
lines changed

7 files changed

+47
-8
lines changed

openqabot/config.py

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,10 @@
1010

1111
from pathlib import Path
1212
from typing import Any
13+
from urllib.parse import urlparse
1314

1415
import osc.conf
15-
from pydantic import Field
16+
from pydantic import Field, field_validator
1617
from pydantic_settings import BaseSettings, SettingsConfigDict
1718

1819

@@ -73,6 +74,15 @@ class Settings(BaseSettings):
7374
# How long to wait for http(s) call in seconds
7475
url_timeout: int = 60
7576

77+
@field_validator("obs_download_url")
78+
@classmethod
79+
def validate_obs_download_url(cls, v: str) -> str:
80+
"""Validate that obs_download_url has a valid hostname."""
81+
if not urlparse(v).netloc:
82+
msg = f"OBS_DOWNLOAD_URL '{v}' does not contain a valid hostname"
83+
raise ValueError(msg)
84+
return v
85+
7686
model_config = SettingsConfigDict(
7787
env_file=".env",
7888
env_file_encoding="utf-8",
@@ -110,6 +120,11 @@ def git_review_bot_user(self) -> str | None:
110120
return self.git_review_bot
111121
return self.obs_group + "-review"
112122

123+
@property
124+
def repo_mirror_host(self) -> str:
125+
"""Extract the repository mirror host from obs_download_url."""
126+
return urlparse(self.obs_download_url).netloc
127+
113128
@property
114129
def obs_products_set(self) -> set[str]:
115130
"""Return the set of OBS products to consider."""

openqabot/loader/gitea.py

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -330,8 +330,6 @@ def verify_repo_exists(
330330
if not product_version:
331331
return True
332332
repo_url = get_repo_url(target, product_version, config)
333-
if not repo_url:
334-
return True
335333
with suppress(requests.exceptions.RequestException):
336334
response = retried_requests.head(repo_url, allow_redirects=True)
337335
if response.status_code == HTTPStatus.NOT_FOUND:
@@ -436,6 +434,7 @@ def add_build_result(
436434
repo_type=config.settings.obs_repo_type or "product",
437435
download_base_url=config.settings.download_base_url,
438436
obs_download_url=config.settings.obs_download_url,
437+
repo_mirror_host=config.settings.repo_mirror_host,
439438
obs_products=config.settings.obs_products_set,
440439
)
441440
target = Repos(product=project, version=product, arch=res.get("arch"))

openqabot/types/gitea.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,4 +14,5 @@ class RepoConfig:
1414
repo_type: str
1515
download_base_url: str
1616
obs_download_url: str
17+
repo_mirror_host: str
1718
obs_products: set[str] | None = None

openqabot/utils.py

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@
99
import re
1010
from copy import deepcopy
1111
from typing import TYPE_CHECKING, Any
12-
from urllib.parse import urlparse
1312

1413
from requests import Session
1514
from requests.adapters import HTTPAdapter
@@ -116,10 +115,7 @@ def get_repo_url(
116115
config: RepoConfig,
117116
) -> str:
118117
"""Construct the repository URL for a given project and architecture."""
119-
host = urlparse(config.obs_download_url).netloc
120-
if not host:
121-
return ""
122-
base = config.download_base_url.replace("%REPO_MIRROR_HOST%", host)
118+
base = config.download_base_url.replace("%REPO_MIRROR_HOST%", config.repo_mirror_host)
123119
project_path = target.product.replace(":", ":/")
124120
return (
125121
f"{base}/{project_path}/{config.repo_type}/repo/{target.version}-{product_version}-{target.arch}/{target.arch}/"

tests/test_config.py

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,9 @@
66
from pathlib import Path
77
from unittest.mock import patch
88

9+
import pytest
10+
from pydantic import ValidationError
11+
912
from openqabot.config import Settings, get_default_obs_url
1013

1114

@@ -89,3 +92,23 @@ def test_insecure_setting() -> None:
8992

9093
settings = Settings(insecure=False)
9194
assert settings.insecure is False
95+
96+
97+
def test_obs_download_url_validation() -> None:
98+
"""Test validation of obs_download_url."""
99+
# Valid URL
100+
settings = Settings(obs_download_url="http://download.suse.de/ibs")
101+
assert settings.obs_download_url == "http://download.suse.de/ibs"
102+
103+
# Invalid URL (no hostname)
104+
with pytest.raises(ValidationError, match="OBS_DOWNLOAD_URL 'invalid-url' does not contain a valid hostname"):
105+
Settings(obs_download_url="invalid-url")
106+
107+
108+
def test_repo_mirror_host_property() -> None:
109+
"""Test the repo_mirror_host property."""
110+
settings = Settings(obs_download_url="http://download.suse.de/ibs")
111+
assert settings.repo_mirror_host == "download.suse.de"
112+
113+
settings = Settings(obs_download_url="https://mirror.example.com/path")
114+
assert settings.repo_mirror_host == "mirror.example.com"

tests/test_loader_gitea_build_results.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -308,6 +308,7 @@ def test_verify_repo_exists_status_codes(
308308
repo_type="product",
309309
download_base_url="http://example.com",
310310
obs_download_url="http://download.suse.de/ibs",
311+
repo_mirror_host="download.suse.de",
311312
),
312313
)
313314

@@ -325,6 +326,7 @@ def test_verify_repo_exists_no_product_version(mocker: MockerFixture) -> None:
325326
repo_type="product",
326327
download_base_url="http://example.com",
327328
obs_download_url="http://download.suse.de/ibs",
329+
repo_mirror_host="download.suse.de",
328330
),
329331
)
330332

@@ -349,6 +351,7 @@ def test_verify_repo_exists_missing_urls(download_base: str, obs_download: str)
349351
repo_type="product",
350352
download_base_url=download_base,
351353
obs_download_url=obs_download,
354+
repo_mirror_host="example.com",
352355
),
353356
)
354357

@@ -364,6 +367,7 @@ def test_verify_repo_exists_invalid_obs_url() -> None:
364367
repo_type="product",
365368
download_base_url="http://example.com",
366369
obs_download_url="invalid-url-no-slashes",
370+
repo_mirror_host="",
367371
),
368372
)
369373

tests/test_loader_gitea_helpers.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,7 @@ def test_add_channel_for_build_result_local() -> None:
162162
repo_type="product",
163163
download_base_url="http://base.url",
164164
obs_download_url="http://obs.url",
165+
repo_mirror_host="obs.url",
165166
obs_products={"all"},
166167
),
167168
)

0 commit comments

Comments
 (0)