Skip to content
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

Jump to bottom

Small refactoring on dapr client and invocation handler. Add improvements on test structure. #1379

Closed
wants to merge 3 commits into from
Closed

Small refactoring on dapr client and invocation handler. Add improvements on test structure. #1379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4b51d03
Small refactoring on dapr client and invocation handler. Add improvem…
RafaelJCamara Oct 24, 2024
ac635b3
Revert namespace to block scoped namespace.
RafaelJCamara Oct 24, 2024
9903a1d
Merge branch 'master' into improvements-dapr-client
RafaelJCamara Oct 24, 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
16 changes: 8 additions & 8 deletions src/Dapr.Client/DaprClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,14 +78,14 @@ public abstract class DaprClient : IDisposable
/// </remarks>
public static HttpClient CreateInvokeHttpClient(string appId = null, string daprEndpoint = null, string daprApiToken = null)
{
var handler = new InvocationHandler()
var handler = new InvocationHandler
{
InnerHandler = new HttpClientHandler(),
DaprApiToken = daprApiToken,
DefaultAppId = appId,
DefaultAppId = appId
};

if (daprEndpoint is string)
if (daprEndpoint is not null)
{
// DaprEndpoint performs validation.
handler.DaprEndpoint = daprEndpoint;
Expand All @@ -94,7 +94,7 @@ public static HttpClient CreateInvokeHttpClient(string appId = null, string dapr
var httpClient = new HttpClient(handler);
httpClient.DefaultRequestHeaders.UserAgent.Add(UserAgent());

if (appId is string)
if (appId is not null)
{
try
{
Expand Down Expand Up @@ -703,7 +703,7 @@ public abstract Task InvokeMethodGrpcAsync<TRequest>(
string methodName,
TRequest data,
CancellationToken cancellationToken = default)
where TRequest : IMessage;
where TRequest : IMessage;

/// <summary>
/// Perform service invocation using gRPC semantics for the application identified by <paramref name="appId" /> and invokes the method
Expand All @@ -720,7 +720,7 @@ public abstract Task<TResponse> InvokeMethodGrpcAsync<TResponse>(
string appId,
string methodName,
CancellationToken cancellationToken = default)
where TResponse : IMessage, new();
where TResponse : IMessage, new();

/// <summary>
/// Perform service invocation using gRPC semantics for the application identified by <paramref name="appId" /> and invokes the method
Expand All @@ -740,8 +740,8 @@ public abstract Task<TResponse> InvokeMethodGrpcAsync<TRequest, TResponse>(
string methodName,
TRequest data,
CancellationToken cancellationToken = default)
where TRequest : IMessage
where TResponse : IMessage, new();
where TRequest : IMessage
where TResponse : IMessage, new();

/// <summary>
/// Gets the current value associated with the <paramref name="key" /> from the Dapr state store.
Expand Down
67 changes: 35 additions & 32 deletions src/Dapr.Client/InvocationHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

using System;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Net.Http;
using System.Threading;
using System.Threading.Tasks;
Expand Down Expand Up @@ -44,6 +45,7 @@ public class InvocationHandler : DelegatingHandler
{
private Uri parsedEndpoint;
private string? apiToken;
private readonly string[] supportedUriSchemes = { "http", "https" };

/// <summary>
/// Initializes a new instance of <see cref="InvocationHandler" />.
Expand All @@ -60,22 +62,18 @@ public InvocationHandler()
/// <returns>The URI of the Dapr HTTP endpoint used for service invocation.</returns>
public string DaprEndpoint
{
get
{
return this.parsedEndpoint.OriginalString;
}
get => this.parsedEndpoint.OriginalString;

set
{
if (value == null)
{
throw new ArgumentNullException(nameof(value));
}
ArgumentNullException.ThrowIfNull(value);

// This will throw a reasonable exception if the URI is invalid.
var uri = new Uri(value, UriKind.Absolute);
if (uri.Scheme != "http" && uri.Scheme != "https")

if (!IsUriSchemeSupported(uri.Scheme))
{
throw new ArgumentException("The URI scheme of the Dapr endpoint must be http or https.", "value");
throw new ArgumentException("The URI scheme of the Dapr endpoint must be http or https.", nameof(value));
}

this.parsedEndpoint = uri;
Expand All @@ -98,63 +96,68 @@ internal string? DaprApiToken
/// <inheritdoc />
protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
{
var original = request.RequestUri;
if (!this.TryRewriteUri(request.RequestUri, out var rewritten))
var originalUri = request.RequestUri;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm disinclined to accept this change which only changes the inline variable. If you're rewriting the method to do something and you need to change the variable so it's more meaningful, by all means, but to change it for the sake of changing it isn't necessary.


if (!this.TryRewriteUri(request.RequestUri, out var rewrittenUri))
{
throw new ArgumentException($"The request URI '{original}' is not a valid Dapr service invocation destination.", nameof(request));
throw new ArgumentException($"The request URI '{originalUri}' is not a valid Dapr service invocation destination.", nameof(request));
}

try
{
var apiTokenHeader = DaprClient.GetDaprApiTokenHeader(this.apiToken);

if (apiTokenHeader is not null)
{
request.Headers.Add(apiTokenHeader.Value.Key, apiTokenHeader.Value.Value);
}
request.RequestUri = rewritten;

request.RequestUri = rewrittenUri;

return await base.SendAsync(request, cancellationToken);
}
finally
{
request.RequestUri = original;
request.RequestUri = originalUri;
request.Headers.Remove("dapr-api-token");
}
}

// Internal for testing
internal bool TryRewriteUri(Uri? uri, [NotNullWhen(true)] out Uri? rewritten)
internal bool TryRewriteUri(Uri? uri, [NotNullWhen(true)] out Uri? rewrittenUri)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing - I'm disinclined to accept this change that renames the argument just because you can. Again, if you're changing the method itself and need to change the name so it's useful in your rewrite, that's one thing, but changing it just for the sake of doing so here, especially for something that's strictly internal for testing purposes, doesn't make sense.

{
// For now the only invalid cases are when the request URI is missing or just silly.
// We may support additional cases for validation in the future (like an allow-list of App-Ids).
if (uri is null || !uri.IsAbsoluteUri || (uri.Scheme != "http" && uri.Scheme != "https"))
if (!IsUriValid(uri))
{
// do nothing
rewritten = null;
rewrittenUri = null;
return false;
}

string host;

if (this.DefaultAppId is not null && uri.Host.Equals(this.DefaultAppId, StringComparison.InvariantCultureIgnoreCase))
{
host = this.DefaultAppId;
}
else
{
host = uri.Host;
}
string host = UriHostContainsValidDefaultAppId(uri!.Host) ? this.DefaultAppId! : uri!.Host;

var builder = new UriBuilder(uri)
{
Scheme = this.parsedEndpoint.Scheme,
Host = this.parsedEndpoint.Host,
Port = this.parsedEndpoint.Port,
Path = $"/v1.0/invoke/{host}/method" + uri.AbsolutePath,
Path = $"/v1.0/invoke/{host}/method" + uri.AbsolutePath
};

rewritten = builder.Uri;
rewrittenUri = builder.Uri;
return true;
}

private bool IsUriSchemeSupported(string uriScheme)
=> supportedUriSchemes.Contains(uriScheme, StringComparer.InvariantCultureIgnoreCase);

// For now the only invalid cases are when the request URI is missing or just silly.
// We may support additional cases for validation in the future (like an allow-list of App-Ids).
private bool IsUriValid(Uri? uri)
=> uri is not null && uri.IsAbsoluteUri && IsUriSchemeSupported(uri.Scheme);

private bool UriHostContainsValidDefaultAppId(string uriHost)
=> this.DefaultAppId is not null &&
uriHost.Equals(this.DefaultAppId, StringComparison.InvariantCultureIgnoreCase);

}
}
23 changes: 5 additions & 18 deletions test/Dapr.Client.Test/DaprClientTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,7 @@ public void CreateInvokeHttpClient_WithAppId()
[Fact]
public void CreateInvokeHttpClient_InvalidAppId()
{
var ex = Assert.Throws<ArgumentException>(() =>
{
// The appId needs to be something that can be used as hostname in a URI.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's useful to retain this comment in the test as it provides more context to the test name and what it's covering

_ = DaprClient.CreateInvokeHttpClient(appId: "");
});
var ex = Assert.Throws<ArgumentException>(() => DaprClient.CreateInvokeHttpClient(appId: string.Empty));

Assert.Contains("The appId must be a valid hostname.", ex.Message);
Assert.IsType<UriFormatException>(ex.InnerException);
Expand All @@ -49,29 +45,23 @@ public void CreateInvokeHttpClient_WithoutAppId()
[Fact]
public void CreateInvokeHttpClient_InvalidDaprEndpoint_InvalidFormat()
{
Assert.Throws<UriFormatException>(() =>
{
_ = DaprClient.CreateInvokeHttpClient(daprEndpoint: "");
});
Assert.Throws<UriFormatException>(() => DaprClient.CreateInvokeHttpClient(daprEndpoint: string.Empty));

// Exception message comes from the runtime, not validating it here.
}

[Fact]
public void CreateInvokeHttpClient_InvalidDaprEndpoint_InvalidScheme()
{
var ex = Assert.Throws<ArgumentException>(() =>
{
_ = DaprClient.CreateInvokeHttpClient(daprEndpoint: "ftp://localhost:3500");
});
var ex = Assert.Throws<ArgumentException>(() => DaprClient.CreateInvokeHttpClient(daprEndpoint: "ftp://localhost:3500"));

Assert.Contains("The URI scheme of the Dapr endpoint must be http or https.", ex.Message);
}

[Fact]
public void GetDaprApiTokenHeader_ApiTokenSet_SetsApiTokenHeader()
{
var token = "test_token";
const string token = "test_token";
var entry = DaprClient.GetDaprApiTokenHeader(token);
Assert.NotNull(entry);
Assert.Equal("test_token", entry.Value.Value);
Expand All @@ -89,10 +79,7 @@ public async Task TestShutdownApi()
{
await using var client = TestClient.CreateForDaprClient();

var request = await client.CaptureGrpcRequestAsync(async daprClient =>
{
await daprClient.ShutdownSidecarAsync();
});
var request = await client.CaptureGrpcRequestAsync(async daprClient => await daprClient.ShutdownSidecarAsync());

request.Dismiss();
}
Expand Down
69 changes: 18 additions & 51 deletions test/Dapr.Client.Test/InvocationHandlerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
using System.Reflection;
using System.Threading;
using System.Threading.Tasks;
using Dapr.Client.Test.TestData;
using Xunit;

#nullable enable
Expand All @@ -30,10 +31,8 @@ public class InvocationHandlerTests
public void DaprEndpoint_InvalidScheme()
{
var handler = new InvocationHandler();
var ex = Assert.Throws<ArgumentException>(() =>
{
handler.DaprEndpoint = "ftp://localhost:3500";
});

var ex = Assert.Throws<ArgumentException>(() => handler.DaprEndpoint = "ftp://localhost:3500");

Assert.Contains("The URI scheme of the Dapr endpoint must be http or https.", ex.Message);
}
Expand All @@ -42,13 +41,18 @@ public void DaprEndpoint_InvalidScheme()
public void DaprEndpoint_InvalidUri()
{
var handler = new InvocationHandler();
Assert.Throws<UriFormatException>(() =>
{
handler.DaprEndpoint = "";
});

Assert.Throws<UriFormatException>(() => handler.DaprEndpoint = string.Empty);

// Exception message comes from the runtime, not validating it here
}

[Fact]
public void DaprEndpoint_NullDaprEndpoint()
{
var handler = new InvocationHandler();
Assert.Throws<ArgumentNullException>(() => handler.DaprEndpoint = default!);
}

[Fact]
public void TryRewriteUri_FailsForNullUri()
Expand Down Expand Up @@ -79,47 +83,10 @@ public void TryRewriteUri_FailsForRelativeUris()
}

[Theory]
[InlineData(null, "http://bank", "https://some.host:3499/v1.0/invoke/bank/method/")]
Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree with removing all these tests - again, they're each verifying a unique and separate variation of the inputs that is worth knowing is covered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not removing them, I just placed them in the class data TryRewriteUriWithNoAppIdRewritesUriToDaprInvokeTestData and all of them are being called here.

The reason why I've done this was to make this testing class a bit more cleaner.

If you don't agree with moving the test data to that class, I can revert this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which I've now seen you want to revert back to the attributes.

[InlineData("bank", "http://bank", "https://some.host:3499/v1.0/invoke/bank/method/")]
[InlineData("Bank", "http://bank", "https://some.host:3499/v1.0/invoke/Bank/method/")]
[InlineData("invalid", "http://bank", "https://some.host:3499/v1.0/invoke/bank/method/")]
[InlineData(null, "http://Bank", "https://some.host:3499/v1.0/invoke/bank/method/")]
[InlineData("Bank", "http://Bank", "https://some.host:3499/v1.0/invoke/Bank/method/")]
[InlineData("bank", "http://Bank", "https://some.host:3499/v1.0/invoke/bank/method/")]
[InlineData("invalid", "http://Bank", "https://some.host:3499/v1.0/invoke/bank/method/")]
[InlineData(null, "http://bank:3939", "https://some.host:3499/v1.0/invoke/bank/method/")]
[InlineData("bank", "http://bank:3939", "https://some.host:3499/v1.0/invoke/bank/method/")]
[InlineData("invalid", "http://bank:3939", "https://some.host:3499/v1.0/invoke/bank/method/")]
[InlineData(null, "http://Bank:3939", "https://some.host:3499/v1.0/invoke/bank/method/")]
[InlineData("Bank", "http://Bank:3939", "https://some.host:3499/v1.0/invoke/Bank/method/")]
[InlineData("invalid", "http://Bank:3939", "https://some.host:3499/v1.0/invoke/bank/method/")]
[InlineData(null, "http://app-id.with.dots", "https://some.host:3499/v1.0/invoke/app-id.with.dots/method/")]
[InlineData("app-id.with.dots", "http://app-id.with.dots", "https://some.host:3499/v1.0/invoke/app-id.with.dots/method/")]
[InlineData("invalid", "http://app-id.with.dots", "https://some.host:3499/v1.0/invoke/app-id.with.dots/method/")]
[InlineData(null, "http://App-id.with.dots", "https://some.host:3499/v1.0/invoke/app-id.with.dots/method/")]
[InlineData("App-id.with.dots", "http://App-id.with.dots", "https://some.host:3499/v1.0/invoke/App-id.with.dots/method/")]
[InlineData("invalid", "http://App-id.with.dots", "https://some.host:3499/v1.0/invoke/app-id.with.dots/method/")]
[InlineData(null, "http://bank:3939/", "https://some.host:3499/v1.0/invoke/bank/method/")]
[InlineData("bank", "http://bank:3939/", "https://some.host:3499/v1.0/invoke/bank/method/")]
[InlineData("invalid", "http://bank:3939/", "https://some.host:3499/v1.0/invoke/bank/method/")]
[InlineData(null, "http://Bank:3939/", "https://some.host:3499/v1.0/invoke/bank/method/")]
[InlineData("Bank", "http://Bank:3939/", "https://some.host:3499/v1.0/invoke/Bank/method/")]
[InlineData("invalid", "http://Bank:3939/", "https://some.host:3499/v1.0/invoke/bank/method/")]
[InlineData(null, "http://bank:3939/some/path", "https://some.host:3499/v1.0/invoke/bank/method/some/path")]
[InlineData("bank", "http://bank:3939/some/path", "https://some.host:3499/v1.0/invoke/bank/method/some/path")]
[InlineData("invalid", "http://bank:3939/some/path", "https://some.host:3499/v1.0/invoke/bank/method/some/path")]
[InlineData(null, "http://Bank:3939/some/path", "https://some.host:3499/v1.0/invoke/bank/method/some/path")]
[InlineData("Bank", "http://Bank:3939/some/path", "https://some.host:3499/v1.0/invoke/Bank/method/some/path")]
[InlineData("invalid", "http://Bank:3939/some/path", "https://some.host:3499/v1.0/invoke/bank/method/some/path")]
[InlineData(null, "http://bank:3939/some/path?q=test&p=another#fragment", "https://some.host:3499/v1.0/invoke/bank/method/some/path?q=test&p=another#fragment")]
[InlineData("bank", "http://bank:3939/some/path?q=test&p=another#fragment", "https://some.host:3499/v1.0/invoke/bank/method/some/path?q=test&p=another#fragment")]
[InlineData("invalid", "http://bank:3939/some/path?q=test&p=another#fragment", "https://some.host:3499/v1.0/invoke/bank/method/some/path?q=test&p=another#fragment")]
[InlineData(null, "http://Bank:3939/some/path?q=test&p=another#fragment", "https://some.host:3499/v1.0/invoke/bank/method/some/path?q=test&p=another#fragment")]
[InlineData("Bank", "http://Bank:3939/some/path?q=test&p=another#fragment", "https://some.host:3499/v1.0/invoke/Bank/method/some/path?q=test&p=another#fragment")]
[InlineData("invalid", "http://Bank:3939/some/path?q=test&p=another#fragment", "https://some.host:3499/v1.0/invoke/bank/method/some/path?q=test&p=another#fragment")]
[ClassData(typeof(TryRewriteUriWithNoAppIdRewritesUriToDaprInvokeTestData))]
public void TryRewriteUri_WithNoAppId_RewritesUriToDaprInvoke(string? appId, string uri, string expected)
{
var handler = new InvocationHandler()
var handler = new InvocationHandler
{
DaprEndpoint = "https://some.host:3499",
DefaultAppId = appId,
Expand All @@ -144,10 +111,10 @@ public async Task SendAsync_InvalidNotSetUri_ThrowsException()
[Fact]
public async Task SendAsync_RewritesUri()
{
var uri = "http://bank/accounts/17?";
const string uri = "http://bank/accounts/17?";

var capture = new CaptureHandler();
var handler = new InvocationHandler()
var handler = new InvocationHandler
{
InnerHandler = capture,

Expand All @@ -171,7 +138,7 @@ public async Task SendAsync_RewritesUri_AndAppId()
var uri = "http://bank/accounts/17?";

var capture = new CaptureHandler();
var handler = new InvocationHandler()
var handler = new InvocationHandler
{
InnerHandler = capture,

Expand All @@ -196,7 +163,7 @@ public async Task SendAsync_RewritesUri_AndAddsApiToken()
var uri = "http://bank/accounts/17?";

var capture = new CaptureHandler();
var handler = new InvocationHandler()
var handler = new InvocationHandler
{
InnerHandler = capture,

Expand Down
Loading
Loading