From 7677d8d8aff408069bb61cea0ed0d2063e1bdad5 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Tue, 27 Oct 2026 15:41:18 +0100 Subject: [PATCH] fix: the Open/Resolved and Chains filters apply an exact rule's slash, case and query options (WR-22) coveredSQL and chainSQL compared the source literally, so /missing/ or /missing?utm=x stayed open after /missing answered it, and /a -> /b/ was no chain next to /b. Both now use one SQL form of the runtime's exact match (prefiltered on the indexed source_key). Pattern and regex rules remain with the per-row badge, which the list_not_found schema text now says. --- controllers/api/notfound.go | 2 +- models/notfound.go | 8 ++++--- models/redirect.go | 39 +++++++++++++++++++++++++++--- redirects_test.go | 48 +++++++++++++++++++++++++++++++++++++ 4 files changed, 90 insertions(+), 7 deletions(-) diff --git a/controllers/api/notfound.go b/controllers/api/notfound.go index 9bd79ea..f4668e2 100644 --- a/controllers/api/notfound.go +++ b/controllers/api/notfound.go @@ -71,7 +71,7 @@ type ListNotFoundInput struct { Page int `json:"page,omitempty" query:"page" jsonschema:"the page number, from 1"` PerPage int `json:"per_page,omitempty" query:"per_page" jsonschema:"rows per page, at most 200 (50 by default)"` Search string `json:"search,omitempty" query:"search" jsonschema:"text the address contains"` - Status string `json:"status,omitempty" query:"status" jsonschema:"open (no redirect answers the address yet) or resolved"` + Status string `json:"status,omitempty" query:"status" jsonschema:"open or resolved: whether an exact redirect or 410 answers the address, with its case, trailing-slash and query options; an address only a pattern or regex rule answers stays open here but has resolved true in its row"` } // NotFoundIDInput names one 404 log entry. diff --git a/models/notfound.go b/models/notfound.go index 3e931af..15d05ec 100644 --- a/models/notfound.go +++ b/models/notfound.go @@ -41,9 +41,11 @@ const ( // FilterScopes lists the 404 log's filter scopes. func (NotFound) FilterScopes() []string { return []string{FilterNotFoundStatus} } -// coveredSQL is true for an entry an exact redirect or 410 answers now -// (the list filter; the Resolved badge also counts patterns and regex). -const coveredSQL = `EXISTS (SELECT 1 FROM golem15_seo_redirects AS r WHERE r.match = 'exact' AND (r.source = golem15_seo_notfound.url OR (r.ignore_case AND lower(r.source) = lower(golem15_seo_notfound.url))))` +// coveredSQL is true for an entry an exact redirect or 410 answers now, +// with the rule's case, trailing-slash and query options as the runtime +// matcher applies them (WR-22). Pattern and regex rules are left to the +// Resolved badge, which runs the compiled rule set. +var coveredSQL = `EXISTS (SELECT 1 FROM golem15_seo_redirects AS r WHERE ` + coversSQL("r", "golem15_seo_notfound.url") + `)` // FilterScope narrows the 404 log to open or resolved entries; an unknown // value matches nothing. diff --git a/models/redirect.go b/models/redirect.go index 3cfb721..8e4f8ab 100644 --- a/models/redirect.go +++ b/models/redirect.go @@ -224,9 +224,42 @@ func (Redirect) FilterScopes() []string { return []string{FilterRedirectStatus, FilterRedirectMatch, FilterRedirectOrigin, FilterRedirectChain} } -// chainSQL is true for a redirect whose target is another redirect's -// source. -const chainSQL = `EXISTS (SELECT 1 FROM golem15_seo_redirects AS next WHERE next.source = golem15_seo_redirects.target AND next.id <> golem15_seo_redirects.id)` +// coversSQL is an SQL condition, true when the exact redirect aliased +// r matches the address addr (an SQL expression of a path with an optional +// query) the way the runtime matcher does (classes.exactMatches): the +// source's path with its ignore_case and trailing_slash options, and, when +// the source has a query, the address's query exactly (WR-22). The +// source_key test (the generated column of classes.exactKey) is a +// necessary condition that lets Postgres use its index; the rest is exact. +// Pattern and regex rules are not covered: their expressions are RE2 and +// only the compiled RedirectSet evaluates them. +func coversSQL(r, addr string) string { + return strings.NewReplacer("R.", r+".", "ADDR", addr).Replace(exactCoversSQL) +} + +// exactCoversSQL is coversSQL with the placeholders R. and ADDR. +const exactCoversSQL = `(R.match = 'exact' + AND R.source_key IN ( + lower(CASE WHEN length(split_part(ADDR, '?', 1)) > 1 AND right(split_part(ADDR, '?', 1), 1) = '/' THEN left(split_part(ADDR, '?', 1), -1) ELSE split_part(ADDR, '?', 1) END), + lower(CASE WHEN length(split_part(ADDR, '?', 1)) > 1 AND right(split_part(ADDR, '?', 1), 1) = '/' THEN left(split_part(ADDR, '?', 1), -1) ELSE split_part(ADDR, '?', 1) END) + || '?' || (CASE WHEN strpos(ADDR, '?') > 0 THEN substr(ADDR, strpos(ADDR, '?') + 1) ELSE '' END)) + AND (strpos(R.source, '?') = 0 + OR substr(R.source, strpos(R.source, '?') + 1) = (CASE WHEN strpos(ADDR, '?') > 0 THEN substr(ADDR, strpos(ADDR, '?') + 1) ELSE '' END)) + AND (split_part(R.source, '?', 1) = split_part(ADDR, '?', 1) + OR (R.ignore_case AND lower(split_part(R.source, '?', 1)) = lower(split_part(ADDR, '?', 1))) + OR (R.trailing_slash AND ( + (CASE WHEN length(split_part(R.source, '?', 1)) > 1 AND right(split_part(R.source, '?', 1), 1) = '/' THEN left(split_part(R.source, '?', 1), -1) ELSE split_part(R.source, '?', 1) END) + = (CASE WHEN length(split_part(ADDR, '?', 1)) > 1 AND right(split_part(ADDR, '?', 1), 1) = '/' THEN left(split_part(ADDR, '?', 1), -1) ELSE split_part(ADDR, '?', 1) END) + OR (R.ignore_case AND lower(CASE WHEN length(split_part(R.source, '?', 1)) > 1 AND right(split_part(R.source, '?', 1), 1) = '/' THEN left(split_part(R.source, '?', 1), -1) ELSE split_part(R.source, '?', 1) END) + = lower(CASE WHEN length(split_part(ADDR, '?', 1)) > 1 AND right(split_part(ADDR, '?', 1), 1) = '/' THEN left(split_part(ADDR, '?', 1), -1) ELSE split_part(ADDR, '?', 1) END))))))` + +// chainSQL is true for a redirect whose target leads into another +// redirect: a path target that another rule's source names literally or +// an exact rule matches with its options (WR-22). Absolute targets on the +// site URL and pattern or regex next hops are only seen by the save-time +// check (the chain notice). +var chainSQL = `EXISTS (SELECT 1 FROM golem15_seo_redirects AS next WHERE next.id <> golem15_seo_redirects.id AND (next.source = golem15_seo_redirects.target OR (golem15_seo_redirects.target LIKE '/%' AND golem15_seo_redirects.target NOT LIKE '//%' AND ` + + coversSQL("next", "golem15_seo_redirects.target") + `)))` // FilterScope narrows the redirects list; an unknown value matches // nothing. diff --git a/redirects_test.go b/redirects_test.go index 8b94fb6..fbe4df8 100644 --- a/redirects_test.go +++ b/redirects_test.go @@ -893,3 +893,51 @@ func TestConcurrentUndo(t *testing.T) { t.Fatalf("undo history rows = %d (err %v), want 1", undos, err) } } + +// TestCoverAndChainFilters checks that the 404 log's Open/Resolved filter +// and the redirects list's Chains filter apply an exact rule's +// trailing-slash, case and query options like the runtime matcher (WR-22). +func TestCoverAndChainFilters(t *testing.T) { + env := bootEnv(t) + agent := env.principal(t, env.admin(t, false, "golem15.seo.manage")) + env.mustRedirect(t, "/missing", "exact", "/found", 301) + env.mustRedirect(t, "/Case", "exact", "/found", 301, func(r *models.Redirect) { r.IgnoreCase = true }) + env.mustRedirect(t, "/q?id=1", "exact", "/found", 301) + for _, u := range []string{"/missing/", "/missing?utm=x", "/case/", "/q?id=1", "/q?id=2", "/MISSING", "/other"} { + if err := env.db.Exec(`INSERT INTO golem15_seo_notfound (url, hits, first_seen, last_hit) VALUES (?, 1, NOW(), NOW())`, u).Error; err != nil { + t.Fatal(err) + } + } + urls := func(status string) []string { + t.Helper() + got := env.mcpExpect(t, http.StatusOK, http.MethodGet, "/_seo/api/v1/notfound?per_page=200&status="+status, agent, nil) + var out []string + for _, row := range got["data"].([]any) { + out = append(out, row.(map[string]any)["url"].(string)) + if resolved := row.(map[string]any)["resolved"].(bool); resolved != (status == "resolved") { + t.Errorf("%s: row %v disagrees with its filter", status, row) + } + } + sort.Strings(out) + return out + } + if got := strings.Join(urls("resolved"), ","); got != "/case/,/missing/,/missing?utm=x,/q?id=1" { + t.Fatalf("resolved = %s", got) + } + if got := strings.Join(urls("open"), ","); got != "/MISSING,/other,/q?id=2" { + t.Fatalf("open = %s", got) + } + + env.mustRedirect(t, "/c2", "exact", "/c3", 301) + env.mustRedirect(t, "/c1", "exact", "/c2/", 301) + env.mustRedirect(t, "/d1", "exact", "/C2", 301) + var chained []string + q := (models.Redirect{}).FilterScope(models.FilterRedirectChain, env.db.Model(&models.Redirect{}), models.ChainOnly) + if err := q.Order("source").Pluck("source", &chained).Error; err != nil { + t.Fatal(err) + } + if got := strings.Join(chained, ","); got != "/c1" { + t.Fatalf("chains = %s, want /c1 (/c2/ reaches /c2 with its trailing slash; /C2 does not, /c2 is case-sensitive)", got) + } +} +