diff --git a/README.md b/README.md index 7cccad6..4ddde7f 100644 --- a/README.md +++ b/README.md @@ -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. | diff --git a/classes/rebuild.go b/classes/rebuild.go index f27f157..92aa3c9 100644 --- a/classes/rebuild.go +++ b/classes/rebuild.go @@ -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 diff --git a/classes/rebuild_test.go b/classes/rebuild_test.go new file mode 100644 index 0000000..eba3c51 --- /dev/null +++ b/classes/rebuild_test.go @@ -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) + } +} diff --git a/controllers/settings.go b/controllers/settings.go index 22f21d1..9ba5485 100644 --- a/controllers/settings.go +++ b/controllers/settings.go @@ -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 diff --git a/lang/en/lang.yaml b/lang/en/lang.yaml index 20bdf7c..dc2c9cc 100644 --- a/lang/en/lang.yaml +++ b/lang/en/lang.yaml @@ -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. diff --git a/lang/pl/lang.yaml b/lang/pl/lang.yaml index 51ce228..b750644 100644 --- a/lang/pl/lang.yaml +++ b/lang/pl/lang.yaml @@ -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ę. diff --git a/models/url.go b/models/url.go index fa50f00..60d4c60 100644 --- a/models/url.go +++ b/models/url.go @@ -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 diff --git a/sitemap_index_test.go b/sitemap_index_test.go index 65ad754..36771fd 100644 --- a/sitemap_index_test.go +++ b/sitemap_index_test.go @@ -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