Don't add AddHttpContextAccessor to Startup.cs (#628)

This makes a few updates to the HttpContext.Current analyzer and code fix provider:

1. Don't add AddHttpContextAccessor to Startup.cs. It's still nice to have that there to make it easier to use IHttpContextAccessor from DI, but it's not required by HttpContextHelper anymore and it seems best to make as few changes as possible to users' startup files.
2. Update HttpContext.Current analyzer to also identify ControllerBase.HttpContext.Current as needing updated since that's a pattern we can arrive at mid-upgrade.
3. Fix HttpContextHelper to include using System.

Fixes #452

CHANGELOG.md

@@ -3,10 +3,18 @@ All notable changes to the .NET Upgrade Assistant will be documented in this fil
 
 The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 
+## Current
+
+### Fixed
+- Updated `HttpContext.Current` analyzer to more correctly identify uses of `HttpContext.Current` that need replaced [#628](https://github.com/dotnet/upgrade-assistant/pull/628).
+
 ## Version 0.2.231403 - 2021-06-14 ([Link](https://www.nuget.org/packages/upgrade-assistant/0.2.231403))
 
 ### Fixed
 - Updated `HttpContext.Current` code fix to use an internal `HttpContextHelper` that will work in multi-project solutions [#599](https://github.com/dotnet/upgrade-assistant/pull/599).
+- Fixed a bug that was preventing the Upgrade Assistant analyzer package from being added to upgraded projects [#620](https://github.com/dotnet/upgrade-assistant/pull/620).
+- Fixed a bug in `PackageLoader` that was causing many extraneous warning messages when verbose logging was enabled [#619](https://github.com/dotnet/upgrade-assistant/pull/619).
+- Fixed a bug in `SourceUpdaterStep` that was leaving an extra .cs file in projects after upgrade which introduced build errors in the project (since the .cs files were already automatically included) [#616](https://github.com/dotnet/upgrade-assistant/pull/616).
 - Exposed Imports in IProjectFile to enable development of custom extensions to add/remove imports. [#612](https://github.com/dotnet/upgrade-assistant/issues/612)
 
 ## Version 0.2.227701 - 2021-05-27 ([Link](https://www.nuget.org/packages/upgrade-assistant/0.2.227701))

src/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers/HttpContextCurrentAnalyzer.cs

@@ -22,6 +22,7 @@ namespace Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers
 
         private const string TargetTypeSimpleName = "HttpContext";
         private const string TargetMember = "Current";
+        private const string TargetPropertySymbolName = "Microsoft.AspNetCore.Mvc.ControllerBase.HttpContext";
 
         private static readonly string[] TargetTypeSymbolNames = new[] { "System.Web.HttpContext", "Microsoft.AspNetCore.Http.HttpContext" };
         private static readonly LocalizableString Title = new LocalizableResourceString(nameof(Resources.HttpContextCurrentTitle), Resources.ResourceManager, typeof(Resources));
@@ -142,9 +143,20 @@ namespace Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers
                     return false;
                 }
             }
+            else if (accessedSymbol is IPropertySymbol propSymbol)
+            {
+                // If the HttpContext reference occurs inside a controller, HttpContext.Current can look
+                // like a reference to the ControllerBase.HttpContext property. However, these should still
+                // be flagged because .Current won't exist in ASP.NET Core. Therefore, bail out for
+                // property symbols only if they are not ControllerBase.HttpContext.
+                if (!TargetPropertySymbolName.Equals(propSymbol.ToDisplayString(), StringComparison.Ordinal))
+                {
+                    return false;
+                }
+            }
             else if (accessedSymbol != null)
             {
-                // If the accessed identifier resolves to a symbol other than a type symbol, bail out
+                // If the accessed identifier resolves to a symbol other than a type or property symbol, bail out
                 // since it's not a reference to System.Web.HttpContext
                 return false;
             }

src/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.CodeFixes/HttpContextCurrentCodeFixer.cs

@@ -78,16 +78,6 @@ namespace Microsoft.DotNet.UpgradeAssistant.Extensions.Default.CodeFixes
 
             var slnEditor = new SolutionEditor(project.Solution);
 
-            // Update Startup.cs to call HttpContextHelper.Initialize
-            var startup = project.Documents.FirstOrDefault(d => d.Name.Equals("Startup.cs", StringComparison.OrdinalIgnoreCase));
-            if (startup is null)
-            {
-                return document.Project.Solution;
-            }
-
-            var startupDocEditor = await slnEditor.GetDocumentEditorAsync(startup.Id, cancellationToken).ConfigureAwait(false);
-            InitializeHttpContextHelperInStartup(startupDocEditor, httpContextHelperClass);
-
             // Update the HttpContext.Current usage to use HttpContextHelper
             var docEditor = await slnEditor.GetDocumentEditorAsync(document.Id, cancellationToken).ConfigureAwait(false);
             var docRoot = (CompilationUnitSyntax)docEditor.OriginalRoot;
@@ -137,42 +127,5 @@ namespace Microsoft.DotNet.UpgradeAssistant.Extensions.Default.CodeFixes
 
             return null;
         }
-
-        private static void InitializeHttpContextHelperInStartup(DocumentEditor editor, INamedTypeSymbol httpContextHelperClass)
-        {
-            var documentRoot = (CompilationUnitSyntax)editor.OriginalRoot;
-
-            // Add using declarations if needed
-            documentRoot = documentRoot
-                .AddUsingIfMissing(httpContextHelperClass.ContainingNamespace.ToString()) // For HttpContextHelper
-                .AddUsingIfMissing("Microsoft.AspNetCore.Http") // For IHttpContextAccessor
-                .AddUsingIfMissing("Microsoft.Extensions.DependencyInjection"); // For AddHttpContextAccessor
-
-            // Add AddHttpContextAccessor call if needed
-            var configureServicesMethod = documentRoot.GetMethodDeclaration<MethodDeclarationSyntax>("ConfigureServices", "IServiceCollection");
-            var serviceCollectionParameter = configureServicesMethod?.ParameterList.Parameters
-                .FirstOrDefault(p => string.Equals(p.Type?.ToString(), "IServiceCollection", StringComparison.Ordinal));
-
-            if (configureServicesMethod != null && serviceCollectionParameter != null)
-            {
-                var configureServicesBody = configureServicesMethod.Body;
-
-                if (configureServicesBody is null)
-                {
-                    return;
-                }
-
-                var addHttpContextAccessorStatement = ParseStatement($"{serviceCollectionParameter.Identifier}.AddHttpContextAccessor();")
-                    .WithWhitespaceTriviaFrom(configureServicesBody.Statements.First());
-
-                // Check whether the statement already exists
-                if (!configureServicesBody.Statements.Any(s => addHttpContextAccessorStatement.IsEquivalentTo(s)))
-                {
-                    documentRoot = documentRoot.ReplaceNode(configureServicesBody, configureServicesBody.AddStatements(addHttpContextAccessorStatement));
-                }
-            }
-
-            editor.ReplaceNode(editor.OriginalRoot, documentRoot);
-        }
     }
 }

src/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.CodeFixes/Templates/HttpContextHelper.cs

@@ -1,4 +1,6 @@
-namespace Microsoft.AspNetCore.Http
+using System;
+
+namespace Microsoft.AspNetCore.Http
 {
     /// <summary>
     /// Temporary helper class for retrieving the current <see cref="HttpContext"/> . This temporary
@@ -19,4 +21,4 @@
 #endif
         public static HttpContext Current => HttpContextAccessor.HttpContext;
     }
-}
+}

tests/tool/Integration.Tests/IntegrationScenarios/WebLibrary/csharp/Upgraded/HttpContextHelper.cs

@@ -0,0 +1,24 @@
+using System;
+
+namespace Microsoft.AspNetCore.Http
+{
+    /// <summary>
+    /// Temporary helper class for retrieving the current <see cref="HttpContext"/> . This temporary
+    /// workaround should be removed in the future and <see cref="HttpContext"/> HttpContext should be retrieved
+    /// from the current controller, middleware, or page instead. If working in another
+    /// component, the current <see cref="HttpContext"/> can be retrieved from an <see cref="IHttpContextAccessor"/>
+    /// retrieved via dependency injection.
+    /// </summary>
+    internal static class HttpContextHelper
+    {
+        private static readonly HttpContextAccessor HttpContextAccessor = new HttpContextAccessor();
+
+        /// <summary>
+        /// Gets the current <see cref="HttpContext"/>. Returns <c>null</c> if there is no current <see cref="HttpContext"/>.
+        /// </summary>
+#if NET5_0_OR_GREATER
+        [Obsolete("Prefer accessing HttpContext via injection", error: false, DiagnosticId = "HttpContextCurrent", UrlFormat = "https://docs.microsoft.com/en-us/aspnet/core/fundamentals/http-context")]
+#endif
+        public static HttpContext Current => HttpContextAccessor.HttpContext;
+    }
+}

tests/tool/Integration.Tests/IntegrationScenarios/WebLibrary/csharp/Upgraded/WebHelpers.cs

@@ -1,9 +1,10 @@
-
+using Microsoft.AspNetCore.Http;
+
 namespace Helper
 {
     public static class WebHelpers
     {
         public static string GetClientAddress() =>
-            Newtonsoft.Json.JsonConvert.SerializeObject(new { Verb = HttpVerbs.Get, Address = HttpContext.Current.Request.UserHostAddress });
+            Newtonsoft.Json.JsonConvert.SerializeObject(new { Verb = HttpVerbs.Get, Address = HttpContextHelper.Current.Request.UserHostAddress });
     }
 }