DaprClient.CreateInvokeHttpClient - Throw exception if some uppercase letters in appId

[TWEESTY]

Describe the proposal

"DaprClient.CreateInvokeHttpClient" method does not work (misbehavior) if the parameter "appId" contains at least one uppercase letter. The "HttpClient" instance has the "BaseAddress" (which is a URI) built with the "appId" parameter but it is NO case-sensitive.

My proposal is to throw a "ArgumentException" if there is at least one uppercase inside the "appId" string, because it will not working and it will save some time to new users (as could be for me).

A bug has already be opened here #937

Source file

The change will be this :

public static HttpClient CreateInvokeHttpClient(string appId = null, string daprEndpoint = null, string daprApiToken = null)
        {
            var handler = new InvocationHandler()
            {
                InnerHandler = new HttpClientHandler(),
                DaprApiToken = daprApiToken
            };

            if (daprEndpoint is string)
            {
                // DaprEndpoint performs validation.
                handler.DaprEndpoint = daprEndpoint;
            }

            var httpClient = new HttpClient(handler);
            httpClient.DefaultRequestHeaders.UserAgent.Add(UserAgent());

            if (appId is string)
            {

               if(appId.Any(char.IsUpper))
                {
                   throw new ArgumentException("The appId cannot contain an uppercase letter.", nameof(appId));
                }

                try
                {
                    httpClient.BaseAddress = new Uri($"http://{appId}");
                }
                catch (UriFormatException inner)
                {
                    throw new ArgumentException("The appId must be a valid hostname.", nameof(appId), inner);
                }
            }

            return httpClient;
        }

If you agree, I will make a PR.
Thanks all.

[philliphoff]

@TWEESTY If I understand correctly, the issue is that CreateInvokeHttpClient() stores the application ID in the BaseAddress property of the returned HttpClient, and that property is treated case-insensitively when it's pulled back out by the InvocationHandler attached to the HttpClient (because hosts are generally case-insensitive for the purpose of HTTP requests)?

But, looking at the code, the BaseAddress appears used solely for the purpose of storing the application ID. I wonder why we would do that vs. just storing the application ID in a property of the InvocationHandler itself, as we do DaprEndpoint, other than it just seemed convenient at the time. That would seemingly allow application IDs with uppercase letters (which should generally be valid to Dapr).

(I'm also curious as to why the application ID is optional in CreateInvokeHttpClient(), as the code seems to assume it's set.)

[TWEESTY]

@philliphoff : Yes, you understand correctly. You're right, we should add a property AppIdto the InvocationHandler, but we need also to set the BaseAddresswith the "AppId" (if set), else an exception will be thrown if you do this like that (ie with no hostname). httpClient.PostAsJsonAsync($"orders", content);

And no, we can called CreateInvokeHttpClient without the appIdparameter, there is no issue to do that. But in order to send a request, we need to pass all the URI (i.e. with the AppId) like that : httpClient.PostAsJsonAsync($"http://order-processor/orders", content);

If the developer called CreateInvokeHttpClient with a non null string for appId, then it is simple, inside the InvocationHandler this Path = $"/v1.0/invoke/{uri.Host}/method" + uri.AbsolutePath, should be replaced by Path = $"/v1.0/invoke/{AppId}/method" + uri.AbsolutePath,

If the developer called CreateInvokeHttpClient with no appId, then inside the InvocationHandler this Path = $"/v1.0/invoke/{uri.Host}/method" + uri.AbsolutePath, should be replaced by Path = $"/v1.0/invoke/{uri.OriginalString.GetHost()}/method" + uri.AbsolutePath,

If we don't set the AppId when called the CreateInvokeHttpClient, we can use the same HttpClient for several services. It's nice.

I guess there is still one issue, how to deal if the developer changes the httpClient.BaseAddress and the developper set appId (when when called the CreateInvokeHttpClient) => Maybe, the InvocationHandler should not replace the path by AppId if AppId is not equal to the URI Host (comparison no case sensitive).

Do not hesitate if you have some questions.

So, it is clear in my head, I will do a MR.