From 2544d80f98b06ea8c44adea924d7ace58e0b96f2 Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Mon, 6 Jul 2026 14:01:24 +0100 Subject: [PATCH] Return 400 for export of an unknown site MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- backend/app/rest/api/migrator.go | 21 +++++++++++++++++++-- backend/app/rest/api/migrator_test.go | 20 ++++++++++++++++++-- backend/app/store/engine/bolt.go | 2 +- backend/app/store/engine/engine.go | 5 +++++ 4 files changed, 43 insertions(+), 5 deletions(-) diff --git a/backend/app/rest/api/migrator.go b/backend/app/rest/api/migrator.go index cc982264..b6c42a61 100644 --- a/backend/app/rest/api/migrator.go +++ b/backend/app/rest/api/migrator.go @@ -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) { diff --git a/backend/app/rest/api/migrator_test.go b/backend/app/rest/api/migrator_test.go index ab1a3f42..21773c9d 100644 --- a/backend/app/rest/api/migrator_test.go +++ b/backend/app/rest/api/migrator_test.go @@ -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) diff --git a/backend/app/store/engine/bolt.go b/backend/app/store/engine/bolt.go index 7816320b..eef96cd6 100644 --- a/backend/app/store/engine/bolt.go +++ b/backend/app/store/engine/bolt.go @@ -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 diff --git a/backend/app/store/engine/engine.go b/backend/app/store/engine/engine.go index 588cd15a..298879cb 100644 --- a/backend/app/store/engine/engine.go +++ b/backend/app/store/engine/engine.go @@ -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