From 908e9d847a445bd49f5166f455842a2b00148e7a Mon Sep 17 00:00:00 2001 From: "D. Ror" Date: Tue, 2 May 2023 12:34:40 -0400 Subject: [PATCH 1/5] Use stricter linting/analysis --- .editorconfig | 3 +++ Backend.Tests/Services/MergeServiceTests.cs | 8 ++++---- Backend/BackendFramework.csproj | 1 + Backend/Controllers/LiftController.cs | 3 ++- Backend/Interfaces/IEmailService.cs | 2 +- .../Interfaces/IMergeBlacklistRepository.cs | 4 ++-- Backend/Interfaces/IWordRepository.cs | 2 +- Backend/Services/InviteService.cs | 4 ++-- Backend/Services/LiftService.cs | 1 + Backend/Services/MergeService.cs | 14 +++++++------- Backend/Services/PermissionService.cs | 12 ++++++------ Backend/Services/StatisticsService.cs | 2 +- Backend/Startup.cs | 18 +++++++++--------- 13 files changed, 40 insertions(+), 34 deletions(-) diff --git a/.editorconfig b/.editorconfig index 1bfc7ce1fe..7c5c30814a 100644 --- a/.editorconfig +++ b/.editorconfig @@ -7,6 +7,9 @@ indent_size = 2 [*.cs] max_line_length = 120 indent_size = 4 +dotnet_diagnostic.CA1710.severity = warning +dotnet_diagnostic.CA1816.severity = none +dotnet_diagnostic.IDE0005.severity = warning [*.py] max_line_length = 99 diff --git a/Backend.Tests/Services/MergeServiceTests.cs b/Backend.Tests/Services/MergeServiceTests.cs index fd57825db8..38aa086355 100644 --- a/Backend.Tests/Services/MergeServiceTests.cs +++ b/Backend.Tests/Services/MergeServiceTests.cs @@ -214,9 +214,9 @@ public void AddMergeToBlacklistErrorTest() _ = _mergeBlacklistRepo.DeleteAll(ProjId).Result; var wordIds0 = new List(); var wordIds1 = new List { "1" }; - Assert.ThrowsAsync( + Assert.ThrowsAsync( async () => { await _mergeService.AddToMergeBlacklist(ProjId, UserId, wordIds0); }); - Assert.ThrowsAsync( + Assert.ThrowsAsync( async () => { await _mergeService.AddToMergeBlacklist(ProjId, UserId, wordIds1); }); } @@ -238,9 +238,9 @@ public void IsInMergeBlacklistErrorTest() _ = _mergeBlacklistRepo.DeleteAll(ProjId).Result; var wordIds0 = new List(); var wordIds1 = new List { "1" }; - Assert.ThrowsAsync( + Assert.ThrowsAsync( async () => { await _mergeService.IsInMergeBlacklist(ProjId, wordIds0); }); - Assert.ThrowsAsync( + Assert.ThrowsAsync( async () => { await _mergeService.IsInMergeBlacklist(ProjId, wordIds1); }); } diff --git a/Backend/BackendFramework.csproj b/Backend/BackendFramework.csproj index c19084da54..9e24a6527b 100644 --- a/Backend/BackendFramework.csproj +++ b/Backend/BackendFramework.csproj @@ -4,6 +4,7 @@ 10.0 enable true + Minimum diff --git a/Backend/Controllers/LiftController.cs b/Backend/Controllers/LiftController.cs index 6db4698e78..97c86c553e 100644 --- a/Backend/Controllers/LiftController.cs +++ b/Backend/Controllers/LiftController.cs @@ -170,7 +170,8 @@ public async Task 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."); } diff --git a/Backend/Interfaces/IEmailService.cs b/Backend/Interfaces/IEmailService.cs index f2ff9433e1..05a7d5c64b 100644 --- a/Backend/Interfaces/IEmailService.cs +++ b/Backend/Interfaces/IEmailService.cs @@ -5,6 +5,6 @@ namespace BackendFramework.Interfaces { public interface IEmailService { - Task SendEmail(MimeMessage msg); + Task SendEmail(MimeMessage message); } } diff --git a/Backend/Interfaces/IMergeBlacklistRepository.cs b/Backend/Interfaces/IMergeBlacklistRepository.cs index c9a00ddd8b..55a0468cab 100644 --- a/Backend/Interfaces/IMergeBlacklistRepository.cs +++ b/Backend/Interfaces/IMergeBlacklistRepository.cs @@ -9,9 +9,9 @@ public interface IMergeBlacklistRepository { Task> GetAll(string projectId, string? userId = null); Task Get(string projectId, string entryId); - Task Create(MergeBlacklistEntry mergeBlacklistEntry); + Task Create(MergeBlacklistEntry blacklistEntry); Task Delete(string projectId, string entryId); Task DeleteAll(string projectId); - Task Update(MergeBlacklistEntry mergeBlacklistEntry); + Task Update(MergeBlacklistEntry blacklistEntry); } } diff --git a/Backend/Interfaces/IWordRepository.cs b/Backend/Interfaces/IWordRepository.cs index d00718807e..1f2db182f7 100644 --- a/Backend/Interfaces/IWordRepository.cs +++ b/Backend/Interfaces/IWordRepository.cs @@ -15,7 +15,7 @@ public interface IWordRepository Task IsFrontierNonempty(string projectId); Task> GetFrontier(string projectId); Task AddFrontier(Word word); - Task> AddFrontier(List word); + Task> AddFrontier(List words); Task DeleteFrontier(string projectId, string wordId); Task DeleteFrontier(string projectId, List wordIds); } diff --git a/Backend/Services/InviteService.cs b/Backend/Services/InviteService.cs index 7e467fdaa9..38b9654578 100644 --- a/Backend/Services/InviteService.cs +++ b/Backend/Services/InviteService.cs @@ -73,7 +73,7 @@ public async Task 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 RemoveTokenAndCreateUserRole(Project project, User user, return true; } - catch (PermissionService.InvalidJwtTokenError) + catch (PermissionService.InvalidJwtTokenException) { return false; } diff --git a/Backend/Services/LiftService.cs b/Backend/Services/LiftService.cs index 240a44e018..22e902305f 100644 --- a/Backend/Services/LiftService.cs +++ b/Backend/Services/LiftService.cs @@ -48,6 +48,7 @@ protected override void InsertPronunciationIfNeeded( } } + // This raises error CA1816, which is currently suppressed in .editorconfig. public override void Dispose() { // TODO: When updating the LiftWriter dependency, check to see if its Dispose() implementation diff --git a/Backend/Services/MergeService.cs b/Backend/Services/MergeService.cs index 5de86cf7b0..782d4c0334 100644 --- a/Backend/Services/MergeService.cs +++ b/Backend/Services/MergeService.cs @@ -114,14 +114,14 @@ public async Task UndoMerge(string projectId, MergeUndoIds ids) } /// Adds a List of wordIds to MergeBlacklist of specified . - /// Throws when wordIds has count less than 2. + /// Throws when wordIds has count less than 2. /// The created. public async Task AddToMergeBlacklist( string projectId, string userId, List 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 AddToMergeBlacklist( } /// Check if List of wordIds is in MergeBlacklist for specified . - /// Throws when wordIds has count less than 2. + /// Throws when wordIds has count less than 2. /// A bool, true if in the blacklist. public async Task IsInMergeBlacklist(string projectId, List 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>> 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) { } } } } diff --git a/Backend/Services/PermissionService.cs b/Backend/Services/PermissionService.cs index 0bef26344f..93651b1f69 100644 --- a/Backend/Services/PermissionService.cs +++ b/Backend/Services/PermissionService.cs @@ -150,14 +150,14 @@ public async Task IsViolationEdit(HttpContext request, string userEditId, } /// Retrieve the User ID from the JWT in a request. - /// Throws when null userId extracted from token. + /// Throws when null userId extracted from token. 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) { } } } diff --git a/Backend/Services/StatisticsService.cs b/Backend/Services/StatisticsService.cs index 38337244b3..1f03a93e52 100644 --- a/Backend/Services/StatisticsService.cs +++ b/Backend/Services/StatisticsService.cs @@ -176,7 +176,7 @@ public async Task 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(); diff --git a/Backend/Startup.cs b/Backend/Startup.cs index 495868f44f..1bde71e6c3 100644 --- a/Backend/Startup.cs +++ b/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(); } From 1e74adbf7e45a68e5c29fea7170cf3998fc1025d Mon Sep 17 00:00:00 2001 From: "D. Ror" Date: Tue, 2 May 2023 12:38:39 -0400 Subject: [PATCH 2/5] ... for Backend.Tests too --- Backend.Tests/Backend.Tests.csproj | 1 + Backend.Tests/Helper/TimeTests.cs | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/Backend.Tests/Backend.Tests.csproj b/Backend.Tests/Backend.Tests.csproj index eef8382e76..2b319f3afc 100644 --- a/Backend.Tests/Backend.Tests.csproj +++ b/Backend.Tests/Backend.Tests.csproj @@ -6,6 +6,7 @@ true cobertura true + Minimum diff --git a/Backend.Tests/Helper/TimeTests.cs b/Backend.Tests/Helper/TimeTests.cs index 03f9dc78d5..bd95f651ca 100644 --- a/Backend.Tests/Helper/TimeTests.cs +++ b/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))); } } From a2f59d28f676714694de151485e80521db0c5763 Mon Sep 17 00:00:00 2001 From: "D. Ror" Date: Tue, 2 May 2023 13:52:31 -0400 Subject: [PATCH 3/5] Prevent unused using statements --- .editorconfig | 4 ++++ Backend/BackendFramework.csproj | 2 ++ 2 files changed, 6 insertions(+) diff --git a/.editorconfig b/.editorconfig index 7c5c30814a..9f5c93b07e 100644 --- a/.editorconfig +++ b/.editorconfig @@ -8,7 +8,11 @@ indent_size = 2 max_line_length = 120 indent_size = 4 dotnet_diagnostic.CA1710.severity = warning +# CA1816 is our only exception to Minimum. 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] diff --git a/Backend/BackendFramework.csproj b/Backend/BackendFramework.csproj index 9e24a6527b..d1dd12b988 100644 --- a/Backend/BackendFramework.csproj +++ b/Backend/BackendFramework.csproj @@ -5,6 +5,8 @@ enable true Minimum + true + true From bcf31f3125143e0ec99f6dd40565f6dcbaf1d901 Mon Sep 17 00:00:00 2001 From: "D. Ror" Date: Tue, 2 May 2023 14:06:21 -0400 Subject: [PATCH 4/5] ... and for Backend.Tests; And try NoWarn --- Backend.Tests/Backend.Tests.csproj | 3 +++ Backend.Tests/Controllers/AudioControllerTests.cs | 3 +-- Backend.Tests/Controllers/BannerControllerTests.cs | 3 +-- Backend.Tests/Mocks/PermissionServiceMock.cs | 1 + Backend/BackendFramework.csproj | 1 + 5 files changed, 7 insertions(+), 4 deletions(-) diff --git a/Backend.Tests/Backend.Tests.csproj b/Backend.Tests/Backend.Tests.csproj index 2b319f3afc..bf2e191b51 100644 --- a/Backend.Tests/Backend.Tests.csproj +++ b/Backend.Tests/Backend.Tests.csproj @@ -7,6 +7,9 @@ cobertura true Minimum + true + true + $(NoWarn);CA1816;CS1591 diff --git a/Backend.Tests/Controllers/AudioControllerTests.cs b/Backend.Tests/Controllers/AudioControllerTests.cs index 727d4d2915..f1bf83173b 100644 --- a/Backend.Tests/Controllers/AudioControllerTests.cs +++ b/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; diff --git a/Backend.Tests/Controllers/BannerControllerTests.cs b/Backend.Tests/Controllers/BannerControllerTests.cs index 61006a2880..3b508d8d2d 100644 --- a/Backend.Tests/Controllers/BannerControllerTests.cs +++ b/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; diff --git a/Backend.Tests/Mocks/PermissionServiceMock.cs b/Backend.Tests/Mocks/PermissionServiceMock.cs index 3812d6d700..889a94c043 100644 --- a/Backend.Tests/Mocks/PermissionServiceMock.cs +++ b/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. /// + /// Same as the real implementation. /// public Task HasProjectPermission(HttpContext? request, Permission permission) { diff --git a/Backend/BackendFramework.csproj b/Backend/BackendFramework.csproj index d1dd12b988..3831c75c58 100644 --- a/Backend/BackendFramework.csproj +++ b/Backend/BackendFramework.csproj @@ -7,6 +7,7 @@ Minimum true true + $(NoWarn);CA1816;CS1591 From dfa1982c90efe5305c7949e1ad7bde8057b6ed22 Mon Sep 17 00:00:00 2001 From: "D. Ror" Date: Tue, 2 May 2023 14:18:11 -0400 Subject: [PATCH 5/5] Update comment --- Backend/Services/LiftService.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Backend/Services/LiftService.cs b/Backend/Services/LiftService.cs index 22e902305f..e4a91c3a8b 100644 --- a/Backend/Services/LiftService.cs +++ b/Backend/Services/LiftService.cs @@ -48,7 +48,7 @@ protected override void InsertPronunciationIfNeeded( } } - // This raises error CA1816, which is currently suppressed in .editorconfig. + // This raises error CA1816, which is currently suppressed in .editorconfig and with . public override void Dispose() { // TODO: When updating the LiftWriter dependency, check to see if its Dispose() implementation