Soft enable CA1810 by pranavkm · Pull Request #33619 · dotnet/aspnetcore

pranavkm · 2021-06-17T18:40:07Z

A lot of the warnings for this rule appear from logging. Given this is going
to be replaced by a source generator, this change soft enables the rule while
fixing warnings that appear in non-logging code

Contributes to #24055

A lot of the warnings for this rule appear from logging. Given this is going to be replaced by a source generator, this change soft enables the rule while fixing warnings that appear in non-logging code

pranavkm · 2021-06-17T18:41:37Z

src/Http/Routing/src/DefaultLinkGenerator.cs

-                    _endpointsFound(logger, endpoints.Select(e => e.DisplayName), address, null);
-                }
-            }
+            [LoggerMessage(EventId = 100, EventName = "EndpointsFound", Level = LogLevel.Debug, Message = "Found the endpoints {Endpoints} for address {Address}")]


@maryamariyan / @eerhardt could you have a look at this and let me know if I'm doing this correctly?

FYI @JamesNK / @davidfowl

The previous EndpointsFound method would check whether logging was enabled and then use LINQ to get endpoint names. I believe that is missing post your changes.

Follow the pattern you used with TemplateFailedRequiredValues?

davidfowl · 2021-06-17T19:46:46Z

src/Http/Http.Abstractions/src/Microsoft.AspNetCore.Http.Abstractions.csproj

  <ItemGroup>
    <Reference Include="Microsoft.AspNetCore.Http.Features" />
    <Reference Include="Microsoft.Net.Http.Headers" />
+    <Reference Include="Microsoft.Extensions.Logging.Abstractions" PrivateAssets="All" />


Why is this private assets?

I had to add a reference for the source generator. Our build complains if you add new references and I didn't want to deal with it.

eerhardt · 2021-06-17T19:51:30Z

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

        }

-        private static class Log
+        internal static partial class Log


Why internal?

I can revert it. I was fiddling around with making the source generator work.

eerhardt · 2021-06-17T19:58:21Z

src/Http/Routing/src/DefaultLinkGenerator.cs

-                }
-            }
+            [LoggerMessage(EventId = 100, EventName = "EndpointsFound", Level = LogLevel.Debug, Message = "Found the endpoints {Endpoints} for address {Address}")]
+            public static partial void EndpointsFound(ILogger logger, object address, IEnumerable<Endpoint> endpoints);


Does the order of {Endpoints} and {Address} need to match in the string with how it is ordered in the method signature?

If not, do the names need to match? Here they differ by case.

I think the generator looks them up by name (case insensitively).

Need to check.

Here's the dope code it generates: https://gist.github.com/pranavkm/dba57e845227045d8e3651b0d49d3418

JamesNK · 2021-06-17T20:42:33Z

src/Http/Routing/src/DefaultLinkGenerator.cs

-                    _endpointsFound(logger, endpoints.Select(e => e.DisplayName), address, null);
-                }
-            }
+            [LoggerMessage(EventId = 100, EventName = "EndpointsFound", Level = LogLevel.Debug, Message = "Found the endpoints {Endpoints} for address {Address}")]


The previous EndpointsFound method would check whether logging was enabled and then use LINQ to get endpoint names. I believe that is missing post your changes.

Follow the pattern you used with TemplateFailedRequiredValues?

JamesNK · 2021-06-17T20:44:16Z

src/Http/Routing/src/DefaultLinkGenerator.cs

-                }
-            }
+            [LoggerMessage(EventId = 100, EventName = "EndpointsFound", Level = LogLevel.Debug, Message = "Found the endpoints {Endpoints} for address {Address}")]
+            public static partial void EndpointsFound(ILogger logger, object address, IEnumerable<Endpoint> endpoints);


Need to check.

JamesNK · 2021-06-17T20:45:48Z

src/Http/Routing/src/DefaultLinkGenerator.cs

+                Message = "Failed to process the template {Template} for {Endpoint}. " +
+                "A required route value is missing, or has a different value from the required default values. " +
+                "Supplied ambient values {AmbientValues} and {Values} with default values {Defaults}")]
+            public static partial void TemplateFailedRequiredValues(ILogger logger, string template, string endpoint, string ambientValues, string values, string defaults);


Do partial methods on log generators always have to be public? Looks like this would only be called from the overload so it could be private.

JamesNK · 2021-06-17T20:47:13Z

src/Http/Routing/src/DefaultLinkGenerator.cs

+                Message = "Failed to process the template {Template} for {Endpoint}. " +
+                "The failure occurred while expanding the template with values {Values} " +
+                "This is usually due to a missing or empty value in a complex segment")]
+            public static partial void TemplateFailedExpansion(ILogger logger, string template, string endpoint, string values);


Is skip log level check added yet? Can add it to attribute when method is called from an overload that already checks for IsEnabled.

Not yet but close: dotnet/runtime#54305

Add comments to attributes where it should be added once supported.

dougbu · 2021-06-18T04:57:14Z

eng/Versions.props

    <MicrosoftExtensionsHostingVersion>6.0.0-preview.6.21314.2</MicrosoftExtensionsHostingVersion>
    <MicrosoftExtensionsHttpVersion>6.0.0-preview.6.21314.2</MicrosoftExtensionsHttpVersion>
-    <MicrosoftExtensionsLoggingAbstractionsVersion>6.0.0-preview.6.21314.2</MicrosoftExtensionsLoggingAbstractionsVersion>
+    <MicrosoftExtensionsLoggingAbstractionsVersion>6.0.0-preview.7.21316.3</MicrosoftExtensionsLoggingAbstractionsVersion>


This will simply cause conflicts or be overwritten the next time we get versions from dotnet/runtime. Suggest retrying after #33560 is in.

maryamariyan · 2021-06-18T03:45:56Z

src/Components/WebAssembly/WebAssembly/src/Rendering/WebAssemblyRenderer.cs

        private static class Log
        {
-            private static readonly Action<ILogger, string, Exception> _unhandledExceptionRenderingComponent;
+            private static readonly Action<ILogger, string, Exception> _unhandledExceptionRenderingComponent = LoggerMessage.Define<string>(


or maybe something like below?

[LoggerMessage(EventId = EventIds.UnhandledExceptionRenderingComponent, Level = LogLevel.Critical, Message = "Unhandled exception rendering component: {Message}")] public static partial void UnhandledExceptionRenderingComponent(ILogger logger, string message);

maryamariyan · 2021-06-18T03:50:31Z

src/Hosting/Hosting/src/Internal/HostingLoggerExtensions.cs

-        {
-            _listeningOnAddress(logger, address, null);
-        }
+        [LoggerMessage(EventId = LoggerEventIds.ServerListeningOnAddresses, EventName = "ServerListeningOnAddresses", Level = LogLevel.Information, Message = "Now listening on: {address}")]


Note, when you don't specify EventName, it would take the method name as the event name for the log method.

Instead of setting EventName, I would rename this method from ListeningOnAddress to ServerListeningOnAddresses.

We want to be explicit about the event name. It is easy for someone to unknowingly change the event name by renaming a method. That would introduce a breaking change.

That is the reason why we have strings today instead of using nameof(MethodName).

maryamariyan · 2021-06-18T04:00:58Z

src/Http/Http/src/Internal/ResponseCookies.cs

-            {
-                _samesiteNotSecure(logger, name, null);
-            }
+            [LoggerMessage(EventId = 1, EventName = "SameSiteNotSecure", Level = LogLevel.Warning, Message = "The cookie '{name}' has set 'SameSite=None' and must also set 'Secure'.")]


When you don't set EventName explicitly the generator uses the log method's name as the event name. So you if you prefer, just remove , EventName = "SameSiteNotSecure" and rename SameSiteCookieNotSecure(..) to SameSiteNotSecure.

maryamariyan · 2021-06-18T04:04:14Z

src/Shared/StackTrace/ExceptionDetails/LoggerExtensions.cs

-        {
-            _failedToReadStackFrameInfo(logger, exception);
-        }
+        [LoggerMessage(EventId = 0, EventName = "FailedToReadStackTraceInfo", Level = LogLevel.Debug, Message = "Failed to read stack trace information for exception.")]


Suggested change

[LoggerMessage(EventId = 0, EventName = "FailedToReadStackTraceInfo", Level = LogLevel.Debug, Message = "Failed to read stack trace information for exception.")]

[LoggerMessage(EventId = 0, Level = LogLevel.Debug, Message = "Failed to read stack trace information for exception.")]

maryamariyan · 2021-06-18T04:28:14Z

src/Middleware/StaticFiles/src/LoggerExtensions.cs

-        {
-            _writeCancelled(logger, ex);
-        }
+        [LoggerMessage(EventId = 14, EventName = "WriteCancelled", Level = LogLevel.Debug, Message = "The file transmission was cancelled")]


Suggested change

[LoggerMessage(EventId = 14, EventName = "WriteCancelled", Level = LogLevel.Debug, Message = "The file transmission was cancelled")]

[LoggerMessage(EventId = 14, Level = LogLevel.Debug, Message = "The file transmission was cancelled")]

maryamariyan · 2021-06-18T05:21:54Z

src/Middleware/CORS/src/CORSLoggerExtensions.cs

-                LogLevel.Information,
-                new EventId(6, "OriginNotAllowed"),
-                "Request origin {origin} does not have permission to access the resource.");
+        [LoggerMessage(EventId = 7, EventName = "AccessControlMethodNotAllowed", Level = LogLevel.Information, Message = "Request method {accessControlRequestMethod} not allowed in CORS policy.")]


Suggested change

[LoggerMessage(EventId = 7, EventName = "AccessControlMethodNotAllowed", Level = LogLevel.Information, Message = "Request method {accessControlRequestMethod} not allowed in CORS policy.")]

[LoggerMessage(EventId = 7, Level = LogLevel.Information, Message = "Request method {accessControlRequestMethod} not allowed in CORS policy.")]

maryamariyan · 2021-06-18T05:22:57Z

src/Middleware/CORS/src/CORSLoggerExtensions.cs

-                LogLevel.Information,
-                new EventId(7, "AccessControlMethodNotAllowed"),
-                "Request method {accessControlRequestMethod} not allowed in CORS policy.");
+        [LoggerMessage(EventId = 8, EventName = "RequestHeaderNotAllowed", Level = LogLevel.Information, Message = "Request header '{requestHeader}' not allowed in CORS policy.")]


Suggested change

[LoggerMessage(EventId = 8, EventName = "RequestHeaderNotAllowed", Level = LogLevel.Information, Message = "Request header '{requestHeader}' not allowed in CORS policy.")]

[LoggerMessage(EventId = 8, Level = LogLevel.Information, Message = "Request header '{requestHeader}' not allowed in CORS policy.")]

maryamariyan · 2021-06-18T05:24:13Z

src/Middleware/CORS/src/CORSLoggerExtensions.cs

-                LogLevel.Information,
-                new EventId(8, "RequestHeaderNotAllowed"),
-                "Request header '{requestHeader}' not allowed in CORS policy.");
+        [LoggerMessage(EventId = 9, EventName = "FailedToSetCorsHeaders", Level = LogLevel.Warning, Message = "Failed to apply CORS Response headers.")]


Suggested change

[LoggerMessage(EventId = 9, EventName = "FailedToSetCorsHeaders", Level = LogLevel.Warning, Message = "Failed to apply CORS Response headers.")]

[LoggerMessage(EventId = 9, Level = LogLevel.Warning, Message = "Failed to apply CORS Response headers.")]

maryamariyan · 2021-06-18T05:25:18Z

src/Middleware/CORS/src/CORSLoggerExtensions.cs

-                LogLevel.Warning,
-                new EventId(9, "FailedToSetCorsHeaders"),
-                "Failed to apply CORS Response headers.");
+        [LoggerMessage(EventId = 10, EventName = "NoCorsPolicyFound", Level = LogLevel.Information, Message = "No CORS policy found for the specified request.")]


Suggested change

[LoggerMessage(EventId = 10, EventName = "NoCorsPolicyFound", Level = LogLevel.Information, Message = "No CORS policy found for the specified request.")]

[LoggerMessage(EventId = 10, Level = LogLevel.Information, Message = "No CORS policy found for the specified request.")]

maryamariyan · 2021-06-18T05:26:09Z

src/Middleware/CORS/src/CORSLoggerExtensions.cs

-        {
-            _isNotPreflightRequest(logger, null);
-        }
+        [LoggerMessage(EventId = 12, EventName = "IsNotPreflightRequest", Level = LogLevel.Debug,


Suggested change

[LoggerMessage(EventId = 12, EventName = "IsNotPreflightRequest", Level = LogLevel.Debug,

[LoggerMessage(EventId = 12, Level = LogLevel.Debug,

maryamariyan · 2021-06-18T05:43:00Z

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

-            {
-                _requestBodyIOException(GetLogger(httpContext), exception);
-            }
+            [LoggerMessage(EventId = 1, EventName = "RequestBodyIOException", Level = LogLevel.Debug, Message = "Reading the request body failed with an IOException.")]


Suggested change

[LoggerMessage(EventId = 1, EventName = "RequestBodyIOException", Level = LogLevel.Debug, Message = "Reading the request body failed with an IOException.")]

[LoggerMessage(EventId = 1, Level = LogLevel.Debug, Message = "Reading the request body failed with an IOException.")]

maryamariyan · 2021-06-18T05:44:11Z

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

-            }
+                => RequestBodyInvalidDataException(GetLogger(httpContext), exception);
+
+            [LoggerMessage(EventId = 2, EventName = "RequestBodyInvalidDataException", Level = LogLevel.Debug, Message = "Reading the request body failed with an InvalidDataException.")]


Suggested change

[LoggerMessage(EventId = 2, EventName = "RequestBodyInvalidDataException", Level = LogLevel.Debug, Message = "Reading the request body failed with an InvalidDataException.")]

[LoggerMessage(EventId = 2, Level = LogLevel.Debug, Message = "Reading the request body failed with an InvalidDataException.")]

maryamariyan · 2021-06-18T05:46:19Z

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

+                => ParameterBindingFailed(GetLogger(httpContext), parameterTypeName, parameterName, sourceValue);
+
+            [LoggerMessage(EventId = 3, EventName = "ParamaterBindingFailed", Level = LogLevel.Debug, Message = @"Failed to bind parameter ""{ParameterType} {ParameterName}"" from ""{SourceValue}"".")]
+            public static partial void ParameterBindingFailed(ILogger logger, string parameterType, string parameterName, string sourceValue);


ParamaterBindingFailed vs. ParameterBindingFailed

pranavkm · 2021-06-29T02:39:21Z

Closing this. Will work on converting to LoggerMessage in independent PRs.

Soft enable CA1810

befb790

A lot of the warnings for this rule appear from logging. Given this is going to be replaced by a source generator, this change soft enables the rule while fixing warnings that appear in non-logging code

pranavkm requested review from a team, BrennanConroy, Pilchie, Tratcher, dougbu, halter73 and javiercn as code owners June 17, 2021 18:40

pranavkm commented Jun 17, 2021

View reviewed changes

davidfowl reviewed Jun 17, 2021

View reviewed changes

eerhardt reviewed Jun 17, 2021

View reviewed changes

Changes

d351ddb

JamesNK reviewed Jun 17, 2021

View reviewed changes

dougbu reviewed Jun 18, 2021

View reviewed changes

maryamariyan reviewed Jun 18, 2021

View reviewed changes

Pilchie added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Jun 21, 2021

Tratcher mentioned this pull request Jun 22, 2021

Soft enable CA1810, CA1822 #33659

Merged

pranavkm closed this Jun 29, 2021

pranavkm deleted the prkrishn/fxcop-5 branch June 29, 2021 02:39

	[LoggerMessage(EventId = 0, EventName = "FailedToReadStackTraceInfo", Level = LogLevel.Debug, Message = "Failed to read stack trace information for exception.")]
	[LoggerMessage(EventId = 0, Level = LogLevel.Debug, Message = "Failed to read stack trace information for exception.")]

	[LoggerMessage(EventId = 14, EventName = "WriteCancelled", Level = LogLevel.Debug, Message = "The file transmission was cancelled")]
	[LoggerMessage(EventId = 14, Level = LogLevel.Debug, Message = "The file transmission was cancelled")]

	[LoggerMessage(EventId = 7, EventName = "AccessControlMethodNotAllowed", Level = LogLevel.Information, Message = "Request method {accessControlRequestMethod} not allowed in CORS policy.")]
	[LoggerMessage(EventId = 7, Level = LogLevel.Information, Message = "Request method {accessControlRequestMethod} not allowed in CORS policy.")]

	[LoggerMessage(EventId = 8, EventName = "RequestHeaderNotAllowed", Level = LogLevel.Information, Message = "Request header '{requestHeader}' not allowed in CORS policy.")]
	[LoggerMessage(EventId = 8, Level = LogLevel.Information, Message = "Request header '{requestHeader}' not allowed in CORS policy.")]

	[LoggerMessage(EventId = 9, EventName = "FailedToSetCorsHeaders", Level = LogLevel.Warning, Message = "Failed to apply CORS Response headers.")]
	[LoggerMessage(EventId = 9, Level = LogLevel.Warning, Message = "Failed to apply CORS Response headers.")]

	[LoggerMessage(EventId = 10, EventName = "NoCorsPolicyFound", Level = LogLevel.Information, Message = "No CORS policy found for the specified request.")]
	[LoggerMessage(EventId = 10, Level = LogLevel.Information, Message = "No CORS policy found for the specified request.")]

	[LoggerMessage(EventId = 12, EventName = "IsNotPreflightRequest", Level = LogLevel.Debug,
	[LoggerMessage(EventId = 12, Level = LogLevel.Debug,

	[LoggerMessage(EventId = 1, EventName = "RequestBodyIOException", Level = LogLevel.Debug, Message = "Reading the request body failed with an IOException.")]
	[LoggerMessage(EventId = 1, Level = LogLevel.Debug, Message = "Reading the request body failed with an IOException.")]

	[LoggerMessage(EventId = 2, EventName = "RequestBodyInvalidDataException", Level = LogLevel.Debug, Message = "Reading the request body failed with an InvalidDataException.")]
	[LoggerMessage(EventId = 2, Level = LogLevel.Debug, Message = "Reading the request body failed with an InvalidDataException.")]

Conversation

pranavkm commented Jun 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl Jun 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maryamariyan Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maryamariyan Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranavkm commented Jun 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

davidfowl Jun 17, 2021 •

edited

Loading

maryamariyan Jun 18, 2021 •

edited

Loading

maryamariyan Jun 18, 2021 •

edited

Loading