Ver código fonte

refactor: change existence-check helpers to return (bool, error)

Follow-up to #4484, which kept the bool-only signature for FeedExists,
CategoryFeedExists (internal/storage/feed.go), and CategoryIDExists
(internal/storage/category.go) and instead logged a real query/connection
error via slog before returning false -- deliberately narrow, since
changing the signature would touch every call site.

This does that follow-up: all three now return (bool, error), and every
call site (14 total, across internal/api, internal/ui,
internal/reader/handler, and internal/validator) is updated to distinguish
a genuine backend failure from "resource not found":

- internal/api/*_handlers.go: response.JSONServerError on a real error,
  existing response.JSONNotFound/JSONBadRequest behavior unchanged
  otherwise.
- internal/ui/*.go: response.HTMLServerError on a real error, existing
  response.HTMLNotFound behavior unchanged otherwise.
- internal/reader/handler/handler.go: locale.NewLocalizedErrorWrapper(err,
  "error.database_error", err) on a real error, matching the exact idiom
  already used elsewhere in this same file for other storage errors.
- internal/validator/feed.go: *locale.LocalizedError (unlike
  *locale.LocalizedErrorWrapper used elsewhere) has no way to carry an
  underlying error, so a signature change here would be a separate, larger
  change to the validator package's error model. Logs the real error via
  slog and falls back to the existing "error.feed_category_not_found"
  message, same as before -- this is a deliberate scoping decision, not an
  oversight, and is called out in the PR description.

Testing: go build ./..., go vet ./..., gofmt -l (clean on all 10 touched
files), and the full go test ./... suite (48 packages, all pass, no
existing test called these 3 functions directly). No new test added, same
reasoning as #4484: no DB-error-injection harness exists for this
package, and this change doesn't alter behavior when there's no error --
every touched call site's non-error branch is behaviorally identical to
before.
shiyongjiang 2 meses atrás
pai
commit
69756868dd

+ 6 - 1
internal/api/category_handlers.go

@@ -146,7 +146,12 @@ func (h *handler) removeCategoryHandler(w http.ResponseWriter, r *http.Request)
 		return
 		return
 	}
 	}
 
 
-	if !h.store.CategoryIDExists(userID, categoryID) {
+	exists, err := h.store.CategoryIDExists(userID, categoryID)
+	if err != nil {
+		response.JSONServerError(w, r, err)
+		return
+	}
+	if !exists {
 		response.JSONNotFound(w, r)
 		response.JSONNotFound(w, r)
 		return
 		return
 	}
 	}

+ 26 - 7
internal/api/entry_handlers.go

@@ -148,15 +148,29 @@ func (h *handler) findEntries(w http.ResponseWriter, r *http.Request, feedID int
 
 
 	userID := request.UserID(r)
 	userID := request.UserID(r)
 	categoryID = request.QueryInt64Param(r, "category_id", categoryID)
 	categoryID = request.QueryInt64Param(r, "category_id", categoryID)
-	if categoryID > 0 && !h.store.CategoryIDExists(userID, categoryID) {
-		response.JSONBadRequest(w, r, errors.New("invalid category ID"))
-		return
+	if categoryID > 0 {
+		exists, err := h.store.CategoryIDExists(userID, categoryID)
+		if err != nil {
+			response.JSONServerError(w, r, err)
+			return
+		}
+		if !exists {
+			response.JSONBadRequest(w, r, errors.New("invalid category ID"))
+			return
+		}
 	}
 	}
 
 
 	feedID = request.QueryInt64Param(r, "feed_id", feedID)
 	feedID = request.QueryInt64Param(r, "feed_id", feedID)
-	if feedID > 0 && !h.store.FeedExists(userID, feedID) {
-		response.JSONBadRequest(w, r, errors.New("invalid feed ID"))
-		return
+	if feedID > 0 {
+		exists, err := h.store.FeedExists(userID, feedID)
+		if err != nil {
+			response.JSONServerError(w, r, err)
+			return
+		}
+		if !exists {
+			response.JSONBadRequest(w, r, errors.New("invalid feed ID"))
+			return
+		}
 	}
 	}
 
 
 	tags := request.QueryStringParamList(r, "tags")
 	tags := request.QueryStringParamList(r, "tags")
@@ -346,7 +360,12 @@ func (h *handler) importFeedEntryHandler(w http.ResponseWriter, r *http.Request)
 		return
 		return
 	}
 	}
 
 
-	if !h.store.FeedExists(userID, feedID) {
+	exists, err := h.store.FeedExists(userID, feedID)
+	if err != nil {
+		response.JSONServerError(w, r, err)
+		return
+	}
+	if !exists {
 		response.JSONBadRequest(w, r, errors.New("feed does not exist"))
 		response.JSONBadRequest(w, r, errors.New("feed does not exist"))
 		return
 		return
 	}
 	}

+ 18 - 3
internal/api/feed_handlers.go

@@ -59,7 +59,12 @@ func (h *handler) refreshFeedHandler(w http.ResponseWriter, r *http.Request) {
 	}
 	}
 
 
 	userID := request.UserID(r)
 	userID := request.UserID(r)
-	if !h.store.FeedExists(userID, feedID) {
+	exists, err := h.store.FeedExists(userID, feedID)
+	if err != nil {
+		response.JSONServerError(w, r, err)
+		return
+	}
+	if !exists {
 		response.JSONNotFound(w, r)
 		response.JSONNotFound(w, r)
 		return
 		return
 	}
 	}
@@ -154,7 +159,12 @@ func (h *handler) markFeedAsReadHandler(w http.ResponseWriter, r *http.Request)
 		return
 		return
 	}
 	}
 
 
-	if !h.store.FeedExists(userID, feedID) {
+	exists, err := h.store.FeedExists(userID, feedID)
+	if err != nil {
+		response.JSONServerError(w, r, err)
+		return
+	}
+	if !exists {
 		response.JSONNotFound(w, r)
 		response.JSONNotFound(w, r)
 		return
 		return
 	}
 	}
@@ -245,7 +255,12 @@ func (h *handler) removeFeedHandler(w http.ResponseWriter, r *http.Request) {
 	}
 	}
 
 
 	userID := request.UserID(r)
 	userID := request.UserID(r)
-	if !h.store.FeedExists(userID, feedID) {
+	exists, err := h.store.FeedExists(userID, feedID)
+	if err != nil {
+		response.JSONServerError(w, r, err)
+		return
+	}
+	if !exists {
 		response.JSONNotFound(w, r)
 		response.JSONNotFound(w, r)
 		return
 		return
 	}
 	}

+ 10 - 2
internal/reader/handler/handler.go

@@ -44,7 +44,11 @@ func CreateFeedFromSubscriptionDiscovery(store *storage.Storage, userID int64, f
 		slog.String("proxy_url", feedCreationRequest.ProxyURL),
 		slog.String("proxy_url", feedCreationRequest.ProxyURL),
 	)
 	)
 
 
-	if !store.CategoryIDExists(userID, feedCreationRequest.CategoryID) {
+	categoryExists, storeErr := store.CategoryIDExists(userID, feedCreationRequest.CategoryID)
+	if storeErr != nil {
+		return nil, locale.NewLocalizedErrorWrapper(storeErr, "error.database_error", storeErr)
+	}
+	if !categoryExists {
 		return nil, locale.NewLocalizedErrorWrapper(ErrCategoryNotFound, "error.category_not_found")
 		return nil, locale.NewLocalizedErrorWrapper(ErrCategoryNotFound, "error.category_not_found")
 	}
 	}
 
 
@@ -108,7 +112,11 @@ func CreateFeed(store *storage.Storage, userID int64, feedCreationRequest *model
 		slog.String("proxy_url", feedCreationRequest.ProxyURL),
 		slog.String("proxy_url", feedCreationRequest.ProxyURL),
 	)
 	)
 
 
-	if !store.CategoryIDExists(userID, feedCreationRequest.CategoryID) {
+	categoryExists, storeErr := store.CategoryIDExists(userID, feedCreationRequest.CategoryID)
+	if storeErr != nil {
+		return nil, locale.NewLocalizedErrorWrapper(storeErr, "error.database_error", storeErr)
+	}
+	if !categoryExists {
 		return nil, locale.NewLocalizedErrorWrapper(ErrCategoryNotFound, "error.category_not_found")
 		return nil, locale.NewLocalizedErrorWrapper(ErrCategoryNotFound, "error.category_not_found")
 	}
 	}
 
 

+ 6 - 12
internal/storage/category.go

@@ -7,7 +7,6 @@ import (
 	"database/sql"
 	"database/sql"
 	"errors"
 	"errors"
 	"fmt"
 	"fmt"
-	"log/slog"
 
 
 	"github.com/lib/pq"
 	"github.com/lib/pq"
 	"miniflux.app/v2/internal/model"
 	"miniflux.app/v2/internal/model"
@@ -30,21 +29,16 @@ func (s *Storage) CategoryTitleExists(userID int64, title string) bool {
 }
 }
 
 
 // CategoryIDExists checks if the given category exists into the database.
 // CategoryIDExists checks if the given category exists into the database.
-func (s *Storage) CategoryIDExists(userID, categoryID int64) bool {
+// The returned error is non-nil only for a genuine query/connection failure;
+// a category that doesn't exist is reported as (false, nil), matching the
+// underlying sql.ErrNoRows case.
+func (s *Storage) CategoryIDExists(userID, categoryID int64) (bool, error) {
 	var result bool
 	var result bool
 	query := `SELECT true FROM categories WHERE user_id=$1 AND id=$2 LIMIT 1`
 	query := `SELECT true FROM categories WHERE user_id=$1 AND id=$2 LIMIT 1`
 	if err := s.db.QueryRow(query, userID, categoryID).Scan(&result); err != nil && !errors.Is(err, sql.ErrNoRows) {
 	if err := s.db.QueryRow(query, userID, categoryID).Scan(&result); err != nil && !errors.Is(err, sql.ErrNoRows) {
-		// See FeedExists in feed.go for why a real query error is worth
-		// logging here: callers treat a false return as "no such category",
-		// so a swallowed error would silently misreport an infrastructure
-		// failure as a missing resource.
-		slog.Error("store: unable to check if category exists",
-			slog.Int64("user_id", userID),
-			slog.Int64("category_id", categoryID),
-			slog.Any("error", err),
-		)
+		return false, fmt.Errorf(`store: unable to check if category exists: %w`, err)
 	}
 	}
-	return result
+	return result, nil
 }
 }
 
 
 // Category returns a category from the database.
 // Category returns a category from the database.

+ 13 - 24
internal/storage/feed.go

@@ -7,7 +7,6 @@ import (
 	"database/sql"
 	"database/sql"
 	"errors"
 	"errors"
 	"fmt"
 	"fmt"
-	"log/slog"
 	"sort"
 	"sort"
 	"time"
 	"time"
 
 
@@ -33,24 +32,17 @@ func (l byStateAndName) Less(i, j int) bool {
 	return l.f[i].Title < l.f[j].Title
 	return l.f[i].Title < l.f[j].Title
 }
 }
 
 
-// FeedExists checks if the given feed exists.
-func (s *Storage) FeedExists(userID, feedID int64) bool {
+// FeedExists checks if the given feed exists. The returned error is
+// non-nil only for a genuine query/connection failure; a feed that doesn't
+// exist is reported as (false, nil), matching the underlying sql.ErrNoRows
+// case.
+func (s *Storage) FeedExists(userID, feedID int64) (bool, error) {
 	var result bool
 	var result bool
 	query := `SELECT true FROM feeds WHERE user_id=$1 AND id=$2 LIMIT 1`
 	query := `SELECT true FROM feeds WHERE user_id=$1 AND id=$2 LIMIT 1`
 	if err := s.db.QueryRow(query, userID, feedID).Scan(&result); err != nil && !errors.Is(err, sql.ErrNoRows) {
 	if err := s.db.QueryRow(query, userID, feedID).Scan(&result); err != nil && !errors.Is(err, sql.ErrNoRows) {
-		// A real query/connection error is not the same thing as "this feed
-		// doesn't exist" -- callers treat a false return as a 404, so a
-		// swallowed error here would silently misreport a genuine
-		// infrastructure failure as a missing resource. Logging at least
-		// makes it observable in server logs, matching the pattern already
-		// used by CountWebAuthnCredentialsByUserID in webauthn.go.
-		slog.Error("store: unable to check if feed exists",
-			slog.Int64("user_id", userID),
-			slog.Int64("feed_id", feedID),
-			slog.Any("error", err),
-		)
+		return false, fmt.Errorf(`store: unable to check if feed exists: %w`, err)
 	}
 	}
-	return result
+	return result, nil
 }
 }
 
 
 // CheckedAt returns when the feed was last checked.
 // CheckedAt returns when the feed was last checked.
@@ -64,19 +56,16 @@ func (s *Storage) CheckedAt(userID, feedID int64) (time.Time, error) {
 	return result, nil
 	return result, nil
 }
 }
 
 
-// CategoryFeedExists returns true if the given feed exists and belongs to the given category.
-func (s *Storage) CategoryFeedExists(userID, categoryID, feedID int64) bool {
+// CategoryFeedExists returns true if the given feed exists and belongs to
+// the given category. The returned error is non-nil only for a genuine
+// query/connection failure; see FeedExists above for the same convention.
+func (s *Storage) CategoryFeedExists(userID, categoryID, feedID int64) (bool, error) {
 	var result bool
 	var result bool
 	query := `SELECT true FROM feeds WHERE user_id=$1 AND category_id=$2 AND id=$3 LIMIT 1`
 	query := `SELECT true FROM feeds WHERE user_id=$1 AND category_id=$2 AND id=$3 LIMIT 1`
 	if err := s.db.QueryRow(query, userID, categoryID, feedID).Scan(&result); err != nil && !errors.Is(err, sql.ErrNoRows) {
 	if err := s.db.QueryRow(query, userID, categoryID, feedID).Scan(&result); err != nil && !errors.Is(err, sql.ErrNoRows) {
-		slog.Error("store: unable to check if feed exists in category",
-			slog.Int64("user_id", userID),
-			slog.Int64("category_id", categoryID),
-			slog.Int64("feed_id", feedID),
-			slog.Any("error", err),
-		)
+		return false, fmt.Errorf(`store: unable to check if feed exists in category: %w`, err)
 	}
 	}
-	return result
+	return result, nil
 }
 }
 
 
 // FeedURLExists returns true if the given feed URL already exists for the user.
 // FeedURLExists returns true if the given feed URL already exists for the user.

+ 6 - 1
internal/ui/category_mark_feed_as_read.go

@@ -15,7 +15,12 @@ func (h *handler) markCategoryFeedAsRead(w http.ResponseWriter, r *http.Request)
 	categoryID := request.RouteInt64Param(r, "categoryID")
 	categoryID := request.RouteInt64Param(r, "categoryID")
 	userID := request.UserID(r)
 	userID := request.UserID(r)
 
 
-	if !h.store.CategoryFeedExists(userID, categoryID, feedID) {
+	exists, err := h.store.CategoryFeedExists(userID, categoryID, feedID)
+	if err != nil {
+		response.HTMLServerError(w, r, err)
+		return
+	}
+	if !exists {
 		response.HTMLNotFound(w, r)
 		response.HTMLNotFound(w, r)
 		return
 		return
 	}
 	}

+ 6 - 1
internal/ui/category_remove_feed.go

@@ -14,7 +14,12 @@ func (h *handler) removeCategoryFeed(w http.ResponseWriter, r *http.Request) {
 	feedID := request.RouteInt64Param(r, "feedID")
 	feedID := request.RouteInt64Param(r, "feedID")
 	categoryID := request.RouteInt64Param(r, "categoryID")
 	categoryID := request.RouteInt64Param(r, "categoryID")
 
 
-	if !h.store.CategoryFeedExists(request.UserID(r), categoryID, feedID) {
+	exists, err := h.store.CategoryFeedExists(request.UserID(r), categoryID, feedID)
+	if err != nil {
+		response.HTMLServerError(w, r, err)
+		return
+	}
+	if !exists {
 		response.HTMLNotFound(w, r)
 		response.HTMLNotFound(w, r)
 		return
 		return
 	}
 	}

+ 6 - 1
internal/ui/feed_remove.go

@@ -13,7 +13,12 @@ import (
 func (h *handler) removeFeed(w http.ResponseWriter, r *http.Request) {
 func (h *handler) removeFeed(w http.ResponseWriter, r *http.Request) {
 	feedID := request.RouteInt64Param(r, "feedID")
 	feedID := request.RouteInt64Param(r, "feedID")
 
 
-	if !h.store.FeedExists(request.UserID(r), feedID) {
+	exists, err := h.store.FeedExists(request.UserID(r), feedID)
+	if err != nil {
+		response.HTMLServerError(w, r, err)
+		return
+	}
+	if !exists {
 		response.HTMLNotFound(w, r)
 		response.HTMLNotFound(w, r)
 		return
 		return
 	}
 	}

+ 30 - 2
internal/validator/feed.go

@@ -4,6 +4,8 @@
 package validator // import "miniflux.app/v2/internal/validator"
 package validator // import "miniflux.app/v2/internal/validator"
 
 
 import (
 import (
+	"log/slog"
+
 	"miniflux.app/v2/internal/locale"
 	"miniflux.app/v2/internal/locale"
 	"miniflux.app/v2/internal/model"
 	"miniflux.app/v2/internal/model"
 	"miniflux.app/v2/internal/storage"
 	"miniflux.app/v2/internal/storage"
@@ -24,7 +26,23 @@ func ValidateFeedCreation(store *storage.Storage, userID int64, request *model.F
 		return locale.NewLocalizedError("error.feed_already_exists")
 		return locale.NewLocalizedError("error.feed_already_exists")
 	}
 	}
 
 
-	if !store.CategoryIDExists(userID, request.CategoryID) {
+	categoryExists, err := store.CategoryIDExists(userID, request.CategoryID)
+	if err != nil {
+		// *locale.LocalizedError (unlike *locale.LocalizedErrorWrapper elsewhere
+		// in this codebase) has no way to carry an underlying error, so a
+		// genuine backend failure here can't be distinguished from "category
+		// not found" in the value returned to the caller. Log it so the
+		// failure is at least observable, and fall back to the existing
+		// user-facing message rather than changing this function's public
+		// return type as part of this PR.
+		slog.Error("validator: unable to check if feed category exists",
+			slog.Int64("user_id", userID),
+			slog.Int64("category_id", request.CategoryID),
+			slog.Any("error", err),
+		)
+		return locale.NewLocalizedError("error.feed_category_not_found")
+	}
+	if !categoryExists {
 		return locale.NewLocalizedError("error.feed_category_not_found")
 		return locale.NewLocalizedError("error.feed_category_not_found")
 	}
 	}
 
 
@@ -88,7 +106,17 @@ func ValidateFeedModification(store *storage.Storage, userID, feedID int64, requ
 	}
 	}
 
 
 	if request.CategoryID != nil {
 	if request.CategoryID != nil {
-		if !store.CategoryIDExists(userID, *request.CategoryID) {
+		categoryExists, err := store.CategoryIDExists(userID, *request.CategoryID)
+		if err != nil {
+			// See the identical rationale in ValidateFeedCreation above.
+			slog.Error("validator: unable to check if feed category exists",
+				slog.Int64("user_id", userID),
+				slog.Int64("category_id", *request.CategoryID),
+				slog.Any("error", err),
+			)
+			return locale.NewLocalizedError("error.feed_category_not_found")
+		}
+		if !categoryExists {
 			return locale.NewLocalizedError("error.feed_category_not_found")
 			return locale.NewLocalizedError("error.feed_category_not_found")
 		}
 		}
 	}
 	}