Change how HttpConnection sets headers (#1806)

- Removed SendUtils.PrepareRequst and instead used HttpClient.DefaultRequstHeaders to set the common headers to apply HttpOptions to all outbound requests
- Modified how we check for the user agent request testing

src/Microsoft.AspNetCore.Http.Connections.Client/DefaultTransportFactory.cs

@@ -52,12 +52,12 @@ namespace Microsoft.AspNetCore.Http.Connections.Client
 
             if ((availableServerTransports & TransportType.ServerSentEvents & _requestedTransportType) == TransportType.ServerSentEvents)
             {
-                return new ServerSentEventsTransport(_httpClient, _httpOptions, _loggerFactory);
+                return new ServerSentEventsTransport(_httpClient, _loggerFactory);
             }
 
             if ((availableServerTransports & TransportType.LongPolling & _requestedTransportType) == TransportType.LongPolling)
             {
-                return new LongPollingTransport(_httpClient, _httpOptions, _loggerFactory);
+                return new LongPollingTransport(_httpClient, _loggerFactory);
             }
 
             throw new InvalidOperationException("No requested transports available on the server.");

src/Microsoft.AspNetCore.Http.Connections.Client/HttpConnection.cs

@@ -293,7 +293,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Client
                 {
                     // Corefx changed the default version and High Sierra curlhandler tries to upgrade request
                     request.Version = new Version(1, 1);
-                    SendUtils.PrepareHttpRequest(request, _httpOptions);
 
                     // ResponseHeadersRead instructs SendAsync to return once headers are read
                     // rather than buffer the entire response. This gives a small perf boost.
@@ -402,6 +401,27 @@ namespace Microsoft.AspNetCore.Http.Connections.Client
             var httpClient = new HttpClient(httpMessageHandler);
             httpClient.Timeout = HttpClientTimeout;
 
+            // Start with the user agent header
+            httpClient.DefaultRequestHeaders.UserAgent.Add(Constants.UserAgentHeader);
+
+            if (_httpOptions != null)
+            {
+                // Apply any headers configured on the HttpOptions
+                if (_httpOptions.Headers != null)
+                {
+                    foreach (var header in _httpOptions.Headers)
+                    {
+                        httpClient.DefaultRequestHeaders.Add(header.Key, header.Value);
+                    }
+                }
+
+                // Apply the authorization header
+                if (_httpOptions.AccessTokenFactory != null)
+                {
+                    httpClient.DefaultRequestHeaders.Add("Authorization", $"Bearer {_httpOptions.AccessTokenFactory()}");
+                }
+            }
+
             return httpClient;
         }
 

src/Microsoft.AspNetCore.Http.Connections.Client/Internal/LongPollingTransport.cs

@@ -18,7 +18,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Client.Internal
     public partial class LongPollingTransport : ITransport
     {
         private readonly HttpClient _httpClient;
-        private readonly HttpOptions _httpOptions;
         private readonly ILogger _logger;
         private IDuplexPipe _application;
         // Volatile so that the poll loop sees the updated value set from a different thread
@@ -29,13 +28,12 @@ namespace Microsoft.AspNetCore.Http.Connections.Client.Internal
         public Task Running { get; private set; } = Task.CompletedTask;
 
         public LongPollingTransport(HttpClient httpClient)
-            : this(httpClient, null, null)
+            : this(httpClient, null)
         { }
 
-        public LongPollingTransport(HttpClient httpClient, HttpOptions httpOptions, ILoggerFactory loggerFactory)
+        public LongPollingTransport(HttpClient httpClient, ILoggerFactory loggerFactory)
         {
             _httpClient = httpClient;
-            _httpOptions = httpOptions;
             _logger = (loggerFactory ?? NullLoggerFactory.Instance).CreateLogger<LongPollingTransport>();
         }
 
@@ -61,7 +59,7 @@ namespace Microsoft.AspNetCore.Http.Connections.Client.Internal
         {
             // Start sending and polling (ask for binary if the server supports it)
             var receiving = Poll(url, _transportCts.Token);
-            var sending = SendUtils.SendMessages(url, _application, _httpClient, _httpOptions, _logger);
+            var sending = SendUtils.SendMessages(url, _application, _httpClient, _logger);
 
             // Wait for send or receive to complete
             var trigger = await Task.WhenAny(receiving, sending);
@@ -115,7 +113,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Client.Internal
                 while (!cancellationToken.IsCancellationRequested)
                 {
                     var request = new HttpRequestMessage(HttpMethod.Get, pollUrl);
-                    SendUtils.PrepareHttpRequest(request, _httpOptions);
 
                     HttpResponseMessage response;
 

src/Microsoft.AspNetCore.Http.Connections.Client/Internal/ServerSentEventsTransport.cs

@@ -16,7 +16,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Client.Internal
     public partial class ServerSentEventsTransport : ITransport
     {
         private readonly HttpClient _httpClient;
-        private readonly HttpOptions _httpOptions;
         private readonly ILogger _logger;
 
         // Volatile so that the SSE loop sees the updated value set from a different thread
@@ -29,10 +28,10 @@ namespace Microsoft.AspNetCore.Http.Connections.Client.Internal
         public Task Running { get; private set; } = Task.CompletedTask;
 
         public ServerSentEventsTransport(HttpClient httpClient)
-            : this(httpClient, null, null)
+            : this(httpClient, null)
         { }
 
-        public ServerSentEventsTransport(HttpClient httpClient, HttpOptions httpOptions, ILoggerFactory loggerFactory)
+        public ServerSentEventsTransport(HttpClient httpClient, ILoggerFactory loggerFactory)
         {
             if (httpClient == null)
             {
@@ -40,7 +39,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Client.Internal
             }
 
             _httpClient = httpClient;
-            _httpOptions = httpOptions;
             _logger = (loggerFactory ?? NullLoggerFactory.Instance).CreateLogger<ServerSentEventsTransport>();
         }
 
@@ -66,7 +64,7 @@ namespace Microsoft.AspNetCore.Http.Connections.Client.Internal
         {
             // Start sending and polling (ask for binary if the server supports it)
             var receiving = OpenConnection(_application, url, startTcs, _transportCts.Token);
-            var sending = SendUtils.SendMessages(url, _application, _httpClient, _httpOptions, _logger);
+            var sending = SendUtils.SendMessages(url, _application, _httpClient, _logger);
 
             // Wait for send or receive to complete
             var trigger = await Task.WhenAny(receiving, sending);
@@ -97,7 +95,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Client.Internal
             Log.StartReceive(_logger);
 
             var request = new HttpRequestMessage(HttpMethod.Get, url);
-            SendUtils.PrepareHttpRequest(request, _httpOptions);
             request.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("text/event-stream"));
 
             HttpResponseMessage response = null;

src/Microsoft.AspNetCore.Http.Connections.Client/SendUtils.cs

@@ -15,8 +15,7 @@ namespace Microsoft.AspNetCore.Http.Connections.Client
 {
     internal static class SendUtils
     {
-        public static async Task SendMessages(Uri sendUrl, IDuplexPipe application, HttpClient httpClient,
+        public static async Task SendMessages(Uri sendUrl, IDuplexPipe application, HttpClient httpClient, ILogger logger)
-            HttpOptions httpOptions, ILogger logger)
         {
             Log.SendStarted(logger);
 
@@ -43,7 +42,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Client
                             var request = new HttpRequestMessage(HttpMethod.Post, sendUrl);
                             // Corefx changed the default version and High Sierra curlhandler tries to upgrade request
                             request.Version = new Version(1, 1);
-                            PrepareHttpRequest(request, httpOptions);
 
                             request.Content = new ReadOnlySequenceContent(buffer);
 
@@ -90,23 +88,6 @@ namespace Microsoft.AspNetCore.Http.Connections.Client
             Log.SendStopped(logger);
         }
 
-        public static void PrepareHttpRequest(HttpRequestMessage request, HttpOptions httpOptions)
-        {
-            if (httpOptions?.Headers != null)
-            {
-                foreach (var header in httpOptions.Headers)
-                {
-                    request.Headers.Add(header.Key, header.Value);
-                }
-            }
-            request.Headers.UserAgent.Add(Constants.UserAgentHeader);
-
-            if (httpOptions?.AccessTokenFactory != null)
-            {
-                request.Headers.Add("Authorization", $"Bearer {httpOptions.AccessTokenFactory()}");
-            }
-        }
-
         private class ReadOnlySequenceContent : HttpContent
         {
             private readonly ReadOnlySequence<byte> _buffer;

test/Microsoft.AspNetCore.SignalR.Client.Tests/HttpConnectionTests.Helpers.cs

@@ -9,21 +9,23 @@ using Microsoft.AspNetCore.Http.Connections.Client;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Logging.Abstractions;
 
+using HttpTransportType = Microsoft.AspNetCore.Http.Connections.TransportType;
+
 namespace Microsoft.AspNetCore.SignalR.Client.Tests
 {
     public partial class HttpConnectionTests
     {
-        private static HttpConnection CreateConnection(HttpMessageHandler httpHandler = null, ILoggerFactory loggerFactory = null, string url = null, ITransport transport = null, ITransportFactory transportFactory = null)
+        private static HttpConnection CreateConnection(HttpMessageHandler httpHandler = null, ILoggerFactory loggerFactory = null, string url = null, ITransport transport = null, ITransportFactory transportFactory = null, HttpTransportType transportType = HttpTransportType.LongPolling)
         {
             var httpOptions = new HttpOptions()
             {
                 HttpMessageHandler = (httpMessageHandler) => httpHandler ?? TestHttpMessageHandler.CreateDefault(),
             };
 
-            return CreateConnection(httpOptions, loggerFactory, url, transport, transportFactory);
+            return CreateConnection(httpOptions, loggerFactory, url, transport, transportFactory, transportType);
         }
 
-        private static HttpConnection CreateConnection(HttpOptions httpOptions, ILoggerFactory loggerFactory = null, string url = null, ITransport transport = null, ITransportFactory transportFactory = null)
+        private static HttpConnection CreateConnection(HttpOptions httpOptions, ILoggerFactory loggerFactory = null, string url = null, ITransport transport = null, ITransportFactory transportFactory = null, HttpTransportType transportType = HttpTransportType.LongPolling)
         {
             loggerFactory = loggerFactory ?? NullLoggerFactory.Instance;
             var uri = new Uri(url ?? "http://fakeuri.org/");
@@ -38,7 +40,7 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests
             }
             else
             {
-                return new HttpConnection(uri, TransportType.LongPolling, loggerFactory, httpOptions);
+                return new HttpConnection(uri, transportType, loggerFactory, httpOptions);
             }
         }
 

test/Microsoft.AspNetCore.SignalR.Client.Tests/HttpConnectionTests.Transport.cs

@@ -5,18 +5,69 @@ using System;
 using System.IO.Pipelines;
 using System.Net;
 using System.Net.Http;
+using System.Reflection;
 using System.Text;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Connections;
+using Microsoft.AspNetCore.Http.Connections;
 using Microsoft.AspNetCore.Http.Connections.Client;
 using Xunit;
 
+using HttpTransportType = Microsoft.AspNetCore.Http.Connections.TransportType;
+
 namespace Microsoft.AspNetCore.SignalR.Client.Tests
 {
     public partial class HttpConnectionTests
     {
         public class Transport
         {
+            [Theory]
+            [InlineData(HttpTransportType.LongPolling)]
+            [InlineData(HttpTransportType.ServerSentEvents)]
+            public async Task HttpConnectionSetsUserAgentOnAllRequests(HttpTransportType transportType)
+            {
+                var testHttpHandler = new TestHttpMessageHandler(autoNegotiate: false);
+                var requestsExecuted = false;
+
+                testHttpHandler.OnRequest((request, next, token) =>
+                {
+                    return Task.FromResult(ResponseUtils.CreateResponse(HttpStatusCode.NoContent));
+                });
+
+                testHttpHandler.OnNegotiate((_, cancellationToken) =>
+                {
+                    return ResponseUtils.CreateResponse(HttpStatusCode.OK, ResponseUtils.CreateNegotiationContent());
+                });
+
+                testHttpHandler.OnRequest(async (request, next, token) =>
+                {
+                    var userAgentHeaderCollection = request.Headers.UserAgent;
+                    var userAgentHeader = Assert.Single(userAgentHeaderCollection);
+                    Assert.Equal("Microsoft.AspNetCore.Http.Connections.Client", userAgentHeader.Product.Name);
+
+                    // user agent version should come from version embedded in assembly metadata
+                    var assemblyVersion = typeof(Constants)
+                            .Assembly
+                            .GetCustomAttribute<AssemblyInformationalVersionAttribute>();
+
+                    Assert.Equal(assemblyVersion.InformationalVersion, userAgentHeader.Product.Version);
+
+                    requestsExecuted = true;
+
+                    return await next();
+                });
+
+                await WithConnectionAsync(
+                    CreateConnection(testHttpHandler, transportType: transportType),
+                    async (connection) =>
+                    {
+                        await connection.StartAsync(TransferFormat.Text).OrTimeout();
+                        await connection.Transport.Output.WriteAsync(Encoding.UTF8.GetBytes("Hello World"));
+                    });
+                // Fail safe in case the code is modified and some requests don't execute as a result
+                Assert.True(requestsExecuted);
+            }
+
             [Fact]
             public async Task CanReceiveData()
             {
@@ -24,7 +75,7 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests
 
                 // Set the long poll up to return a single message over a few polls.
                 var requestCount = 0;
-                var messageFragments = new[] {"This ", "is ", "a ", "test"};
+                var messageFragments = new[] { "This ", "is ", "a ", "test" };
                 testHttpHandler.OnLongPoll(cancellationToken =>
                 {
                     if (requestCount >= messageFragments.Length)

test/Microsoft.AspNetCore.SignalR.Client.Tests/LongPollingTransportTests.cs

@@ -387,49 +387,6 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests
             }
         }
 
-        [Fact]
-        public async Task LongPollingTransportSetsUserAgent()
-        {
-            HttpHeaderValueCollection<ProductInfoHeaderValue> userAgentHeaderCollection = null;
-
-            var mockHttpHandler = new Mock<HttpMessageHandler>();
-            mockHttpHandler.Protected()
-                .Setup<Task<HttpResponseMessage>>("SendAsync", ItExpr.IsAny<HttpRequestMessage>(), ItExpr.IsAny<CancellationToken>())
-                .Returns<HttpRequestMessage, CancellationToken>(async (request, cancellationToken) =>
-                {
-                    userAgentHeaderCollection = request.Headers.UserAgent;
-                    await Task.Yield();
-                    return ResponseUtils.CreateResponse(HttpStatusCode.OK);
-                });
-
-            using (var httpClient = new HttpClient(mockHttpHandler.Object))
-            {
-                var longPollingTransport = new LongPollingTransport(httpClient);
-
-                try
-                {
-                    var pair = DuplexPipe.CreateConnectionPair(PipeOptions.Default, PipeOptions.Default);
-
-                    await longPollingTransport.StartAsync(new Uri("http://fakeuri.org"), pair.Application, TransferFormat.Text, connection: new TestConnection());
-                }
-                finally
-                {
-                    await longPollingTransport.StopAsync();
-                }
-            }
-
-            Assert.NotNull(userAgentHeaderCollection);
-            var userAgentHeader = Assert.Single(userAgentHeaderCollection);
-            Assert.Equal("Microsoft.AspNetCore.Http.Connections.Client", userAgentHeader.Product.Name);
-
-            // user agent version should come from version embedded in assembly metadata
-            var assemblyVersion = typeof(Constants)
-                .Assembly
-                .GetCustomAttribute<AssemblyInformationalVersionAttribute>();
-
-            Assert.Equal(assemblyVersion.InformationalVersion, userAgentHeader.Product.Version);
-        }
-
         [Theory]
         [InlineData(TransferFormat.Text | TransferFormat.Binary)] // Multiple values not allowed
         [InlineData((TransferFormat)42)] // Unexpected value

test/Microsoft.AspNetCore.SignalR.Client.Tests/ServerSentEventsTransportTests.cs

@@ -299,42 +299,6 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests
             }
         }
 
-        [Fact]
-        public async Task SSETransportSetsUserAgent()
-        {
-            HttpHeaderValueCollection<ProductInfoHeaderValue> userAgentHeaderCollection = null;
-
-            var mockHttpHandler = new Mock<HttpMessageHandler>();
-            mockHttpHandler.Protected()
-                .Setup<Task<HttpResponseMessage>>("SendAsync", ItExpr.IsAny<HttpRequestMessage>(), ItExpr.IsAny<CancellationToken>())
-                .Returns<HttpRequestMessage, CancellationToken>(async (request, cancellationToken) =>
-                {
-                    userAgentHeaderCollection = request.Headers.UserAgent;
-                    await Task.Yield();
-                    return new HttpResponseMessage { Content = new StringContent(string.Empty) };
-                });
-
-            using (var httpClient = new HttpClient(mockHttpHandler.Object))
-            {
-                var sseTransport = new ServerSentEventsTransport(httpClient);
-
-                var pair = DuplexPipe.CreateConnectionPair(PipeOptions.Default, PipeOptions.Default);
-                await sseTransport.StartAsync(new Uri("http://fakeuri.org"), pair.Application, TransferFormat.Text, connection: Mock.Of<IConnection>()).OrTimeout();
-                await sseTransport.StopAsync().OrTimeout();
-            }
-
-            Assert.NotNull(userAgentHeaderCollection);
-            var userAgentHeader = Assert.Single(userAgentHeaderCollection);
-            Assert.Equal("Microsoft.AspNetCore.Http.Connections.Client", userAgentHeader.Product.Name);
-
-            // user agent version should come from version embedded in assembly metadata
-            var assemblyVersion = typeof(Constants)
-                .Assembly
-                .GetCustomAttribute<AssemblyInformationalVersionAttribute>();
-
-            Assert.Equal(assemblyVersion.InformationalVersion, userAgentHeader.Product.Version);
-        }
-
         [Theory]
         [InlineData(TransferFormat.Binary)] // Binary not supported
         [InlineData(TransferFormat.Text | TransferFormat.Binary)] // Multiple values not allowed