Fix URL port handling in get_url_path_parts#2933
Conversation
📝 WalkthroughWalkthroughModified URL parsing logic in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
📚 Docs Preview: https://pr-2933.datamodel-code-generator.pages.dev |
CodSpeed Performance ReportMerging #2933 will improve performance by 19.71%Comparing
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | WallTime | test_perf_all_options_enabled |
6.6 s | 5.7 s | +15.66% |
| ⚡ | WallTime | test_perf_openapi_large |
2.9 s | 2.5 s | +14.67% |
| ⚡ | WallTime | test_perf_deep_nested |
6 s | 5.1 s | +18.04% |
| ⚡ | WallTime | test_perf_complex_refs |
2 s | 1.7 s | +19.11% |
| ⚡ | WallTime | test_perf_kubernetes_style_pydantic_v2 |
2.6 s | 2.2 s | +18.18% |
| ⚡ | WallTime | test_perf_large_models_pydantic_v2 |
3.6 s | 3 s | +19.71% |
| ⚡ | WallTime | test_perf_graphql_style_pydantic_v2 |
820.5 ms | 691.2 ms | +18.71% |
| ⚡ | WallTime | test_perf_duplicate_names |
1,003.3 ms | 843.1 ms | +19% |
| ⚡ | WallTime | test_perf_multiple_files_input |
3.7 s | 3.1 s | +18.59% |
| ⚡ | WallTime | test_perf_stripe_style_pydantic_v2 |
2 s | 1.7 s | +18.57% |
| ⚡ | WallTime | test_perf_aws_style_openapi_pydantic_v2 |
1.9 s | 1.6 s | +18.07% |
Footnotes
-
98 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2933 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 92 92
Lines 16957 16969 +12
Branches 1976 1976
=========================================
+ Hits 16957 16969 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Breaking Change AnalysisResult: No breaking changes detected Reasoning: This PR is a bug fix that changes This analysis was performed by Claude Code Action |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @tests/main/openapi/test_main_openapi.py:
- Around line 1816-1861: The test test_main_http_openapi_with_custom_port
currently uses an internal $ref and thus never exercises external fetching;
update the OpenAPI schema in that test to include an external $ref (for example
$ref: "http://127.0.0.1:8123/definitions.yaml" or a relative
"./definitions.yaml") so the generator will perform a second HTTP fetch, mock
httpx.get to return two distinct mock responses (one for the main openapi.json
and one for definitions.yaml) and then assert httpx_get_mock was called for both
URLs preserving port 8123 (e.g., assert_called_with for
"http://127.0.0.1:8123/openapi.json" and for
"http://127.0.0.1:8123/definitions.yaml"), ensuring the main function (main)
preserves the custom port on subsequent $ref requests.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/datamodel_code_generator/parser/base.pytests/main/openapi/test_main_openapi.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/main/openapi/test_main_openapi.py (1)
src/datamodel_code_generator/__main__.py (2)
main(1076-1429)Exit(129-135)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: py312-black24 on Ubuntu
- GitHub Check: py312-black23 on Ubuntu
- GitHub Check: py312-isort7 on Ubuntu
- GitHub Check: 3.12 on Windows
- GitHub Check: py312-pydantic1 on Ubuntu
- GitHub Check: py312-isort5 on Ubuntu
- GitHub Check: 3.10 on Ubuntu
- GitHub Check: py312-isort6 on Ubuntu
- GitHub Check: 3.11 on Windows
- GitHub Check: 3.10 on Windows
- GitHub Check: py312-black22 on Ubuntu
- GitHub Check: 3.13 on Ubuntu
- GitHub Check: 3.14 on Windows
- GitHub Check: 3.11 on Ubuntu
- GitHub Check: 3.13 on Windows
- GitHub Check: Analyze (python)
- GitHub Check: benchmarks
🔇 Additional comments (1)
src/datamodel_code_generator/parser/base.py (1)
1069-1075: LGTM! Correct fix for preserving custom ports in URLs.The change from
url.hostnametourl.netloccorrectly preserves the port number and userinfo in the base URL. This ensures that subsequent requests for$refresources use the original custom port instead of defaulting to port 80.
url.netlocincludes:[userinfo@]host[:port]url.hostnameincludes only:hostThis fix aligns with the standard URL format and resolves the regression where custom ports were lost in
$refresolution.
| def test_main_http_openapi_with_custom_port(mocker: MockerFixture, output_file: Path) -> None: | ||
| """Test OpenAPI code generation from HTTP URL with custom port preserves port in refs.""" | ||
| schema_content = """\ | ||
| openapi: "3.0.0" | ||
| info: | ||
| title: Minimal API | ||
| version: "1.0.0" | ||
| paths: {} | ||
| components: | ||
| schemas: | ||
| Item: | ||
| type: object | ||
| properties: | ||
| id: | ||
| type: string | ||
| owner: | ||
| $ref: "#/components/schemas/User" | ||
| User: | ||
| type: object | ||
| properties: | ||
| name: | ||
| type: string | ||
| """ | ||
| mock_response = mocker.Mock() | ||
| mock_response.text = schema_content | ||
|
|
||
| httpx_get_mock = mocker.patch("httpx.get", return_value=mock_response) | ||
|
|
||
| from datamodel_code_generator.__main__ import main | ||
|
|
||
| return_code = main(["--url", "http://127.0.0.1:8123/openapi.json", "--output", str(output_file)]) | ||
| assert return_code == Exit.OK | ||
|
|
||
| result = output_file.read_text(encoding="utf-8") | ||
| assert "class Item" in result | ||
| assert "class User" in result | ||
|
|
||
| httpx_get_mock.assert_called_once_with( | ||
| "http://127.0.0.1:8123/openapi.json", | ||
| headers=None, | ||
| verify=True, | ||
| follow_redirects=True, | ||
| params=None, | ||
| timeout=30.0, | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Test doesn't verify the fix for external $refs with custom ports.
The test uses an internal $ref (#/components/schemas/User at line 1832), which doesn't trigger additional HTTP requests. According to issue #2931, the bug occurs when "subsequent requests for $ref resources attempt to connect to port 80 instead of the original custom port."
To properly verify the fix, the test should include an external $ref (e.g., $ref: "http://127.0.0.1:8123/definitions.yaml" or a relative $ref: "./definitions.yaml"), mock multiple httpx.get calls, and assert that all requests preserve the custom port.
🔎 Proposed enhancement to test external refs
Consider following the pattern from test_main_http_openapi (lines 1773-1813):
def test_main_http_openapi_with_custom_port(mocker: MockerFixture, output_file: Path) -> None:
"""Test OpenAPI code generation from HTTP URL with custom port preserves port in refs."""
- schema_content = """\
+ main_schema = """\
openapi: "3.0.0"
info:
title: Minimal API
version: "1.0.0"
paths: {}
components:
schemas:
Item:
type: object
properties:
id:
type: string
owner:
- $ref: "#/components/schemas/User"
- User:
- type: object
- properties:
- name:
- type: string
+ $ref: "http://127.0.0.1:8123/definitions.yaml#/components/schemas/User"
"""
- mock_response = mocker.Mock()
- mock_response.text = schema_content
+
+ definitions_schema = """\
+components:
+ schemas:
+ User:
+ type: object
+ properties:
+ name:
+ type: string
+"""
- httpx_get_mock = mocker.patch("httpx.get", return_value=mock_response)
+ def get_mock_response(url: str) -> mocker.Mock:
+ mock = mocker.Mock()
+ if "definitions.yaml" in url:
+ mock.text = definitions_schema
+ else:
+ mock.text = main_schema
+ return mock
+
+ httpx_get_mock = mocker.patch("httpx.get", side_effect=get_mock_response)
from datamodel_code_generator.__main__ import main
return_code = main(["--url", "http://127.0.0.1:8123/openapi.json", "--output", str(output_file)])
assert return_code == Exit.OK
result = output_file.read_text(encoding="utf-8")
assert "class Item" in result
assert "class User" in result
- httpx_get_mock.assert_called_once_with(
- "http://127.0.0.1:8123/openapi.json",
- headers=None,
- verify=True,
- follow_redirects=True,
- params=None,
- timeout=30.0,
- )
+ httpx_get_mock.assert_has_calls([
+ call(
+ "http://127.0.0.1:8123/openapi.json",
+ headers=None,
+ verify=True,
+ follow_redirects=True,
+ params=None,
+ timeout=30.0,
+ ),
+ call(
+ "http://127.0.0.1:8123/definitions.yaml",
+ headers=None,
+ verify=True,
+ follow_redirects=True,
+ params=None,
+ timeout=30.0,
+ ),
+ ])This would verify that both the initial request and the subsequent request for the external $ref preserve the custom port 8123.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @tests/main/openapi/test_main_openapi.py around lines 1816-1861, The test
test_main_http_openapi_with_custom_port currently uses an internal $ref and thus
never exercises external fetching; update the OpenAPI schema in that test to
include an external $ref (for example $ref:
"http://127.0.0.1:8123/definitions.yaml" or a relative "./definitions.yaml") so
the generator will perform a second HTTP fetch, mock httpx.get to return two
distinct mock responses (one for the main openapi.json and one for
definitions.yaml) and then assert httpx_get_mock was called for both URLs
preserving port 8123 (e.g., assert_called_with for
"http://127.0.0.1:8123/openapi.json" and for
"http://127.0.0.1:8123/definitions.yaml"), ensuring the main function (main)
preserves the custom port on subsequent $ref requests.
|
🎉 Released in 0.52.2 This PR is now available in the latest release. See the release notes for details. |
1 participant
Fixes: #2931
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.