NO MERGE - code quality analysis spike #463

stephenfuqua · 2022-07-07T16:00:48Z

In this spike, I have added two code analyzers to the "Entry Point" projects: Microsoft's standard C# analyzer, and the SonarSource analyzer. The spike introduces an editorconfig file with substantial rule customization. This customization is based on the actual code practices in Analytics Middle Tier and here in these three projects updated projects.

Most of the "little" fixes were applied automatically by running dotnet format in the three projects. Some of the files only show a change at the top of the file - because a byte order mark was removed. I have confirmed that IdentityContext previously had a BOM and the format command removed it. Another small change is an extra line at the end of the file. This is a standard convention in the Unix-like world; its absence can impact the ability to open or display the file properly in some tools.

stephenfuqua · 2022-07-07T16:12:38Z

.gitignore

@@ -123,3 +121,7 @@ environment.json

 # Sandbox Admin
 /**/SandboxAdminLog*
+
+# Log output
+*.sarif


Sarif is a type of log file for reporting on things like build errors. GitHub's CodeQL analyzer can read these sarif files and incorporate the results into the build process.

stephenfuqua · 2022-07-07T16:14:28Z

Application/EdFi.Ods.SandboxAdmin/Controllers/AccountController.cs

@@ -64,7 +64,7 @@ public async Task<ActionResult> Create(CreateLoginModel model)
            }

            // Failed Model Validation
-            Response.StatusCode = (int)HttpStatusCode.BadRequest;
+            Response.StatusCode = (int) HttpStatusCode.BadRequest;


We have a mix of the two styles in our code base, and I had to choose one to enforce. Personally, I don't have a strong feeling about this, and I selected Microsoft's default option.

stephenfuqua · 2022-07-07T16:15:14Z

Application/EdFi.Ods.SandboxAdmin/Controllers/Api/ClientController.cs


                while (currentException != null)
                {
                    Trace.TraceError("{0}: {1}", currentException.Message, currentException.StackTrace);
-                    message += string.Format("{0}:{1}", counter, currentException.Message);
+                    message.AppendLine($"{counter}:{currentException.Message}");


This is one of the code smells detected by the SonarSource analyzer. Arguably I should have kept it consistent with Append instead of AppendLine - but if this unhandled exception ever occurs, it will be easier to interpret the concatenated messages if there are linebreaks.

stephenfuqua · 2022-07-07T16:18:58Z

Application/EdFi.Ods.SandboxAdmin/Controllers/Api/ClientController.cs

                    counter++;
                    currentException = currentException.InnerException;
                }

-                throw new Exception(string.Format("Unhandled exception with these messages: {0}", message));
+                // This is not an ideal exception type, but it is better than a plain Exception
+                throw new InvalidOperationException(string.Format("Unhandled exception with these messages: {0}", message));


Open to suggestions - including the suggestion that we eschew Microsoft's advice, and accept throwing Exception. In fact, our current C# best practices only suggest not throwing Exception. We discussed that in detail 2-3 years ago and came to that conclusion. So on further reflection, I'll leave this here for discussion, but I'm inclined to globally downgrade the warning not to throw Exception to a mere Suggestion instead of Warning.

This is where I disagree with the advice, and we've had this discussion previously, but to recap my opinion on this -- I feel that there are only 2 times when a specifically typed exception should be thrown in an application (as opposed to a reusable library/framework):

When there is an existing exception that matches perfectly as documented. InvalidOperationException is documented as "the exception that is thrown when a method call is invalid for the object's current state.". That doesn't fit here. There is nothing about the state of the controller that impacted the ability to invoke the GetClients method (controllers don't even have state, generally).

When there is a need to attach definitive semantics to an exception so that it can be handled in a particular way elsewhere in the system (e.g. to be able to make a decision on which status code to return in the HTTP response). In most cases this is not needed (a 500 status is perfect) and the addition of new custom exception types for each occurrence is an onerous development and maintenance requirement, while an arbitrary usage of a different type of the exception than Exception is pointless and equally lacking in semantic meaning.

I actually agree with that position, though I had to jog my memory on those details by actually working through the example.

The only reason I would consider leaving this as a Suggestion (which, to be honest, might not ever be noticed) rather than completely disabling the advice is to help prompt people to think about whether or not there is an existing appropriate exception.

Latest commit restores the original code and tweaks the editorconfig so that this issue is only a suggestion.

stephenfuqua · 2022-07-07T16:20:17Z

Application/EdFi.Ods.SandboxAdmin/Controllers/Api/SandboxController.cs

@@ -10,7 +10,9 @@

 namespace EdFi.Ods.SandboxAdmin.Controllers.Api
 {
+#pragma warning disable S101 // Types should be named in PascalCase, however, uppercase is preferred for acronyms such as "DTO"


We may be able to go a step further and recognize DTO as legitimate. https://docs.microsoft.com/en-us/visualstudio/code-quality/how-to-customize-the-code-analysis-dictionary?view=vs-2022

Personally, I dislike the "DTO" suffix entirely. I generally find it to be an unnecessary addition to the name.

Application/EdFi.Ods.SandboxAdmin/Models/Results/AdminActionResult.cs

stephenfuqua · 2022-07-07T16:25:21Z

Application/EdFi.Ods.SandboxAdmin/Services/BackgroundJobService.cs

-                    // Change the recurrence to suit your needs using Cron functions or a unix CRON expressions (i.e. "* */6 * * *" = every 6 hours)
-                    RecurringJob.AddOrUpdate("RebuildSandboxes", () => _engine.RebuildSandboxes(), Cron.Daily(), TimeZoneInfo.Local);
-                }
+                // Change the recurrence to suit your needs using Cron functions or a unix CRON expressions (i.e. "* */6 * * *" = every 6 hours)


This is another case where the SonarSource analyzer suggested turning the loop into a Linq statement. And in this case, I agree. However, I rewrote in a better style than the linter gave me originally. And I haven't tested this or even checked to see if there are unit tests - because this is just a spike.

If there aren't unit tests already, or if mocking RecurringJob.AddOrUpdate is difficult, then we might just leave this existing code alone. It might not be worth the effort to test it.

stephenfuqua · 2022-07-07T16:26:38Z

Application/EdFi.Ods.SandboxAdmin/Services/IBackgroundJobService.cs

+// The Ed-Fi Alliance licenses this file to you under the Apache License, Version 2.0.
+// See the LICENSE and NOTICES files in the project root for more information.
+
+namespace EdFi.Ods.Sandbox.Admin.Services


Another missing header corrected!

stephenfuqua · 2022-07-07T16:42:57Z

Application/EdFi.Ods.SandboxAdmin/Services/PasswordService.cs

@@ -48,12 +48,10 @@ public async Task<PasswordResetResult> ValidateRequest(string userEmail, string
                return PasswordResetResult.BadUsername;
            }

-            if (!await _identityProvider.VerifyUserEmailConfirmed(userEmail))
+            if (!await _identityProvider.VerifyUserEmailConfirmed(userEmail)
+                 && !await _identityProvider.VerifyUserPassword(userEmail, secret))


Code smell recommendation: unify the if statements. I agreed with that and applied this fix manually. This is not covered by dotnet format.

stephenfuqua · 2022-07-07T16:44:44Z

Application/EdFi.Ods.SandboxAdmin/Services/UserAccountManager.cs

@@ -72,9 +72,9 @@ public async Task<CreateLoginResult> Create(CreateLoginModel model)
               .AddFailingField(x => x.Email);
        }

-        public async Task<bool> Login(string userEmail, string password, bool isPersistent = false)
+        public async Task<bool> Login(string userName, string password, bool isPersistent)


Another example of matching signature matching, in this case to an interface. I hesitated about the default value for isPersistent. No build failures though, so I let it go through. But it would need to be tested. If we really did need a version that defaults to isPersistent, I would prefer to see that done with a separate method that only has the two arguments and then delegates to this method. That way we have a signature that perfectly matches the interface.

Agree with the default arguments...

One semi-related question though... is the concept for that first argument best represented as "user name" or "username"?

Good question, one I didn't put thought into because I wasn't going to change the signature in at the interface level in this POC.

gmcelhanon · 2022-07-09T18:18:11Z

Application/EdFi.Ods.SandboxAdmin/Extensions/HealthCheckServiceExtensions.cs

@@ -1,4 +1,4 @@
-// SPDX-License-Identifier: Apache-2.0
+// SPDX-License-Identifier: Apache-2.0


Why are all these headers getting tweaked?

Because the original had a BOM, and dotnet format removed it.

stephenfuqua · 2022-07-11T18:18:18Z

Application/.editorconfig

+# Lower the priority
+dotnet_diagnostic.S4136.severity=suggestion # Method overloads should be grouped together
+dotnet_diagnostic.S1135.severity=suggestion # Complete TODO comments
+dotnet_diagnostic.S112.severity=suggestion # 'System.Exception' should not be thrown by user code.


Lowered this from Warning to Suggestion

stephenfuqua added 6 commits July 7, 2022 09:10

Ignore sarif files

8b07548

An initial editorconfig

99502e8

Add code analyzers to entry point projects

2f44128

Tweak: do not want unused imports

13f4bf6

Style fixes in SwaggerUI

4cb0505

Addressing style inconsistencies and code smells

d996553

stephenfuqua marked this pull request as draft July 7, 2022 16:01

stephenfuqua commented Jul 7, 2022

View reviewed changes

Restore property

b72f0a7

gmcelhanon reviewed Jul 9, 2022

View reviewed changes

Restore throwing of an Exception

37e09b3

stephenfuqua commented Jul 11, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NO MERGE - code quality analysis spike #463

NO MERGE - code quality analysis spike #463

stephenfuqua commented Jul 7, 2022 •

edited

Loading

stephenfuqua Jul 7, 2022

stephenfuqua Jul 7, 2022

stephenfuqua Jul 7, 2022

stephenfuqua Jul 7, 2022

gmcelhanon Jul 9, 2022 •

edited

Loading

stephenfuqua Jul 11, 2022

stephenfuqua Jul 11, 2022

stephenfuqua Jul 7, 2022

gmcelhanon Jul 9, 2022

stephenfuqua Jul 7, 2022

stephenfuqua Jul 7, 2022

stephenfuqua Jul 7, 2022

stephenfuqua Jul 7, 2022

gmcelhanon Jul 9, 2022

stephenfuqua Jul 11, 2022

gmcelhanon Jul 9, 2022

stephenfuqua Jul 11, 2022

stephenfuqua Jul 11, 2022

		@@ -1,4 +1,4 @@
		// SPDX-License-Identifier: Apache-2.0
		// SPDX-License-Identifier: Apache-2.0

NO MERGE - code quality analysis spike #463

Are you sure you want to change the base?

NO MERGE - code quality analysis spike #463

Conversation

stephenfuqua commented Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmcelhanon Jul 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenfuqua commented Jul 7, 2022 •

edited

Loading

gmcelhanon Jul 9, 2022 •

edited

Loading