From 6a2ca07b6683f63991518fdea299c6394867771d Mon Sep 17 00:00:00 2001 From: Florian Sylvain Date: Wed, 23 Sep 2026 22:09:06 +0200 Subject: [PATCH] refac: repository updates, image attachment and deletes report the domain not-found error for an unknown id, the same for articles, users and images, and the test fakes stop returning GORM errors --- api/handlers/article/articleHandler.go | 4 + features/write_not_found_errors.feature | 97 +++++++ features/write_not_found_errors.qa.md | 65 +++++ internal/domain/image/imageDomain.go | 3 + .../repositories/articleRepository.go | 12 +- .../repositories/imageRepository.go | 2 +- .../persistence/repositories/notFound.go | 10 + .../repositories/userRepository.go | 9 +- test/article_test.go | 13 +- test/image_test.go | 4 +- test/use_case_baseline_test.go | 100 ++++--- test/user_test.go | 11 +- test/write_not_found_errors_qa_test.go | 30 ++ test/write_not_found_errors_test.go | 259 ++++++++++++++++++ 14 files changed, 553 insertions(+), 66 deletions(-) create mode 100644 features/write_not_found_errors.feature create mode 100644 features/write_not_found_errors.qa.md create mode 100644 test/write_not_found_errors_qa_test.go create mode 100644 test/write_not_found_errors_test.go diff --git a/api/handlers/article/articleHandler.go b/api/handlers/article/articleHandler.go index b005ee4..f585ed0 100644 --- a/api/handlers/article/articleHandler.go +++ b/api/handlers/article/articleHandler.go @@ -127,6 +127,10 @@ func (h *Handler) DeleteArticle(w http.ResponseWriter, r *http.Request) { } if err := h.DeleteUseCase.DeleteArticle(uint32(id)); err != nil { + if errors.Is(err, domain.ErrArticleNotFound) { + http.Error(w, apperr.ArticleNotFoundErrorMessage, http.StatusNotFound) + return + } http.Error(w, "Failed to delete article", http.StatusInternalServerError) return } diff --git a/features/write_not_found_errors.feature b/features/write_not_found_errors.feature new file mode 100644 index 0000000..8f3ad6c --- /dev/null +++ b/features/write_not_found_errors.feature @@ -0,0 +1,97 @@ +Feature: Domain not-found errors on repository writes, image attachment and deletes + + As the maintainer of RenewCMS + I want an update, an image attachment or a delete against an unknown article, user or image id + to report a domain not-found error instead of a raw GORM error or a silent no-op + So that a caller can rely on errors.Is(err, article.ErrArticleNotFound), errors.Is(err, user.ErrUserNotFound) + or errors.Is(err, image.ErrImageNotFound) on every repository method, not just the read side, + and so that DELETE /v1/article/{id} reports 404 instead of leaking a 500 for an id that no longer exists + + Background: + Given internal/domain/article defines the sentinel article.ErrArticleNotFound + And internal/domain/user defines the sentinel user.ErrUserNotFound + And internal/domain/image defines the sentinel image.ErrImageNotFound + And ArticleRepository.UpdateBody, UpdateIsOnline, Delete and AddImage return article.ErrArticleNotFound, + not gorm.ErrRecordNotFound and not a silent no-op, when the article id does not match any row + And ArticleRepository.AddImage returns image.ErrImageNotFound, not gorm.ErrRecordNotFound, + when the article exists but the image id does not match any row + And UserRepository.UpdateVerificationStatus, UpdatePassword, UpdatePasswordResetCode and Delete return + user.ErrUserNotFound, not gorm.ErrRecordNotFound and not a silent no-op, when the user id does not match any row + And ImageRepository.Delete returns image.ErrImageNotFound, not gorm.ErrRecordNotFound, when the image id + does not match any row + And DELETE /v1/article/{id} sits behind the jwtauth Verifier and Authenticator group, and requires a valid jwt cookie + And UpdateArticle, AddImage, UpdateUser, DeleteUser, CreateImage and DeleteImage have no handler mounted in + api/router.go today, so this slice has no other HTTP-observable surface beyond DELETE /v1/article/{id} + + Scenario Outline: The article repository reports a missing article as a domain error on a write + Given a repository backed by an in-memory SQLite database containing an article titled "Backyard Renovation Update" with id 1 + When is called with id 9 + Then the result is the error article.ErrArticleNotFound + And errors.Is(err, gorm.ErrRecordNotFound) is false + + Examples: + | method | extra | + | UpdateBody | and body "New body" | + | UpdateIsOnline | and is_online true | + | Delete | | + + Scenario: AddImage reports a missing article as a domain error, not a missing image + Given a repository backed by an in-memory SQLite database containing an article with id 1 + And the same database containing an image with id 1 + When AddImage is called with article id 9 and image id 1 + Then the result is the error article.ErrArticleNotFound + And errors.Is(err, gorm.ErrRecordNotFound) is false + + Scenario: AddImage reports a missing image as a domain error once the article exists + Given a repository backed by an in-memory SQLite database containing an article with id 1 + And the same database containing no image with id 9 + When AddImage is called with article id 1 and image id 9 + Then the result is the error image.ErrImageNotFound + And errors.Is(err, gorm.ErrRecordNotFound) is false + + Scenario Outline: The user repository reports a missing user as a domain error on a write + Given a repository backed by an in-memory SQLite database containing a user with id 1, username "alice" and email "alice@example.com" + When is called with id 9 + Then the result is the error user.ErrUserNotFound + And errors.Is(err, gorm.ErrRecordNotFound) is false + + Examples: + | method | extra | + | UpdateVerificationStatus | and is_verified true | + | UpdatePassword | and password "newHashedPassword" | + | UpdatePasswordResetCode | and code "newHashedResetCode" | + | Delete | | + + Scenario: The image repository reports a missing image as a domain error on delete + Given a repository backed by an in-memory SQLite database containing an image with id 1 + When Delete is called with id 9 + Then the result is the error image.ErrImageNotFound + And errors.Is(err, gorm.ErrRecordNotFound) is false + + Scenario: The test fakes mirror the real repositories on every write not-found path + Given the in-memory article, user and image fakes used in test/ + When UpdateBody, UpdateIsOnline, Delete or AddImage is called against an id absent from the fake's map + Then the fake returns article.ErrArticleNotFound or image.ErrImageNotFound, matching the real repository + And when UpdateVerificationStatus, UpdatePassword, UpdatePasswordResetCode or Delete is called against an id + absent from the fake's map, the fake returns user.ErrUserNotFound, matching the real repository + And none of the fakes construct or return gorm.ErrRecordNotFound anymore + + Scenario: Deleting an unknown article answers 404, not a silent success or a 500 + Given a valid jwt cookie for a user with id 1 + And the article repository has no article with id 9999 + When the client sends DELETE /v1/article/9999 with that cookie + Then the response status is 404 + And the response body is the text "Article not found" + + Scenario: Deleting an existing article still answers 204 + Given a valid jwt cookie for a user with id 1 + And the article repository contains an article with id 1 + When the client sends DELETE /v1/article/1 with that cookie + Then the response status is 204 + And the article repository no longer contains an article with id 1 + + Scenario: The delete-article route still requires authentication before existence is even checked + Given no jwt cookie is sent + And the article repository has no article with id 9999 + When the client sends DELETE /v1/article/9999 without a cookie + Then the response status is 401 diff --git a/features/write_not_found_errors.qa.md b/features/write_not_found_errors.qa.md new file mode 100644 index 0000000..a7ac7d0 --- /dev/null +++ b/features/write_not_found_errors.qa.md @@ -0,0 +1,65 @@ +# QA procedure: write_not_found_errors + +Package: `test` (black-box, extending `article_test.go` / `user_test.go` / `image_test.go` / `not_found_errors_test.go`). + +## Setup shared by every case +1. Call `validator.InitValidator()` once. +2. Update the in-memory fakes (`mockArticleRepo`, `mockUserRepo`, `mockImageRepo` in `test/`) so that: + - `mockArticleRepo.UpdateBody`, `UpdateIsOnline` and `AddImage` return `article.ErrArticleNotFound` / + `image.ErrImageNotFound` (per the missing id) instead of `gorm.ErrRecordNotFound`; + - `mockArticleRepo.Delete` returns `article.ErrArticleNotFound` when the id isn't in the map, instead of + silently succeeding; + - `mockUserRepo.UpdateVerificationStatus`, `UpdatePassword`, `UpdatePasswordResetCode` return + `user.ErrUserNotFound` instead of `gorm.ErrRecordNotFound`; + - `mockUserRepo.Delete` returns `user.ErrUserNotFound` when the id isn't in the map, instead of silently + succeeding; + - `mockImageRepo.Delete` returns `image.ErrImageNotFound` instead of `gorm.ErrRecordNotFound`. + No fake in `test/` should reference `gorm.ErrRecordNotFound` (or import `gorm.io/gorm`) after this slice, + except where still needed to assert a real repository's behavior in a repository-level test. +3. Wire the real use cases (`useCases.NewUpdateArticleUseCase`, `useCases.NewDeleteArticleUseCase`, + `useCases.NewUpdateUserUseCase`, `useCases.NewDeleteUserUseCase`, `useCases.NewDeleteImageUseCase`) over + the fakes for the use-case-level checks. +4. Build `article.NewHandler(...)` from the article use cases plus a test `jwtauth.New("HS256", []byte("test-secret"), nil)`, + and mount it under a `chi.NewRouter()` group wrapped by `jwtauth.Verifier(token)` + `jwtauth.Authenticator(token)`, + mirroring `api/router.go`. No live server. + +## Repository-level checks (real persistence repositories, in-memory SQLite, no HTTP) +Use the existing `openInMemoryDb(t)` helper from `test/not_found_errors_test.go`. + +5. `db.AutoMigrate(&models.Article{}, &models.Image{})`. Build a real `persistence.NewArticleRepository(db)` and + `persistence.NewImageRepository(db)`. Seed one article (id 1) and one image (id 1) via `Create`. + - `UpdateBody(9, "New body")` → assert `errors.Is(err, article.ErrArticleNotFound)` true, + `errors.Is(err, gorm.ErrRecordNotFound)` false. + - `UpdateIsOnline(9, true)` → same assertions. + - `Delete(9)` → same assertions. Then `Delete(1)` on the seeded article → assert `err == nil` and a + subsequent `Get(1)` returns `article.ErrArticleNotFound` (the row is actually gone). + - `AddImage(9, 1)` (unknown article, known image) → assert `errors.Is(err, article.ErrArticleNotFound)` true. + - `AddImage(1, 9)` (known article, unknown image) → assert `errors.Is(err, image.ErrImageNotFound)` true, + `errors.Is(err, gorm.ErrRecordNotFound)` false. +6. On the same or a fresh in-memory DB, `db.AutoMigrate(&models.User{})`. Build a real + `persistence.NewUserRepository(db)`, seed a user id 1. + - `UpdateVerificationStatus(9, true)`, `UpdatePassword(9, "x")`, `UpdatePasswordResetCode(9, "y")` → each + asserts `errors.Is(err, user.ErrUserNotFound)` true, `errors.Is(err, gorm.ErrRecordNotFound)` false. + - `Delete(9)` → same assertions. Then `Delete(1)` on the seeded user → assert `err == nil` and a + subsequent `Get(1)` returns `user.ErrUserNotFound`. +7. `db.AutoMigrate(&models.Image{})`. Build a real `persistence.NewImageRepository(db)`, seed one image via + direct DB insert (or `Create` with a fake `io.Reader` and `UPLOAD_DIR` pointed at `t.TempDir()`). + - `Delete(9)` → assert `errors.Is(err, image.ErrImageNotFound)` true, `errors.Is(err, gorm.ErrRecordNotFound)` false. + +## Fake-level checks (mirrors the real repositories, package-internal, no HTTP) +8. For each of `mockArticleRepo`, `mockUserRepo`, `mockImageRepo`, call the write methods listed in step 2 + against an id absent from the fake's map and assert they return the matching domain sentinel, never + `gorm.ErrRecordNotFound`. + +## HTTP checks (fakes, per the shared setup above) — DELETE /v1/article/{id} only +9. Mint a valid jwt cookie for user id 1 (as in `not_found_errors_test.go`, step 8 there). +10. `DELETE /v1/article/9999` with that cookie, empty repo id 9999 → assert status `404`, body + `"Article not found\n"`. +11. `DELETE /v1/article/1` with that cookie, repo seeded with an article id 1 → assert status `204`, and that + the fake no longer holds id 1 afterward. +12. `DELETE /v1/article/9999` with **no** cookie → assert status `401` (regression: jwtauth still gates the route). + +## Manual verification (not exercised by the Go test) +- None. `UpdateArticle`, `AddImage`, `UpdateUser`, `DeleteUser`, `CreateImage` and `DeleteImage` have no HTTP + route today (not mounted in `api/router.go`), so there is no SPA-visible behavior or manual HTTP check for + them in this slice — only the repository/fake-level checks above apply. No SMTP involvement. diff --git a/internal/domain/image/imageDomain.go b/internal/domain/image/imageDomain.go index 595fb80..ad91adf 100644 --- a/internal/domain/image/imageDomain.go +++ b/internal/domain/image/imageDomain.go @@ -1,10 +1,13 @@ package image import ( + "errors" "io" "time" ) +var ErrImageNotFound = errors.New("image not found") + type ImageInput struct { Content io.Reader Filename string diff --git a/internal/infrastructure/persistence/repositories/articleRepository.go b/internal/infrastructure/persistence/repositories/articleRepository.go index 953e21a..0f7652c 100644 --- a/internal/infrastructure/persistence/repositories/articleRepository.go +++ b/internal/infrastructure/persistence/repositories/articleRepository.go @@ -2,6 +2,7 @@ package persistence import ( domain "RenewCMS/internal/domain/article" + imageDomain "RenewCMS/internal/domain/image" "RenewCMS/internal/infrastructure/persistence/mappers" entity "RenewCMS/internal/infrastructure/persistence/models" @@ -89,7 +90,7 @@ func (a *ArticleRepository) UpdateBody(id uint32, body string) (domain.Article, var localArticle entity.Article err := a.db.Model(&entity.Article{}).First(&localArticle, id).Error if err != nil { - return domain.Article{}, err + return domain.Article{}, mapNotFoundErr(err, domain.ErrArticleNotFound) } localArticle.Body = body @@ -107,7 +108,7 @@ func (a *ArticleRepository) UpdateIsOnline(id uint32, isOnline bool) (domain.Art var localArticle entity.Article err := a.db.Model(&entity.Article{}).First(&localArticle, id).Error if err != nil { - return domain.Article{}, err + return domain.Article{}, mapNotFoundErr(err, domain.ErrArticleNotFound) } localArticle.IsOnline = isOnline @@ -120,20 +121,21 @@ func (a *ArticleRepository) UpdateIsOnline(id uint32, isOnline bool) (domain.Art } func (a *ArticleRepository) Delete(id uint32) error { - return a.db.Delete(&entity.Article{}, id).Error + result := a.db.Delete(&entity.Article{}, id) + return deleteOrNotFound(result, domain.ErrArticleNotFound) } func (a *ArticleRepository) AddImage(articleId uint32, imageId uint32) error { var localArticle entity.Article err := a.db.Model(&entity.Article{}).First(&localArticle, articleId).Error if err != nil { - return err + return mapNotFoundErr(err, domain.ErrArticleNotFound) } var localImage entity.Image err = a.db.Model(&entity.Image{}).First(&localImage, imageId).Error if err != nil { - return err + return mapNotFoundErr(err, imageDomain.ErrImageNotFound) } err = a.db.Model(&localArticle).Association("Images").Append(&localImage) diff --git a/internal/infrastructure/persistence/repositories/imageRepository.go b/internal/infrastructure/persistence/repositories/imageRepository.go index 58f7046..bc1f160 100644 --- a/internal/infrastructure/persistence/repositories/imageRepository.go +++ b/internal/infrastructure/persistence/repositories/imageRepository.go @@ -65,7 +65,7 @@ func (i ImageRepository) Delete(id uint32) error { var entityImage entity.Image err := i.db.Model(&entity.Image{}).First(&entityImage, id).Error if err != nil { - return err + return mapNotFoundErr(err, domain.ErrImageNotFound) } fileName := filepath.Base(entityImage.Path) diff --git a/internal/infrastructure/persistence/repositories/notFound.go b/internal/infrastructure/persistence/repositories/notFound.go index 331d321..7784879 100644 --- a/internal/infrastructure/persistence/repositories/notFound.go +++ b/internal/infrastructure/persistence/repositories/notFound.go @@ -12,3 +12,13 @@ func mapNotFoundErr(err error, sentinel error) error { } return err } + +func deleteOrNotFound(result *gorm.DB, sentinel error) error { + if result.Error != nil { + return result.Error + } + if result.RowsAffected == 0 { + return sentinel + } + return nil +} diff --git a/internal/infrastructure/persistence/repositories/userRepository.go b/internal/infrastructure/persistence/repositories/userRepository.go index ede74bf..9009760 100644 --- a/internal/infrastructure/persistence/repositories/userRepository.go +++ b/internal/infrastructure/persistence/repositories/userRepository.go @@ -46,7 +46,8 @@ func (u *UserRepository) Create(user domainUser.User) (domainUser.User, error) { } func (u *UserRepository) Delete(id uint32) error { - return u.db.Delete(&domainUser.User{}, id).Error + result := u.db.Delete(&domainUser.User{}, id) + return deleteOrNotFound(result, domainUser.ErrUserNotFound) } func (u *UserRepository) GetAll() []domainUser.User { @@ -85,7 +86,7 @@ func (u *UserRepository) UpdateVerificationStatus(userId uint32, isVerified bool var localUser entity.User err := u.db.Model(&entity.User{}).First(&localUser, userId).Error if err != nil { - return domainUser.User{}, err + return domainUser.User{}, mapNotFoundErr(err, domainUser.ErrUserNotFound) } localUser.IsVerified = isVerified @@ -101,7 +102,7 @@ func (u *UserRepository) UpdatePassword(userId uint32, password string) (domainU var localUser entity.User err := u.db.Model(&entity.User{}).First(&localUser, userId).Error if err != nil { - return domainUser.User{}, err + return domainUser.User{}, mapNotFoundErr(err, domainUser.ErrUserNotFound) } localUser.Password = password @@ -117,7 +118,7 @@ func (u *UserRepository) UpdatePasswordResetCode(userId uint32, code string) (do var localUser entity.User err := u.db.Model(&entity.User{}).First(&localUser, userId).Error if err != nil { - return domainUser.User{}, err + return domainUser.User{}, mapNotFoundErr(err, domainUser.ErrUserNotFound) } localUser.PasswordResetCode = code diff --git a/test/article_test.go b/test/article_test.go index 71d3bfb..c040d2a 100644 --- a/test/article_test.go +++ b/test/article_test.go @@ -7,8 +7,6 @@ import ( "errors" "sort" "testing" - - "gorm.io/gorm" ) type mockArticleRepo struct { @@ -99,7 +97,7 @@ func (m *mockArticleRepo) UpdateBody(id uint32, body string) (article.Article, e } a, ok := m.articles[id] if !ok { - return article.Article{}, gorm.ErrRecordNotFound + return article.Article{}, article.ErrArticleNotFound } a.Body = body m.articles[id] = a @@ -112,7 +110,7 @@ func (m *mockArticleRepo) UpdateIsOnline(id uint32, isOnline bool) (article.Arti } a, ok := m.articles[id] if !ok { - return article.Article{}, gorm.ErrRecordNotFound + return article.Article{}, article.ErrArticleNotFound } a.IsOnline = isOnline m.articles[id] = a @@ -123,6 +121,9 @@ func (m *mockArticleRepo) Delete(id uint32) error { if err := m.fail.trigger("Delete"); err != nil { return err } + if _, ok := m.articles[id]; !ok { + return article.ErrArticleNotFound + } delete(m.articles, id) return nil } @@ -133,11 +134,11 @@ func (m *mockArticleRepo) AddImage(articleId uint32, imageId uint32) error { } a, ok := m.articles[articleId] if !ok { - return gorm.ErrRecordNotFound + return article.ErrArticleNotFound } img, ok := m.images[imageId] if !ok { - return gorm.ErrRecordNotFound + return imageDomain.ErrImageNotFound } a.Images = append(a.Images, img) m.articles[articleId] = a diff --git a/test/image_test.go b/test/image_test.go index f51779c..5b7440d 100644 --- a/test/image_test.go +++ b/test/image_test.go @@ -7,8 +7,6 @@ import ( "io" "strings" "testing" - - "gorm.io/gorm" ) var errUnsupportedImageContentType = errors.New("the file must be a PNG, JPEG, WEBP, or SVG image") @@ -51,7 +49,7 @@ func (m *mockImageRepo) Delete(id uint32) error { return err } if _, ok := m.images[id]; !ok { - return gorm.ErrRecordNotFound + return imageDomain.ErrImageNotFound } delete(m.images, id) return nil diff --git a/test/use_case_baseline_test.go b/test/use_case_baseline_test.go index 704c217..707a4e3 100644 --- a/test/use_case_baseline_test.go +++ b/test/use_case_baseline_test.go @@ -11,8 +11,6 @@ import ( "strings" "testing" "time" - - "gorm.io/gorm" ) func TestCreateArticleTitleLengthLimit(t *testing.T) { @@ -258,8 +256,8 @@ func TestUpdateArticleDelegatesEachOperation(t *testing.T) { repo := newRepo() uc := useCases.NewUpdateArticleUseCase(repo) _, err := uc.UpdateBody(9, "New body") - if !errors.Is(err, gorm.ErrRecordNotFound) { - t.Errorf("expected gorm.ErrRecordNotFound, got %v", err) + if !errors.Is(err, article.ErrArticleNotFound) { + t.Errorf("expected article.ErrArticleNotFound, got %v", err) } }) @@ -279,8 +277,8 @@ func TestUpdateArticleDelegatesEachOperation(t *testing.T) { repo := newRepo() uc := useCases.NewUpdateArticleUseCase(repo) _, err := uc.UpdateIsOnline(9, true) - if !errors.Is(err, gorm.ErrRecordNotFound) { - t.Errorf("expected gorm.ErrRecordNotFound, got %v", err) + if !errors.Is(err, article.ErrArticleNotFound) { + t.Errorf("expected article.ErrArticleNotFound, got %v", err) } }) @@ -304,8 +302,8 @@ func TestUpdateArticleDelegatesEachOperation(t *testing.T) { repo := newRepo() uc := useCases.NewUpdateArticleUseCase(repo) err := uc.AddImage(9, 1) - if !errors.Is(err, gorm.ErrRecordNotFound) { - t.Errorf("expected gorm.ErrRecordNotFound, got %v", err) + if !errors.Is(err, article.ErrArticleNotFound) { + t.Errorf("expected article.ErrArticleNotFound, got %v", err) } }) @@ -313,25 +311,34 @@ func TestUpdateArticleDelegatesEachOperation(t *testing.T) { repo := newRepo() uc := useCases.NewUpdateArticleUseCase(repo) err := uc.AddImage(1, 9) - if !errors.Is(err, gorm.ErrRecordNotFound) { - t.Errorf("expected gorm.ErrRecordNotFound, got %v", err) + if !errors.Is(err, imageDomain.ErrImageNotFound) { + t.Errorf("expected image.ErrImageNotFound, got %v", err) } }) } -func TestDeleteArticleSucceedsSilently(t *testing.T) { - for _, id := range []uint32{1, 9} { - t.Run(fmt.Sprintf("id_%d", id), func(t *testing.T) { - repo := newMockArticleRepo() - repo.articles[1] = article.Article{ID: 1} - uc := useCases.NewDeleteArticleUseCase(repo) +func TestDeleteArticleReportsMissingArticle(t *testing.T) { + t.Run("id_1", func(t *testing.T) { + repo := newMockArticleRepo() + repo.articles[1] = article.Article{ID: 1} + uc := useCases.NewDeleteArticleUseCase(repo) - err := uc.DeleteArticle(id) - if err != nil { - t.Errorf("expected no error, got %v", err) - } - }) - } + err := uc.DeleteArticle(1) + if err != nil { + t.Errorf("expected no error, got %v", err) + } + }) + + t.Run("id_9", func(t *testing.T) { + repo := newMockArticleRepo() + repo.articles[1] = article.Article{ID: 1} + uc := useCases.NewDeleteArticleUseCase(repo) + + err := uc.DeleteArticle(9) + if !errors.Is(err, article.ErrArticleNotFound) { + t.Errorf("expected article.ErrArticleNotFound, got %v", err) + } + }) } func TestDeleteArticlePropagatesRepositoryFailure(t *testing.T) { @@ -425,8 +432,8 @@ func TestDeleteImageRequiresExistence(t *testing.T) { uc := useCases.NewDeleteImageUseCase(repo) err := uc.DeleteImage(9) - if !errors.Is(err, gorm.ErrRecordNotFound) { - t.Errorf("expected gorm.ErrRecordNotFound, got %v", err) + if !errors.Is(err, imageDomain.ErrImageNotFound) { + t.Errorf("expected image.ErrImageNotFound, got %v", err) } }) } @@ -604,8 +611,8 @@ func TestUpdateUserDelegatesEachOperation(t *testing.T) { repo := newRepo() uc := useCases.NewUpdateUserUseCase(repo, &stubHasher{}) _, err := uc.UpdateVerificationStatus(9, true) - if !errors.Is(err, gorm.ErrRecordNotFound) { - t.Errorf("expected gorm.ErrRecordNotFound, got %v", err) + if !errors.Is(err, user.ErrUserNotFound) { + t.Errorf("expected user.ErrUserNotFound, got %v", err) } }) @@ -625,8 +632,8 @@ func TestUpdateUserDelegatesEachOperation(t *testing.T) { repo := newRepo() uc := useCases.NewUpdateUserUseCase(repo, &stubHasher{}) _, err := uc.UpdatePassword(9, "newpassword123") - if !errors.Is(err, gorm.ErrRecordNotFound) { - t.Errorf("expected gorm.ErrRecordNotFound, got %v", err) + if !errors.Is(err, user.ErrUserNotFound) { + t.Errorf("expected user.ErrUserNotFound, got %v", err) } }) @@ -646,25 +653,34 @@ func TestUpdateUserDelegatesEachOperation(t *testing.T) { repo := newRepo() uc := useCases.NewUpdateUserUseCase(repo, &stubHasher{}) _, err := uc.UpdatePasswordResetCode(9, "reset-code-123") - if !errors.Is(err, gorm.ErrRecordNotFound) { - t.Errorf("expected gorm.ErrRecordNotFound, got %v", err) + if !errors.Is(err, user.ErrUserNotFound) { + t.Errorf("expected user.ErrUserNotFound, got %v", err) } }) } -func TestDeleteUserSucceedsSilently(t *testing.T) { - for _, id := range []uint32{1, 9} { - t.Run(fmt.Sprintf("id_%d", id), func(t *testing.T) { - repo := newMockUserRepo() - repo.users[1] = user.User{ID: 1} - uc := useCases.NewDeleteUserUseCase(repo) +func TestDeleteUserReportsMissingUser(t *testing.T) { + t.Run("id_1", func(t *testing.T) { + repo := newMockUserRepo() + repo.users[1] = user.User{ID: 1} + uc := useCases.NewDeleteUserUseCase(repo) - err := uc.DeleteUser(id) - if err != nil { - t.Errorf("expected no error, got %v", err) - } - }) - } + err := uc.DeleteUser(1) + if err != nil { + t.Errorf("expected no error, got %v", err) + } + }) + + t.Run("id_9", func(t *testing.T) { + repo := newMockUserRepo() + repo.users[1] = user.User{ID: 1} + uc := useCases.NewDeleteUserUseCase(repo) + + err := uc.DeleteUser(9) + if !errors.Is(err, user.ErrUserNotFound) { + t.Errorf("expected user.ErrUserNotFound, got %v", err) + } + }) } func TestSendMailForwardsArgumentsVerbatim(t *testing.T) { diff --git a/test/user_test.go b/test/user_test.go index 6d9668d..b2eaf30 100644 --- a/test/user_test.go +++ b/test/user_test.go @@ -6,8 +6,6 @@ import ( "errors" "sort" "testing" - - "gorm.io/gorm" ) type stubHasher struct { @@ -106,6 +104,9 @@ func (m *mockUserRepo) Delete(id uint32) error { if err := m.fail.trigger("Delete"); err != nil { return err } + if _, ok := m.users[id]; !ok { + return user.ErrUserNotFound + } delete(m.users, id) return nil } @@ -116,7 +117,7 @@ func (m *mockUserRepo) UpdateVerificationStatus(userId uint32, isVerified bool) } u, ok := m.users[userId] if !ok { - return user.User{}, gorm.ErrRecordNotFound + return user.User{}, user.ErrUserNotFound } u.IsVerified = isVerified m.users[userId] = u @@ -129,7 +130,7 @@ func (m *mockUserRepo) UpdatePassword(userId uint32, password string) (user.User } u, ok := m.users[userId] if !ok { - return user.User{}, gorm.ErrRecordNotFound + return user.User{}, user.ErrUserNotFound } u.Password = password m.users[userId] = u @@ -142,7 +143,7 @@ func (m *mockUserRepo) UpdatePasswordResetCode(userId uint32, code string) (user } u, ok := m.users[userId] if !ok { - return user.User{}, gorm.ErrRecordNotFound + return user.User{}, user.ErrUserNotFound } u.PasswordResetCode = code m.users[userId] = u diff --git a/test/write_not_found_errors_qa_test.go b/test/write_not_found_errors_qa_test.go new file mode 100644 index 0000000..f923f1e --- /dev/null +++ b/test/write_not_found_errors_qa_test.go @@ -0,0 +1,30 @@ +package test + +import ( + domainArticle "RenewCMS/internal/domain/article" + domainUser "RenewCMS/internal/domain/user" + "errors" + "testing" +) + +func TestDeletingTheSeededArticleActuallyRemovesTheRow(t *testing.T) { + repo := newSeededArticleRepository(t) + + if err := repo.Delete(1); err != nil { + t.Fatalf("expected Delete(1) to succeed, got %v", err) + } + if _, err := repo.Get(1); !errors.Is(err, domainArticle.ErrArticleNotFound) { + t.Errorf("expected article.ErrArticleNotFound after delete, got %v", err) + } +} + +func TestDeletingTheSeededUserActuallyRemovesTheRow(t *testing.T) { + repo := newSeededUserRepository(t) + + if err := repo.Delete(1); err != nil { + t.Fatalf("expected Delete(1) to succeed, got %v", err) + } + if _, err := repo.Get(1); !errors.Is(err, domainUser.ErrUserNotFound) { + t.Errorf("expected user.ErrUserNotFound after delete, got %v", err) + } +} diff --git a/test/write_not_found_errors_test.go b/test/write_not_found_errors_test.go new file mode 100644 index 0000000..e7e9301 --- /dev/null +++ b/test/write_not_found_errors_test.go @@ -0,0 +1,259 @@ +package test + +import ( + "RenewCMS/api/handlers/article" + "RenewCMS/api/pkg/apperr" + validator "RenewCMS/api/pkg/validator" + domainArticle "RenewCMS/internal/domain/article" + domainImage "RenewCMS/internal/domain/image" + domainUser "RenewCMS/internal/domain/user" + "RenewCMS/internal/infrastructure/persistence/models" + persistence "RenewCMS/internal/infrastructure/persistence/repositories" + "RenewCMS/internal/infrastructure/useCases" + "errors" + "net/http" + "net/http/httptest" + "testing" + + "github.com/go-chi/chi/v5" + "github.com/go-chi/jwtauth/v5" + "gorm.io/gorm" +) + +func newSeededArticleRepository(t *testing.T) *persistence.ArticleRepository { + t.Helper() + db := openInMemoryDb(t) + if err := db.AutoMigrate(&models.Article{}, &models.Image{}); err != nil { + t.Fatalf("failed to migrate: %v", err) + } + repo := persistence.NewArticleRepository(db) + if _, err := repo.Create(domainArticle.Article{Title: "Backyard Renovation Update", Body: "Body"}); err != nil { + t.Fatalf("failed to seed article: %v", err) + } + return repo +} + +func assertWriteResultIsArticleNotFound(t *testing.T, err error) { + t.Helper() + if !errors.Is(err, domainArticle.ErrArticleNotFound) { + t.Errorf("expected article.ErrArticleNotFound, got %v", err) + } + if errors.Is(err, gorm.ErrRecordNotFound) { + t.Errorf("expected the domain error, not gorm.ErrRecordNotFound") + } +} + +func assertWriteResultIsImageNotFound(t *testing.T, err error) { + t.Helper() + if !errors.Is(err, domainImage.ErrImageNotFound) { + t.Errorf("expected image.ErrImageNotFound, got %v", err) + } + if errors.Is(err, gorm.ErrRecordNotFound) { + t.Errorf("expected the domain error, not gorm.ErrRecordNotFound") + } +} + +func assertWriteResultIsUserNotFound(t *testing.T, err error) { + t.Helper() + if !errors.Is(err, domainUser.ErrUserNotFound) { + t.Errorf("expected user.ErrUserNotFound, got %v", err) + } + if errors.Is(err, gorm.ErrRecordNotFound) { + t.Errorf("expected the domain error, not gorm.ErrRecordNotFound") + } +} + +func TestTheArticleRepositoryReportsAMissingArticleAsADomainErrorOnAWrite(t *testing.T) { + t.Run("UpdateBody and body New body", func(t *testing.T) { + repo := newSeededArticleRepository(t) + _, err := repo.UpdateBody(9, "New body") + assertWriteResultIsArticleNotFound(t, err) + }) + + t.Run("UpdateIsOnline and is_online true", func(t *testing.T) { + repo := newSeededArticleRepository(t) + _, err := repo.UpdateIsOnline(9, true) + assertWriteResultIsArticleNotFound(t, err) + }) + + t.Run("Delete", func(t *testing.T) { + repo := newSeededArticleRepository(t) + err := repo.Delete(9) + assertWriteResultIsArticleNotFound(t, err) + }) +} + +func TestAddImageReportsAMissingArticleAsADomainErrorNotAMissingImage(t *testing.T) { + repo := newSeededArticleRepository(t) + + err := repo.AddImage(9, 1) + assertWriteResultIsArticleNotFound(t, err) +} + +func TestAddImageReportsAMissingImageAsADomainErrorOnceTheArticleExists(t *testing.T) { + repo := newSeededArticleRepository(t) + + err := repo.AddImage(1, 9) + assertWriteResultIsImageNotFound(t, err) +} + +func newSeededUserRepository(t *testing.T) *persistence.UserRepository { + t.Helper() + db := openInMemoryDb(t) + if err := db.AutoMigrate(&models.User{}); err != nil { + t.Fatalf("failed to migrate: %v", err) + } + repo := persistence.NewUserRepository(db) + if _, err := repo.Create(domainUser.User{Username: "alice", Password: "hunter2example", Email: "alice@example.com", VerificationCode: "code"}); err != nil { + t.Fatalf("failed to seed user: %v", err) + } + return repo +} + +func TestTheUserRepositoryReportsAMissingUserAsADomainErrorOnAWrite(t *testing.T) { + t.Run("UpdateVerificationStatus and is_verified true", func(t *testing.T) { + repo := newSeededUserRepository(t) + _, err := repo.UpdateVerificationStatus(9, true) + assertWriteResultIsUserNotFound(t, err) + }) + + t.Run("UpdatePassword and password newHashedPassword", func(t *testing.T) { + repo := newSeededUserRepository(t) + _, err := repo.UpdatePassword(9, "newHashedPassword") + assertWriteResultIsUserNotFound(t, err) + }) + + t.Run("UpdatePasswordResetCode and code newHashedResetCode", func(t *testing.T) { + repo := newSeededUserRepository(t) + _, err := repo.UpdatePasswordResetCode(9, "newHashedResetCode") + assertWriteResultIsUserNotFound(t, err) + }) + + t.Run("Delete", func(t *testing.T) { + repo := newSeededUserRepository(t) + err := repo.Delete(9) + assertWriteResultIsUserNotFound(t, err) + }) +} + +func TestTheImageRepositoryReportsAMissingImageAsADomainErrorOnDelete(t *testing.T) { + db := openInMemoryDb(t) + if err := db.AutoMigrate(&models.Image{}); err != nil { + t.Fatalf("failed to migrate: %v", err) + } + if err := db.Create(&models.Image{Path: "/static/uploadedImages/one.png"}).Error; err != nil { + t.Fatalf("failed to seed image: %v", err) + } + repo := persistence.NewImageRepository(db) + + err := repo.Delete(9) + assertWriteResultIsImageNotFound(t, err) +} + +func TestTheFakesMirrorTheRealRepositoriesOnEveryWriteNotFoundPath(t *testing.T) { + articleRepo := newMockArticleRepo() + articleRepo.articles[1] = domainArticle.Article{ID: 1, Title: "Backyard Renovation Update"} + + if _, err := articleRepo.UpdateBody(9, "New body"); !errors.Is(err, domainArticle.ErrArticleNotFound) { + t.Errorf("expected article.ErrArticleNotFound, got %v", err) + } + if _, err := articleRepo.UpdateIsOnline(9, true); !errors.Is(err, domainArticle.ErrArticleNotFound) { + t.Errorf("expected article.ErrArticleNotFound, got %v", err) + } + if err := articleRepo.Delete(9); !errors.Is(err, domainArticle.ErrArticleNotFound) { + t.Errorf("expected article.ErrArticleNotFound, got %v", err) + } + if err := articleRepo.AddImage(9, 1); !errors.Is(err, domainArticle.ErrArticleNotFound) { + t.Errorf("expected article.ErrArticleNotFound, got %v", err) + } + articleRepo.images[1] = &domainImage.Image{ID: 1} + if err := articleRepo.AddImage(1, 9); !errors.Is(err, domainImage.ErrImageNotFound) { + t.Errorf("expected image.ErrImageNotFound, got %v", err) + } + + userRepo := newMockUserRepo() + userRepo.users[1] = domainUser.User{ID: 1, Username: "alice"} + if _, err := userRepo.UpdateVerificationStatus(9, true); !errors.Is(err, domainUser.ErrUserNotFound) { + t.Errorf("expected user.ErrUserNotFound, got %v", err) + } + if _, err := userRepo.UpdatePassword(9, "newHashedPassword"); !errors.Is(err, domainUser.ErrUserNotFound) { + t.Errorf("expected user.ErrUserNotFound, got %v", err) + } + if _, err := userRepo.UpdatePasswordResetCode(9, "newHashedResetCode"); !errors.Is(err, domainUser.ErrUserNotFound) { + t.Errorf("expected user.ErrUserNotFound, got %v", err) + } + if err := userRepo.Delete(9); !errors.Is(err, domainUser.ErrUserNotFound) { + t.Errorf("expected user.ErrUserNotFound, got %v", err) + } +} + +func newDeleteArticleRouter(articleRepo *mockArticleRepo, token *jwtauth.JWTAuth) *chi.Mux { + articleHandler := article.NewHandler( + useCases.NewCreateArticleUseCase(articleRepo), + useCases.NewGetArticleUseCase(articleRepo), + useCases.NewListArticlesUseCase(articleRepo), + useCases.NewDeleteArticleUseCase(articleRepo), + ) + + r := chi.NewRouter() + r.Route("/v1", func(r chi.Router) { + r.Group(func(r chi.Router) { + r.Use(jwtauth.Verifier(token)) + r.Use(jwtauth.Authenticator(token)) + r.Route("/article", func(r chi.Router) { + r.Delete("/{id}", articleHandler.DeleteArticle) + }) + }) + }) + return r +} + +func TestWriteNotFoundErrorsHttp(t *testing.T) { + validator.InitValidator() + token := jwtauth.New("HS256", []byte("test-secret"), nil) + + t.Run("Deleting an unknown article answers 404, not a silent success or a 500", func(t *testing.T) { + articleRepo := newMockArticleRepo() + router := newDeleteArticleRouter(articleRepo, token) + req := httptest.NewRequest(http.MethodDelete, "/v1/article/9999", nil) + setJwtCookie(t, req, token, 1) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusNotFound { + t.Fatalf("expected status 404, got %d", rec.Code) + } + if rec.Body.String() != apperr.ArticleNotFoundErrorMessage+"\n" { + t.Errorf("unexpected body: %q", rec.Body.String()) + } + }) + + t.Run("Deleting an existing article still answers 204", func(t *testing.T) { + articleRepo := newMockArticleRepo() + articleRepo.articles[1] = domainArticle.Article{ID: 1, Title: "Backyard Renovation Update"} + router := newDeleteArticleRouter(articleRepo, token) + req := httptest.NewRequest(http.MethodDelete, "/v1/article/1", nil) + setJwtCookie(t, req, token, 1) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusNoContent { + t.Fatalf("expected status 204, got %d", rec.Code) + } + if _, exists := articleRepo.articles[1]; exists { + t.Errorf("expected article 1 to have been deleted") + } + }) + + t.Run("The delete-article route still requires authentication before existence is even checked", func(t *testing.T) { + articleRepo := newMockArticleRepo() + router := newDeleteArticleRouter(articleRepo, token) + req := httptest.NewRequest(http.MethodDelete, "/v1/article/9999", nil) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusUnauthorized { + t.Fatalf("expected status 401, got %d", rec.Code) + } + }) +}