Botbuilder-dotnet: Remove comment only ProductInfoHeaderValue

[EricDahlvang]

Version

all, but particularly 4.11.+

Describe the bug

In 4.11, we have added a constructor to ConnectorClient which takes an HttpClient. We then pass that HttpClient down to the underlying ServicClient's constructor. Due to the fact that we have a ProductInfoHeaderValue which has no Product (comment only), the underlying ServiceClient will throw and catch 4 exceptions.

I've created a PR to address this within ServiceClient, but they have recommended we remove the comment only ProductInfoHeaderValue:
https://github.com/Azure/azure-sdk-for-net/pull/16252

To Reproduce

Steps to reproduce the behavior:
Run this test:

        [Fact]
        public void ConnectorClient_CustomHttpClient_UserAgentHeaderAdds()
        {
            var httpClient = new HttpClient();
            ConnectorClient.AddDefaultRequestHeaders(httpClient);

            using (var connector = new ConnectorClient(new Uri("http://localhost/"), new MicrosoftAppCredentials(string.Empty, string.Empty), customHttpClient: httpClient, disposeHttpClient: false))
            {
                // Use the connector
            }

            Assert.Equal(6, httpClient.DefaultRequestHeaders.UserAgent.Count);
        }

Notice, none of the ServiceClient UserAgent headers have been added.

Expected behavior

Underlying ServiceClient UserAgent headers are added.

Remove the comment only product info header, and re-run the above test. It will pass.
https://github.com/microsoft/botbuilder-dotnet/blob/main/libraries/Microsoft.Bot.Connector/ConnectorClientEx.cs#L200

[EricDahlvang]

The recommendation for anyone seeing a null reference exception due to this is to ignore.

[sebnema]

After I upgraded VS2019 Version 16.8.2 and .NetCore3.1 version, I started seeing this error. What I actually recognized is the error was related to Microsoft.Rest.ServiceClient and even earlier versions was giving the error. But somehow suppressed.
I dig into the reason for couple of days. Here is what I found.

Null reference error is thrown on AddUserAgentEntry method in Microsoft.Rest.ServiceClient (I tried for both Microsoft.Rest.ClientRuntime.dll v2.3.20 and v2.3.21). The caller method SetUserAgent has a try catch around it. So the error is suppressed and only thrown in debug mode.
The Stack trace of the error is " at Microsoft.Rest.ServiceClient`1.<>c.b__55_0(ProductInfoHeaderValue left, ProductInfoHeaderValue right)"
Despite of "left.Product" returned null, I found out the root cause of the error is HttpClient.DefaultRequestHeaders.UserAgent field which is filled in AddDefaultRequestHeaders method in Microsoft.Bot.Connector.ConnectorClient.

       private void AddUserAgentEntry(ProductInfoHeaderValue pInfoHeaderValue)
        {
            lock (lockUserAgent)
            {
                if (!HttpClient.DefaultRequestHeaders.UserAgent.Contains(pInfoHeaderValue, new ObjectComparer<ProductInfoHeaderValue>((ProductInfoHeaderValue left, ProductInfoHeaderValue right) => left.Product.Name.Equals(right.Product.Name, StringComparison.OrdinalIgnoreCase))))
                {
                    HttpClient.DefaultRequestHeaders.UserAgent.Add(pInfoHeaderValue);
                }
            }
        }

In my case, the UserAgent collection has these values --> "Microsoft-BotFramework/3.1" , "BotBuilder/4.0.0.0",
"(.NETCoreApp,Version=v3.1; Microsoft Windows 10.0.18363; X64)"

In the code below, ProductInfoHeaderValue parsedValue is not parsed correctly for this value "(.NETCoreApp,Version=v3.1; Microsoft Windows 10.0.18363; X64)". So the Product field becomes null.

 public static void AddDefaultRequestHeaders(HttpClient httpClient)
        {
            lock (httpClient)
            {
                ProductInfoHeaderValue item = new ProductInfoHeaderValue("Microsoft-BotFramework", "3.1");
                if (!httpClient.DefaultRequestHeaders.UserAgent.Contains(item))
                {
                    httpClient.DefaultRequestHeaders.UserAgent.Add(item);
                }
                ProductInfoHeaderValue item2 = new ProductInfoHeaderValue("BotBuilder", GetClientVersion());
                if (!httpClient.DefaultRequestHeaders.UserAgent.Contains(item2))
                {
                    httpClient.DefaultRequestHeaders.UserAgent.Add(item2);
                }
                if (ProductInfoHeaderValue.TryParse("(" + GetASPNetVersion() + "; " + GetOsVersion() + "; " + GetArchitecture() + ")", out ProductInfoHeaderValue parsedValue) && !httpClient.DefaultRequestHeaders.UserAgent.Contains(parsedValue))
                {
                    httpClient.DefaultRequestHeaders.UserAgent.Add(parsedValue);
                }
                httpClient.DefaultRequestHeaders.ExpectContinue = false;
            }
        }

I can provide more info if you need.

[EricDahlvang]

Hi @sebnema

I'm sorry you've spent time investigating this. The root cause is a known issue. You are correct, that the problem is in ServiceClient and has been present for years now. They are apprehensive to take a fix at this level of the core product: https://github.com/Azure/azure-sdk-for-net/pull/16252

The reason the Bot Builder sdk has not had this issue until 4.11.0 is because we were not using the ServiceClient constructor with HttpClient param, until 4.11.0: https://github.com/microsoft/botbuilder-dotnet/commit/918c54778e0c0e48187560661f208e034ceffb31#diff-914b91edd6e7e20fb74559dea5488a0b0ee7d63a197cd703135fb1adf44d079a

Our plan to resolve this in 4.12.0 is to replace the comment only ProductInfoHeaderValue in the Bot Builder SDK with one containing a product.

[sebnema]

Thanks @EricDahlvang.