diff --git a/api/handlers/article/articleHandler.go b/api/handlers/article/articleHandler.go index 34f803e..b005ee4 100644 --- a/api/handlers/article/articleHandler.go +++ b/api/handlers/article/articleHandler.go @@ -23,7 +23,7 @@ type Handler struct { } type PostArticle struct { - Title string `json:"title" validate:"required,min=3,max=50"` + Title string `json:"title" validate:"required,min=3,max=200"` Body string `json:"body" validate:"required,max=10000"` } @@ -81,6 +81,10 @@ func (h *Handler) PostArticle(w http.ResponseWriter, r *http.Request) { Body: localArticle.Body, }) if err != nil { + if errors.Is(err, domain.ErrInvalidTitle) { + http.Error(w, apperr.ArticleTitleErrorMessage, http.StatusBadRequest) + return + } http.Error(w, "Failed to create article", http.StatusInternalServerError) return } diff --git a/api/pkg/apperr/messages.go b/api/pkg/apperr/messages.go index 9d86fb3..0f84a72 100644 --- a/api/pkg/apperr/messages.go +++ b/api/pkg/apperr/messages.go @@ -4,3 +4,4 @@ const LogsErrorMessage = "Access to the requested resource is forbidden due to i const BodyErrorMessage = "The request cannot be processed due to a mismatch in the format of the body." const IdUint32ErrorMessage = "The server expects the ID to be in the format of an unsigned 32-bit integer (uint32)." const ArticleNotFoundErrorMessage = "Article not found" +const ArticleTitleErrorMessage = "Article title must be between 3 and 200 characters." diff --git a/features/article_title_rules.feature b/features/article_title_rules.feature new file mode 100644 index 0000000..3620714 --- /dev/null +++ b/features/article_title_rules.feature @@ -0,0 +1,85 @@ +Feature: The article title rule is the same in the request and the domain + + As the maintainer of RenewCMS + I want PostArticle's title validation and article.CreateArticle's title validation to + enforce the identical rule, counted the identical way + So that a title the request layer accepts can never be rejected by the domain as a + surprise 500, and a domain title rejection that does happen is always reported as a + 400 with a client-facing apperr message instead of a raw 500 + + Background: + Given internal/domain/article defines the sentinel article.ErrInvalidTitle + And CreateArticleUseCase.CreateArticle rejects a title whose length, counted with + utf8.RuneCountInString (runes, not bytes), is below 3 or above 200, returning + article.ErrInvalidTitle and storing nothing + And api/handlers/article/articleHandler.go's PostArticle.Title carries + validate:"required,min=3,max=200", and go-playground/validator's min/max on a + string already count runes with utf8.RuneCountInString, so the request tag and the + domain check now agree on both the bound (3-200) and the unit (runes) + And api/pkg/apperr defines ArticleTitleErrorMessage as "Article title must be + between 3 and 200 characters.", a client-facing string distinct from both the raw + validator error text and "Failed to create article" + And Handler.PostArticle maps errors.Is(err, article.ErrInvalidTitle) from + CreateArticleUseCase.CreateArticle to a 400 response with body + apperr.ArticleTitleErrorMessage; any other error from CreateArticle still answers + 500 with body "Failed to create article" + And POST /v1/article/ sits behind the jwtauth Verifier and Authenticator group and + requires a valid jwt cookie + And UpdateArticleUseCase exposes only UpdateBody, AddImage and UpdateIsOnline; no + route or use case updates an article's title after creation, so this feature only + concerns article creation + + Scenario Outline: CreateArticle now measures title length in runes, not bytes + Given an empty article repository fake + When CreateArticle is called with a title made of repetitions of the + 4-byte rune "😀" and a body "Some body text" + Then the result is + + Examples: + | rune_count | outcome | + | 2 | ErrInvalidTitle, nothing stored | + | 3 | created | + | 200 | created | + | 201 | ErrInvalidTitle, nothing stored | + + Scenario Outline: POST /v1/article/ enforces the same 3-200 rune boundary over HTTP + Given a valid jwt cookie for a user with id 1 + When the client sends POST /v1/article/ with that cookie, a title of ASCII + characters and a body "Some body text" + Then the response status is + And + + Examples: + | length | status | storage_effect | + | 2 | 400 | not created | + | 3 | 201 | created, is_online false | + | 200 | 201 | created, is_online false | + | 201 | 400 | not created | + + Scenario: A 200-rune multibyte title is accepted end to end over HTTP + Given a valid jwt cookie for a user with id 1 + When the client sends POST /v1/article/ with that cookie, a title made of 200 + repetitions of the 4-byte rune "😀" (200 runes, 800 bytes) and a body + "Some body text" + Then the response status is 201 + And the response body is the created article's JSON with that title + + Scenario: The handler maps a domain title rejection to 400, not 500 + Given a valid jwt cookie for a user with id 1 + And the article repository fake is configured so its next Create call returns + article.ErrInvalidTitle directly, standing in for any layer beneath the use + case's own check rejecting the title with that same sentinel + When the client sends POST /v1/article/ with that cookie, title "Valid Title" and + body "Some body text" (both already satisfying PostArticle's own validate tags) + Then the response status is 400 + And the response body is the text apperr.ArticleTitleErrorMessage + And the response body is not "Failed to create article" + + Scenario: A non-title repository failure during creation still answers 500 + Given a valid jwt cookie for a user with id 1 + And the article repository fake is configured to fail on its next Create call with + a generic repository error, not article.ErrInvalidTitle + When the client sends POST /v1/article/ with that cookie, title "Valid Title" and + body "Some body text" + Then the response status is 500 + And the response body is the text "Failed to create article" diff --git a/features/article_title_rules.qa.md b/features/article_title_rules.qa.md new file mode 100644 index 0000000..223f702 --- /dev/null +++ b/features/article_title_rules.qa.md @@ -0,0 +1,76 @@ +# QA procedure: article_title_rules + +Package: `test` (black-box, alongside `article_test.go` / `use_case_baseline_test.go` / +`not_found_errors_test.go` / the hasher-domain-interface tests). + +## Fakes needed +1. Extend `mockArticleRepo` (`test/article_test.go`) / its shared `failSwitch` + (`test/fakes_common.go`) so a configured failure can return a specific error, not + only the generic `errRepositoryFailure`. Today `failSwitch.trigger` always returns + the same sentinel; add a way to say "the next call to `Create` returns + `article.ErrInvalidTitle`" specifically, distinct from "the next call to `Create` + returns a generic repository error" (e.g. a `failNextWith(method string, err error)` + on `failSwitch`, keeping `errRepositoryFailure` as the default so existing + generic-failure tests don't change). +2. Add a `createCalled` flag (or equivalent) to `mockArticleRepo` if not already + present, so a test can assert `Create` was never reached when request-layer + validation rejects a title first. + +## Use-case-level checks (fake repo, no HTTP) +3. `useCases.NewCreateArticleUseCase(repo)`, `CreateArticle(...)` with an empty fake + repo: + - Title `strings.Repeat("😀", 2)` (2 runes, 8 bytes) -> `errors.Is(err, + article.ErrInvalidTitle)`, nothing stored. + - Title `strings.Repeat("😀", 3)` (3 runes, 12 bytes) -> no error, created, + `is_online` false, one article stored. + - Title `strings.Repeat("😀", 200)` (200 runes, 800 bytes) -> no error, created. + Proves the check counts runes (`utf8.RuneCountInString`), not `len()` bytes. + - Title `strings.Repeat("😀", 201)` (201 runes, 804 bytes) -> `errors.Is(err, + article.ErrInvalidTitle)`, nothing stored. + - Keep the existing ASCII boundary cases in `TestCreateArticleTitleLengthLimit` + (0/2/3/200/201) passing unchanged, as a regression check that byte length and + rune length coincide for ASCII. + +## HTTP checks (real CreateArticleUseCase over the fake repo, mirroring api/router.go) +4. Call `validator.InitValidator()`. +5. Build `useCases.NewCreateArticleUseCase(fakeRepo)` plus `GetArticleUseCase`, + `ListArticlesUseCase`, `DeleteArticleUseCase` (real, over the same fake repo). Build + `article.NewHandler(create, get, list, delete)`. Mount its routes inside a group + wrapped by `jwtauth.Verifier(token)` + `jwtauth.Authenticator(token)` on a fresh + `chi.NewRouter()`, with a test `jwtauth.New("HS256", []byte("test-secret"), nil)`, as + in `api/router.go`. +6. Mint a valid jwt cookie for user id 1 (`token.Encode(map[string]any{"user_id": + uint32(1)})`, set as the `jwt` cookie on each `httptest.NewRequest`). +7. `POST /v1/article/` with a 2-ASCII-character title (`"ab"`) and a valid body -> + status `400`, `fakeRepo.createCalled` is false. Assert the body is neither + `apperr.ArticleTitleErrorMessage` nor `"Failed to create article"`, to keep the two + 400 sources (request-layer vs. domain-layer) distinguishable in the test. +8. `POST /v1/article/` with a 3-ASCII-character title -> status `201`, response body + decodes to the created article JSON, `is_online` false. +9. `POST /v1/article/` with a 200-ASCII-character title (`strings.Repeat("a", 200)`) -> + status `201`, created. +10. `POST /v1/article/` with a 201-ASCII-character title -> status `400`, `Create` + never called. +11. `POST /v1/article/` with a title of 200 repetitions of "😀" (200 runes, 800 bytes) + and a valid body -> status `201`, created, response JSON's `title` equals the sent + title. +12. Configure the fake repo (per step 1) so its next `Create` call returns + `article.ErrInvalidTitle` directly. `POST /v1/article/` with title "Valid Title" + and a valid body (both well inside 3-200 runes, so `validator.Validate.Struct` + passes and `CreateArticleUseCase`'s own check would also pass) -> status `400`, + body `apperr.ArticleTitleErrorMessage + "\n"`. This scenario is deliberately + artificial: with the rule aligned, no real HTTP input can make + `CreateArticleUseCase.CreateArticle`'s own check return `article.ErrInvalidTitle` + once `validator.Validate.Struct` has already accepted the same title, so the only + way to exercise `Handler.PostArticle`'s mapping branch today is to have the fake + repository hand back that sentinel directly, standing in for a future or + repository-level source of the same error. +13. Configure the fake repo to fail on its next `Create` with the generic + `errRepositoryFailure` (existing `failNext("Create")`). `POST /v1/article/` with + title "Valid Title" and a valid body -> status `500`, body `"Failed to create + article\n"` -- regression proof a non-title failure still answers 500. +14. `POST /v1/article/` with no jwt cookie -> status `401` (regression: the route is + still gated). + +## Manual verification (not exercised by the Go test) +- None. This slice has no SPA-visible behaviour and no SMTP involvement. diff --git a/internal/domain/article/articleDomain.go b/internal/domain/article/articleDomain.go index 5052402..c144161 100644 --- a/internal/domain/article/articleDomain.go +++ b/internal/domain/article/articleDomain.go @@ -4,6 +4,7 @@ import ( domain "RenewCMS/internal/domain/image" "errors" "time" + "unicode/utf8" ) var ( @@ -12,6 +13,19 @@ var ( ErrArticleOffline = errors.New("article is offline") ) +const ( + MinTitleRunes = 3 + MaxTitleRunes = 200 +) + +func ValidateTitle(title string) error { + runeCount := utf8.RuneCountInString(title) + if runeCount < MinTitleRunes || runeCount > MaxTitleRunes { + return ErrInvalidTitle + } + return nil +} + type Article struct { ID uint32 `json:"id"` Title string `json:"title"` diff --git a/internal/infrastructure/useCases/CreateArticle.go b/internal/infrastructure/useCases/CreateArticle.go index 62dd036..35c7ab4 100644 --- a/internal/infrastructure/useCases/CreateArticle.go +++ b/internal/infrastructure/useCases/CreateArticle.go @@ -18,8 +18,8 @@ func NewCreateArticleUseCase(articleRepository article.Repository) *CreateArticl } func (g *CreateArticleUseCase) CreateArticle(createArticle CreateArticleCommand) (article.Article, error) { - if len(createArticle.Title) < 3 || len(createArticle.Title) > 200 { - return article.Article{}, article.ErrInvalidTitle + if err := article.ValidateTitle(createArticle.Title); err != nil { + return article.Article{}, err } return g.articleRepository.Create(article.Article{ diff --git a/test/article_test.go b/test/article_test.go index 158e288..71d3bfb 100644 --- a/test/article_test.go +++ b/test/article_test.go @@ -12,10 +12,11 @@ import ( ) type mockArticleRepo struct { - articles map[uint32]article.Article - images map[uint32]*imageDomain.Image - nextID uint32 - fail failSwitch + articles map[uint32]article.Article + images map[uint32]*imageDomain.Image + nextID uint32 + fail failSwitch + createCalled bool } func newMockArticleRepo() *mockArticleRepo { @@ -82,6 +83,7 @@ func (m *mockArticleRepo) GetAll() []article.Article { } func (m *mockArticleRepo) Create(a article.Article) (article.Article, error) { + m.createCalled = true if err := m.fail.trigger("Create"); err != nil { return article.Article{}, err } diff --git a/test/article_title_rules_test.go b/test/article_title_rules_test.go new file mode 100644 index 0000000..935fa85 --- /dev/null +++ b/test/article_title_rules_test.go @@ -0,0 +1,255 @@ +package test + +import ( + "RenewCMS/api/handlers/article" + "RenewCMS/api/pkg/apperr" + validator "RenewCMS/api/pkg/validator" + domainArticle "RenewCMS/internal/domain/article" + "RenewCMS/internal/infrastructure/useCases" + "bytes" + "encoding/json" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "reflect" + "strings" + "testing" + + "github.com/go-chi/chi/v5" + "github.com/go-chi/jwtauth/v5" +) + +func newArticleTitleRulesRouter(repo *mockArticleRepo, token *jwtauth.JWTAuth) *chi.Mux { + articleHandler := article.NewHandler( + useCases.NewCreateArticleUseCase(repo), + useCases.NewGetArticleUseCase(repo), + useCases.NewListArticlesUseCase(repo), + useCases.NewDeleteArticleUseCase(repo), + ) + + 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.Post("/", articleHandler.PostArticle) + }) + }) + }) + return r +} + +func postArticleRequest(t *testing.T, token *jwtauth.JWTAuth, title string, body string) *http.Request { + t.Helper() + payload, err := json.Marshal(map[string]string{"title": title, "body": body}) + if err != nil { + t.Fatalf("failed to marshal body: %v", err) + } + req := httptest.NewRequest(http.MethodPost, "/v1/article/", bytes.NewReader(payload)) + req.Header.Set("Content-Type", "application/json") + setJwtCookie(t, req, token, 1) + return req +} + +func TestArticleTitleTagMatchesDomainBounds(t *testing.T) { + field, ok := reflect.TypeOf(article.PostArticle{}).FieldByName("Title") + if !ok { + t.Fatal("expected PostArticle to have a Title field") + } + + expected := fmt.Sprintf("required,min=%d,max=%d", domainArticle.MinTitleRunes, domainArticle.MaxTitleRunes) + got := field.Tag.Get("validate") + if got != expected { + t.Errorf("expected validate tag %q, got %q", expected, got) + } +} + +func TestArticleTitleRunesInUseCase(t *testing.T) { + cases := []struct { + runeCount int + created bool + }{ + {2, false}, + {3, true}, + {200, true}, + {201, false}, + } + + for _, tc := range cases { + t.Run(fmt.Sprintf("rune_count_%d", tc.runeCount), func(t *testing.T) { + repo := newMockArticleRepo() + uc := useCases.NewCreateArticleUseCase(repo) + + title := strings.Repeat("\U0001F600", tc.runeCount) + result, err := uc.CreateArticle(useCases.CreateArticleCommand{Title: title, Body: "Some body text"}) + + if tc.created { + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if !repo.createCalled { + t.Errorf("expected the repository's Create to have been called") + } + if len(repo.articles) != 1 { + t.Errorf("expected 1 article stored, got %d", len(repo.articles)) + } + if result.Title != title { + t.Errorf("expected title %q, got %q", title, result.Title) + } + } else { + if !errors.Is(err, domainArticle.ErrInvalidTitle) { + t.Errorf("expected ErrInvalidTitle, got %v", err) + } + if repo.createCalled { + t.Errorf("expected the repository's Create not to have been called") + } + if len(repo.articles) != 0 { + t.Errorf("expected nothing stored, got %d article(s)", len(repo.articles)) + } + } + }) + } +} + +func TestArticleTitleRunesOverHttp(t *testing.T) { + validator.InitValidator() + token := jwtauth.New("HS256", []byte("test-secret"), nil) + + cases := []struct { + length int + status int + created bool + }{ + {2, http.StatusBadRequest, false}, + {3, http.StatusCreated, true}, + {200, http.StatusCreated, true}, + {201, http.StatusBadRequest, false}, + } + + for _, tc := range cases { + t.Run(fmt.Sprintf("length_%d", tc.length), func(t *testing.T) { + repo := newMockArticleRepo() + router := newArticleTitleRulesRouter(repo, token) + + title := strings.Repeat("a", tc.length) + req := postArticleRequest(t, token, title, "Some body text") + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != tc.status { + t.Fatalf("expected status %d, got %d, body %q", tc.status, rec.Code, rec.Body.String()) + } + + if !tc.created { + if repo.createCalled { + t.Errorf("expected Create not to have been called") + } + if strings.Contains(rec.Body.String(), apperr.ArticleTitleErrorMessage) { + t.Errorf("expected a request-layer validation error, got the domain error message: %q", rec.Body.String()) + } + if strings.Contains(rec.Body.String(), "Failed to create article") { + t.Errorf("expected a request-layer validation error, got the generic failure message: %q", rec.Body.String()) + } + } + + if tc.created { + var got domainArticle.Article + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatalf("failed to decode response: %v", err) + } + if got.IsOnline { + t.Errorf("expected is_online false, got true") + } + if len(repo.articles) != 1 { + t.Errorf("expected 1 article stored, got %d", len(repo.articles)) + } + } else { + if len(repo.articles) != 0 { + t.Errorf("expected nothing stored, got %d article(s)", len(repo.articles)) + } + } + }) + } + + t.Run("A 200-rune multibyte title is accepted end to end over HTTP", func(t *testing.T) { + repo := newMockArticleRepo() + router := newArticleTitleRulesRouter(repo, token) + + title := strings.Repeat("\U0001F600", 200) + req := postArticleRequest(t, token, title, "Some body text") + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusCreated { + t.Fatalf("expected status 201, got %d, body %q", rec.Code, rec.Body.String()) + } + + var got domainArticle.Article + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatalf("failed to decode response: %v", err) + } + if got.Title != title { + t.Errorf("expected title %q, got %q", title, got.Title) + } + }) + + t.Run("The handler maps a domain title rejection to 400, not 500", func(t *testing.T) { + repo := newMockArticleRepo() + repo.fail.failNextWith("Create", domainArticle.ErrInvalidTitle) + router := newArticleTitleRulesRouter(repo, token) + + req := postArticleRequest(t, token, "Valid Title", "Some body text") + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("expected status 400, got %d", rec.Code) + } + if rec.Body.String() != apperr.ArticleTitleErrorMessage+"\n" { + t.Errorf("unexpected body: %q", rec.Body.String()) + } + if strings.Contains(rec.Body.String(), "Failed to create article") { + t.Errorf("did not expect the generic failure message, got %q", rec.Body.String()) + } + }) + + t.Run("A non-title repository failure during creation still answers 500", func(t *testing.T) { + repo := newMockArticleRepo() + repo.fail.failNext("Create") + router := newArticleTitleRulesRouter(repo, token) + + req := postArticleRequest(t, token, "Valid Title", "Some body text") + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusInternalServerError { + t.Fatalf("expected status 500, got %d", rec.Code) + } + if rec.Body.String() != "Failed to create article\n" { + t.Errorf("unexpected body: %q", rec.Body.String()) + } + }) + + t.Run("POST /v1/article/ without a jwt cookie is rejected", func(t *testing.T) { + repo := newMockArticleRepo() + router := newArticleTitleRulesRouter(repo, token) + + payload, err := json.Marshal(map[string]string{"title": "Valid Title", "body": "Some body text"}) + if err != nil { + t.Fatalf("failed to marshal body: %v", err) + } + req := httptest.NewRequest(http.MethodPost, "/v1/article/", bytes.NewReader(payload)) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusUnauthorized { + t.Fatalf("expected status 401, got %d", rec.Code) + } + if repo.createCalled { + t.Errorf("expected Create not to have been called") + } + }) +} diff --git a/test/fakes_common.go b/test/fakes_common.go index 12a6651..da51962 100644 --- a/test/fakes_common.go +++ b/test/fakes_common.go @@ -6,15 +6,27 @@ var errRepositoryFailure = errors.New("repository failure") type failSwitch struct { method string + err error } func (f *failSwitch) failNext(method string) { f.method = method + f.err = nil +} + +func (f *failSwitch) failNextWith(method string, err error) { + f.method = method + f.err = err } func (f *failSwitch) trigger(method string) error { if f.method == method { f.method = "" + if f.err != nil { + err := f.err + f.err = nil + return err + } return errRepositoryFailure } return nil