From 192081c2921131f29ff3d26e9d6778f192a666ab Mon Sep 17 00:00:00 2001 From: Jason Song Date: Fri, 30 Dec 2022 10:54:45 +0800 Subject: [PATCH 1/6] docs: fix comments --- modules/sitemap/sitemap.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/modules/sitemap/sitemap.go b/modules/sitemap/sitemap.go index ceb65c1c8d24d..7caa1db8cc73e 100644 --- a/modules/sitemap/sitemap.go +++ b/modules/sitemap/sitemap.go @@ -14,13 +14,13 @@ import ( // sitemapFileLimit contains the maximum size of a sitemap file const sitemapFileLimit = 50 * 1024 * 1024 -// Url represents a single sitemap entry +// URL represents a single sitemap entry type URL struct { URL string `xml:"loc"` LastMod *time.Time `xml:"lastmod,omitempty"` } -// SitemapUrl represents a sitemap +// Sitemap represents a sitemap type Sitemap struct { XMLName xml.Name Namespace string `xml:"xmlns,attr"` @@ -36,7 +36,7 @@ func NewSitemap() *Sitemap { } } -// NewSitemap creates a sitemap index. +// NewSitemapIndex creates a sitemap index. func NewSitemapIndex() *Sitemap { return &Sitemap{ XMLName: xml.Name{Local: "sitemapindex"}, @@ -49,7 +49,7 @@ func (s *Sitemap) Add(u URL) { s.URLs = append(s.URLs, u) } -// Write writes the sitemap to a response +// WriteTo writes the sitemap to a response func (s *Sitemap) WriteTo(w io.Writer) (int64, error) { if len(s.URLs) > 50000 { return 0, fmt.Errorf("The sitemap contains too many URLs: %d", len(s.URLs)) From 262c7562d2bd673d764661646aac7152abce1659 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Fri, 30 Dec 2022 11:16:53 +0800 Subject: [PATCH 2/6] test: TestNewSitemap & TestNewSitemapIndex --- modules/sitemap/sitemap_test.go | 141 ++++++++++++++++++++++++-------- 1 file changed, 108 insertions(+), 33 deletions(-) diff --git a/modules/sitemap/sitemap_test.go b/modules/sitemap/sitemap_test.go index ab879b272e70f..757270750dd0b 100644 --- a/modules/sitemap/sitemap_test.go +++ b/modules/sitemap/sitemap_test.go @@ -14,47 +14,122 @@ import ( "github.com/stretchr/testify/assert" ) -func TestOk(t *testing.T) { - testReal := func(s *Sitemap, name string, urls []URL, expected string) { - for _, url := range urls { - s.Add(url) - } - buf := &bytes.Buffer{} - _, err := s.WriteTo(buf) - assert.NoError(t, nil, err) - assert.Equal(t, xml.Header+"<"+name+" xmlns=\"http://www.sitemaps.org/schemas/sitemap/0.9\">"+expected+"\n", buf.String()) +func TestNewSitemap(t *testing.T) { + ts := time.Unix(1651322008, 0).UTC() + + tests := []struct { + name string + urls []URL + want string + }{ + { + name: "empty", + urls: []URL{}, + want: xml.Header + `` + + "" + + "\n", + }, + { + name: "regular", + urls: []URL{ + {URL: "https://gitea.io/test1", LastMod: &ts}, + }, + want: xml.Header + `` + + "https://gitea.io/test12022-04-30T12:33:28Z" + + "\n", + }, + { + name: "without lastmod", + urls: []URL{ + {URL: "https://gitea.io/test1"}, + }, + want: xml.Header + `` + + "https://gitea.io/test1" + + "\n", + }, + { + name: "multiple", + urls: []URL{ + {URL: "https://gitea.io/test1", LastMod: &ts}, + {URL: "https://gitea.io/test2", LastMod: nil}, + }, + want: xml.Header + `` + + "https://gitea.io/test12022-04-30T12:33:28Z" + + "https://gitea.io/test1" + + "\n", + }, } - test := func(urls []URL, expected string) { - testReal(NewSitemap(), "urlset", urls, expected) - testReal(NewSitemapIndex(), "sitemapindex", urls, expected) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s := NewSitemap() + for _, url := range tt.urls { + s.Add(url) + } + buf := &bytes.Buffer{} + _, err := s.WriteTo(buf) + assert.NoError(t, nil, err) + assert.Equalf(t, tt.want, buf.String(), "NewSitemap()") + }) } +} +func TestNewSitemapIndex(t *testing.T) { ts := time.Unix(1651322008, 0).UTC() - test( - []URL{}, - "", - ) - test( - []URL{ - {URL: "https://gitea.io/test1", LastMod: &ts}, + tests := []struct { + name string + urls []URL + want string + }{ + { + name: "empty", + urls: []URL{}, + want: xml.Header + `` + + "" + + "\n", + }, + { + name: "regular", + urls: []URL{ + {URL: "https://gitea.io/test1", LastMod: &ts}, + }, + want: xml.Header + `` + + "https://gitea.io/test12022-04-30T12:33:28Z" + + "\n", }, - "https://gitea.io/test12022-04-30T12:33:28Z", - ) - test( - []URL{ - {URL: "https://gitea.io/test2", LastMod: nil}, + { + name: "without lastmod", + urls: []URL{ + {URL: "https://gitea.io/test1"}, + }, + want: xml.Header + `` + + "https://gitea.io/test1" + + "\n", }, - "https://gitea.io/test2", - ) - test( - []URL{ - {URL: "https://gitea.io/test1", LastMod: &ts}, - {URL: "https://gitea.io/test2", LastMod: nil}, + { + name: "multiple", + urls: []URL{ + {URL: "https://gitea.io/test1", LastMod: &ts}, + {URL: "https://gitea.io/test2", LastMod: nil}, + }, + want: xml.Header + `` + + "https://gitea.io/test12022-04-30T12:33:28Z" + + "https://gitea.io/test1" + + "\n", }, - "https://gitea.io/test12022-04-30T12:33:28Z"+ - "https://gitea.io/test2", - ) + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s := NewSitemapIndex() + for _, url := range tt.urls { + s.Add(url) + } + buf := &bytes.Buffer{} + _, err := s.WriteTo(buf) + assert.NoError(t, nil, err) + assert.Equalf(t, tt.want, buf.String(), "NewSitemapIndex()") + }) + } } func TestTooManyURLs(t *testing.T) { From fea51988e55bc4d4b95c41d165540a9cdaf5c987 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Fri, 30 Dec 2022 11:27:20 +0800 Subject: [PATCH 3/6] fix: sitemap index --- modules/sitemap/sitemap.go | 34 +++++++++++++++++++++++---------- modules/sitemap/sitemap_test.go | 4 ++-- 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/modules/sitemap/sitemap.go b/modules/sitemap/sitemap.go index 7caa1db8cc73e..576ea978feb44 100644 --- a/modules/sitemap/sitemap.go +++ b/modules/sitemap/sitemap.go @@ -11,8 +11,14 @@ import ( "time" ) -// sitemapFileLimit contains the maximum size of a sitemap file -const sitemapFileLimit = 50 * 1024 * 1024 +const ( + sitemapFileLimit = 50 * 1024 * 1024 // the maximum size of a sitemap file + urlsLimit = 50000 + + schemaURL = "http://www.sitemaps.org/schemas/sitemap/0.9" + urlsetName = "urlset" + sitemapindexName = "sitemapindex" +) // URL represents a single sitemap entry type URL struct { @@ -25,34 +31,42 @@ type Sitemap struct { XMLName xml.Name Namespace string `xml:"xmlns,attr"` - URLs []URL `xml:"url"` + URLs []URL `xml:"url"` + Sitemaps []URL `xml:"sitemap"` } // NewSitemap creates a sitemap func NewSitemap() *Sitemap { return &Sitemap{ - XMLName: xml.Name{Local: "urlset"}, - Namespace: "http://www.sitemaps.org/schemas/sitemap/0.9", + XMLName: xml.Name{Local: urlsetName}, + Namespace: schemaURL, } } // NewSitemapIndex creates a sitemap index. func NewSitemapIndex() *Sitemap { return &Sitemap{ - XMLName: xml.Name{Local: "sitemapindex"}, - Namespace: "http://www.sitemaps.org/schemas/sitemap/0.9", + XMLName: xml.Name{Local: sitemapindexName}, + Namespace: schemaURL, } } // Add adds a URL to the sitemap func (s *Sitemap) Add(u URL) { - s.URLs = append(s.URLs, u) + if s.XMLName.Local == sitemapindexName { + s.Sitemaps = append(s.Sitemaps, u) + } else { + s.URLs = append(s.URLs, u) + } } // WriteTo writes the sitemap to a response func (s *Sitemap) WriteTo(w io.Writer) (int64, error) { - if len(s.URLs) > 50000 { - return 0, fmt.Errorf("The sitemap contains too many URLs: %d", len(s.URLs)) + if l := len(s.URLs); l > urlsLimit { + return 0, fmt.Errorf("The sitemap contains too many URLs: %d", l) + } + if l := len(s.Sitemaps); l > urlsLimit { + return 0, fmt.Errorf("The sitemap contains too many URLs: %d", l) } buf := bytes.NewBufferString(xml.Header) if err := xml.NewEncoder(buf).Encode(s); err != nil { diff --git a/modules/sitemap/sitemap_test.go b/modules/sitemap/sitemap_test.go index 757270750dd0b..ad7f5d8720609 100644 --- a/modules/sitemap/sitemap_test.go +++ b/modules/sitemap/sitemap_test.go @@ -55,7 +55,7 @@ func TestNewSitemap(t *testing.T) { }, want: xml.Header + `` + "https://gitea.io/test12022-04-30T12:33:28Z" + - "https://gitea.io/test1" + + "https://gitea.io/test2" + "\n", }, } @@ -114,7 +114,7 @@ func TestNewSitemapIndex(t *testing.T) { }, want: xml.Header + `` + "https://gitea.io/test12022-04-30T12:33:28Z" + - "https://gitea.io/test1" + + "https://gitea.io/test2" + "\n", }, } From ed20256271d4063e262574291f11a1022f87d3ec Mon Sep 17 00:00:00 2001 From: Jason Song Date: Fri, 30 Dec 2022 11:37:59 +0800 Subject: [PATCH 4/6] test: merge cases --- modules/sitemap/sitemap_test.go | 73 ++++++++++++++++++++------------- 1 file changed, 44 insertions(+), 29 deletions(-) diff --git a/modules/sitemap/sitemap_test.go b/modules/sitemap/sitemap_test.go index ad7f5d8720609..4c0d5266fcfa4 100644 --- a/modules/sitemap/sitemap_test.go +++ b/modules/sitemap/sitemap_test.go @@ -6,7 +6,6 @@ package sitemap import ( "bytes" "encoding/xml" - "fmt" "strings" "testing" "time" @@ -18,9 +17,10 @@ func TestNewSitemap(t *testing.T) { ts := time.Unix(1651322008, 0).UTC() tests := []struct { - name string - urls []URL - want string + name string + urls []URL + want string + wantErr string }{ { name: "empty", @@ -58,6 +58,18 @@ func TestNewSitemap(t *testing.T) { "https://gitea.io/test2" + "\n", }, + { + name: "too many urls", + urls: make([]URL, 50001), + wantErr: "The sitemap contains too many URLs: 50001", + }, + { + name: "too big file", + urls: []URL{ + {URL: strings.Repeat("b", 50*1024*1024+1)}, + }, + wantErr: "The sitemap is too big: 52428932", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -67,8 +79,12 @@ func TestNewSitemap(t *testing.T) { } buf := &bytes.Buffer{} _, err := s.WriteTo(buf) - assert.NoError(t, nil, err) - assert.Equalf(t, tt.want, buf.String(), "NewSitemap()") + if tt.wantErr != "" { + assert.EqualError(t, err, tt.wantErr) + } else { + assert.NoError(t, err) + assert.Equalf(t, tt.want, buf.String(), "NewSitemap()") + } }) } } @@ -77,9 +93,10 @@ func TestNewSitemapIndex(t *testing.T) { ts := time.Unix(1651322008, 0).UTC() tests := []struct { - name string - urls []URL - want string + name string + urls []URL + want string + wantErr string }{ { name: "empty", @@ -117,6 +134,18 @@ func TestNewSitemapIndex(t *testing.T) { "https://gitea.io/test2" + "\n", }, + { + name: "too many urls", + urls: make([]URL, 50001), + wantErr: "The sitemap contains too many URLs: 50001", + }, + { + name: "too big file", + urls: []URL{ + {URL: strings.Repeat("b", 50*1024*1024+1)}, + }, + wantErr: "The sitemap is too big: 52428952", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -126,26 +155,12 @@ func TestNewSitemapIndex(t *testing.T) { } buf := &bytes.Buffer{} _, err := s.WriteTo(buf) - assert.NoError(t, nil, err) - assert.Equalf(t, tt.want, buf.String(), "NewSitemapIndex()") + if tt.wantErr != "" { + assert.EqualError(t, err, tt.wantErr) + } else { + assert.NoError(t, err) + assert.Equalf(t, tt.want, buf.String(), "NewSitemapIndex()") + } }) } } - -func TestTooManyURLs(t *testing.T) { - s := NewSitemap() - for i := 0; i < 50001; i++ { - s.Add(URL{URL: fmt.Sprintf("https://gitea.io/test%d", i)}) - } - buf := &bytes.Buffer{} - _, err := s.WriteTo(buf) - assert.EqualError(t, err, "The sitemap contains too many URLs: 50001") -} - -func TestSitemapTooBig(t *testing.T) { - s := NewSitemap() - s.Add(URL{URL: strings.Repeat("b", sitemapFileLimit)}) - buf := &bytes.Buffer{} - _, err := s.WriteTo(buf) - assert.EqualError(t, err, "The sitemap is too big: 52428931") -} From d544a88c7aae5316dc2c9cb0cdde88ba0e578983 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Fri, 30 Dec 2022 22:27:50 +0800 Subject: [PATCH 5/6] Update modules/sitemap/sitemap.go Co-authored-by: delvh --- modules/sitemap/sitemap.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/sitemap/sitemap.go b/modules/sitemap/sitemap.go index 576ea978feb44..2522c785f746f 100644 --- a/modules/sitemap/sitemap.go +++ b/modules/sitemap/sitemap.go @@ -63,10 +63,10 @@ func (s *Sitemap) Add(u URL) { // WriteTo writes the sitemap to a response func (s *Sitemap) WriteTo(w io.Writer) (int64, error) { if l := len(s.URLs); l > urlsLimit { - return 0, fmt.Errorf("The sitemap contains too many URLs: %d", l) + return 0, fmt.Errorf("The sitemap contains %d URLs, but only %d are allowed", l, urlsLimit) } if l := len(s.Sitemaps); l > urlsLimit { - return 0, fmt.Errorf("The sitemap contains too many URLs: %d", l) + return 0, fmt.Errorf("The sitemap contains %d sub-sitemaps, but only %d are allowed", l, urlsLimit) } buf := bytes.NewBufferString(xml.Header) if err := xml.NewEncoder(buf).Encode(s); err != nil { From 9476f2ad738716192f67c25d3523dd62c1ef9f74 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Fri, 30 Dec 2022 22:33:49 +0800 Subject: [PATCH 6/6] fix: error format --- modules/sitemap/sitemap.go | 2 +- modules/sitemap/sitemap_test.go | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/modules/sitemap/sitemap.go b/modules/sitemap/sitemap.go index 2522c785f746f..280ca1d710070 100644 --- a/modules/sitemap/sitemap.go +++ b/modules/sitemap/sitemap.go @@ -76,7 +76,7 @@ func (s *Sitemap) WriteTo(w io.Writer) (int64, error) { return 0, err } if buf.Len() > sitemapFileLimit { - return 0, fmt.Errorf("The sitemap is too big: %d", buf.Len()) + return 0, fmt.Errorf("The sitemap has %d bytes, but only %d are allowed", buf.Len(), sitemapFileLimit) } return buf.WriteTo(w) } diff --git a/modules/sitemap/sitemap_test.go b/modules/sitemap/sitemap_test.go index 4c0d5266fcfa4..1180463cd79e2 100644 --- a/modules/sitemap/sitemap_test.go +++ b/modules/sitemap/sitemap_test.go @@ -61,14 +61,14 @@ func TestNewSitemap(t *testing.T) { { name: "too many urls", urls: make([]URL, 50001), - wantErr: "The sitemap contains too many URLs: 50001", + wantErr: "The sitemap contains 50001 URLs, but only 50000 are allowed", }, { name: "too big file", urls: []URL{ {URL: strings.Repeat("b", 50*1024*1024+1)}, }, - wantErr: "The sitemap is too big: 52428932", + wantErr: "The sitemap has 52428932 bytes, but only 52428800 are allowed", }, } for _, tt := range tests { @@ -135,16 +135,16 @@ func TestNewSitemapIndex(t *testing.T) { "\n", }, { - name: "too many urls", + name: "too many sitemaps", urls: make([]URL, 50001), - wantErr: "The sitemap contains too many URLs: 50001", + wantErr: "The sitemap contains 50001 sub-sitemaps, but only 50000 are allowed", }, { name: "too big file", urls: []URL{ {URL: strings.Repeat("b", 50*1024*1024+1)}, }, - wantErr: "The sitemap is too big: 52428952", + wantErr: "The sitemap has 52428952 bytes, but only 52428800 are allowed", }, } for _, tt := range tests {