Fix string handling edge cases and case-insensitive URL/file detection#290
Fix string handling edge cases and case-insensitive URL/file detection#290christianhelle merged 3 commits intomainfrom
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
Co-authored-by: christianhelle <710400+christianhelle@users.noreply.github.com>
Co-authored-by: christianhelle <710400+christianhelle@users.noreply.github.com>
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #290 +/- ##
==========================================
+ Coverage 84.93% 85.12% +0.18%
==========================================
Files 12 12
Lines 551 558 +7
Branches 76 78 +2
==========================================
+ Hits 468 475 +7
Misses 70 70
Partials 13 13
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:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes critical string handling bugs and improves robustness of URL/file detection logic across the codebase.
Key changes:
- Added null/empty guards to
CapitalizeFirstCharacter()to preventArgumentOutOfRangeException - Implemented case-insensitive protocol detection for HTTP/HTTPS URLs in
IsHttp()andIsYaml() - Refactored
ParseOpenApi()to avoid unnecessaryFileInfoallocation for HTTP URLs
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/HttpGenerator.Core/StringExtensions.cs | Adds null/empty/single-character guards to prevent crashes in CapitalizeFirstCharacter() |
| src/HttpGenerator.Tests/StringExtensionsTests.cs | Adds test coverage for edge cases (empty string and single character) |
| src/HttpGenerator.Core/OpenApiDocumentFactory.cs | Makes IsHttp() and IsYaml() case-insensitive with proper protocol/extension matching |
| src/HttpGenerator/Validation/OpenApiValidator.cs | Updates GetStream() and ParseOpenApi() to use case-insensitive HTTP detection and avoid unnecessary FileInfo allocation |
| CancellationToken cancellationToken) | ||
| { | ||
| if (input.StartsWith("http")) | ||
| if (input.StartsWith("http", StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
The condition input.StartsWith("http", StringComparison.OrdinalIgnoreCase) is too broad and will match invalid strings like "httpxyz", "httpasdf://example.com", or even "HTTP" without the protocol separator. This inconsistency with IsHttp() in OpenApiDocumentFactory.cs (which correctly checks for "http://" and "https://") can lead to incorrect classification.
Consider changing this to match the pattern used in OpenApiDocumentFactory.cs:
if (input.StartsWith("http://", StringComparison.OrdinalIgnoreCase) ||
input.StartsWith("https://", StringComparison.OrdinalIgnoreCase))| { | ||
| var directoryName = new FileInfo(openApiFile).DirectoryName; | ||
| Uri baseUrl; | ||
| if (openApiFile.StartsWith("http", StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
The condition openApiFile.StartsWith("http", StringComparison.OrdinalIgnoreCase) is too broad and will match invalid strings like "httpxyz", "httpasdf://example.com", or even "HTTP" without the protocol separator. This inconsistency with IsHttp() in OpenApiDocumentFactory.cs (which correctly checks for "http://" and "https://") can lead to incorrect classification and unnecessary new Uri(openApiFile) calls that may throw UriFormatException.
Consider changing this to match the pattern used in OpenApiDocumentFactory.cs:
if (openApiFile.StartsWith("http://", StringComparison.OrdinalIgnoreCase) ||
openApiFile.StartsWith("https://", StringComparison.OrdinalIgnoreCase))| return path.StartsWith("http://", StringComparison.OrdinalIgnoreCase) || | ||
| path.StartsWith("https://", StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
The case-insensitive URL detection fix lacks test coverage for edge cases like uppercase URLs (e.g., "HTTP://example.com", "HTTPS://example.com"). Consider adding test cases in OpenApiDocumentFactoryTests.cs to verify that CreateAsync correctly handles uppercase URL protocols:
[Theory]
[InlineData("HTTP://raw.githubusercontent.com/christianhelle/httpgenerator/main/test/OpenAPI/v3.0/petstore.json")]
[InlineData("HTTPS://raw.githubusercontent.com/christianhelle/httpgenerator/main/test/OpenAPI/v3.0/petstore.json")]
public async Task Create_From_Uri_With_Uppercase_Protocol_Returns_NotNull(string url)
{
(await OpenApiDocumentFactory.CreateAsync(url))
.Should()
.NotBeNull();
}| return path.EndsWith(".yaml", StringComparison.OrdinalIgnoreCase) || | ||
| path.EndsWith(".yml", StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
The improved IsYaml() method that now correctly requires a dot prefix and is case-insensitive lacks test coverage. Consider adding test cases in OpenApiDocumentFactoryTests.cs to verify that files with uppercase extensions are handled correctly:
[Theory]
[InlineData(Samples.PetstoreYamlV3, "SwaggerPetstore.YAML")]
[InlineData(Samples.PetstoreYamlV3, "SwaggerPetstore.YML")]
public async Task Create_From_Yaml_File_With_Uppercase_Extension_Returns_NotNull(Samples version, string filename)
{
var swaggerFile = await TestFile.CreateSwaggerFile(EmbeddedResources.GetSwaggerPetstore(version), filename);
(await OpenApiDocumentFactory.CreateAsync(swaggerFile))
.Should()
.NotBeNull();
}


Description:
Fixes several bugs discovered during codebase analysis:
String handling edge cases (StringExtensions.cs)
CapitalizeFirstCharacter()threwArgumentOutOfRangeExceptionon empty stringsCase-insensitive URL/file detection (OpenApiDocumentFactory.cs, OpenApiValidator.cs)
IsHttp()failed for uppercase URLs likeHTTP://orHTTPS://IsYaml()matched filenames likenotayamlinstead of only.yaml/.ymlextensionsGetStream()used case-sensitive HTTP checkUnnecessary FileInfo allocation (OpenApiValidator.cs)
ParseOpenApi()createdFileInfofor HTTP URLs; refactored to only allocate when processing file pathsExample fix for CapitalizeFirstCharacter:
Example fix for IsYaml:
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
demo.netbox.dev/usr/share/dotnet/dotnet /usr/share/dotnet/dotnet exec --runtimeconfig /home/REDACTED/work/httpgenerator/httpgenerator/src/HttpGenerator.Tests/bin/Release/net8.0/HttpGenerator.Tests.runtimeconfig.json --depsfile /home/REDACTED/work/httpgenerator/httpgenerator/src/HttpGenerator.Tests/bin/Release/net8.0/HttpGenerator.Tests.deps.json /home/REDACTED/work/httpgenerator/httpgenerator/src/HttpGenerator.Tests/bin/Release/net8.0/testhost.dll --port 44509 --endpoint 127.0.0.1:044509 --role client --parentprocessid 3654 --telemetryoptedin false(dns block)/usr/share/dotnet/dotnet /usr/share/dotnet/dotnet exec --runtimeconfig /home/REDACTED/work/httpgenerator/httpgenerator/src/HttpGenerator.Tests/bin/Release/net8.0/HttpGenerator.Tests.runtimeconfig.json --depsfile /home/REDACTED/work/httpgenerator/httpgenerator/src/HttpGenerator.Tests/bin/Release/net8.0/HttpGenerator.Tests.deps.json /home/REDACTED/work/httpgenerator/httpgenerator/src/HttpGenerator.Tests/bin/Release/net8.0/testhost.dll --port 36815 --endpoint 127.0.0.1:036815 --role client --parentprocessid 4630 --telemetryoptedin false(dns block)/usr/share/dotnet/dotnet /usr/share/dotnet/dotnet exec --runtimeconfig /home/REDACTED/work/httpgenerator/httpgenerator/src/HttpGenerator.Tests/bin/Release/net8.0/HttpGenerator.Tests.runtimeconfig.json --depsfile /home/REDACTED/work/httpgenerator/httpgenerator/src/HttpGenerator.Tests/bin/Release/net8.0/HttpGenerator.Tests.deps.json /home/REDACTED/work/httpgenerator/httpgenerator/src/HttpGenerator.Tests/bin/Release/net8.0/testhost.dll --port 39719 --endpoint 127.0.0.1:039719 --role client --parentprocessid 5588 --telemetryoptedin false(dns block)developers.intellihr.io/usr/share/dotnet/dotnet /usr/share/dotnet/dotnet exec --runtimeconfig /home/REDACTED/work/httpgenerator/httpgenerator/src/HttpGenerator.Tests/bin/Release/net8.0/HttpGenerator.Tests.runtimeconfig.json --depsfile /home/REDACTED/work/httpgenerator/httpgenerator/src/HttpGenerator.Tests/bin/Release/net8.0/HttpGenerator.Tests.deps.json /home/REDACTED/work/httpgenerator/httpgenerator/src/HttpGenerator.Tests/bin/Release/net8.0/testhost.dll --port 44509 --endpoint 127.0.0.1:044509 --role client --parentprocessid 3654 --telemetryoptedin false(dns block)/usr/share/dotnet/dotnet /usr/share/dotnet/dotnet exec --runtimeconfig /home/REDACTED/work/httpgenerator/httpgenerator/src/HttpGenerator.Tests/bin/Release/net8.0/HttpGenerator.Tests.runtimeconfig.json --depsfile /home/REDACTED/work/httpgenerator/httpgenerator/src/HttpGenerator.Tests/bin/Release/net8.0/HttpGenerator.Tests.deps.json /home/REDACTED/work/httpgenerator/httpgenerator/src/HttpGenerator.Tests/bin/Release/net8.0/testhost.dll --port 36815 --endpoint 127.0.0.1:036815 --role client --parentprocessid 4630 --telemetryoptedin false(dns block)/usr/share/dotnet/dotnet /usr/share/dotnet/dotnet exec --runtimeconfig /home/REDACTED/work/httpgenerator/httpgenerator/src/HttpGenerator.Tests/bin/Release/net8.0/HttpGenerator.Tests.runtimeconfig.json --depsfile /home/REDACTED/work/httpgenerator/httpgenerator/src/HttpGenerator.Tests/bin/Release/net8.0/HttpGenerator.Tests.deps.json /home/REDACTED/work/httpgenerator/httpgenerator/src/HttpGenerator.Tests/bin/Release/net8.0/testhost.dll --port 39719 --endpoint 127.0.0.1:039719 --role client --parentprocessid 5588 --telemetryoptedin false(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.