From 4468ef1410dbec95b7e73ca6033818480bdffaa0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Guillot?= Date: Sun, 3 Jan 2021 22:28:04 -0800 Subject: [PATCH] Refactor category validation --- api/category.go | 44 +++++++++++++--------------- api/entry.go | 2 +- api/feed.go | 4 +-- api/payload.go | 16 ---------- model/category.go | 47 ++++++++---------------------- model/category_test.go | 61 --------------------------------------- reader/handler/handler.go | 2 +- reader/opml/handler.go | 7 +---- storage/category.go | 36 ++++++++++++++++------- ui/category_save.go | 34 +++++++--------------- ui/category_update.go | 28 +++++++++--------- ui/form/category.go | 17 ----------- validator/category.go | 36 +++++++++++++++++++++++ 13 files changed, 122 insertions(+), 212 deletions(-) delete mode 100644 model/category_test.go create mode 100644 validator/category.go diff --git a/api/category.go b/api/category.go index 21733177..9a351581 100644 --- a/api/category.go +++ b/api/category.go @@ -5,36 +5,32 @@ package api // import "miniflux.app/api" import ( - "errors" + json_parser "encoding/json" "net/http" "time" "miniflux.app/http/request" "miniflux.app/http/response/json" "miniflux.app/model" + "miniflux.app/validator" ) func (h *handler) createCategory(w http.ResponseWriter, r *http.Request) { userID := request.UserID(r) - categoryRequest, err := decodeCategoryRequest(r.Body) + var categoryRequest model.CategoryRequest + if err := json_parser.NewDecoder(r.Body).Decode(&categoryRequest); err != nil { + json.BadRequest(w, r, err) + return + } + + if validationErr := validator.ValidateCategoryCreation(h.store, userID, &categoryRequest); validationErr != nil { + json.BadRequest(w, r, validationErr.Error()) + return + } + + category, err := h.store.CreateCategory(userID, &categoryRequest) if err != nil { - json.BadRequest(w, r, err) - return - } - - category := &model.Category{UserID: userID, Title: categoryRequest.Title} - if err := category.ValidateCategoryCreation(); err != nil { - json.BadRequest(w, r, err) - return - } - - if c, err := h.store.CategoryByTitle(userID, category.Title); err != nil || c != nil { - json.BadRequest(w, r, errors.New("This category already exists")) - return - } - - if err := h.store.CreateCategory(category); err != nil { json.ServerError(w, r, err) return } @@ -57,18 +53,18 @@ func (h *handler) updateCategory(w http.ResponseWriter, r *http.Request) { return } - categoryRequest, err := decodeCategoryRequest(r.Body) - if err != nil { + var categoryRequest model.CategoryRequest + if err := json_parser.NewDecoder(r.Body).Decode(&categoryRequest); err != nil { json.BadRequest(w, r, err) return } - category.Title = categoryRequest.Title - if err := category.ValidateCategoryModification(); err != nil { - json.BadRequest(w, r, err) + if validationErr := validator.ValidateCategoryModification(h.store, userID, category.ID, &categoryRequest); validationErr != nil { + json.BadRequest(w, r, validationErr.Error()) return } + categoryRequest.Patch(category) err = h.store.UpdateCategory(category) if err != nil { json.ServerError(w, r, err) @@ -115,7 +111,7 @@ func (h *handler) removeCategory(w http.ResponseWriter, r *http.Request) { userID := request.UserID(r) categoryID := request.RouteInt64Param(r, "categoryID") - if !h.store.CategoryExists(userID, categoryID) { + if !h.store.CategoryIDExists(userID, categoryID) { json.NotFound(w, r) return } diff --git a/api/entry.go b/api/entry.go index 86396943..62867696 100644 --- a/api/entry.go +++ b/api/entry.go @@ -95,7 +95,7 @@ func (h *handler) findEntries(w http.ResponseWriter, r *http.Request, feedID int userID := request.UserID(r) categoryID := request.QueryInt64Param(r, "category_id", 0) - if categoryID > 0 && !h.store.CategoryExists(userID, categoryID) { + if categoryID > 0 && !h.store.CategoryIDExists(userID, categoryID) { json.BadRequest(w, r, errors.New("Invalid category ID")) return } diff --git a/api/feed.go b/api/feed.go index 1ffbfa1d..ca90944e 100644 --- a/api/feed.go +++ b/api/feed.go @@ -38,7 +38,7 @@ func (h *handler) createFeed(w http.ResponseWriter, r *http.Request) { return } - if !h.store.CategoryExists(userID, feedInfo.CategoryID) { + if !h.store.CategoryIDExists(userID, feedInfo.CategoryID) { json.BadRequest(w, r, errors.New("This category_id doesn't exists or doesn't belongs to this user")) return } @@ -123,7 +123,7 @@ func (h *handler) updateFeed(w http.ResponseWriter, r *http.Request) { feedChanges.Update(originalFeed) - if !h.store.CategoryExists(userID, originalFeed.Category.ID) { + if !h.store.CategoryIDExists(userID, originalFeed.Category.ID) { json.BadRequest(w, r, errors.New("This category_id doesn't exists or doesn't belongs to this user")) return } diff --git a/api/payload.go b/api/payload.go index 9b9256ac..b24c73ea 100644 --- a/api/payload.go +++ b/api/payload.go @@ -182,19 +182,3 @@ func decodeEntryStatusRequest(r io.ReadCloser) ([]int64, string, error) { return p.EntryIDs, p.Status, nil } - -type categoryRequest struct { - Title string `json:"title"` -} - -func decodeCategoryRequest(r io.ReadCloser) (*categoryRequest, error) { - var payload categoryRequest - - decoder := json.NewDecoder(r) - defer r.Close() - if err := decoder.Decode(&payload); err != nil { - return nil, fmt.Errorf("Unable to decode JSON object: %v", err) - } - - return &payload, nil -} diff --git a/model/category.go b/model/category.go index 40a132d5..5d8498ad 100644 --- a/model/category.go +++ b/model/category.go @@ -4,51 +4,28 @@ package model // import "miniflux.app/model" -import ( - "errors" - "fmt" -) +import "fmt" -// Category represents a category in the system. +// Category represents a feed category. type Category struct { - ID int64 `json:"id,omitempty"` - Title string `json:"title,omitempty"` - UserID int64 `json:"user_id,omitempty"` - FeedCount int `json:"nb_feeds,omitempty"` + ID int64 `json:"id"` + Title string `json:"title"` + UserID int64 `json:"user_id"` + FeedCount int `json:"-"` } func (c *Category) String() string { return fmt.Sprintf("ID=%d, UserID=%d, Title=%s", c.ID, c.UserID, c.Title) } -// ValidateCategoryCreation validates a category during the creation. -func (c Category) ValidateCategoryCreation() error { - if c.Title == "" { - return errors.New("The title is mandatory") - } - - if c.UserID == 0 { - return errors.New("The userID is mandatory") - } - - return nil +// CategoryRequest represents the request to create or update a category. +type CategoryRequest struct { + Title string `json:"title"` } -// ValidateCategoryModification validates a category during the modification. -func (c Category) ValidateCategoryModification() error { - if c.Title == "" { - return errors.New("The title is mandatory") - } - - if c.UserID == 0 { - return errors.New("The userID is mandatory") - } - - if c.ID <= 0 { - return errors.New("The ID is mandatory") - } - - return nil +// Patch updates category fields. +func (cr *CategoryRequest) Patch(category *Category) { + category.Title = cr.Title } // Categories represents a list of categories. diff --git a/model/category_test.go b/model/category_test.go deleted file mode 100644 index 47a524df..00000000 --- a/model/category_test.go +++ /dev/null @@ -1,61 +0,0 @@ -// Copyright 2017 Frédéric Guillot. All rights reserved. -// Use of this source code is governed by the Apache 2.0 -// license that can be found in the LICENSE file. - -package model // import "miniflux.app/model" - -import "testing" - -func TestValidateCategoryCreation(t *testing.T) { - category := &Category{} - if err := category.ValidateCategoryCreation(); err == nil { - t.Error(`An empty category should generate an error`) - } - - category = &Category{Title: "Test"} - if err := category.ValidateCategoryCreation(); err == nil { - t.Error(`A category without userID should generate an error`) - } - - category = &Category{UserID: 42} - if err := category.ValidateCategoryCreation(); err == nil { - t.Error(`A category without title should generate an error`) - } - - category = &Category{Title: "Test", UserID: 42} - if err := category.ValidateCategoryCreation(); err != nil { - t.Error(`All required fields are filled, it should not generate any error`) - } -} - -func TestValidateCategoryModification(t *testing.T) { - category := &Category{} - if err := category.ValidateCategoryModification(); err == nil { - t.Error(`An empty category should generate an error`) - } - - category = &Category{Title: "Test"} - if err := category.ValidateCategoryModification(); err == nil { - t.Error(`A category without userID should generate an error`) - } - - category = &Category{UserID: 42} - if err := category.ValidateCategoryModification(); err == nil { - t.Error(`A category without title should generate an error`) - } - - category = &Category{ID: -1, Title: "Test", UserID: 42} - if err := category.ValidateCategoryModification(); err == nil { - t.Error(`An invalid categoryID should generate an error`) - } - - category = &Category{ID: 0, Title: "Test", UserID: 42} - if err := category.ValidateCategoryModification(); err == nil { - t.Error(`An invalid categoryID should generate an error`) - } - - category = &Category{ID: 1, Title: "Test", UserID: 42} - if err := category.ValidateCategoryModification(); err != nil { - t.Error(`All required fields are filled, it should not generate any error`) - } -} diff --git a/reader/handler/handler.go b/reader/handler/handler.go index 005558fb..37e1dd50 100644 --- a/reader/handler/handler.go +++ b/reader/handler/handler.go @@ -50,7 +50,7 @@ type FeedCreationArgs struct { func CreateFeed(store *storage.Storage, args *FeedCreationArgs) (*model.Feed, error) { defer timer.ExecutionTime(time.Now(), fmt.Sprintf("[CreateFeed] FeedURL=%s", args.FeedURL)) - if !store.CategoryExists(args.UserID, args.CategoryID) { + if !store.CategoryIDExists(args.UserID, args.CategoryID) { return nil, errors.NewLocalizedError(errCategoryNotFound) } diff --git a/reader/opml/handler.go b/reader/opml/handler.go index 98612439..7661cd4c 100644 --- a/reader/opml/handler.go +++ b/reader/opml/handler.go @@ -65,12 +65,7 @@ func (h *Handler) Import(userID int64, data io.Reader) error { } if category == nil { - category = &model.Category{ - UserID: userID, - Title: subscription.CategoryName, - } - - err := h.store.CreateCategory(category) + category, err = h.store.CreateCategory(userID, &model.CategoryRequest{Title: subscription.CategoryName}) if err != nil { logger.Error("[OPML:Import] %v", err) return fmt.Errorf(`unable to create this category: %q`, subscription.CategoryName) diff --git a/storage/category.go b/storage/category.go index 07206c08..dde112da 100644 --- a/storage/category.go +++ b/storage/category.go @@ -15,13 +15,21 @@ import ( // AnotherCategoryExists checks if another category exists with the same title. func (s *Storage) AnotherCategoryExists(userID, categoryID int64, title string) bool { var result bool - query := `SELECT true FROM categories WHERE user_id=$1 AND id != $2 AND title=$3` + query := `SELECT true FROM categories WHERE user_id=$1 AND id != $2 AND lower(title)=lower($3) LIMIT 1` s.db.QueryRow(query, userID, categoryID, title).Scan(&result) return result } -// CategoryExists checks if the given category exists into the database. -func (s *Storage) CategoryExists(userID, categoryID int64) bool { +// CategoryTitleExists checks if the given category exists into the database. +func (s *Storage) CategoryTitleExists(userID int64, title string) bool { + var result bool + query := `SELECT true FROM categories WHERE user_id=$1 AND lower(title)=lower($2) LIMIT 1` + s.db.QueryRow(query, userID, title).Scan(&result) + return result +} + +// CategoryIDExists checks if the given category exists into the database. +func (s *Storage) CategoryIDExists(userID, categoryID int64) bool { var result bool query := `SELECT true FROM categories WHERE user_id=$1 AND id=$2` s.db.QueryRow(query, userID, categoryID).Scan(&result) @@ -135,26 +143,34 @@ func (s *Storage) CategoriesWithFeedCount(userID int64) (model.Categories, error } // CreateCategory creates a new category. -func (s *Storage) CreateCategory(category *model.Category) error { +func (s *Storage) CreateCategory(userID int64, request *model.CategoryRequest) (*model.Category, error) { + var category model.Category + query := ` INSERT INTO categories (user_id, title) VALUES ($1, $2) RETURNING - id + id, + user_id, + title ` err := s.db.QueryRow( query, - category.UserID, - category.Title, - ).Scan(&category.ID) + userID, + request.Title, + ).Scan( + &category.ID, + &category.UserID, + &category.Title, + ) if err != nil { - return fmt.Errorf(`store: unable to create category: %v`, err) + return nil, fmt.Errorf(`store: unable to create category %q: %v`, request.Title, err) } - return nil + return &category, nil } // UpdateCategory updates an existing category. diff --git a/ui/category_save.go b/ui/category_save.go index fe8f11d3..6bf9d7df 100644 --- a/ui/category_save.go +++ b/ui/category_save.go @@ -15,10 +15,11 @@ import ( "miniflux.app/ui/form" "miniflux.app/ui/session" "miniflux.app/ui/view" + "miniflux.app/validator" ) func (h *handler) saveCategory(w http.ResponseWriter, r *http.Request) { - user, err := h.store.UserByID(request.UserID(r)) + loggedUser, err := h.store.UserByID(request.UserID(r)) if err != nil { html.ServerError(w, r, err) return @@ -30,34 +31,19 @@ func (h *handler) saveCategory(w http.ResponseWriter, r *http.Request) { view := view.New(h.tpl, r, sess) view.Set("form", categoryForm) view.Set("menu", "categories") - view.Set("user", user) - view.Set("countUnread", h.store.CountUnreadEntries(user.ID)) - view.Set("countErrorFeeds", h.store.CountUserFeedsWithErrors(user.ID)) + view.Set("user", loggedUser) + view.Set("countUnread", h.store.CountUnreadEntries(loggedUser.ID)) + view.Set("countErrorFeeds", h.store.CountUserFeedsWithErrors(loggedUser.ID)) - if err := categoryForm.Validate(); err != nil { - view.Set("errorMessage", err.Error()) + categoryRequest := &model.CategoryRequest{Title: categoryForm.Title} + + if validationErr := validator.ValidateCategoryCreation(h.store, loggedUser.ID, categoryRequest); validationErr != nil { + view.Set("errorMessage", validationErr.TranslationKey) html.OK(w, r, view.Render("create_category")) return } - duplicateCategory, err := h.store.CategoryByTitle(user.ID, categoryForm.Title) - if err != nil { - html.ServerError(w, r, err) - return - } - - if duplicateCategory != nil { - view.Set("errorMessage", "error.category_already_exists") - html.OK(w, r, view.Render("create_category")) - return - } - - category := model.Category{ - Title: categoryForm.Title, - UserID: user.ID, - } - - if err = h.store.CreateCategory(&category); err != nil { + if _, err = h.store.CreateCategory(loggedUser.ID, categoryRequest); err != nil { logger.Error("[UI:SaveCategory] %v", err) view.Set("errorMessage", "error.unable_to_create_category") html.OK(w, r, view.Render("create_category")) diff --git a/ui/category_update.go b/ui/category_update.go index 38f148df..80a053fa 100644 --- a/ui/category_update.go +++ b/ui/category_update.go @@ -11,13 +11,15 @@ import ( "miniflux.app/http/response/html" "miniflux.app/http/route" "miniflux.app/logger" + "miniflux.app/model" "miniflux.app/ui/form" "miniflux.app/ui/session" "miniflux.app/ui/view" + "miniflux.app/validator" ) func (h *handler) updateCategory(w http.ResponseWriter, r *http.Request) { - user, err := h.store.UserByID(request.UserID(r)) + loggedUser, err := h.store.UserByID(request.UserID(r)) if err != nil { html.ServerError(w, r, err) return @@ -42,24 +44,20 @@ func (h *handler) updateCategory(w http.ResponseWriter, r *http.Request) { view.Set("form", categoryForm) view.Set("category", category) view.Set("menu", "categories") - view.Set("user", user) - view.Set("countUnread", h.store.CountUnreadEntries(user.ID)) - view.Set("countErrorFeeds", h.store.CountUserFeedsWithErrors(user.ID)) + view.Set("user", loggedUser) + view.Set("countUnread", h.store.CountUnreadEntries(loggedUser.ID)) + view.Set("countErrorFeeds", h.store.CountUserFeedsWithErrors(loggedUser.ID)) - if err := categoryForm.Validate(); err != nil { - view.Set("errorMessage", err.Error()) - html.OK(w, r, view.Render("edit_category")) + categoryRequest := &model.CategoryRequest{Title: categoryForm.Title} + + if validationErr := validator.ValidateCategoryModification(h.store, loggedUser.ID, category.ID, categoryRequest); validationErr != nil { + view.Set("errorMessage", validationErr.TranslationKey) + html.OK(w, r, view.Render("create_category")) return } - if h.store.AnotherCategoryExists(user.ID, category.ID, categoryForm.Title) { - view.Set("errorMessage", "error.category_already_exists") - html.OK(w, r, view.Render("edit_category")) - return - } - - err = h.store.UpdateCategory(categoryForm.Merge(category)) - if err != nil { + categoryRequest.Patch(category) + if err := h.store.UpdateCategory(category); err != nil { logger.Error("[UI:UpdateCategory] %v", err) view.Set("errorMessage", "error.unable_to_update_category") html.OK(w, r, view.Render("edit_category")) diff --git a/ui/form/category.go b/ui/form/category.go index b1bed2e4..1a50e3a2 100644 --- a/ui/form/category.go +++ b/ui/form/category.go @@ -6,9 +6,6 @@ package form // import "miniflux.app/ui/form" import ( "net/http" - - "miniflux.app/errors" - "miniflux.app/model" ) // CategoryForm represents a feed form in the UI @@ -16,20 +13,6 @@ type CategoryForm struct { Title string } -// Validate makes sure the form values are valid. -func (c CategoryForm) Validate() error { - if c.Title == "" { - return errors.NewLocalizedError("error.title_required") - } - return nil -} - -// Merge update the given category fields. -func (c CategoryForm) Merge(category *model.Category) *model.Category { - category.Title = c.Title - return category -} - // NewCategoryForm returns a new CategoryForm. func NewCategoryForm(r *http.Request) *CategoryForm { return &CategoryForm{ diff --git a/validator/category.go b/validator/category.go new file mode 100644 index 00000000..9a62b4e0 --- /dev/null +++ b/validator/category.go @@ -0,0 +1,36 @@ +// Copyright 2021 Frédéric Guillot. All rights reserved. +// Use of this source code is governed by the Apache 2.0 +// license that can be found in the LICENSE file. + +package validator // import "miniflux.app/validator" + +import ( + "miniflux.app/model" + "miniflux.app/storage" +) + +// ValidateCategoryCreation validates category creation. +func ValidateCategoryCreation(store *storage.Storage, userID int64, request *model.CategoryRequest) *ValidationError { + if request.Title == "" { + return NewValidationError("error.title_required") + } + + if store.CategoryTitleExists(userID, request.Title) { + return NewValidationError("error.category_already_exists") + } + + return nil +} + +// ValidateCategoryModification validates category modification. +func ValidateCategoryModification(store *storage.Storage, userID, categoryID int64, request *model.CategoryRequest) *ValidationError { + if request.Title == "" { + return NewValidationError("error.title_required") + } + + if store.AnotherCategoryExists(userID, categoryID, request.Title) { + return NewValidationError("error.category_already_exists") + } + + return nil +}