Content negotiation fails in subsequent requests accepting same media type

- #3140
- clone `MediaTypeHeaderValue` instance before updating it when content negotiation succeeds
  - avoids changes to `MediaTypeConstants` properties and `OutputFormatter.SupportedMediaTypes` entries
  - `MediaTypeHeaderValue.Clone()` does not exist in our DNX Core fork of this class
  - in previous implementation, was called defensively rather than when required
- update `WebApiCompatShimBasicTest` functional tests to use `MvcTestFixture<TStartup>` everywhere
  - #3140 blocked that final migration
  - remove `TestHelper` since it's no longer referenced

nits:
- remove comments mentioning `TestHelper`
- correct spelling of "negotiation"

src/Microsoft.AspNet.Mvc.WebApiCompatShim/ContentNegotiator/DefaultContentNegotiator.cs

@@ -9,8 +9,6 @@ using System.Diagnostics;
 using System.Linq;
 using System.Net.Http.Headers;
 using System.Text;
-using Microsoft.AspNet.Mvc;
-using Microsoft.Extensions.Internal;
 
 namespace System.Net.Http.Formatting
 {
@@ -85,14 +83,25 @@ namespace System.Net.Http.Formatting
             // We found a best formatter
             if (bestFormatterMatch != null)
             {
-                // Find the best character encoding for the selected formatter
+                var bestMediaType = bestFormatterMatch.MediaType;
+
+                // Find the best character encoding for the selected formatter.
                 var bestEncodingMatch = SelectResponseCharacterEncoding(request, bestFormatterMatch.Formatter);
                 if (bestEncodingMatch != null)
                 {
-                    bestFormatterMatch.MediaType.CharSet = bestEncodingMatch.WebName;
+                    // Clone media type value since this is not done defensively in this implementation.
+                    // `MediaTypeHeaderValue` lacks a Clone() method in this runtime. Fortunately, this is the only
+                    // update to an existing instance we need.
+                    var clonedMediaType = new MediaTypeHeaderValue(bestMediaType.MediaType);
+                    foreach (var parameter in bestMediaType.Parameters)
+                    {
+                        clonedMediaType.Parameters.Add(new NameValueHeaderValue(parameter.Name, parameter.Value));
+                    }
+
+                    bestMediaType = clonedMediaType;
+                    bestMediaType.CharSet = bestEncodingMatch.WebName;
                 }
 
-                var bestMediaType = bestFormatterMatch.MediaType;
                 var bestFormatter =
                     bestFormatterMatch.Formatter.GetPerRequestFormatterInstance(type, request, bestMediaType);
                 return new ContentNegotiationResult(bestFormatter, bestMediaType);

test/Microsoft.AspNet.Mvc.FunctionalTests/FilteredDefaultAssemblyProviderFixtureOfT.cs

@@ -5,8 +5,8 @@ using System;
 using System.Collections.Generic;
 using System.Linq;
 using Microsoft.AspNet.Mvc.Infrastructure;
-using Microsoft.Extensions.PlatformAbstractions;
 using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.PlatformAbstractions;
 
 namespace Microsoft.AspNet.Mvc.FunctionalTests
 {
@@ -15,9 +15,9 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests
     {
         protected override void AddAdditionalServices(IServiceCollection services)
         {
-            // TestHelper.CreateServer normally replaces the DefaultAssemblyProvider with a provider that limits the
+            // MvcTestFixture<TStartup> normally replaces the DefaultAssemblyProvider with an IAssemblyProvider
-            // set of candidate assemblies to the executing application. Switch it back to using a filtered default
+            // implementation that limits the set of candidate assemblies to the executing application. Switch it back
-            // assembly provider.
+            // to using a filtered default assembly provider.
             services.AddTransient<IAssemblyProvider, FilteredDefaultAssemblyProvider>();
         }
 

test/Microsoft.AspNet.Mvc.FunctionalTests/TestHelper.cs

@@ -1,125 +0,0 @@
-// Copyright (c) .NET Foundation. All rights reserved.
-// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
-
-using System;
-using System.IO;
-using System.Reflection;
-using Microsoft.AspNet.Builder;
-using Microsoft.AspNet.Hosting;
-using Microsoft.AspNet.Mvc.Infrastructure;
-using Microsoft.AspNet.TestHost;
-using Microsoft.Extensions.PlatformAbstractions;
-using Microsoft.Extensions.DependencyInjection;
-
-namespace Microsoft.AspNet.Mvc.FunctionalTests
-{
-    public static class TestHelper
-    {
-        // Path from Mvc\\test\\Microsoft.AspNet.Mvc.FunctionalTests
-        private static readonly string WebsitesDirectoryPath = Path.Combine("..", "WebSites");
-
-        public static TestServer CreateServer(Action<IApplicationBuilder> builder, string applicationWebSiteName)
-        {
-            return CreateServer(
-                builder,
-                applicationWebSiteName,
-                applicationPath: null,
-                configureServices: (Action<IServiceCollection>)null);
-        }
-
-        public static TestServer CreateServer(
-            Action<IApplicationBuilder> builder,
-            string applicationWebSiteName,
-            Action<IServiceCollection> configureServices)
-        {
-            return CreateServer(
-                builder,
-                applicationWebSiteName,
-                applicationPath: null,
-                configureServices: configureServices);
-        }
-
-        private static TestServer CreateServer(
-            Action<IApplicationBuilder> builder,
-            string applicationWebSiteName,
-            string applicationPath,
-            Action<IServiceCollection> configureServices)
-        {
-            return TestServer.Create(
-                builder,
-                services => AddTestServices(services, applicationWebSiteName, applicationPath, configureServices));
-        }
-
-        private static void AddTestServices(
-            IServiceCollection services,
-            string applicationWebSiteName,
-            string applicationPath,
-            Action<IServiceCollection> configureServices)
-        {
-            applicationPath = applicationPath ?? WebsitesDirectoryPath;
-
-            // Get current IApplicationEnvironment; likely added by the host.
-            var provider = services.BuildServiceProvider();
-            var originalEnvironment = provider.GetRequiredService<IApplicationEnvironment>();
-
-            // When an application executes in a regular context, the application base path points to the root
-            // directory where the application is located, for example MvcSample.Web. However, when executing
-            // an application as part of a test, the ApplicationBasePath of the IApplicationEnvironment points
-            // to the root folder of the test project.
-            // To compensate for this, we need to calculate the original path and override the application
-            // environment value so that components like the view engine work properly in the context of the
-            // test.
-            var applicationBasePath = CalculateApplicationBasePath(
-                originalEnvironment,
-                applicationWebSiteName,
-                applicationPath);
-            var environment = new TestApplicationEnvironment(
-                originalEnvironment,
-                applicationWebSiteName,
-                applicationBasePath);
-            services.AddInstance<IApplicationEnvironment>(environment);
-            var hostingEnvironment = new HostingEnvironment();
-            hostingEnvironment.Initialize(applicationBasePath, config: null);
-            services.AddInstance<IHostingEnvironment>(hostingEnvironment);
-
-            // Injecting a custom assembly provider. Overrides AddMvc() because that uses TryAdd().
-            var assemblyProvider = CreateAssemblyProvider(applicationWebSiteName);
-            services.AddInstance(assemblyProvider);
-
-            // Avoid using pooled memory, we don't have a guarantee that our services will get disposed.
-            services.AddInstance<IHttpResponseStreamWriterFactory>(new TestHttpResponseStreamWriterFactory());
-
-            if (configureServices != null)
-            {
-                configureServices(services);
-            }
-        }
-
-        // Calculate the path relative to the application base path.
-        private static string CalculateApplicationBasePath(
-            IApplicationEnvironment appEnvironment,
-            string applicationWebSiteName,
-            string websitePath)
-        {
-            // Mvc/test/WebSites/applicationWebSiteName
-            return Path.GetFullPath(
-                Path.Combine(appEnvironment.ApplicationBasePath, websitePath, applicationWebSiteName));
-        }
-
-        private static IAssemblyProvider CreateAssemblyProvider(string siteName)
-        {
-            // Creates a service type that will limit MVC to only the controllers in the test site.
-            // We only want this to happen when running in-process.
-            var assembly = Assembly.Load(new AssemblyName(siteName));
-            var provider = new StaticAssemblyProvider
-            {
-                CandidateAssemblies =
-                {
-                    assembly,
-                },
-            };
-
-            return provider;
-        }
-    }
-}

test/Microsoft.AspNet.Mvc.FunctionalTests/WebApiCompatShimBasicTest.cs

@@ -10,10 +10,8 @@ using System.Net.Http.Formatting;
 using System.Net.Http.Headers;
 using System.Threading.Tasks;
 using System.Web.Http;
-using Microsoft.AspNet.Builder;
 using Microsoft.AspNet.Testing;
 using Microsoft.AspNet.Testing.xunit;
-using Microsoft.Extensions.DependencyInjection;
 using Newtonsoft.Json;
 using Xunit;
 
@@ -21,11 +19,6 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests
 {
     public class WebApiCompatShimBasicTest : IClassFixture<MvcTestFixture<WebApiCompatShimWebSite.Startup>>
     {
-        private const string SiteName = nameof(WebApiCompatShimWebSite);
-        private readonly Action<IApplicationBuilder> _app = new WebApiCompatShimWebSite.Startup().Configure;
-        private readonly Action<IServiceCollection> _configureServices =
-            new WebApiCompatShimWebSite.Startup().ConfigureServices;
-
         public WebApiCompatShimBasicTest(MvcTestFixture<WebApiCompatShimWebSite.Startup> fixture)
         {
             Client = fixture.Client;
@@ -288,16 +281,13 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests
         public async Task ApiController_CreateResponse_Conneg(string accept, string mediaType)
         {
             // Arrange
-            var server = TestHelper.CreateServer(_app, SiteName, _configureServices);
-            var client = server.CreateClient();
-
             var request = new HttpRequestMessage(
                 HttpMethod.Get,
                 "http://localhost/api/Blog/HttpRequestMessage/GetUser");
             request.Headers.Accept.ParseAdd(accept);
 
             // Act
-            var response = await client.SendAsync(request);
+            var response = await Client.SendAsync(request);
             var user = await response.Content.ReadAsAsync<WebApiCompatShimWebSite.User>();
 
             // Assert
@@ -312,15 +302,12 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests
         public async Task ApiController_CreateResponse_HardcodedMediaType(string mediaType)
         {
             // Arrange
-            var server = TestHelper.CreateServer(_app, SiteName, _configureServices);
-            var client = server.CreateClient();
-
             var request = new HttpRequestMessage(
                 HttpMethod.Get,
                 "http://localhost/api/Blog/HttpRequestMessage/GetUser?mediaType=" + mediaType);
 
             // Act
-            var response = await client.SendAsync(request);
+            var response = await Client.SendAsync(request);
             var user = await response.Content.ReadAsAsync<WebApiCompatShimWebSite.User>();
 
             // Assert
@@ -337,16 +324,13 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests
         public async Task ApiController_CreateResponse_Conneg_Error(string accept, string mediaType)
         {
             // Arrange
-            var server = TestHelper.CreateServer(_app, SiteName, _configureServices);
-            var client = server.CreateClient();
-
             var request = new HttpRequestMessage(
                 HttpMethod.Get,
                 "http://localhost/api/Blog/HttpRequestMessage/Fail");
             request.Headers.Accept.ParseAdd(accept);
 
             // Act
-            var response = await client.SendAsync(request);
+            var response = await Client.SendAsync(request);
             var error = await response.Content.ReadAsAsync<HttpError>();
 
             // Assert

test/WebSites/WebApiCompatShimWebSite/Controllers/HttpRequestMessage/HttpRequestMessageController.cs

@@ -70,7 +70,7 @@ namespace WebApiCompatShimWebSite
 
             if (mediaType == null)
             {
-                // This will perform content negotation
+                // This will perform content negotiation
                 return Request.CreateResponse<User>(HttpStatusCode.OK, user);
             }
             else
@@ -93,7 +93,7 @@ namespace WebApiCompatShimWebSite
         [HttpGet]
         public HttpResponseMessage Fail()
         {
-            // This will perform content negotation
+            // This will perform content negotiation
             return Request.CreateErrorResponse(HttpStatusCode.InternalServerError, "It failed.");
         }