Backend Refactor (#895)

* Remove redundant parentheses

* Fix typos

* Remove unusued variables and duplicated magic string value

* Remove impossible null checks

* Correctly update type signature of WordRepo and WordService methods
Make all custom exceptions Serializable
Turn Backend Test compile warnings into errors

* Fix implementation and signature of UserEditRepository.GetUserEdit & UserEditApiServices to properly handle missing IDs

* Fix type signature and implementation of UserController.Get

* Avoid shadowing local variable name

* Fix UserRoleApiServices.GetUserRole to properly handle missing IDs

* Require a string rather than string? be returned as given the case statement, it is a program error if this is not the case

* Remove possible null assignment to test variable

* Update UserApiServices.GetUserAvatar to properly handle missing users

Backend.Tests/Backend.Tests.csproj

@@ -3,6 +3,7 @@
     <TargetFramework>netcoreapp3.1</TargetFramework>
     <IsPackable>false</IsPackable>
     <Nullable>enable</Nullable>
+    <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
   </PropertyGroup>
   <ItemGroup>
     <PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.8.3" />

Backend.Tests/Controllers/AudioControllerTests.cs

@@ -54,11 +54,12 @@ private static Word RandomWord()
         [Test]
         public void TestAudioImport()
         {
-            var filePath = Path.Combine(Util.AssetsDir, "sound.mp3");
+            const string soundFileName = "sound.mp3";
+            var filePath = Path.Combine(Util.AssetsDir, soundFileName);
 
             // Open the file to read to controller.
-            using var fstream = File.OpenRead(filePath);
-            var formFile = new FormFile(fstream, 0, fstream.Length, "name", "sound.mp3");
+            using var stream = File.OpenRead(filePath);
+            var formFile = new FormFile(stream, 0, stream.Length, "name", soundFileName);
             var fileUpload = new FileUpload { File = formFile, Name = "FileName" };
 
             var word = _wordRepo.Create(RandomWord()).Result;
@@ -82,7 +83,7 @@ public void DeleteAudio()
             origWord.Audio.Add("a.wav");
 
             // Test delete function
-            var action = _audioController.Delete(_projId, origWord.Id, "a.wav").Result;
+            _ = _audioController.Delete(_projId, origWord.Id, "a.wav").Result;
 
             // Original word persists
             Assert.IsTrue(_wordRepo.GetAllWords(_projId).Result.Count == 2);

Backend.Tests/Controllers/AvatarControllerTests.cs

@@ -56,9 +56,9 @@ private static void DeleteAvatarFile(string userId)
         public void TestAvatarImport()
         {
             var filePath = Path.Combine(Util.AssetsDir, "combine.png");
-            using var fstream = File.OpenRead(filePath);
+            using var stream = File.OpenRead(filePath);
 
-            var formFile = new FormFile(fstream, 0, fstream.Length, "dave", "combine.png");
+            var formFile = new FormFile(stream, 0, stream.Length, "dave", "combine.png");
             var fileUpload = new FileUpload { File = formFile, Name = "FileName" };
 
             _ = _avatarController.UploadAvatar(_jwtAuthenticatedUser.Id, fileUpload).Result;

Backend.Tests/Controllers/LiftControllerTests.cs

@@ -223,7 +223,9 @@ public async Task TestExportDeleted()
 
             var wordToUpdate = _wordRepo.Create(word).Result;
             wordToDelete = _wordRepo.Create(wordToDelete).Result;
-            var untouchedWord = _wordRepo.Create(secondWord).Result;
+
+            // Create untouched word.
+            _ = _wordRepo.Create(secondWord).Result;
 
             word.Id = "";
             word.Vernacular = "updated";
@@ -248,7 +250,8 @@ public async Task TestExportDeleted()
             var exportPath = Path.Combine(extractedExportDir,
                 Path.Combine("Lift", "NewLiftFile.lift"));
             var text = await File.ReadAllTextAsync(exportPath, Encoding.UTF8);
-            //TODO: Add SIL or other XML assertion library and verify with xpath that the correct entries are kept vs deleted
+            // TODO: Add SIL or other XML assertion library and verify with xpath that the correct entries are
+            //      kept vs deleted
             // Make sure we exported 2 live and one dead entry
             Assert.That(Regex.Matches(text, "<entry").Count, Is.EqualTo(3));
             // There is only one deleted word
@@ -292,9 +295,9 @@ public void TestRoundtrip(RoundTripObj roundTripObj)
 
             // Upload the zip file.
             // Generate api parameter with filestream.
-            using (var fstream = File.OpenRead(pathToStartZip))
+            using (var stream = File.OpenRead(pathToStartZip))
             {
-                var fileUpload = InitFile(fstream, roundTripObj.Filename);
+                var fileUpload = InitFile(stream, roundTripObj.Filename);
 
                 // Make api call.
                 var result = _liftController.UploadLiftFile(proj1.Id, fileUpload).Result;
@@ -350,11 +353,6 @@ public void TestRoundtrip(RoundTripObj roundTripObj)
 
             // Init the project the .zip info is added to.
             var proj2 = _projServ.Create(RandomProject()).Result;
-            if (proj2 is null)
-            {
-                Assert.Fail();
-                return;
-            }
 
             // Upload the exported words again.
             // Generate api parameter with filestream.

Backend.Tests/Controllers/ProjectControllerTests.cs

@@ -110,7 +110,7 @@ public void TestGetProject()
             _projectService.Create(RandomProject());
 
             var action = _controller.Get(project.Id).Result;
-            Assert.That(action, Is.InstanceOf<ObjectResult>());
+            Assert.IsInstanceOf<ObjectResult>(action);
 
             var foundProjects = ((ObjectResult)action).Value as Project;
             Assert.AreEqual(project, foundProjects);
@@ -175,8 +175,8 @@ public void TestParseSemanticDomains()
         public void TestProjectDuplicateCheck()
         {
             var project1 = _projectService.Create(RandomProject()).Result;
-            var project2 = _projectService.Create(RandomProject()).Result;
-            var project3 = _projectService.Create(RandomProject()).Result;
+            _ = _projectService.Create(RandomProject()).Result;
+            _ = _projectService.Create(RandomProject()).Result;
             var modProject = project1.Clone();
             modProject.Name = "Proj";
             _ = _controller.Put(modProject.Id, modProject);

Backend.Tests/Controllers/UserControllerTests.cs

@@ -61,12 +61,19 @@ public void TestGetUser()
             _userService.Create(RandomUser());
 
             var action = _controller.Get(user.Id).Result;
-            Assert.That(action, Is.InstanceOf<ObjectResult>());
+            Assert.IsInstanceOf<ObjectResult>(action);
 
             var foundUser = ((ObjectResult)action).Value as User;
             Assert.AreEqual(user, foundUser);
         }
 
+        [Test]
+        public void TestGetMissingUser()
+        {
+            var action = _controller.Get("INVALID_USER_ID").Result;
+            Assert.IsInstanceOf<NotFoundObjectResult>(action);
+        }
+
         [Test]
         public void TestCreateUser()
         {

Backend.Tests/Controllers/UserEditControllerTests.cs

@@ -84,7 +84,7 @@ public void TestGetUserEdit()
             _userEditRepo.Create(RandomUserEdit());
 
             var action = _userEditController.Get(_projId, userEdit.Id).Result;
-            Assert.That(action, Is.InstanceOf<ObjectResult>());
+            Assert.IsInstanceOf<ObjectResult>(action);
 
             var foundUserEdit = ((ObjectResult)action).Value as UserEdit;
             Assert.AreEqual(userEdit, foundUserEdit);
@@ -137,11 +137,17 @@ public void TestGoalToUserEdit()
             const int modGoalIndex = 0;
             var wrapperObj = new UserEditObjectWrapper(modGoalIndex, stringUserEdit);
 
-            var action = _userEditController.Put(_projId, origUserEdit.Id, wrapperObj);
+            _ = _userEditController.Put(_projId, origUserEdit.Id, wrapperObj);
 
             Assert.That(_userEditRepo.GetAllUserEdits(_projId).Result, Has.Count.EqualTo(count + 1));
-            Assert.Contains(stringUserEdit, _userEditRepo.GetUserEdit(
-                _projId, origUserEdit.Id).Result.Edits[modGoalIndex].StepData);
+
+            var userEdit = _userEditRepo.GetUserEdit(_projId, origUserEdit.Id).Result;
+            if (userEdit is null)
+            {
+                Assert.Fail();
+                return;
+            }
+            Assert.Contains(stringUserEdit, userEdit.Edits[modGoalIndex].StepData);
         }
 
         [Test]
@@ -169,5 +175,12 @@ public void TestDeleteAllUserEdits()
 
             Assert.That(_userEditRepo.GetAllUserEdits(_projId).Result, Has.Count.EqualTo(0));
         }
+
+        [Test]
+        public void TestGetMissingUserEdit()
+        {
+            var action = _userEditController.Get(_projId, "INVALID_USER_EDIT_ID").Result;
+            Assert.IsInstanceOf<NotFoundObjectResult>(action);
+        }
     }
 }

Backend.Tests/Controllers/UserRoleControllerTests.cs

@@ -84,13 +84,19 @@ public void TestGetUserRole()
             _userRoleService.Create(RandomUserRole());
 
             var action = _userRoleController.Get(_projId, userRole.Id).Result;
-
-            Assert.That(action, Is.InstanceOf<ObjectResult>());
+            Assert.IsInstanceOf<ObjectResult>(action);
 
             var foundUserRole = ((ObjectResult)action).Value as UserRole;
             Assert.AreEqual(userRole, foundUserRole);
         }
 
+        [Test]
+        public void TestGetMissingUserRole()
+        {
+            var action = _userRoleController.Get(_projId, "INVALID_USER_ROLE_ID").Result;
+            Assert.IsInstanceOf<NotFoundObjectResult>(action);
+        }
+
         [Test]
         public void TestGetUserRolesMissingProject()
         {

Backend.Tests/Controllers/WordControllerTests.cs

@@ -64,11 +64,11 @@ private Word RandomWord()
                     new SemanticDomain(), new SemanticDomain(), new SemanticDomain()
                 };
 
-                foreach (var semdom in sense.SemanticDomains)
+                foreach (var semDom in sense.SemanticDomains)
                 {
-                    semdom.Name = Util.RandString();
-                    semdom.Id = Util.RandString();
-                    semdom.Description = Util.RandString();
+                    semDom.Name = Util.RandString();
+                    semDom.Id = Util.RandString();
+                    semDom.Description = Util.RandString();
                 }
             }
 
@@ -96,8 +96,7 @@ public void TestGetWord()
             _repo.Create(RandomWord());
 
             var action = _wordController.Get(_projId, word.Id).Result;
-
-            Assert.That(action, Is.InstanceOf<ObjectResult>());
+            Assert.IsInstanceOf<ObjectResult>(action);
 
             var foundWord = ((ObjectResult)action).Value as Word;
             Assert.AreEqual(word, foundWord);
@@ -136,7 +135,7 @@ public void UpdateWord()
             var origWord = _repo.Create(RandomWord()).Result;
 
             var modWord = origWord.Clone();
-            modWord.Vernacular = "Yoink";
+            modWord.Vernacular = "NewVernacular";
 
             var id = (string)((ObjectResult)_wordController.Put(_projId, modWord.Id, modWord).Result).Value;
 
@@ -158,7 +157,7 @@ public void DeleteWord()
             var origWord = _repo.Create(RandomWord()).Result;
 
             // Test delete function
-            var action = _wordController.Delete(_projId, origWord.Id).Result;
+            _ = _wordController.Delete(_projId, origWord.Id).Result;
 
             // Original word persists
             Assert.Contains(origWord, _repo.GetAllWords(_projId).Result);
@@ -213,6 +212,11 @@ public void MergeWordsIdentity()
             // Check that new word has the right history
             Assert.That(newWords.First().History, Has.Count.EqualTo(1));
             var intermediateWord = _repo.GetWord(_projId, newWords.First().History.First()).Result;
+            if (intermediateWord is null)
+            {
+                Assert.Fail();
+                return;
+            }
             Assert.That(intermediateWord.History, Has.Count.EqualTo(1));
             Assert.AreEqual(intermediateWord.History.First(), thisWord.Id);
         }
@@ -245,18 +249,24 @@ public void MergeWords()
             var newWordList = _wordService.Merge(_projId, parentChildMergeObject).Result;
 
             // Check for parent is in the db
-            var dbParent = newWordList.FirstOrDefault();
-            Assert.IsNotNull(dbParent);
+            var dbParent = newWordList.First();
             Assert.AreEqual(dbParent.Senses.Count, 3);
             Assert.AreEqual(dbParent.History.Count, 3);
 
-            // Check the separarte words were made
+            // Check that separate words were made
             Assert.AreEqual(newWordList.Count, 4);
 
             foreach (var word in newWordList)
             {
                 Assert.Contains(_repo.GetWord(_projId, word.Id).Result, _repo.GetAllWords(_projId).Result);
             }
         }
+
+        [Test]
+        public void TestGetMissingWord()
+        {
+            var action = _wordController.Get(_projId, "INVALID_WORD_ID").Result;
+            Assert.IsInstanceOf<NotFoundObjectResult>(action);
+        }
     }
 }

Backend.Tests/Helper/Sanitization.cs

@@ -11,7 +11,7 @@ public class SanitizationTests
         {
             "a",
             "1",
-            "a-5",
+            "a-5"
         };
 
         [TestCaseSource(nameof(_validIds))]

Backend.Tests/Mocks/UserEditRepositoryMock.cs

@@ -22,10 +22,17 @@ public Task<List<UserEdit>> GetAllUserEdits(string projectId)
             return Task.FromResult(cloneList.Where(userEdit => userEdit.ProjectId == projectId).ToList());
         }
 
-        public Task<UserEdit> GetUserEdit(string projectId, string userEditId)
+        public Task<UserEdit?> GetUserEdit(string projectId, string userEditId)
         {
-            var foundUserEdit = _userEdits.Single(ue => ue.Id == userEditId);
-            return Task.FromResult(foundUserEdit.Clone());
+            try
+            {
+                var foundUserEdit = _userEdits.Single(ue => ue.Id == userEditId);
+                return Task.FromResult<UserEdit?>(foundUserEdit.Clone());
+            }
+            catch (InvalidOperationException)
+            {
+                return Task.FromResult<UserEdit?>(null);
+            }
         }
 
         public Task<UserEdit> Create(UserEdit userEdit)

Backend.Tests/Mocks/UserRoleServiceMock.cs

@@ -23,10 +23,17 @@ public Task<List<UserRole>> GetAllUserRoles(string projectId)
             return Task.FromResult(cloneList.Where(userRole => userRole.ProjectId == projectId).ToList());
         }
 
-        public Task<UserRole> GetUserRole(string projectId, string userRoleId)
+        public Task<UserRole?> GetUserRole(string projectId, string userRoleId)
         {
-            var foundUserRole = _userRoles.Single(userRole => userRole.Id == userRoleId);
-            return Task.FromResult(foundUserRole.Clone());
+            try
+            {
+                var foundUserRole = _userRoles.Single(userRole => userRole.Id == userRoleId);
+                return Task.FromResult<UserRole?>(foundUserRole.Clone());
+            }
+            catch (InvalidOperationException)
+            {
+                return Task.FromResult<UserRole?>(null);
+            }
         }
 
         public Task<UserRole> Create(UserRole userRole)

Backend.Tests/Mocks/UserServiceMock.cs

@@ -22,10 +22,17 @@ public Task<List<User>> GetAllUsers()
             return Task.FromResult(_users.Select(user => user.Clone()).ToList());
         }
 
-        public Task<User> GetUser(string id)
+        public Task<User?> GetUser(string id, bool sanitize = true)
         {
-            var foundUser = _users.Single(user => user.Id == id);
-            return Task.FromResult(foundUser.Clone());
+            try
+            {
+                var foundUser = _users.Single(user => user.Id == id);
+                return Task.FromResult<User?>(foundUser.Clone());
+            }
+            catch (InvalidOperationException)
+            {
+                return Task.FromResult<User?>(null);
+            }
         }
 
         public Task<string?> GetUserAvatar(string id)
@@ -84,7 +91,7 @@ public Task<ResultOfUpdate> Update(string id, User user, bool updateIsAdmin = fa
                 }
 
                 foundUser = MakeJwt(foundUser).Result;
-                return Task.FromResult<User?>(foundUser);
+                return Task.FromResult(foundUser);
             }
             catch (InvalidOperationException)
             {

Backend.Tests/Mocks/WordRepositoryMock.cs

@@ -24,10 +24,17 @@ public Task<List<Word>> GetAllWords(string projectId)
             return Task.FromResult(_words.Select(word => word.Clone()).ToList());
         }
 
-        public Task<Word> GetWord(string projectId, string wordId)
+        public Task<Word?> GetWord(string projectId, string wordId)
         {
-            var foundWord = _words.Single(word => word.Id == wordId);
-            return Task.FromResult(foundWord.Clone());
+            try
+            {
+                var foundWord = _words.Single(word => word.Id == wordId);
+                return Task.FromResult<Word?>(foundWord.Clone());
+            }
+            catch (InvalidOperationException)
+            {
+                return Task.FromResult<Word?>(null);
+            }
         }
 
         public Task<Word> Create(Word word)

Backend.Tests/Models/ProjectTests.cs

@@ -123,8 +123,8 @@ public void TestNotEquals()
         public void TestToString()
         {
             var system = new WritingSystem { Name = Name, Bcp47 = Bcp47 };
-            var systring = system.ToString();
-            Assert.IsTrue(systring.Contains(Name) && systring.Contains(Bcp47));
+            var sysString = system.ToString();
+            Assert.IsTrue(sysString.Contains(Name) && sysString.Contains(Bcp47));
         }
 
         [Test]