all: fix tests

CHANGELOG.md

@@ -32,6 +32,31 @@ NOTE: Add new changes BELOW THIS COMMENT.
   control/querylog/config` accept and return query log config JSON file.  The
   format of request body is described in `openapi/openapi.yaml`.
 
+### Changed
+
+#### Configuration Changes
+
+In this release, the schema version has changed from 16 to 17.
+
+- Property `statistics.interval`, which in schema versions 16 and earlier used
+  to be an integer number of days, is now a string with a human-readable
+  duration:
+
+  ```yaml
+  # BEFORE:
+  'statistics':
+    # …
+    'interval': 1
+
+  # AFTER:
+  'statistics':
+    # …
+    'interval': '24h'
+  ```
+
+  To rollback this change, convert the property back into days and change the
+  `schema_version` back to `16`.
+
 ### Fixed
 
 - Failing service installation via script on FreeBSD ([#5431]).

internal/home/config.go

@@ -486,7 +486,7 @@ func (c *configuration) write() (err error) {
 	if Context.stats != nil {
 		statsConf := stats.Config{}
 		Context.stats.WriteDiskConfig(&statsConf)
-		config.Stats.Interval = timeutil.Duration{Duration: statsConf.LimitIvl}
+		config.Stats.Interval = timeutil.Duration{Duration: statsConf.Limit}
 		config.Stats.Enabled = statsConf.Enabled
 		config.Stats.Ignored = statsConf.Ignored.Values()
 		sort.Strings(config.Stats.Ignored)

internal/home/dns.go

@@ -51,7 +51,7 @@ func initDNS() (err error) {
 
 	statsConf := stats.Config{
 		Filename:       filepath.Join(baseDir, "stats.db"),
-		LimitIvl:       config.Stats.Interval.Duration,
+		Limit:          config.Stats.Interval.Duration,
 		ConfigModified: onConfigModified,
 		HTTPRegister:   httpRegister,
 		Enabled:        config.Stats.Enabled,

internal/querylog/http.go

@@ -38,16 +38,19 @@ type configJSON struct {
 
 // configJSONv2 is the JSON structure for the querylog configuration.
 type configJSONv2 struct {
-	// Enabled shows if the querylog is enabled.  It is an [aghalg.NullBool]
+	// Enabled shows if the querylog is enabled.  It is an
-	// to be able to tell when it's set without using pointers.
+	// [aghalg.NullBool] to be able to tell when it's set without using
+	// pointers.
 	Enabled aghalg.NullBool `json:"enabled"`
 
-	// AnonymizeClientIP shows if the clients' IP addresses must be anonymized.
+	// AnonymizeClientIP shows if the clients' IP addresses must be
-	// It is an [aghalg.NullBool] to be able to tell when it's set without using
+	// anonymized.  It is an [aghalg.NullBool] to be able to tell when it's
-	// pointers.
+	// set without using pointers.
+	//
+	// TODO(a.garipov): Consider using separate setting for statistics.
 	AnonymizeClientIP aghalg.NullBool `json:"anonymize_client_ip"`
 
-	// Interval is the querylog rotation interval.
+	// Interval is the querylog rotation interval in milliseconds
 	Interval float64 `json:"interval"`
 
 	// Ignored is the list of host names, which should not be written to

internal/stats/http.go

@@ -47,7 +47,7 @@ func (s *StatsCtx) handleStats(w http.ResponseWriter, r *http.Request) {
 	defer s.lock.Unlock()
 
 	start := time.Now()
-	resp, ok := s.getData(uint32(s.limitIvl.Hours()))
+	resp, ok := s.getData(uint32(s.limit.Hours()))
 	log.Debug("stats: prepared data in %v", time.Since(start))
 
 	if !ok {
@@ -72,7 +72,7 @@ type configRespV2 struct {
 	// to be able to tell when it's set without using pointers.
 	Enabled aghalg.NullBool `json:"enabled"`
 
-	// Interval is the statistics rotation interval.
+	// Interval is the statistics rotation interval in milliseconds.
 	Interval float64 `json:"interval"`
 
 	// Ignored is the list of host names, which should not be counted.
@@ -86,7 +86,7 @@ func (s *StatsCtx) handleStatsInfo(w http.ResponseWriter, r *http.Request) {
 	s.lock.Lock()
 	defer s.lock.Unlock()
 
-	days := uint32(s.limitIvl.Hours() / 24)
+	days := uint32(s.limit.Hours() / 24)
 
 	ok := checkInterval(days)
 	if !ok || (s.enabled && days == 0) {
@@ -107,7 +107,7 @@ func (s *StatsCtx) handleStatsInfoV2(w http.ResponseWriter, r *http.Request) {
 
 	resp := configRespV2{
 		Enabled:  aghalg.BoolToNullBool(s.enabled),
-		Interval: float64(s.limitIvl.Milliseconds()),
+		Interval: float64(s.limit.Milliseconds()),
 		Ignored:  s.ignored.Values(),
 	}
 	err := aghhttp.WriteJSONResponse(w, r, resp)
@@ -169,7 +169,7 @@ func (s *StatsCtx) handleStatsConfigV2(w http.ResponseWriter, r *http.Request) {
 
 	s.enabled = reqData.Enabled == aghalg.NBTrue
 
-	s.limitIvl = ivl
+	s.limit = ivl
 
 	if len(reqData.Ignored) > 0 {
 		set, serr := aghnet.NewDomainNameSet(reqData.Ignored)

internal/stats/http_test.go

@@ -27,6 +27,7 @@ func TestHandleStatsV2(t *testing.T) {
 		name     string
 		body     configRespV2
 		wantCode int
+		wantErr  string
 	}{{
 		name: "set_ivl_1_hour",
 		body: configRespV2{
@@ -35,6 +36,7 @@ func TestHandleStatsV2(t *testing.T) {
 			Ignored:  nil,
 		},
 		wantCode: http.StatusOK,
+		wantErr:  "",
 	}, {
 		name: "small_interval",
 		body: configRespV2{
@@ -43,6 +45,7 @@ func TestHandleStatsV2(t *testing.T) {
 			Ignored:  []string{},
 		},
 		wantCode: http.StatusBadRequest,
+		wantErr:  "unsupported interval\n",
 	}, {
 		name: "big_interval",
 		body: configRespV2{
@@ -51,6 +54,7 @@ func TestHandleStatsV2(t *testing.T) {
 			Ignored:  []string{},
 		},
 		wantCode: http.StatusBadRequest,
+		wantErr:  "unsupported interval\n",
 	}, {
 		name: "set_ignored_ivl_1_year",
 		body: configRespV2{
@@ -58,10 +62,11 @@ func TestHandleStatsV2(t *testing.T) {
 			Interval: float64(year.Milliseconds()),
 			Ignored: []string{
 				"ignor.ed",
-				"ignor.ed",
+				"ignored.to",
 			},
 		},
-		wantCode: http.StatusBadRequest,
+		wantCode: http.StatusOK,
+		wantErr:  "",
 	}, {
 		name: "ignored_duplicate",
 		body: configRespV2{
@@ -73,6 +78,7 @@ func TestHandleStatsV2(t *testing.T) {
 			},
 		},
 		wantCode: http.StatusBadRequest,
+		wantErr:  "ignored: duplicate host name \"ignor.ed\"\n",
 	}, {
 		name: "ignored_empty",
 		body: configRespV2{
@@ -83,6 +89,7 @@ func TestHandleStatsV2(t *testing.T) {
 			},
 		},
 		wantCode: http.StatusBadRequest,
+		wantErr:  "ignored: host name is empty\n",
 	}}
 
 	for _, tc := range testCases {
@@ -91,7 +98,7 @@ func TestHandleStatsV2(t *testing.T) {
 
 			conf := Config{
 				Filename: filepath.Join(t.TempDir(), "stats.db"),
-				LimitIvl: time.Hour * 24,
+				Limit:    time.Hour * 24,
 				Enabled:  true,
 				UnitID:   func() (id uint32) { return 0 },
 				HTTPRegister: func(
@@ -106,6 +113,7 @@ func TestHandleStatsV2(t *testing.T) {
 
 			s, err := New(conf)
 			require.NoError(t, err)
+
 			s.Start()
 			testutil.CleanupAndRequireSuccess(t, s.Close)
 
@@ -119,22 +127,27 @@ func TestHandleStatsV2(t *testing.T) {
 
 			req := httptest.NewRequest(http.MethodPut, configPut, bytes.NewReader(buf))
 			rw := httptest.NewRecorder()
+
 			handlers[configPut].ServeHTTP(rw, req)
 			require.Equal(t, tc.wantCode, rw.Code)
 
-			if tc.wantCode == http.StatusOK {
+			if tc.wantCode != http.StatusOK {
+				assert.Equal(t, tc.wantErr, rw.Body.String())
+
+				return
+			}
+
 			resp := httptest.NewRequest(http.MethodGet, configGet, nil)
 			rw = httptest.NewRecorder()
+
 			handlers[configGet].ServeHTTP(rw, resp)
 			require.Equal(t, http.StatusOK, rw.Code)
-				data := rw.Body.Bytes()
 
 			ans := configRespV2{}
-				jerr := json.Unmarshal(data, &ans)
+			jerr := json.Unmarshal(rw.Body.Bytes(), &ans)
 			require.NoError(t, jerr)
 
 			assert.Equal(t, tc.body, ans)
-			}
 		})
 	}
 }

internal/stats/stats.go

@@ -43,9 +43,9 @@ type Config struct {
 	// Filename is the name of the database file.
 	Filename string
 
-	// LimitIvl is the interval for collecting statistics into the current
+	// Limit is an upper limit for collecting statistics into the
-	// unit.
+	// current unit.
-	LimitIvl time.Duration
+	Limit time.Duration
 
 	// Enabled tells if the statistics are enabled.
 	Enabled bool
@@ -106,9 +106,9 @@ type StatsCtx struct {
 	// enabled tells if the statistics are enabled.
 	enabled bool
 
-	// limitIvl is the interval for collecting statistics into the current
+	// limit is an upper limit for collecting statistics into the
-	// unit.
+	// current unit.
-	limitIvl time.Duration
+	limit time.Duration
 
 	// ignored is the list of host names, which should not be counted.
 	ignored *stringutil.Set
@@ -128,15 +128,15 @@ func New(conf Config) (s *StatsCtx, err error) {
 		ignored:        conf.Ignored,
 	}
 
-	if conf.LimitIvl < time.Hour {
+	if conf.Limit < time.Hour {
 		return nil, errors.Error("interval: less than an hour")
 	}
 
-	if conf.LimitIvl > timeutil.Day*365 {
+	if conf.Limit > timeutil.Day*365 {
 		return nil, errors.Error("interval: more than a year")
 	}
 
-	s.limitIvl = conf.LimitIvl
+	s.limit = conf.Limit
 
 	if s.unitIDGen = newUnitID; conf.UnitID != nil {
 		s.unitIDGen = conf.UnitID
@@ -157,7 +157,7 @@ func New(conf Config) (s *StatsCtx, err error) {
 		return nil, fmt.Errorf("stats: opening a transaction: %w", err)
 	}
 
-	deleted := deleteOldUnits(tx, id-uint32(s.limitIvl.Hours())-1)
+	deleted := deleteOldUnits(tx, id-uint32(s.limit.Hours())-1)
 	udb = loadUnitFromDB(tx, id)
 
 	err = finishTxn(tx, deleted > 0)
@@ -238,7 +238,7 @@ func (s *StatsCtx) Update(e Entry) {
 	s.lock.Lock()
 	defer s.lock.Unlock()
 
-	if !s.enabled || s.limitIvl == 0 {
+	if !s.enabled || s.limit == 0 {
 		return
 	}
 
@@ -270,7 +270,7 @@ func (s *StatsCtx) WriteDiskConfig(dc *Config) {
 	s.lock.Lock()
 	defer s.lock.Unlock()
 
-	dc.LimitIvl = s.limitIvl
+	dc.Limit = s.limit
 	dc.Enabled = s.enabled
 	dc.Ignored = s.ignored
 }
@@ -280,7 +280,7 @@ func (s *StatsCtx) TopClientsIP(maxCount uint) (ips []netip.Addr) {
 	s.lock.Lock()
 	defer s.lock.Unlock()
 
-	limit := uint32(s.limitIvl.Hours())
+	limit := uint32(s.limit.Hours())
 	if !s.enabled || limit == 0 {
 		return nil
 	}
@@ -384,7 +384,7 @@ func (s *StatsCtx) flush() (cont bool, sleepFor time.Duration) {
 		return false, 0
 	}
 
-	limit := uint32(s.limitIvl.Hours())
+	limit := uint32(s.limit.Hours())
 	if limit == 0 || ptr.id == id {
 		return true, time.Second
 	}
@@ -450,7 +450,7 @@ func (s *StatsCtx) periodicFlush() {
 func (s *StatsCtx) setLimitLocked(limitDays int) {
 	if limitDays != 0 {
 		s.enabled = true
-		s.limitIvl = time.Duration(int(timeutil.Day) * limitDays)
+		s.limit = time.Duration(int(timeutil.Day) * limitDays)
 		log.Debug("stats: set limit: %d days", limitDays)
 
 		return

internal/stats/stats_internal_test.go

@@ -9,6 +9,7 @@ import (
 	"time"
 
 	"github.com/AdguardTeam/golibs/testutil"
+	"github.com/AdguardTeam/golibs/timeutil"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -37,7 +38,7 @@ func TestStats_races(t *testing.T) {
 	conf := Config{
 		UnitID:   idGen,
 		Filename: filepath.Join(t.TempDir(), "./stats.db"),
-		LimitIvl: time.Hour * 24,
+		Limit:    timeutil.Day,
 	}
 
 	s, err := New(conf)

internal/stats/stats_test.go

@@ -9,11 +9,11 @@ import (
 	"path/filepath"
 	"sync/atomic"
 	"testing"
-	"time"
 
 	"github.com/AdguardTeam/AdGuardHome/internal/stats"
 	"github.com/AdguardTeam/golibs/netutil"
 	"github.com/AdguardTeam/golibs/testutil"
+	"github.com/AdguardTeam/golibs/timeutil"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -53,7 +53,7 @@ func TestStats(t *testing.T) {
 	handlers := map[string]http.Handler{}
 	conf := stats.Config{
 		Filename: filepath.Join(t.TempDir(), "stats.db"),
-		LimitIvl: time.Hour * 24,
+		Limit:    timeutil.Day,
 		Enabled:  true,
 		UnitID:   constUnitID,
 		HTTPRegister: func(_, url string, handler http.HandlerFunc) {
@@ -159,7 +159,7 @@ func TestLargeNumbers(t *testing.T) {
 
 	conf := stats.Config{
 		Filename:     filepath.Join(t.TempDir(), "stats.db"),
-		LimitIvl:     time.Hour * 24,
+		Limit:        timeutil.Day,
 		Enabled:      true,
 		UnitID:       func() (id uint32) { return atomic.LoadUint32(&curHour) },
 		HTTPRegister: func(_, url string, handler http.HandlerFunc) { handlers[url] = handler },