Skip to content

Refactor URI construction and enhance test coverage #126

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 8, 2024
Merged

Refactor URI construction and enhance test coverage #126

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c73a68b
Refactor URI construction and enhance test coverage
richard-einfinity Oct 8, 2024
d10933d
Merge branch 'main' into bugfix/UriConcatenation
adrianhall Oct 8, 2024
fd2005d
Refactor URI handling and update related tests
richard-einfinity Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/CommunityToolkit.Datasync.Client/Offline/Operations/DeleteOperation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ internal class DeleteOperation(DatasyncOperation operation) : ExecutableOperatio
/// <returns>The result of the push operation (async).</returns>
internal override async Task<ServiceResponse> ExecuteAsync(EntityDatasyncOptions options, CancellationToken cancellationToken = default)
{
Uri endpoint = MakeAbsoluteUri(options.HttpClient.BaseAddress, options.Endpoint);
using HttpRequestMessage request = new(HttpMethod.Delete, new Uri(endpoint, operation.ItemId));
Uri endpoint = MakeAbsoluteUri(options.HttpClient.BaseAddress, options.Endpoint, operation.ItemId);
using HttpRequestMessage request = new(HttpMethod.Delete, endpoint);
if (!string.IsNullOrEmpty(operation.EntityVersion))
{
request.Headers.IfMatch.Add(new EntityTagHeaderValue($"\"{operation.EntityVersion}\""));
Expand Down
9 changes: 6 additions & 3 deletions src/CommunityToolkit.Datasync.Client/Offline/Operations/ExecutableOperation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,19 +22,22 @@ internal abstract class ExecutableOperation
/// </summary>
/// <param name="baseAddress">The base address from the client.</param>
/// <param name="relativeOrAbsoluteUri">A relative or absolute URI</param>
/// <param name="itemId">The id of the item to append to the URI.</param>
/// <returns></returns>
internal static Uri MakeAbsoluteUri(Uri? baseAddress, Uri relativeOrAbsoluteUri)
internal static Uri MakeAbsoluteUri(Uri? baseAddress, Uri relativeOrAbsoluteUri, string? itemId = null)
{
itemId ??= string.Empty;

if (relativeOrAbsoluteUri.IsAbsoluteUri)
{
return new Uri($"{relativeOrAbsoluteUri.ToString().TrimEnd('/')}/");
return new Uri($"{relativeOrAbsoluteUri.ToString().TrimEnd('/')}/{itemId}");
}

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}");
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adrianhall hopefully that's it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

Expand Down
4 changes: 2 additions & 2 deletions src/CommunityToolkit.Datasync.Client/Offline/Operations/ReplaceOperation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ internal class ReplaceOperation(DatasyncOperation operation) : ExecutableOperati
/// <returns>The result of the push operation (async).</returns>
internal override async Task<ServiceResponse> ExecuteAsync(EntityDatasyncOptions options, CancellationToken cancellationToken = default)
{
Uri endpoint = MakeAbsoluteUri(options.HttpClient.BaseAddress, options.Endpoint);
using HttpRequestMessage request = new(HttpMethod.Put, new Uri(endpoint, operation.ItemId))
Uri endpoint = MakeAbsoluteUri(options.HttpClient.BaseAddress, options.Endpoint, operation.ItemId);
using HttpRequestMessage request = new(HttpMethod.Put, endpoint)
{
Content = new StringContent(operation.Item, JsonMediaType)
};
Expand Down
31 changes: 31 additions & 0 deletions tests/CommunityToolkit.Datasync.Client.Test/Offline/Operations/ExecutableOperation_Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,37 @@ public void MakeAbsoluteUri_Works(string ba, string bb, string expected)
actual.ToString().Should().Be(expected);
}


[Theory]
[InlineData(null, "https://test.zumo.com/tables/movies", "123", "https://test.zumo.com/tables/movies/123")]
[InlineData(null, "https://test.zumo.com/tables/movies/", "123", "https://test.zumo.com/tables/movies/123")]
[InlineData("https://test.zumo.com", "/tables/movies", "123", "https://test.zumo.com/tables/movies/123")]
[InlineData("https://test.zumo.com", "/tables/movies/", "123", "https://test.zumo.com/tables/movies/123")]
[InlineData("https://test.zumo.com/", "/tables/movies", "123", "https://test.zumo.com/tables/movies/123")]
[InlineData("https://test.zumo.com/", "/tables/movies/", "123", "https://test.zumo.com/tables/movies/123")]
[InlineData("https://test.zumo.com/tables", "movies", "123", "https://test.zumo.com/movies/123")]
[InlineData("https://test.zumo.com/tables", "movies/", "123", "https://test.zumo.com/movies/123")]
[InlineData("https://test.zumo.com/tables", "/api/movies", "123", "https://test.zumo.com/api/movies/123")]
[InlineData("https://test.zumo.com/tables", "/api/movies/", "123", "https://test.zumo.com/api/movies/123")]
[InlineData(null, "https://test.zumo.com/tables/movies", "E0B4E855-4C95-4E9F-8859-581CABB4B66F:5BE95902-65E3-4A21-97B8-D38F36331E8D", "https://test.zumo.com/tables/movies/E0B4E855-4C95-4E9F-8859-581CABB4B66F:5BE95902-65E3-4A21-97B8-D38F36331E8D")]
[InlineData(null, "https://test.zumo.com/tables/movies/", "E0B4E855-4C95-4E9F-8859-581CABB4B66F:5BE95902-65E3-4A21-97B8-D38F36331E8D", "https://test.zumo.com/tables/movies/E0B4E855-4C95-4E9F-8859-581CABB4B66F:5BE95902-65E3-4A21-97B8-D38F36331E8D")]
[InlineData("https://test.zumo.com", "/tables/movies", "E0B4E855-4C95-4E9F-8859-581CABB4B66F:5BE95902-65E3-4A21-97B8-D38F36331E8D", "https://test.zumo.com/tables/movies/E0B4E855-4C95-4E9F-8859-581CABB4B66F:5BE95902-65E3-4A21-97B8-D38F36331E8D")]
[InlineData("https://test.zumo.com", "/tables/movies/", "E0B4E855-4C95-4E9F-8859-581CABB4B66F:5BE95902-65E3-4A21-97B8-D38F36331E8D", "https://test.zumo.com/tables/movies/E0B4E855-4C95-4E9F-8859-581CABB4B66F:5BE95902-65E3-4A21-97B8-D38F36331E8D")]
[InlineData("https://test.zumo.com/", "/tables/movies", "E0B4E855-4C95-4E9F-8859-581CABB4B66F:5BE95902-65E3-4A21-97B8-D38F36331E8D", "https://test.zumo.com/tables/movies/E0B4E855-4C95-4E9F-8859-581CABB4B66F:5BE95902-65E3-4A21-97B8-D38F36331E8D")]
[InlineData("https://test.zumo.com/", "/tables/movies/", "E0B4E855-4C95-4E9F-8859-581CABB4B66F:5BE95902-65E3-4A21-97B8-D38F36331E8D", "https://test.zumo.com/tables/movies/E0B4E855-4C95-4E9F-8859-581CABB4B66F:5BE95902-65E3-4A21-97B8-D38F36331E8D")]
[InlineData("https://test.zumo.com/tables", "movies", "E0B4E855-4C95-4E9F-8859-581CABB4B66F:5BE95902-65E3-4A21-97B8-D38F36331E8D", "https://test.zumo.com/movies/E0B4E855-4C95-4E9F-8859-581CABB4B66F:5BE95902-65E3-4A21-97B8-D38F36331E8D")]
[InlineData("https://test.zumo.com/tables", "movies/", "E0B4E855-4C95-4E9F-8859-581CABB4B66F:5BE95902-65E3-4A21-97B8-D38F36331E8D", "https://test.zumo.com/movies/E0B4E855-4C95-4E9F-8859-581CABB4B66F:5BE95902-65E3-4A21-97B8-D38F36331E8D")]
[InlineData("https://test.zumo.com/tables", "/api/movies", "E0B4E855-4C95-4E9F-8859-581CABB4B66F:5BE95902-65E3-4A21-97B8-D38F36331E8D", "https://test.zumo.com/api/movies/E0B4E855-4C95-4E9F-8859-581CABB4B66F:5BE95902-65E3-4A21-97B8-D38F36331E8D")]
[InlineData("https://test.zumo.com/tables", "/api/movies/", "E0B4E855-4C95-4E9F-8859-581CABB4B66F:5BE95902-65E3-4A21-97B8-D38F36331E8D", "https://test.zumo.com/api/movies/E0B4E855-4C95-4E9F-8859-581CABB4B66F:5BE95902-65E3-4A21-97B8-D38F36331E8D")]
public void MakeAbsoluteUri_WorksWithId(string ba, string bb, string itemId, string expected)
{
Uri arg1 = string.IsNullOrEmpty(ba) ? null : new Uri(ba, UriKind.Absolute);
Uri arg2 = bb.StartsWith("http") ? new Uri(bb, UriKind.Absolute) : new Uri(bb, UriKind.Relative);
Uri actual = ExecutableOperation.MakeAbsoluteUri(arg1, arg2, itemId);

actual.ToString().Should().Be(expected);
}

[Fact]
public void MakeAbsoluteUri_BaseAddressRelative()
{
Expand Down
Loading