Tighten backend linting (#2106)

.editorconfig

@@ -7,6 +7,13 @@ indent_size = 2
 [*.cs]
 max_line_length = 120
 indent_size = 4
+dotnet_diagnostic.CA1710.severity = warning
+# CA1816 is our only exception to <AnalysisMode>Minimum</AnalysisMode>.
+dotnet_diagnostic.CA1816.severity = none
+# CS1591 is our only exception to EnforceCodeStyleInBuild+GenerateDocumentationFile.
+dotnet_diagnostic.CS1591.severity = none
+# IDE0005 requires both EnforceCodeStyleInBuild and GenerateDocumentationFile set to true.
+dotnet_diagnostic.IDE0005.severity = warning
 
 [*.py]
 max_line_length = 99

Backend.Tests/Backend.Tests.csproj

@@ -6,6 +6,10 @@
     <CollectCoverage>true</CollectCoverage>
     <CoverletOutputFormat>cobertura</CoverletOutputFormat>
     <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
+    <AnalysisMode>Minimum</AnalysisMode>
+    <EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild>
+    <GenerateDocumentationFile>true</GenerateDocumentationFile>
+    <NoWarn>$(NoWarn);CA1816;CS1591</NoWarn>
   </PropertyGroup>
   <ItemGroup>
     <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.5.0" />

Backend.Tests/Controllers/AudioControllerTests.cs

@@ -1,5 +1,4 @@
-using System;
-using System.IO;
+using System.IO;
 using Backend.Tests.Mocks;
 using BackendFramework.Controllers;
 using BackendFramework.Interfaces;

Backend.Tests/Controllers/BannerControllerTests.cs

@@ -1,5 +1,4 @@
-using System.Threading.Tasks;
-using Backend.Tests.Mocks;
+using Backend.Tests.Mocks;
 using BackendFramework.Controllers;
 using BackendFramework.Interfaces;
 using BackendFramework.Models;

Backend.Tests/Helper/TimeTests.cs

@@ -19,8 +19,8 @@ public void TestToUtcIso8601()
         public void TestUtcNowIso8601()
         {
             var time = Time.UtcNowIso8601();
-            Assert.That(time.Contains("T"));
-            Assert.That(time.EndsWith("Z"));
+            Assert.That(time.Contains('T'));
+            Assert.That(time.EndsWith('Z'));
             Assert.That(DateTime.Now, Is.EqualTo(DateTime.Parse(time)).Within(TimeSpan.FromSeconds(10)));
         }
     }

Backend.Tests/Mocks/PermissionServiceMock.cs

@@ -69,6 +69,7 @@ public bool IsCurrentUserAuthorized(HttpContext? request)
         /// Note this parameter is nullable in the mock implementation even though the real implementation it is not
         /// to support unit testing when `HttpContext`s are not available.
         /// </param>
+        /// <param name="permission"> Same as the real implementation. </param>
         /// </summary>
         public Task<bool> HasProjectPermission(HttpContext? request, Permission permission)
         {

Backend.Tests/Services/MergeServiceTests.cs

@@ -214,9 +214,9 @@ public void AddMergeToBlacklistErrorTest()
             _ = _mergeBlacklistRepo.DeleteAll(ProjId).Result;
             var wordIds0 = new List<string>();
             var wordIds1 = new List<string> { "1" };
-            Assert.ThrowsAsync<MergeService.InvalidBlacklistEntryError>(
+            Assert.ThrowsAsync<MergeService.InvalidBlacklistEntryException>(
                 async () => { await _mergeService.AddToMergeBlacklist(ProjId, UserId, wordIds0); });
-            Assert.ThrowsAsync<MergeService.InvalidBlacklistEntryError>(
+            Assert.ThrowsAsync<MergeService.InvalidBlacklistEntryException>(
                 async () => { await _mergeService.AddToMergeBlacklist(ProjId, UserId, wordIds1); });
         }
 
@@ -238,9 +238,9 @@ public void IsInMergeBlacklistErrorTest()
             _ = _mergeBlacklistRepo.DeleteAll(ProjId).Result;
             var wordIds0 = new List<string>();
             var wordIds1 = new List<string> { "1" };
-            Assert.ThrowsAsync<MergeService.InvalidBlacklistEntryError>(
+            Assert.ThrowsAsync<MergeService.InvalidBlacklistEntryException>(
                 async () => { await _mergeService.IsInMergeBlacklist(ProjId, wordIds0); });
-            Assert.ThrowsAsync<MergeService.InvalidBlacklistEntryError>(
+            Assert.ThrowsAsync<MergeService.InvalidBlacklistEntryException>(
                 async () => { await _mergeService.IsInMergeBlacklist(ProjId, wordIds1); });
         }
 

Backend/BackendFramework.csproj

@@ -4,6 +4,10 @@
     <LangVersion>10.0</LangVersion>
     <Nullable>enable</Nullable>
     <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
+    <AnalysisMode>Minimum</AnalysisMode>
+    <EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild>
+    <GenerateDocumentationFile>true</GenerateDocumentationFile>
+    <NoWarn>$(NoWarn);CA1816;CS1591</NoWarn>
   </PropertyGroup>
   <ItemGroup>
     <None Remove="Data\sdList.txt" />

Backend/Controllers/LiftController.cs

@@ -170,7 +170,8 @@ public async Task<IActionResult> UploadLiftFile(string projectId, [FromForm] Fil
             }
             catch (Exception e)
             {
-                _logger.LogError(e, $"Error importing lift file {fileUpload.Name} into project {projectId}.");
+                _logger.LogError(e, "Error importing lift file {FileName} into project {ProjectId}",
+                    fileUpload.Name, projectId);
                 return BadRequest("Error processing the lift data. Contact support for help.");
             }
 

Backend/Interfaces/IEmailService.cs

@@ -5,6 +5,6 @@ namespace BackendFramework.Interfaces
 {
     public interface IEmailService
     {
-        Task<bool> SendEmail(MimeMessage msg);
+        Task<bool> SendEmail(MimeMessage message);
     }
 }

Backend/Interfaces/IMergeBlacklistRepository.cs

@@ -9,9 +9,9 @@ public interface IMergeBlacklistRepository
     {
         Task<List<MergeBlacklistEntry>> GetAll(string projectId, string? userId = null);
         Task<MergeBlacklistEntry?> Get(string projectId, string entryId);
-        Task<MergeBlacklistEntry> Create(MergeBlacklistEntry mergeBlacklistEntry);
+        Task<MergeBlacklistEntry> Create(MergeBlacklistEntry blacklistEntry);
         Task<bool> Delete(string projectId, string entryId);
         Task<bool> DeleteAll(string projectId);
-        Task<ResultOfUpdate> Update(MergeBlacklistEntry mergeBlacklistEntry);
+        Task<ResultOfUpdate> Update(MergeBlacklistEntry blacklistEntry);
     }
 }

Backend/Interfaces/IWordRepository.cs

@@ -15,7 +15,7 @@ public interface IWordRepository
         Task<bool> IsFrontierNonempty(string projectId);
         Task<List<Word>> GetFrontier(string projectId);
         Task<Word> AddFrontier(Word word);
-        Task<List<Word>> AddFrontier(List<Word> word);
+        Task<List<Word>> AddFrontier(List<Word> words);
         Task<bool> DeleteFrontier(string projectId, string wordId);
         Task<long> DeleteFrontier(string projectId, List<string> wordIds);
     }

Backend/Services/InviteService.cs

@@ -73,7 +73,7 @@ public async Task<bool> RemoveTokenAndCreateUserRole(Project project, User user,
                 var updatedUser = await _permissionService.MakeJwt(user);
                 if (updatedUser is null)
                 {
-                    throw new PermissionService.InvalidJwtTokenError(
+                    throw new PermissionService.InvalidJwtTokenException(
                         "Unable to generate JWT.");
                 }
 
@@ -86,7 +86,7 @@ public async Task<bool> RemoveTokenAndCreateUserRole(Project project, User user,
 
                 return true;
             }
-            catch (PermissionService.InvalidJwtTokenError)
+            catch (PermissionService.InvalidJwtTokenException)
             {
                 return false;
             }

Backend/Services/LiftService.cs

@@ -48,6 +48,7 @@ protected override void InsertPronunciationIfNeeded(
             }
         }
 
+        // This raises error CA1816, which is currently suppressed in .editorconfig and with <NoWarn>.
         public override void Dispose()
         {
             // TODO: When updating the LiftWriter dependency, check to see if its Dispose() implementation

Backend/Services/MergeService.cs

@@ -114,14 +114,14 @@ public async Task<bool> UndoMerge(string projectId, MergeUndoIds ids)
         }
 
         /// <summary> Adds a List of wordIds to MergeBlacklist of specified <see cref="Project"/>. </summary>
-        /// <exception cref="InvalidBlacklistEntryError"> Throws when wordIds has count less than 2. </exception>
+        /// <exception cref="InvalidBlacklistEntryException"> Throws when wordIds has count less than 2. </exception>
         /// <returns> The <see cref="MergeBlacklistEntry"/> created. </returns>
         public async Task<MergeBlacklistEntry> AddToMergeBlacklist(
             string projectId, string userId, List<string> wordIds)
         {
             if (wordIds.Count < 2)
             {
-                throw new InvalidBlacklistEntryError("Cannot blacklist a list of fewer than 2 wordIds.");
+                throw new InvalidBlacklistEntryException("Cannot blacklist a list of fewer than 2 wordIds.");
             }
             // When we switch from individual to common blacklist, the userId argument here should be removed.
             var blacklist = await _mergeBlacklistRepo.GetAll(projectId, userId);
@@ -137,13 +137,13 @@ public async Task<MergeBlacklistEntry> AddToMergeBlacklist(
         }
 
         /// <summary> Check if List of wordIds is in MergeBlacklist for specified <see cref="Project"/>. </summary>
-        /// <exception cref="InvalidBlacklistEntryError"> Throws when wordIds has count less than 2. </exception>
+        /// <exception cref="InvalidBlacklistEntryException"> Throws when wordIds has count less than 2. </exception>
         /// <returns> A bool, true if in the blacklist. </returns>
         public async Task<bool> IsInMergeBlacklist(string projectId, List<string> wordIds, string? userId = null)
         {
             if (wordIds.Count < 2)
             {
-                throw new InvalidBlacklistEntryError("Cannot blacklist a list of fewer than 2 wordIds.");
+                throw new InvalidBlacklistEntryException("Cannot blacklist a list of fewer than 2 wordIds.");
             }
             var blacklist = await _mergeBlacklistRepo.GetAll(projectId, userId);
             foreach (var entry in blacklist)
@@ -218,11 +218,11 @@ public async Task<List<List<Word>>> GetPotentialDuplicates(
         }
 
         [Serializable]
-        public class InvalidBlacklistEntryError : Exception
+        public class InvalidBlacklistEntryException : Exception
         {
-            public InvalidBlacklistEntryError() { }
+            public InvalidBlacklistEntryException() { }
 
-            public InvalidBlacklistEntryError(string message) : base(message) { }
+            public InvalidBlacklistEntryException(string message) : base(message) { }
         }
     }
 }

Backend/Services/PermissionService.cs

@@ -150,14 +150,14 @@ public async Task<bool> IsViolationEdit(HttpContext request, string userEditId,
         }
 
         /// <summary>Retrieve the User ID from the JWT in a request. </summary>
-        /// <exception cref="InvalidJwtTokenError"> Throws when null userId extracted from token. </exception>
+        /// <exception cref="InvalidJwtTokenException"> Throws when null userId extracted from token. </exception>
         public string GetUserId(HttpContext request)
         {
             var jsonToken = GetJwt(request);
             var userId = ((JwtSecurityToken)jsonToken).Payload["UserId"].ToString();
             if (userId is null)
             {
-                throw new InvalidJwtTokenError();
+                throw new InvalidJwtTokenException();
             }
 
             return userId;
@@ -236,13 +236,13 @@ public string GetUserId(HttpContext request)
         }
 
         [Serializable]
-        public class InvalidJwtTokenError : Exception
+        public class InvalidJwtTokenException : Exception
         {
-            public InvalidJwtTokenError() { }
+            public InvalidJwtTokenException() { }
 
-            public InvalidJwtTokenError(string message) : base(message) { }
+            public InvalidJwtTokenException(string msg) : base(msg) { }
 
-            public InvalidJwtTokenError(string message, Exception innerException) : base(message, innerException) { }
+            public InvalidJwtTokenException(string msg, Exception innerException) : base(msg, innerException) { }
         }
     }
 

Backend/Services/StatisticsService.cs

@@ -176,7 +176,7 @@ public async Task<ChartRootData> GetProgressEstimationLineChartRoot(string proje
             workshopSchedule.Sort();
             var totalCountList = totalCountDictionary.Values.ToList();
             var pastDays = workshopSchedule.FindAll(day =>
-                ParseDateTimePermissivelyWithException(day).CompareTo(DateTime.Now) <= 0).Count();
+                ParseDateTimePermissivelyWithException(day).CompareTo(DateTime.Now) <= 0).Count;
             // calculate average daily count
             // If pastDays is two or more, and pastDays equals the number of days on which at least one word was added
             var min = totalCountList.Min();

Backend/Startup.cs

@@ -68,7 +68,7 @@ private class EnvironmentNotConfiguredException : Exception { }
                 return contents;
             }
 
-            _logger.LogError($"Environment variable: `{name}` is not defined. {error}");
+            _logger.LogError("Environment variable: {Name} is not defined. {Error}", name, error);
             return defaultValue;
         }
 
@@ -109,8 +109,8 @@ public void ConfigureServices(IServiceCollection services)
             const int minKeyLength = 128 / 8;
             if (secretKey is null || secretKey.Length < minKeyLength)
             {
-                _logger.LogError($"Must set {secretKeyEnvName} environment variable " +
-                                 $"to string of length {minKeyLength} or longer.");
+                _logger.LogError("Must set {EnvName} environment variable to string of length {MinLength} or longer.",
+                    secretKeyEnvName, minKeyLength);
                 throw new EnvironmentNotConfiguredException();
             }
 
@@ -339,30 +339,30 @@ private bool CreateAdminUser(IUserRepository userRepo)
             var existingUser = userRepo.GetAllUsers().Result.Find(x => x.Username == username);
             if (existingUser is not null)
             {
-                _logger.LogInformation($"User {username} already exists. Updating password and granting " +
-                                       "admin permissions.");
+                _logger.LogInformation(
+                    "User {User} already exists. Updating password and granting admin permissions.", username);
                 if (userRepo.ChangePassword(existingUser.Id, password).Result == ResultOfUpdate.NotFound)
                 {
-                    _logger.LogError($"Failed to find user {username}.");
+                    _logger.LogError("Failed to find user {User}.", username);
                     throw new AdminUserCreationException();
                 }
 
                 existingUser.IsAdmin = true;
                 if (userRepo.Update(existingUser.Id, existingUser, true).Result == ResultOfUpdate.NotFound)
                 {
-                    _logger.LogError($"Failed to find user {username}.");
+                    _logger.LogError("Failed to find user {User}.", username);
                     throw new AdminUserCreationException();
                 }
 
                 return true;
             }
 
-            _logger.LogInformation($"Creating admin user: {username} ({adminEmail})");
+            _logger.LogInformation("Creating admin user: {User} ({Email}).", username, adminEmail);
             var user = new User { Username = username, Password = password, Email = adminEmail, IsAdmin = true };
             var returnedUser = userRepo.Create(user).Result;
             if (returnedUser is null)
             {
-                _logger.LogError($"Failed to create admin user {username}, {adminEmail}.");
+                _logger.LogError("Failed to create admin user {User} ({Email}).", username, adminEmail);
                 throw new AdminUserCreationException();
             }