Skip to content

Commit 19f9ff1

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 fc4a87f commit 19f9ff1

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

@@ -70,6 +71,15 @@ class Settings(BaseSettings):
7071
# How long to wait for http(s) call in seconds
7172
url_timeout: int = 60
7273

74+
@field_validator("obs_download_url")
75+
@classmethod
76+
def validate_obs_download_url(cls, v: str) -> str:
77+
"""Validate that obs_download_url has a valid hostname."""
78+
if not urlparse(v).netloc:
79+
msg = f"OBS_DOWNLOAD_URL '{v}' does not contain a valid hostname"
80+
raise ValueError(msg)
81+
return v
82+
7383
model_config = SettingsConfigDict(
7484
env_file=".env",
7585
env_file_encoding="utf-8",
@@ -102,6 +112,11 @@ def git_review_bot_user(self) -> str | None:
102112
return self.git_review_bot
103113
return self.obs_group + "-review"
104114

115+
@property
116+
def repo_mirror_host(self) -> str:
117+
"""Extract the repository mirror host from obs_download_url."""
118+
return urlparse(self.obs_download_url).netloc
119+
105120
@property
106121
def obs_products_set(self) -> set[str]:
107122
"""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
@@ -337,8 +337,6 @@ def verify_repo_exists(
337337
if not product_version:
338338
return True
339339
repo_url = get_repo_url(target, product_version, config)
340-
if not repo_url:
341-
return True
342340
with suppress(requests.exceptions.RequestException):
343341
response = retried_requests.head(repo_url, allow_redirects=True)
344342
if response.status_code == HTTPStatus.NOT_FOUND:
@@ -443,6 +441,7 @@ def add_build_result(
443441
repo_type=config.settings.obs_repo_type or "product",
444442
download_base_url=config.settings.download_base_url,
445443
obs_download_url=config.settings.obs_download_url,
444+
repo_mirror_host=config.settings.repo_mirror_host,
446445
obs_products=config.settings.obs_products_set,
447446
)
448447
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

@@ -80,3 +83,23 @@ def test_obs_web_url_property() -> None:
8083

8184
settings = Settings(obs_url="https://some.api.server.com")
8285
assert settings.obs_web_url == "https://some.build.server.com"
86+
87+
88+
def test_obs_download_url_validation() -> None:
89+
"""Test validation of obs_download_url."""
90+
# Valid URL
91+
settings = Settings(obs_download_url="http://download.suse.de/ibs")
92+
assert settings.obs_download_url == "http://download.suse.de/ibs"
93+
94+
# Invalid URL (no hostname)
95+
with pytest.raises(ValidationError, match="OBS_DOWNLOAD_URL 'invalid-url' does not contain a valid hostname"):
96+
Settings(obs_download_url="invalid-url")
97+
98+
99+
def test_repo_mirror_host_property() -> None:
100+
"""Test the repo_mirror_host property."""
101+
settings = Settings(obs_download_url="http://download.suse.de/ibs")
102+
assert settings.repo_mirror_host == "download.suse.de"
103+
104+
settings = Settings(obs_download_url="https://mirror.example.com/path")
105+
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
@@ -157,6 +157,7 @@ def test_add_channel_for_build_result_local() -> None:
157157
repo_type="product",
158158
download_base_url="http://base.url",
159159
obs_download_url="http://obs.url",
160+
repo_mirror_host="obs.url",
160161
obs_products={"all"},
161162
),
162163
)

0 commit comments

Comments
 (0)