Uh oh!
There was an error while loading. Please reload this page.
Enhance server creation logic and add unit tests - #2525
Conversation
Updated the `MakeServers` method to include a check for `defaultUrl` when creating servers. The method now returns early if all relevant parameters are null, and the host assignment from `defaultUrl` now preserves the port. Added new unit tests in `OpenApiServerTests.cs` to ensure that base URLs with ports are correctly handled. Tests cover scenarios for base URLs with a port, with a port and path, and with a non-standard port.
Vincent Biret (baywet)
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
Here are a few comments to help drive this forward.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Refactor `MakeServers` method for better null checks on `host`, `basePath`, `schemes`, and `defaultUrl`. Enhance `BuildUrl` to remove default ports (80 for HTTP, 443 for HTTPS) from generated URLs. Add unit tests in `OpenApiServerTests.cs` to verify correct URL generation when standard ports are used.
Vincent Biret (baywet)
left a comment
There was a problem hiding this comment.
Thank you for making the changes!
An additional minor comment and I think we'll be ready to merge.
I'd like the v2 PR to be updated and ready to go before we merge this one.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Vincent Biret <vincentbiret@hotmail.com>
Vincent Biret (baywet)
left a comment
There was a problem hiding this comment.
Thank you for making the changes!
Can you stand up a PR for v2/main branch please before we can merge everything?
Mahdigln
commented
Oct 3, 2025
Thank you! I've applied the changes |
Mahdigln
commented
Oct 3, 2025
Sure! I'll create a separate PR for the v2/main branch. |
Mahdigln
commented
Oct 3, 2025
I'm unable to create a pull request directly from the main branch - it seems I don't have the necessary permissions or there's a branch protection rule. |
Mahdigln
commented
Oct 6, 2025
Hi |
Vincent Biret (baywet)
commented
Oct 6, 2025
@microsoft-github-policy-service rerun |
Vincent Biret (baywet)
commented
Oct 6, 2025
@mahdigIn the CLA issue is fixed, sometimes it fails for random reasons. Just to clarify my ask:
You'll need to be working on a branch from your fork for this, like you've done for the v1 patch. Let me know if you have any additional comments or questions. |
Mahdigln
commented
Oct 6, 2025
Thank you very much for your help and clarification. 🙏 |
Uh oh!
There was an error while loading. Please reload this page.
Fix port omission from BaseUrl in V2 server deserialization
Problem
When setting a BaseUrl with a non-standard port and the OpenAPI V2 document doesn't contain any server definitions, the port number was being omitted from the default server URL.
Example:
• Input: BaseUrl = "http://demo.testfire.net:8080"
• Current behavior: Server URL = "http://demo.testfire.net" ❌
• Expected behavior: Server URL = "http://demo.testfire.net:8080" ✅
Root Cause
The MakeServers method in OpenApiDocumentDeserializer.cs was using UriComponents.NormalizedHost which only extracts the hostname without the port number.
Solution
Changed UriComponents.NormalizedHost to UriComponents.Host | UriComponents.Port to preserve port information when extracting host details from the BaseUrl. This approach is consistent with the existing pattern used in OpenApiDocument.WriteHostInfoV2.
Additionally, fixed the early return logic to allow server creation from BaseUrl even when no host, basePath, or schemes are defined in the document.
Changes
• Line 154: Changed from UriComponents.NormalizedHost to UriComponents.Host | UriComponents.Port
• Line 144: Modified early return condition to check for defaultUrl != null
• Added 3 new test cases to verify port preservation in various scenarios
Testing
• All existing tests pass ✅
• Added comprehensive test coverage for port preservation scenarios
• 16/16 OpenApiServerTests passing
• 78/78 V2Tests passing
FixesWhen BaseUrl has a port, the default server url doesn't contain the port #1294