fix: list an additional address once when a content record has it

- an additional address whose absolute URL a listed record already has is
  left out with the new reason duplicate (models.ReasonDuplicate)
- the settings screen counts it under Already listed by a content type
- a record left out by the filter or the type switch does not count
This commit is contained in:
Jakub Zych
2026-11-04 19:22:56 +01:00
parent 9e5c393709
commit 096b6f6bfe
8 changed files with 147 additions and 8 deletions

View File

@@ -191,7 +191,7 @@ func (p *Plugin) Register(app *backpack.App) error {
}
```
`Decide` gets one page of records of one content type (or the additional addresses, type key `extra`) and answers one decision per candidate in order. An error or a wrong number of decisions fails the rebuild, and the files stay as they were. Excluded addresses are counted by reason in the settings screen (noindex, canonical, redirect; a type switched off counts as type).
`Decide` gets one page of records of one content type (or the additional addresses, type key `extra`) and answers one decision per candidate in order. An error or a wrong number of decisions fails the rebuild, and the files stay as they were. Excluded addresses are counted by reason in the settings screen (noindex, canonical, redirect; a type switched off counts as type). An additional address that a content record already lists (the same absolute URL) is left out as `duplicate`, so the files carry it once, with the record's lastmod and alternates; it is not sent to `Decide`.
### Listing
@@ -259,7 +259,7 @@ The files table lists the image and News files too. A save refuses an invalid si
| `classes.Listing` | The `contract.Listing` implementation. |
| `classes.ExtraRecordID` | The record id of an additional address. |
| `models.URL`, `models.Alternate`, `models.Ref` | A row of the slot table, an hreflang alternate and a parent reference. |
| `models.ExtraTypeKey`, `models.ExtraGroup`, `models.ReasonDeleted`, `models.ReasonUnpublished` | Additional addresses and the holes' reasons. |
| `models.ExtraTypeKey`, `models.ExtraGroup`, `models.ReasonDeleted`, `models.ReasonUnpublished`, `models.ReasonDuplicate` | Additional addresses, the holes' reasons and the reason of an additional address a content record already lists. |
| `models.Settings`, `models.DefaultSettings`, `models.ExtraURL`, `models.DefaultFileSize`, `models.MaxFileSize` | The settings row; `Validate` and `Problems` are its save checks. |
| `models.DefaultHTMLPageSize`, `models.MinHTMLPageSize`, `models.MaxHTMLPageSize`, `models.DefaultHTMLPaths`, `models.ValidHTMLPath`, `models.HTMLPathProblem` | The HTML sitemap settings and their checks. |
| `models.ValidNewsSource`, `models.NewsSourceType`, `models.NewsSourceCategory`, `models.MaxNewsPublication` | The News settings' checks. |

View File

@@ -177,7 +177,8 @@ func Rebuild(ctx context.Context, app *backpack.App) (RebuildResult, error) {
// first lastmod of a new row. The hreflang alternates of a record are its
// listed addresses (D-55 to D-57). Each address keeps its page's images
// (PageImages, on site), the type's kind, the record's first publication
// and its parents.
// and its parents. An additional address a listed record already has is
// left out as models.ReasonDuplicate (markListedExtras).
func collect(ctx context.Context, reg *signpost.Registry, filter contract.Filter, settings models.Settings, types map[string]models.TypeSetting, site string) (collected, error) {
out := collected{unpublished: map[slotKey]bool{}}
seen := map[slotKey]bool{}
@@ -255,8 +256,19 @@ func collect(ctx context.Context, reg *signpost.Registry, filter contract.Filter
}
}
}
extras := extraRows(settings.Extra())
markListedExtras(out.rows, extras, site)
if err := batch(extras); err != nil {
return out, err
}
return out, nil
}
// extraRows turns the settings' additional addresses into rows; an address
// that is not allowed is skipped.
func extraRows(list []models.ExtraURL) []wanted {
var extras []wanted
for _, e := range settings.Extra() {
for _, e := range list {
u := strings.TrimSpace(e.URL)
if !models.AllowedAddress(u) {
continue
@@ -271,10 +283,39 @@ func collect(ctx context.Context, reg *signpost.Registry, filter contract.Filter
firstMod: mod,
})
}
if err := batch(extras); err != nil {
return out, err
return extras
}
// markListedExtras leaves out (models.ReasonDuplicate) every additional
// address whose absolute URL a content record already lists, so an address
// both in a content type and in the additional addresses (a definition
// migrated from the old sitemap, for example) is in the files once, with
// the record's lastmod and alternates. A record left out by the filter or
// the type switch does not count: the editor's additional address then
// stays listed. The rows keep their place in the slot table, so the
// settings screen counts them by reason.
func markListedExtras(records, extras []wanted, site string) {
listed := map[string]bool{}
for _, w := range records {
if w.reason == "" && !w.extra {
listed[addressKey(site, w.url)] = true
}
}
return out, nil
for i := range extras {
if extras[i].reason == "" && listed[addressKey(site, extras[i].url)] {
extras[i].reason = models.ReasonDuplicate
}
}
}
// addressKey is the absolute form of an address for comparison, or the
// address itself when it has none (a path without a site URL).
func addressKey(site, u string) string {
u = strings.TrimSpace(u)
if a := Absolute(site, u); a != "" {
return a
}
return u
}
// decide asks filter about the rows not already left out (a type switched

51
classes/rebuild_test.go Normal file
View File

@@ -0,0 +1,51 @@
package classes
import (
"testing"
"git.golem15.com/golem15/sm-sitemap-plugin/contract"
"git.golem15.com/golem15/sm-sitemap-plugin/models"
)
// TestMarkListedExtras pins the de-duplication of the additional
// addresses: an address a listed record has (as a path or as the same
// absolute URL) is left out as duplicate; a record left out by the filter
// or the type switch, another address and a record's other locale do not
// count; an address already left out keeps its reason.
func TestMarkListedExtras(t *testing.T) {
const site = "https://example.test"
records := []wanted{
{key: slotKey{"page", "1", "en"}, url: "/"},
{key: slotKey{"page", "2", "en"}, url: "/blog"},
{key: slotKey{"page", "3", "en"}, url: "/faq", reason: contract.ReasonNoindex},
{key: slotKey{"page", "4", "en"}, url: "/off", reason: contract.ReasonType},
{key: slotKey{"page", "5", "pl"}, url: "/pl/o-nas"},
}
extras := extraRows([]models.ExtraURL{
{URL: "/"},
{URL: site + "/blog"},
{URL: "/faq"},
{URL: "/off"},
{URL: "/kontakt"},
{URL: "/o-nas"},
{URL: "javascript:alert(1)"},
})
if len(extras) != 6 {
t.Fatalf("extraRows kept %d addresses, want 6 (the javascript: one is not allowed)", len(extras))
}
extras[5].reason = contract.ReasonNoindex
markListedExtras(records, extras, site)
want := []string{models.ReasonDuplicate, models.ReasonDuplicate, "", "", "", contract.ReasonNoindex}
for i, w := range extras {
if w.reason != want[i] {
t.Errorf("extra %s: reason %q, want %q", w.url, w.reason, want[i])
}
}
// Without a site URL the paths are compared as they are.
extras = extraRows([]models.ExtraURL{{URL: "/blog"}, {URL: site + "/blog"}})
markListedExtras(records, extras, "")
if extras[0].reason != models.ReasonDuplicate || extras[1].reason != "" {
t.Fatalf("without a site URL: %q %q, want duplicate and listed", extras[0].reason, extras[1].reason)
}
}

View File

@@ -31,7 +31,7 @@ const (
// The reasons the files partial counts as "left out", in display order
// (P33-11). A deleted or unpublished address is a hole, not left out.
var leftOutReasons = []string{contract.ReasonNoindex, contract.ReasonCanonical, contract.ReasonRedirect, contract.ReasonType}
var leftOutReasons = []string{contract.ReasonNoindex, contract.ReasonCanonical, contract.ReasonRedirect, contract.ReasonType, models.ReasonDuplicate}
// settingsAdminController edits the sitemap settings singleton: Files,
// Content types (added by the plugin's own form extension) and Additional

View File

@@ -69,6 +69,7 @@ files:
reason_canonical: Canonical URL points elsewhere
reason_redirect: Address redirects
reason_type: Content type turned off
reason_duplicate: Already listed by a content type
index_note: Search engines read the index at :url, and robots.txt points to it. The files are rebuilt after content changes and every night; saving this form rebuilds them now.
empty: No files yet. They are built within a few minutes after the first save.
building: Rebuilding now. Reload the page in a minute.

View File

@@ -69,6 +69,7 @@ files:
reason_canonical: Adres kanoniczny wskazuje inny adres
reason_redirect: Adres przekierowuje
reason_type: Wyłączony typ treści
reason_duplicate: Już na liście typu treści
index_note: Wyszukiwarki czytają indeks pod :url, a robots.txt go wskazuje. Pliki są odbudowywane po zmianach treści i co noc; zapisanie tego formularza odbudowuje je od razu.
empty: Nie ma jeszcze plików. Powstaną w ciągu kilku minut po pierwszym zapisie.
building: Trwa odbudowa. Odśwież stronę za minutę.

View File

@@ -23,6 +23,9 @@ const (
ReasonDeleted = "deleted"
// ReasonUnpublished is an address whose record is not published.
ReasonUnpublished = "unpublished"
// ReasonDuplicate is an additional address that a content type
// already lists (the same absolute URL), so the files carry it once.
ReasonDuplicate = "duplicate"
)
// Alternate is one hreflang alternate of an address: Hreflang the locale

View File

@@ -760,6 +760,48 @@ func TestNoFilter(t *testing.T) {
}
}
// TestExtraDuplicates pins the de-duplication of the additional addresses
// against the content records: an additional address a listed record has
// (as a path or as the same absolute URL) is in the files once, counted as
// left out with the reason duplicate, and listed again once the record is
// gone.
func TestExtraDuplicates(t *testing.T) {
e := bootIndex(t)
mod := time.Date(2026, 10, 1, 8, 0, 0, 0, time.UTC)
contentStore.put(pageType, page1(1, "a", mod))
contentStore.put(pageType, page1(2, "b", mod))
if err := e.gdb.Exec(`UPDATE golem15_sitemap_settings SET extra_urls = '[{"url":"/pages/a"},{"url":"https://api.example.test/pages/b"},{"url":"/kontakt"}]' WHERE id = 1`).Error; err != nil {
t.Fatal(err)
}
e.rebuild(t)
pages := locs(e.get(t, "/_sitemap/files/sitemap-pages-1.xml").Body.String())
if strings.Join(pages, " ") != "https://api.example.test/pages/a https://api.example.test/pages/b https://api.example.test/kontakt" {
t.Fatalf("pages file = %v, want each address once", pages)
}
var n int64
e.gdb.Raw(`SELECT COUNT(*) FROM golem15_sitemap_urls WHERE type_key = 'extra' AND excluded_reason = ?`, models.ReasonDuplicate).Scan(&n)
if n != 2 {
t.Fatalf("%d additional addresses left out as duplicate, want 2", n)
}
s1 := e.call(t, http.MethodGet, "/golem15/sitemap/settings/partials/sitemap_files", e.admin(t, permSitemap), nil).Body.String()
if !strings.Contains(s1, "Already listed by a content type") {
t.Fatalf("S1 lacks the duplicate reason:\n%s", s1)
}
contentStore.remove(pageType, 2)
e.rebuild(t)
pages = locs(e.get(t, "/_sitemap/files/sitemap-pages-1.xml").Body.String())
// The additional address's row has been in the table since the first
// rebuild, so it keeps its place.
if strings.Join(pages, " ") != "https://api.example.test/pages/a https://api.example.test/pages/b https://api.example.test/kontakt" {
t.Fatalf("pages file after the record is gone = %v, want the additional address listed again", pages)
}
e.gdb.Raw(`SELECT COUNT(*) FROM golem15_sitemap_urls WHERE type_key = 'extra' AND excluded_reason = ?`, models.ReasonDuplicate).Scan(&n)
if n != 1 {
t.Fatalf("%d additional addresses left out as duplicate after the record is gone, want 1", n)
}
}
// TestListing pins contract.Listing.Entries: a group's listed addresses in
// pages of Limit with Next and Total, absolute URLs with title and
// lastmod, the locale filter keeping addresses without a locale, and an