Explorar el Código

fix: log swallowed DB errors in existence-check helpers

FeedExists, CategoryFeedExists, and CategoryIDExists in internal/storage
discarded the error from Scan() entirely. Since these run a
`SELECT true ... LIMIT 1` query, sql.ErrNoRows is the expected "doesn't
exist" case, but any OTHER error (a dropped connection, a query timeout,
pool exhaustion) took the same silent path -- callers in feed_handlers.go,
category_handlers.go, and entry_handlers.go treat a false return as
"404 Not Found", so a real infrastructure failure was indistinguishable
from a genuinely missing resource, and was never logged anywhere.

Kept the existing bool-only signature deliberately narrow in scope (a
signature change cascades into ~11 call sites across internal/api,
internal/ui, internal/validator, and internal/reader/handler, each with a
different error-handling convention -- too large a change for one focused
PR per this repo's own contribution guidelines). Instead, a genuine
(non-ErrNoRows) error is now logged via slog.Error before returning,
mirroring the existing pattern in CountWebAuthnCredentialsByUserID
(internal/storage/webauthn.go) -- so a real outage is at least observable
in server logs, even though the HTTP status code returned to the client
is unchanged by this PR.

Also fixed the same underlying pattern one layer up in
internal/ui/share.go's createSharedEntry: `if err != nil || entry == nil`
collapsed a genuine GetEntry() query error and an ordinary "no such
share link" into the same HTMLNotFound response. Split into two checks,
mirroring the HTMLServerError/HTMLNotFound split already used a few
lines earlier in the same function for EntryShareCode's error.

No new test added: there's no existing DB-error-injection test harness
for this package (no sqlmock, no integration-tagged test file for
feed.go/category.go), and building one just for this defensive/
observability-only change felt disproportionate to the fix's size.
Verified instead via `go build ./...`, `go vet ./...`, and the full
existing `go test ./...` suite (all pass, confirming no behavior
regression), plus gofmt -l reporting clean on all three touched files.
shiyongjiang hace 1 mes
padre
commit
6ed1006a14
Se han modificado 3 ficheros con 39 adiciones y 4 borrados
  1. 12 1
      internal/storage/category.go
  2. 22 2
      internal/storage/feed.go
  3. 5 1
      internal/ui/share.go

+ 12 - 1
internal/storage/category.go

@@ -7,6 +7,7 @@ import (
 	"database/sql"
 	"errors"
 	"fmt"
+	"log/slog"
 
 	"github.com/lib/pq"
 	"miniflux.app/v2/internal/model"
@@ -32,7 +33,17 @@ func (s *Storage) CategoryTitleExists(userID int64, title string) bool {
 func (s *Storage) CategoryIDExists(userID, categoryID int64) bool {
 	var result bool
 	query := `SELECT true FROM categories WHERE user_id=$1 AND id=$2 LIMIT 1`
-	s.db.QueryRow(query, userID, categoryID).Scan(&result)
+	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 result
 }
 

+ 22 - 2
internal/storage/feed.go

@@ -7,6 +7,7 @@ import (
 	"database/sql"
 	"errors"
 	"fmt"
+	"log/slog"
 	"sort"
 	"time"
 
@@ -36,7 +37,19 @@ func (l byStateAndName) Less(i, j int) bool {
 func (s *Storage) FeedExists(userID, feedID int64) bool {
 	var result bool
 	query := `SELECT true FROM feeds WHERE user_id=$1 AND id=$2 LIMIT 1`
-	s.db.QueryRow(query, userID, feedID).Scan(&result)
+	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 result
 }
 
@@ -55,7 +68,14 @@ func (s *Storage) CheckedAt(userID, feedID int64) (time.Time, error) {
 func (s *Storage) CategoryFeedExists(userID, categoryID, feedID int64) bool {
 	var result bool
 	query := `SELECT true FROM feeds WHERE user_id=$1 AND category_id=$2 AND id=$3 LIMIT 1`
-	s.db.QueryRow(query, userID, categoryID, feedID).Scan(&result)
+	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 result
 }
 

+ 5 - 1
internal/ui/share.go

@@ -47,7 +47,11 @@ func (h *handler) sharedEntry(w http.ResponseWriter, r *http.Request) {
 			WithShareCode(shareCode).
 			GetEntry()
 
-		if err != nil || entry == nil {
+		if err != nil {
+			response.HTMLServerError(w, r, err)
+			return
+		}
+		if entry == nil {
 			response.HTMLNotFound(w, r)
 			return
 		}