Refactor URI construction and enhance test coverage #126
Conversation
Refactored `DeleteOperation` and `ReplaceOperation` to directly include `operation.ItemId` in URI construction via `MakeAbsoluteUri`. Updated `ExecutableOperation` to support an optional `itemId` parameter in `MakeAbsoluteUri`. Added `MakeAbsoluteUri_WorksWithId` theory test to validate various URI construction scenarios.
|
@dotnet-policy-service agree |
|
Resolves #124 |
| } | ||
|
|
||
| if (baseAddress != null) | ||
| { | ||
| if (baseAddress.IsAbsoluteUri) | ||
| { | ||
| return new Uri($"{new Uri(baseAddress, relativeOrAbsoluteUri).ToString().TrimEnd('/')}/"); | ||
| return new Uri($"{new Uri(baseAddress, relativeOrAbsoluteUri).ToString().TrimEnd('/')}/{itemId}"); | ||
| } |
There was a problem hiding this comment.
I think we may need to trim the slash if it is at the end. In some scenarios, the ASP.NET Core routing middleware treats a http://server/foo/ as lexically different to http://server/foo
There was a problem hiding this comment.
Hi @adrianhall,
No problem. I'll update to remove the trailing slash and update the tests. I know in my current environment all the other operations work as expected against the server.
There was a problem hiding this comment.
@adrianhall hopefully that's it.
There was a problem hiding this comment.
Nope - this definitely has an issue. See this one:
return new Uri($"{relativeOrAbsoluteUri.ToString().TrimEnd('/')}{itemId}");
If relativeOrAbsoluteUri ends with a / and itemId is set, then there is no separator.
Probably better to construct a string with the right URI in it. Then return new Uri(uri.TrimEnd('/')); at the end.
Modified `MakeAbsoluteUri` in `ExecutableOperation.cs` to handle `itemId` more robustly by conditionally formatting it with a leading slash if it is not empty and avoiding trailing slashes before `itemId`. Updated test cases in `OfflineDbContext_Tests.cs`, `AddOperation_Tests.cs`, and `ExecutableOperation_Tests.cs` to reflect the new URI formatting rules, specifically removing trailing slashes before query parameters in the expected URIs.
| } | ||
|
|
||
| if (baseAddress != null) | ||
| { | ||
| if (baseAddress.IsAbsoluteUri) | ||
| { | ||
| return new Uri($"{new Uri(baseAddress, relativeOrAbsoluteUri).ToString().TrimEnd('/')}/"); | ||
| return new Uri($"{new Uri(baseAddress, relativeOrAbsoluteUri).ToString().TrimEnd('/')}/{itemId}"); | ||
| } |
There was a problem hiding this comment.
Nope - this definitely has an issue. See this one:
return new Uri($"{relativeOrAbsoluteUri.ToString().TrimEnd('/')}{itemId}");
If relativeOrAbsoluteUri ends with a / and itemId is set, then there is no separator.
Probably better to construct a string with the right URI in it. Then return new Uri(uri.TrimEnd('/')); at the end.
| { | ||
| itemId = string.IsNullOrWhiteSpace(itemId) ? string.Empty : $"/{itemId}"; |
There was a problem hiding this comment.
@adrianhall the separator is set here if the itemId is specified.
There was a problem hiding this comment.
Ah - I get it. Although, use of IsNullOrWhiteSpace does change the semantics of the method. If someone submits " ", I believe it gets rejected before this point, so should be ok.
Assuming the tests pass, I'll merge this.
|
Explicitly: internal static Uri MakeAbsoluteUri(Uri? baseAddress, Uri relativeOrAbsoluteUri, string? itemId = null)
{
itemId ??= string.Empty;
string uri = string.Empty;
if (relativeOrAbsoluteUri.IsAbsoluteUri)
{
uri = $"{relativeOrAbsoluteUri.ToString().TrimEnd('/')}/{itemId}";
return new Uri(uri.TrimEnd('/'));
}
if (baseAddress?.IsAbsoluteUri ==true)
{
uri = $"{new Uri(baseAddress, relativeOrAbsoluteUri).ToString().TrimEnd('/')}/{itemId}";
return new Uri(uri.TrimEnd('/'));
}
throw new UriFormatException("Invalid combination of baseAddress and relativeUri");
} |
Refactored
DeleteOperationandReplaceOperationto directly includeoperation.ItemIdin URI construction viaMakeAbsoluteUri. UpdatedExecutableOperationto support an optionalitemIdparameter inMakeAbsoluteUri. AddedMakeAbsoluteUri_WorksWithIdtheory test to validate various URI construction scenarios.