internal: fix MaxSizeSubjectsListForLog off-by-one when maxToDisplay is 0 (#7970)

* internal: fix MaxSizeSubjectsListForLog off-by-one when maxToDisplay is 0

The loop appended before checking the length, so with maxToDisplay=0
one domain leaked into the output before the break check. Reorder so
the length check comes first.

Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>

* Update internal/logs.go

Co-authored-by: Zen Dodd <mail@steadytao.com>
Signed-off-by: Mohammed Al Sahaf <mohammed@caffeinatedwonders.com>

* fix doc

Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>

---------

Signed-off-by: Mohammed Al Sahaf <msaa1990@gmail.com>
Signed-off-by: Mohammed Al Sahaf <mohammed@caffeinatedwonders.com>
Co-authored-by: Zen Dodd <mail@steadytao.com>

internal/logs.go

@@ -2,18 +2,18 @@ package internal
 
 import "fmt"
 
-// MaxSizeSubjectsListForLog returns the keys in the map as a slice of maximum length
-// maxToDisplay. It is useful for logging domains being managed, for example, since a
-// map is typically needed for quick lookup, but a slice is needed for logging, and this
-// can be quite a doozy since there may be a huge amount (hundreds of thousands).
+// MaxSizeSubjectsListForLog returns up to maxToDisplay keys from subjects.
+// If any keys are omitted, it appends one additional entry summarising the
+// omitted count. This bounds the number of subject names placed in the log
+// while making truncation explicit.
 func MaxSizeSubjectsListForLog(subjects map[string]struct{}, maxToDisplay int) []string {
 	numberOfNamesToDisplay := min(len(subjects), maxToDisplay)
 	domainsToDisplay := make([]string, 0, numberOfNamesToDisplay)
 	for domain := range subjects {
-		domainsToDisplay = append(domainsToDisplay, domain)
 		if len(domainsToDisplay) >= numberOfNamesToDisplay {
 			break
 		}
+		domainsToDisplay = append(domainsToDisplay, domain)
 	}
 	if len(subjects) > maxToDisplay {
 		domainsToDisplay = append(domainsToDisplay, fmt.Sprintf("(and %d more...)", len(subjects)-maxToDisplay))

internal/logs_test.go

@@ -0,0 +1,34 @@
+// Copyright 2015 Matthew Holt and The Caddy Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package internal
+
+import "testing"
+
+// TestMaxSizeSubjectsListForLog_MaxIsZero is the regression test for
+// the off-by-one where a domain leaked into the output when maxToDisplay
+// was 0.
+func TestMaxSizeSubjectsListForLog_MaxIsZero(t *testing.T) {
+	subjects := map[string]struct{}{
+		"a.com": {},
+		"b.com": {},
+	}
+	got := MaxSizeSubjectsListForLog(subjects, 0)
+	if len(got) != 1 {
+		t.Fatalf("expected only the suffix entry, got %d items: %v", len(got), got)
+	}
+	if len(got[0]) < 4 || got[0][:4] != "(and" {
+		t.Errorf("expected the only entry to be the '(and N more...)' suffix, got %q", got[0])
+	}
+}