dapr / dotnet-sdk

Dapr SDK for .NET
Apache License 2.0
1.11k stars 337 forks source link

Allow HttpRequestMessage to accept relative URLs in service invocation #907

Open cgillum opened 2 years ago

cgillum commented 2 years ago

Background

The Dapr SDK allows invoking service methods using an HttpRequestMessage parameter. However, this overload requires a fully qualified URL to the Dapr sidecar. Otherwise, an InvalidOperationException is thrown.

HttpRequestMessage httpRequest = new(input.HttpMethod, input.MethodName);
return await this.daprClient.InvokeMethodAsync<object?>(httpRequest);

Here is the exception:

System.InvalidOperationException
  HResult=0x80131509
  Message=An invalid request URI was provided. Either the request URI must be an absolute URI or BaseAddress must be set.
  Source=System.Net.Http
  StackTrace:
   at System.Net.Http.HttpClient.PrepareRequestMessage(HttpRequestMessage request)
   at System.Net.Http.HttpClient.CheckRequestBeforeSend(HttpRequestMessage request)
   at System.Net.Http.HttpClient.SendAsync(HttpRequestMessage request, HttpCompletionOption completionOption, CancellationToken cancellationToken)
   at System.Net.Http.HttpClient.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
   at Dapr.Client.DaprClientGrpc.<InvokeMethodWithResponseAsync>d__27.MoveNext()
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Dapr.Client.DaprClientGrpc.<InvokeMethodAsync>d__29`1.MoveNext()
    ...
    [Call Stack Truncated]

Generally speaking, code that already has a fully configured DaprClient should not need to provide redundant endpoint information for the Dapr sidecar. In many cases, it may not be practical for code to know what it is - for example, when getting the DaprClient from dependency injection, in which case the configuration comes from the config system.

Proposal

Remove the requirement in InvokeMethodAsync for the HttpRequestMessage parameter to use a fully qualified URL. The DaprClient implementation can construct the real URL itself using its own internally tracked endpoint. This will make this particular method much easier to use.

cgillum commented 2 years ago

Shortly after posting this, I noticed that there's a simple workaround to the problem, demonstrated here.

Here's my updated code, which uses DaprClient.CreateInvokeMethodRequest to create a preconfigured HttpRequestMessage object. It works great and doesn't require my code to know the Dapr sidecar address:

HttpRequestMessage httpRequest = this.daprClient.CreateInvokeMethodRequest(
    input.HttpMethod,
    input.AppId,
    input.MethodName,
    input.Data);

await this.daprClient.InvokeMethodAsync(httpRequest);

I'll leave this open in case the maintainers think it would be value to accept user-provided HttpRequestMessage arguments with relative URLs, but my own issue has been resolved.

rynowak commented 2 years ago

Thanks for the feedback @cgillum

Irrespective of this actual proposal ... I'd recommend using https://github.com/dapr/dotnet-sdk/blob/master/src/Dapr.Client/DaprClient.cs#L75 to create an HttpClient and just using the standard .NET APIs. There's a wealth of existing libraries and knowledge for how to use HttpClient.

cgillum commented 2 years ago

Oh, that's even better! The next problem I needed to tackle was how to customize the parsing of the return value. Going with HttpClient directly will make that much easier. Thanks, @rynowak!

rynowak commented 2 years ago

Oh, that's even better! The next problem I needed to tackle was how to customize the parsing of the return value. Going with HttpClient directly will make that much easier. Thanks, @rynowak!

Glad that this helps!

The APIs you're referring to are meant to support the original Dapr service invocation RPC - which does a lot of tricky stuff. For example, it tries to support interop between gRPC and HTTP - which hasn't been very successful for folks in practice.

I don't think we'll ever remove those APIs and break the people using them, but I definitely would tell new users to start with the HTTP or gRPC proxy support and not the invoke RPC.

cgillum commented 2 years ago

Got it. I have a couple suggestions related to this:

cgillum commented 2 years ago

Oh, one thing I just realized about CreateInvokeHttpClient is that it's static and requires me to provide an App ID and a Dapr endpoint address. โ˜น๏ธ How do I get these values? Currently I'm using dependency injection to get the DaprClient object and I'm not required to explicitly know what these values are. It would be ideal if I could get the HttpClient from the DI-injected DaprClient rather than creating my own from scratch...

rynowak commented 2 years ago

You need the appId of the destination - not the current app. As for the endpoint - how is that configured today? That has to be configured by whomever put the DaprClient in DI - they either are using the defaults or they got this information from somewhere.

So the DaprClient won't help you with that either way.

rynowak commented 2 years ago

@cgillum - if you want to try and explain what you're doing, probably help you better to figure out the best path. Building frameworks is really a different thing than building apps, and it seems like you're building a framework ๐Ÿ˜†

cgillum commented 2 years ago

You need the appId of the destination - not the current app.

Oops, my bad. Disregard my comment about the app ID.

As for the endpoint - how is that configured today?

I'm not doing any explicit configuration of the endpoint. Rather, I'm relying on the default behavior of DaprClient that is given to me by dependency injection.

serviceCollection.AddDaprClient();

Looking at some of the code in the SDK, it's my understanding that the endpoint is inferred using the DAPR_GRPC_PORT environment variable. Ideally, I wouldn't need to redundantly look this up myself if the SDK already knows how to look it up for me.

Building frameworks is really a different thing than building apps, and it seems like you're building a framework

You're not wrong. ๐Ÿ˜‰ However, it seemed to me that the issues I'm running into could be encountered by app developers as well. I believe the patterns I'm following are pretty general. (the "framework" I'm building is actually just a throwaway POC, so these kinds of issues are not blockers, but I thought it might be valuable to raise them anyways).

rynowak commented 2 years ago

Looking at some of the code in the SDK, it's my understanding that the endpoint is inferred using the DAPR_GRPC_PORT environment variable. Ideally, I wouldn't need to redundantly look this up myself if the SDK already knows how to look it up for me.

This is the case, if you omit the endpoint argument then the SDK will do it for you.

You're not wrong. ๐Ÿ˜‰ However, it seemed to me that the issues I'm running into could be encountered by app developers as well. I believe the patterns I'm following are pretty general. (the "framework" I'm building is actually just a throwaway POC, so these kinds of issues are not blockers, but I thought it might be valuable to raise them anyways).

Alright cool ๐Ÿ˜† - feel free to reach out if you want to spend some time talking in details.

ericsampson commented 1 year ago

I don't think we'll ever remove those APIs and break the people using them, but I definitely would tell new users to start with the HTTP or gRPC proxy support and not the invoke RPC.

Could they be marked with the [Obsolete] attribute to help guide people into the pit of success?