diff --git a/batch_test.go b/batch_test.go index a48df0f..d5c8400 100644 --- a/batch_test.go +++ b/batch_test.go @@ -126,6 +126,12 @@ func TestBatch(t *testing.T) { if after := env.snapshot(t); after != before { t.Fatalf("a refused batch wrote:\n%s", after) } + coerced := env.mcp(t, http.MethodPost, "/_seo/api/v1/batch", agent, map[string]any{"changes": []any{ + map[string]any{"op": "create_redirect", "redirect": map[string]any{"source": "/s307", "target": "/pages/a", "status": 307}}, + }}) + if coerced.Code != http.StatusUnprocessableEntity || !strings.Contains(coerced.Body.String(), `"status"`) { + t.Fatalf("batch with status 307 = %d %s, want 422 on status", coerced.Code, coerced.Body.String()) + } if rec := env.mcp(t, http.MethodPost, "/_seo/api/v1/batch", agent, map[string]any{"changes": []any{map[string]any{"op": "explode"}}}); rec.Code != http.StatusUnprocessableEntity { t.Fatalf("unknown op = %d, want 422", rec.Code) } diff --git a/classes/batch.go b/classes/batch.go index dc3b995..478847a 100644 --- a/classes/batch.go +++ b/classes/batch.go @@ -49,6 +49,36 @@ type RedirectFields struct { Note *string `json:"note,omitempty"` } +// Problems refuses a status, match or query option outside the allowed +// values (WR-17), so a caller learns its request was not taken rather than +// getting a default in its place; an empty match or query is the default. +func (f RedirectFields) Problems() map[string][]models.Problem { + out := map[string][]models.Problem{} + invalid := []models.Problem{{Key: "golem15.seo::lang.api.value_invalid"}} + if f.Status != nil { + switch models.RedirectStatus(*f.Status) { + case models.Status301, models.Status302, models.Status410: + default: + out["status"] = invalid + } + } + if f.Match != nil { + switch strings.TrimSpace(*f.Match) { + case "", models.RedirectExact, models.RedirectPattern, models.RedirectRegex: + default: + out["match"] = invalid + } + } + if f.Query != nil { + switch strings.TrimSpace(*f.Query) { + case "", models.QueryIgnore, models.QueryPass: + default: + out["query"] = invalid + } + } + return out +} + // Apply writes the fields that are set onto r. func (f RedirectFields) Apply(r *models.Redirect) { if f.Source != nil { @@ -380,6 +410,9 @@ func (s *Service) applyChange(ctx context.Context, tx *gorm.DB, index int, ch Ba } } if ch.Redirect != nil { + if problems := ch.Redirect.Problems(); len(problems) > 0 { + return &ProblemsError{Problems: problems} + } ch.Redirect.Apply(&r) } check, err := s.SaveRedirect(ctx, tx, &r) diff --git a/classes/transfer.go b/classes/transfer.go index 3fa61d3..91232fd 100644 --- a/classes/transfer.go +++ b/classes/transfer.go @@ -422,6 +422,16 @@ func (s *Service) planRedirect(ctx context.Context, tx *gorm.DB, row CSVRow) (pl f.TrailingSlash = &b } } + if problems := f.Problems(); len(problems) > 0 { + p.status = "refused" + for _, column := range []string{"status", "match", "query"} { + if _, bad := problems[column]; bad { + v, _ := cell(row, column) + p.notes = append(p.notes, s.PhraseCtx(ctx, "golem15.seo::lang.transfer.row_invalid", map[string]string{"field": column, "message": v})) + } + } + return p, nil, nil + } var rows []models.Redirect if err := tx.Where("source = ? AND match = ?", strings.TrimSpace(source), strings.TrimSpace(match)).Limit(1).Find(&rows).Error; err != nil { return p, nil, err diff --git a/controllers/api/redirects.go b/controllers/api/redirects.go index 460f5f7..be96df2 100644 --- a/controllers/api/redirects.go +++ b/controllers/api/redirects.go @@ -137,6 +137,9 @@ func CreateRedirect(service func() *classes.Service) http.HandlerFunc { if err := decode(r, maxBody, &body); err != nil { return 0, nil, err } + if problems := body.Problems(); len(problems) > 0 { + return 0, nil, &classes.ProblemsError{Problems: problems} + } row := models.NewRedirect() row.Origin = classes.RedirectOrigin(ctx) body.Apply(&row) @@ -165,6 +168,9 @@ func UpdateRedirect(service func() *classes.Service) http.HandlerFunc { if err := decode(r, maxBody, &body); err != nil { return 0, nil, err } + if problems := body.Problems(); len(problems) > 0 { + return 0, nil, &classes.ProblemsError{Problems: problems} + } var row models.Redirect var check classes.RedirectCheck err = write(ctx, svc, func(ctx context.Context, tx *gorm.DB) error { diff --git a/mcp_test.go b/mcp_test.go index e4bc9af..b78798b 100644 --- a/mcp_test.go +++ b/mcp_test.go @@ -281,6 +281,15 @@ func TestManagementAPI(t *testing.T) { t.Fatalf("deleted rule = %d, want 404", rec.Code) } + // A status, match or query outside the allowed values is refused, not + // replaced by the default (WR-17). + for field, value := range map[string]any{"status": 308, "match": "Regex", "query": "keep"} { + rec := env.mcp(t, http.MethodPost, "/_seo/api/v1/redirects", agent, map[string]any{"source": "/coerced", "target": "/y", field: value}) + if rec.Code != http.StatusUnprocessableEntity || !strings.Contains(rec.Body.String(), `"`+field+`"`) { + t.Fatalf("create_redirect %s=%v = %d %s, want 422 on %s", field, value, rec.Code, rec.Body.String(), field) + } + } + // Redirects: a chain is saved with a warning, a loop refused. first := env.mcpExpect(t, http.StatusCreated, http.MethodPost, "/_seo/api/v1/redirects", agent, map[string]any{"source": "/x", "target": "/y"}) if d, _ := first["data"].(map[string]any); d["origin"] != "mcp" || d["status"] != float64(301) { diff --git a/transfer_test.go b/transfer_test.go index 2fea39c..fbbcb4f 100644 --- a/transfer_test.go +++ b/transfer_test.go @@ -89,6 +89,10 @@ func TestCSVTransfer(t *testing.T) { } // Refusals of the file as a whole. + odd := env.mcpExpect(t, http.StatusOK, http.MethodPost, "/_seo/api/v1/import", agent, map[string]any{"dataset": "redirects", "csv": "source;match;target;query\n/g1;glob;/x;ignore\n/g2;exact;/x;keep\n", "delimiter": ";", "dry_run": true}) + if s := summary(t, odd); s["refused"] != 2 { + t.Fatalf("import with an unknown match and query = %v, want 2 refused rows (WR-17)", odd) + } rec := env.mcp(t, http.MethodPost, "/_seo/api/v1/import", agent, map[string]any{"dataset": "redirects", "csv": "target;status\n/x;301\n", "delimiter": ";", "dry_run": true}) if rec.Code != http.StatusUnprocessableEntity || !strings.Contains(rec.Body.String(), `"error":"missing_column"`) || !strings.Contains(rec.Body.String(), "source") { t.Fatalf("missing column = %d %s", rec.Code, rec.Body.String())