Return 400 for export of an unknown site
exportCtrl mapped every export failure to 500 Internal Server Error, so requesting a backup for a non-existent site (e.g. wrong -s/--site) came back as a misleading 500 instead of a client error — inconsistent with the rest of the admin/public API, which returns 400 + ErrSiteNotFound for site-lookup failures. Add an engine.ErrSiteNotFound sentinel (wrapped at the bolt db-lookup so the existing "site %q not found" message is unchanged) and map it to 400 + rest.ErrSiteNotFound in exportCtrl; genuine internal failures (gzip close/write) still return 500.
This commit is contained in:
committed by
Umputun
parent
5c0798fe10
commit
2544d80f98
@@ -4,11 +4,13 @@ import (
|
||||
"bytes"
|
||||
"compress/gzip"
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"io"
|
||||
"net/http"
|
||||
"os"
|
||||
"strconv"
|
||||
"strings"
|
||||
"sync"
|
||||
"time"
|
||||
|
||||
@@ -18,6 +20,7 @@ import (
|
||||
|
||||
"github.com/umputun/remark42/backend/app/migrator"
|
||||
"github.com/umputun/remark42/backend/app/rest"
|
||||
"github.com/umputun/remark42/backend/app/store/engine"
|
||||
)
|
||||
|
||||
// Migrator rest with import and export controllers
|
||||
@@ -151,7 +154,8 @@ func (m *Migrator) exportCtrl(w http.ResponseWriter, r *http.Request) {
|
||||
var buf bytes.Buffer
|
||||
gzWriter := gzip.NewWriter(&buf)
|
||||
if _, err := m.NativeExporter.Export(gzWriter, siteID); err != nil {
|
||||
rest.SendErrorJSON(w, r, http.StatusInternalServerError, err, "export failed", rest.ErrInternal)
|
||||
code, errCode := exportErrStatus(err)
|
||||
rest.SendErrorJSON(w, r, code, err, "export failed", errCode)
|
||||
return
|
||||
}
|
||||
if err := gzWriter.Close(); err != nil {
|
||||
@@ -171,10 +175,23 @@ func (m *Migrator) exportCtrl(w http.ResponseWriter, r *http.Request) {
|
||||
|
||||
// stream mode - write directly to response
|
||||
if _, err := m.NativeExporter.Export(w, siteID); err != nil {
|
||||
rest.SendErrorJSON(w, r, http.StatusInternalServerError, err, "export failed", rest.ErrInternal)
|
||||
code, errCode := exportErrStatus(err)
|
||||
rest.SendErrorJSON(w, r, code, err, "export failed", errCode)
|
||||
}
|
||||
}
|
||||
|
||||
// exportErrStatus maps an export failure to an HTTP status and error code: an unknown
|
||||
// site is a client error (400), anything else is treated as internal (500).
|
||||
// The bolt store returns the engine.ErrSiteNotFound sentinel; the rpc store loses typed
|
||||
// errors over jrpc, so the "not found" message is matched as a fallback (export only ever
|
||||
// hits a site-level lookup, so a "not found" here can only mean the site).
|
||||
func exportErrStatus(err error) (status, errCode int) {
|
||||
if errors.Is(err, engine.ErrSiteNotFound) || strings.Contains(err.Error(), "not found") {
|
||||
return http.StatusBadRequest, rest.ErrSiteNotFound
|
||||
}
|
||||
return http.StatusInternalServerError, rest.ErrInternal
|
||||
}
|
||||
|
||||
// POST /remap?site=site-id
|
||||
// remap urls in comments based on given rules (oldUrl newUrl)
|
||||
func (m *Migrator) remapCtrl(w http.ResponseWriter, r *http.Request) {
|
||||
|
||||
@@ -391,15 +391,31 @@ func TestMigrator_Export(t *testing.T) {
|
||||
require.Equal(t, http.StatusAccepted, resp.StatusCode)
|
||||
waitForMigrationCompletion(t, ts)
|
||||
|
||||
// export wrong site, should result in error
|
||||
// export unknown site is a client error, not internal
|
||||
req, err = http.NewRequest("GET", ts.URL+"/api/v1/admin/export?mode=file&site=test", http.NoBody)
|
||||
require.NoError(t, err)
|
||||
req.SetBasicAuth("admin", "password")
|
||||
resp, err = client.Do(req)
|
||||
require.NoError(t, err)
|
||||
errBody, err := io.ReadAll(resp.Body)
|
||||
require.NoError(t, err)
|
||||
resp.Body.Close()
|
||||
require.Equal(t, http.StatusInternalServerError, resp.StatusCode)
|
||||
require.Equal(t, http.StatusBadRequest, resp.StatusCode)
|
||||
require.Equal(t, "application/json", resp.Header.Get("Content-Type"))
|
||||
assert.Contains(t, string(errBody), `"code":6`) // rest.ErrSiteNotFound, not ErrInternal
|
||||
assert.Contains(t, string(errBody), `not found`) // error detail names the missing site
|
||||
|
||||
// unknown site in stream mode is also a client error
|
||||
req, err = http.NewRequest("GET", ts.URL+"/api/v1/admin/export?mode=stream&site=test", http.NoBody)
|
||||
require.NoError(t, err)
|
||||
req.SetBasicAuth("admin", "password")
|
||||
resp, err = client.Do(req)
|
||||
require.NoError(t, err)
|
||||
errBody, err = io.ReadAll(resp.Body)
|
||||
require.NoError(t, err)
|
||||
resp.Body.Close()
|
||||
require.Equal(t, http.StatusBadRequest, resp.StatusCode)
|
||||
assert.Contains(t, string(errBody), `"code":6`)
|
||||
|
||||
// check file mode
|
||||
req, err = http.NewRequest("GET", ts.URL+"/api/v1/admin/export?mode=file&site=remark42", http.NoBody)
|
||||
|
||||
@@ -1018,7 +1018,7 @@ func (b *BoltDB) db(siteID string) (*bolt.DB, error) {
|
||||
if res, ok := b.dbs[siteID]; ok {
|
||||
return res, nil
|
||||
}
|
||||
return nil, fmt.Errorf("site %q not found", siteID)
|
||||
return nil, fmt.Errorf("site %q %w", siteID, ErrSiteNotFound)
|
||||
}
|
||||
|
||||
// makeRef creates reference combining url and comment id
|
||||
|
||||
@@ -4,6 +4,7 @@ package engine
|
||||
// Includes default implementation with boltdb
|
||||
|
||||
import (
|
||||
"errors"
|
||||
"sort"
|
||||
"strings"
|
||||
"time"
|
||||
@@ -11,6 +12,10 @@ import (
|
||||
"github.com/umputun/remark42/backend/app/store"
|
||||
)
|
||||
|
||||
// ErrSiteNotFound is returned by engines when the requested site does not exist.
|
||||
// Its message is "not found" so wrapping it as `site %q %w` reads "site \"x\" not found".
|
||||
var ErrSiteNotFound = errors.New("not found")
|
||||
|
||||
// NOTE: matryer/moq should be installed globally and works with `go generate ./...`
|
||||
//go:generate moq --out engine_mock.go . Interface
|
||||
|
||||
|
||||
Reference in New Issue
Block a user