Validate parent_dir and base_url with actionable errors #33
marvin8
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/issue-26-cli-validation"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #26
Validates the two CLI inputs up front and fails with actionable messages instead of surfacing cryptic errors mid-run:
parent_dirmissing →FileNotFoundErrornaming the pathparent_diris a file →NotADirectoryErrornaming the pathbase_urlwithout anhttp:///https://scheme →ValueErrorValidation is extracted into
_validate_inputs()to keepmainunder the complexipy threshold. Covered by Tryke tests (red → green).WuMing
Found 3 issue(s). See inline comments below.
@ -18,0 +30,4 @@"""if not parent_dir.exists():raise FileNotFoundError(f"parent_dir does not exist: {parent_dir}")if not parent_dir.is_dir():code [LOW]
Validation checks existence and directory type but not read/traverse permissions. A directory that exists and is a directory but lacks permissions will still raise a raw PermissionError later at
parent_dir.iterdir(). Consider adding anos.access(parent_dir, os.R_OK | os.X_OK)check and raising PermissionError with an actionable message in_validate_inputs.@ -18,0 +32,4 @@raise FileNotFoundError(f"parent_dir does not exist: {parent_dir}")if not parent_dir.is_dir():raise NotADirectoryError(f"parent_dir is not a directory: {parent_dir}")if not base_url.startswith(("http://", "https://")):code [MEDIUM]
URL scheme validation is case-sensitive. URL schemes are case-insensitive per RFC 3986, so valid inputs like
HTTPS://example.comorHttp://example.comwill be incorrectly rejected with ValueError. Normalize before checking, e.g.base_url.lower().startswith(('http://', 'https://')), or useurllib.parse.urlparseand compare the lowercased scheme.@ -116,0 +158,4 @@def test_valid_base_url_accepted() -> None:with tempfile.TemporaryDirectory() as tmp:_make_channel(Path(tmp))main(Path(tmp), "https://pod.example.com")tests [MEDIUM]
Test calls main() without any explicit assertion or expectation. It only relies on an exception failing the test; add an explicit assertion such as expect(...).not_to_raise or verify the expected outcome.
WuMing review — 2 fixed, 1 declined
src/sub2pod/cli.pyline 35✅ Fixed in commit
dd1408f— scheme check now usesbase_url.lower().startswith(("http://", "https://")).tests/test_cli.pyline 161✅ Fixed in commit
dd1408f—test_valid_base_url_acceptednow assertsfeed.xmlexists, and a new test covers the case-insensitive scheme.src/sub2pod/cli.pyline 33🔴 Declined — issue #26 scopes validation to existence + directory-ness.
os.accessis unreliable here (returns true as root and under ACLs), so it would not reliably produce the actionable error it aims for. Happy to add permission handling as a follow-up if you want it.WuMing
Found 2 issue(s). See inline comments below.
@ -18,0 +32,4 @@raise FileNotFoundError(f"parent_dir does not exist: {parent_dir}")if not parent_dir.is_dir():raise NotADirectoryError(f"parent_dir is not a directory: {parent_dir}")if not base_url.lower().startswith(("http://", "https://")):code [MEDIUM]
The prefix check accepts malformed URLs like "http://" or "https://" (scheme with no host) because it only validates that base_url starts with http:// or https://. These pass _validate_inputs but can still produce invalid feed links later. Use urllib.parse.urlparse/urlsplit to require scheme in {'http','https'} and a non-empty netloc before accepting.
@ -116,0 +147,4 @@@test("base_url without a scheme raises ValueError")def test_schemaless_base_url_raises() -> None:with tempfile.TemporaryDirectory() as tmp:expect(lambda: main(Path(tmp), "not-a-url")).to_raise(ValueError)code [LOW]
There is no test for hostless URLs such as "http://" or "https://". Such values start with a valid scheme prefix but have no host, so the current validation lets them through. Add tests that these raise ValueError (or add support in validation first).
WuMing review — fixed (hostless URL validation)
src/sub2pod/cli.pyline 35✅ Fixed in commit
e9b24a8— validation now usesurllib.parse.urlparseand requires a non-emptynetlocin addition to anhttp/httpsscheme.tests/test_cli.pyline 150✅ Fixed in commit
e9b24a8— addedtest_hostless_http_base_url_raisesandtest_hostless_https_base_url_raises.WuMing
Found 1 issue(s). See inline comments below.
@ -18,0 +34,4 @@if not parent_dir.is_dir():raise NotADirectoryError(f"parent_dir is not a directory: {parent_dir}")parsed = urlparse(base_url)if parsed.scheme.lower() not in ("http", "https") or not parsed.netloc:code [MEDIUM]
Using parsed.netloc to check for a host lets URLs with empty hosts but non-empty ports through (e.g. http://:80 has netloc ':80'), and whitespace-only hosts are also truthy. This defeats the stated host validation. Check parsed.hostname instead, e.g.
if parsed.scheme.lower() not in ("http", "https") or not (parsed.hostname and parsed.hostname.strip()):, and add a test forhttp://:80.src/sub2pod/cli.pyline 37✅ Fixed in commit
78c84dc— validation now checksparsed.hostname and parsed.hostname.strip(). Added tests forhttp://:80and a whitespace-only host.WuMing
Found 1 issue(s). See inline comments below.
@ -18,0 +34,4 @@if not parent_dir.is_dir():raise NotADirectoryError(f"parent_dir is not a directory: {parent_dir}")parsed = urlparse(base_url)if parsed.scheme.lower() not in ("http", "https") or not (parsed.hostname and parsed.hostname.strip()):code [MEDIUM]
The base_url validation only checks that parsed.hostname is non-empty after stripping, so invalid hostnames and ports are accepted. For example, https://example.com:bad and https:// example.com both pass because parsed.hostname returns a truthy value. These will lead to invalid podcast URLs later instead of an actionable ValueError. Validate parsed.hostname against allowed hostname characters, reject surrounding/embedded whitespace, and/or validate parsed.port (catching ValueError) before accepting.
src/sub2pod/cli.pyline 37✅ Fixed in commit
7631cf0— validation now rejects any whitespace in the hostname and catchesValueErrorfromparsed.port(invalid ports). Added tests forhttps://example.com:badandhttps:// example.com.WuMing
Found 2 issue(s). See inline comments below.
@ -18,0 +30,4 @@parsed = urlparse(base_url)if parsed.scheme.lower() not in ("http", "https"):raise ValueError(f"base_url must be an http:// or https:// URL, got {base_url!r}")hostname = parsed.hostnamecode [MEDIUM]
parsed.hostnamecan itself raiseValueErrorfor malformed ports (e.g.https://example.com:bad) before thetryaroundparsed.portis reached. As a result, the intended actionable messagebase_url has an invalid portis unreachable for non-numeric ports. Move hostname extraction inside the try/except or accessparsed.portbeforeparsed.hostnameso the custom error is used.@ -116,0 +191,4 @@@test("invalid port (https://example.com:bad) raises ValueError")def test_invalid_port_raises() -> None:with tempfile.TemporaryDirectory() as tmp:expect(lambda: main(Path(tmp), "https://example.com:bad")).to_raise(ValueError)code [LOW]
This test only asserts that a ValueError is raised, not the actionable message. A regression that leaks urllib's lower-level
Port could not be cast...message (instead ofbase_url has an invalid port) would still pass.WuMing review — 1 fixed, 1 declined
src/sub2pod/cli.pyline 33🔴 Declined — this is not correct. Verified empirically:
urlparse("https://example.com:bad").hostnamereturns"example.com"without raising; only.portraisesValueError. Thehostnameextraction before thetryis therefore safe, and thebase_url has an invalid portmessage is reachable for non-numeric ports.tests/test_cli.pyline 194✅ Fixed in commit
e7cdd73—test_invalid_port_raisesnow asserts the message contains "invalid port".e7cdd73c16a54ffe344bWuMing
Found 1 issue(s). See inline comments below.
@ -116,0 +154,4 @@with tempfile.TemporaryDirectory() as tmp:expect(lambda: main(Path(tmp), "ftp://example.com")).to_raise(ValueError)@test("valid https base_url is accepted")code [LOW]
The validation tests cover
https://and uppercaseHTTPS://, but there is no test that a lowercasehttp://base_url is accepted. Since the code explicitly allowshttp, add a test to cover this branch.tests/test_cli.pyline 157✅ Fixed in commit
a2445c5— addedtest_valid_http_base_url_acceptedcovering a lowercasehttp://pod.example.com.