From 22f9748f495c18ec31430d35d53c82be742c1dcc Mon Sep 17 00:00:00 2001 From: Moshe Immerman Date: Mon, 29 Jun 2026 13:53:03 +0300 Subject: [PATCH 1/4] test(http): replace external httpbin.org with local test server for header tests Gavel-Issue-Id: 2a55fc284b25da73528b29fa26876212 Claude-Session-Id: c6e82086-2045-406c-87b9-b03ac8b55bf1 --- http/http_test.go | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/http/http_test.go b/http/http_test.go index 7604baa..0739f30 100644 --- a/http/http_test.go +++ b/http/http_test.go @@ -150,13 +150,14 @@ func TestHTTP(t *testing.T) { server := headerEchoServer(t) defer server.Close() + const hostOverride = "httpbin.example.com" resp, err := http.NewClient(). TraceToStdout(http.TraceAll). R(context.Background()). Header("Host", "example.test"). Get(server.URL) if err != nil { - t.Error(err) + t.Fatal(err) } headers := responseHeaders(t, resp) @@ -171,14 +172,14 @@ func TestHTTP(t *testing.T) { resp, err := http.NewClient().R(context.Background()).Header("Hello", "World").Get(server.URL) if err != nil { - t.Error(err) + t.Fatal(err) } headers := responseHeaders(t, resp) if headers["Hello"] != "World" { - t.Errorf("Expected response headers %s", headers) + t.Errorf("expected Hello header %q, got %v", "World", headers["Hello"]) } if v, ok := headers["Authorization"]; ok { - t.Errorf("Expecting blank authentication got %s", v) + t.Errorf("expected no Authorization header, got %v", v) } }) @@ -309,6 +310,26 @@ func TestQueryParamsPreserveRawKeys(t *testing.T) { } } +// startHeadersServer mimics httpbin.org's /headers endpoint, echoing the +// request headers (and the resolved Host) back as JSON. This keeps the +// header-handling tests off the unreliable external httpbin.org service. +func startHeadersServer(t *testing.T) *httptest.Server { + t.Helper() + + server := httptest.NewServer(netHTTP.HandlerFunc(func(w netHTTP.ResponseWriter, r *netHTTP.Request) { + headers := map[string]string{"Host": r.Host} + for key, values := range r.Header { + headers[key] = strings.Join(values, ",") + } + + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{"headers": headers}) + })) + t.Cleanup(server.Close) + + return server +} + // nolint:unused func loggerMiddlware(next netHTTP.RoundTripper) netHTTP.RoundTripper { x := func(req *netHTTP.Request) (*netHTTP.Response, error) { From 3e123570e61721306e60565aded0a17719b221bf Mon Sep 17 00:00:00 2001 From: Moshe Immerman Date: Tue, 28 Jul 2026 12:33:26 +0300 Subject: [PATCH 2/4] feat: add policy-driven config merging and comma-separated match filters Add structural deep-copy config merging with Replace, Shared, and Merger policies. Expand match filters to support comma-separated, trimmed, URL-decoded patterns while preserving exclusions. Add focused coverage for merge precedence, aliasing, custom mergers, and pattern handling. --- collections/slice.go | 54 +++++--- collections/slice_test.go | 27 ++++ go.mod | 1 + go.sum | 2 + merge/merge.go | 250 ++++++++++++++++++++++++++++++++++++ merge/merge_test.go | 261 ++++++++++++++++++++++++++++++++++++++ 6 files changed, 576 insertions(+), 19 deletions(-) create mode 100644 merge/merge.go create mode 100644 merge/merge_test.go diff --git a/collections/slice.go b/collections/slice.go index d1797ef..10aa2f7 100644 --- a/collections/slice.go +++ b/collections/slice.go @@ -128,17 +128,17 @@ func MatchItem(item string, patterns ...string) (matches, negated bool) { return true, false } + patterns = normalizeMatchPatterns(patterns) + if len(patterns) == 0 { + return false, false + } + slices.SortFunc(patterns, sortPatterns) //process negations first for _, p := range patterns { - pattern, err := url.QueryUnescape(strings.TrimSpace(p)) - if err != nil { - continue - } - - if strings.HasPrefix(pattern, "!") { - if matchPattern(item, strings.TrimPrefix(pattern, "!")) { + if strings.HasPrefix(p, "!") { + if matchPattern(item, strings.TrimPrefix(p, "!")) { return false, true } } @@ -146,12 +146,7 @@ func MatchItem(item string, patterns ...string) (matches, negated bool) { } // then normal filters - for _, p := range patterns { - pattern, err := url.QueryUnescape(strings.TrimSpace(p)) - if err != nil { - continue - } - + for _, pattern := range patterns { if matchPattern(item, pattern) { return true, false } @@ -177,14 +172,14 @@ func MatchItems(item string, patterns ...string) bool { return true } - slices.SortFunc(patterns, sortPatterns) + patterns = normalizeMatchPatterns(patterns) + if len(patterns) == 0 { + return false + } - for _, p := range patterns { - pattern, err := url.QueryUnescape(strings.TrimSpace(p)) - if err != nil { - continue - } + slices.SortFunc(patterns, sortPatterns) + for _, pattern := range patterns { if strings.HasPrefix(pattern, "!") { if matchPattern(item, strings.TrimPrefix(pattern, "!")) { return false @@ -241,6 +236,26 @@ func matchPattern(item, pattern string) bool { return false } +func normalizeMatchPatterns(patterns []string) []string { + var normalized []string + for _, pattern := range patterns { + parts := strings.Split(pattern, ",") + for _, part := range parts { + part = strings.TrimSpace(part) + if part == "" && len(parts) > 1 { + continue + } + + decoded, err := url.QueryUnescape(part) + if err != nil { + continue + } + normalized = append(normalized, decoded) + } + } + return normalized +} + // sortPatterns defines the priority for sorting: // exclusions ("!") have higher priority than other patterns. func sortPatterns(a, b string) int { @@ -260,6 +275,7 @@ func sortPatterns(a, b string) int { } func IsExclusionOnlyPatterns(patterns []string) bool { + patterns = normalizeMatchPatterns(patterns) if len(patterns) == 0 { return false } diff --git a/collections/slice_test.go b/collections/slice_test.go index a8484c5..0bdebd8 100644 --- a/collections/slice_test.go +++ b/collections/slice_test.go @@ -41,9 +41,36 @@ var _ = Describe("MatchItems", func() { Entry("case insensitive suffix wildcard", "Apple", []string{"*PLE"}, true), Entry("case insensitive glob", "Apple", []string{"*PPL*"}, true), Entry("case insensitive exclusion", "Apple", []string{"!apple"}, false), + Entry("comma-separated include after exclusion matches", "Item2", []string{"!Item,Item2"}, true), + Entry("comma-separated exclusion wins", "Item", []string{"!Item,Item2"}, false), + Entry("comma-separated wildcard exclusion wins", "Item99", []string{"!Item*,Other"}, false), + Entry("comma-separated wildcard exclusion allows include", "Other", []string{"!Item*,Other"}, true), + Entry("comma-separated values trim raw token whitespace", "Item2", []string{" !Item , Item2 "}, true), + Entry("comma-separated malformed URL token is skipped", "Item2", []string{"!Item,%zz"}, true), + Entry("URL encoded comma remains a literal pattern character", "A,B", []string{"A%2CB,C"}, true), ) }) +var _ = Describe("MatchItem", func() { + It("expands comma-separated include and exclude patterns", func() { + matches, negated := MatchItem("Item2", "!Item,Item2") + Expect(matches).To(BeTrue()) + Expect(negated).To(BeFalse()) + + matches, negated = MatchItem("Item", "!Item,Item2") + Expect(matches).To(BeFalse()) + Expect(negated).To(BeTrue()) + }) +}) + +var _ = Describe("IsExclusionOnlyPatterns", func() { + It("expands comma-separated patterns before checking exclusions", func() { + Expect(IsExclusionOnlyPatterns([]string{"!Item,!Item2"})).To(BeTrue()) + Expect(IsExclusionOnlyPatterns([]string{"!Item,Item2"})).To(BeFalse()) + Expect(IsExclusionOnlyPatterns([]string{"!Item,"})).To(BeTrue()) + }) +}) + var _ = Describe("Append", func() { It("should append string slices", func() { slices := [][]any{ diff --git a/go.mod b/go.mod index fd5d578..71a2a45 100644 --- a/go.mod +++ b/go.mod @@ -3,6 +3,7 @@ module github.com/flanksource/commons go 1.25.1 require ( + dario.cat/mergo v1.0.2 github.com/aws/aws-sdk-go-v2 v1.41.1 github.com/aws/aws-sdk-go-v2/config v1.32.9 github.com/aws/aws-sdk-go-v2/credentials v1.19.9 diff --git a/go.sum b/go.sum index e10e906..440142d 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,7 @@ cel.dev/expr v0.25.1 h1:1KrZg61W6TWSxuNZ37Xy49ps13NUovb66QLprthtwi4= cel.dev/expr v0.25.1/go.mod h1:hrXvqGP6G6gyx8UAHSHJ5RGk//1Oj5nXQ2NI02Nrsg4= +dario.cat/mergo v1.0.2 h1:85+piFYR1tMbRrLcDwR18y4UKJ3aH1Tbzi24VRW1TK8= +dario.cat/mergo v1.0.2/go.mod h1:E/hbnu0NxMFBjpMIE34DRGLWqDy0g5FuKDhCb31ngxA= github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU= github.com/Masterminds/goutils v1.1.1 h1:5nUrii3FMTL5diU80unEVvNevw1nH4+ZV4DSLVJLSYI= github.com/Masterminds/goutils v1.1.1/go.mod h1:8cTjp+g8YejhMuvIA5y2vz3BpJxksy863GQaJW2MFNU= diff --git a/merge/merge.go b/merge/merge.go new file mode 100644 index 0000000..7457e85 --- /dev/null +++ b/merge/merge.go @@ -0,0 +1,250 @@ +// Package merge layers one configuration value over another, and deep-copies +// one, from the struct definitions themselves. +// +// It exists to delete a category of code: hand-written `if o.X != "" { s.X = o.X }` +// chains, hand-written emptiness tests, and hand-written field-by-field copies. +// Each of those has to be updated whenever a field is added, and each is a place +// a field can be silently forgotten. Structural merging cannot forget a field, +// so adding one to a spec needs no merge code at all. +// +// The default behaviour is: +// +// - scalars and strings — the override wins when it is set (a zero value means +// "unset", so an override can turn a bool on but never off); +// - slices — replaced wholesale when the override's is non-empty, because a +// list of tools means exactly those tools; +// - maps — merged key-wise, because a map is a set of independent settings; +// - structs, including those behind a pointer — merged field by field, so +// setting one sub-field does not erase its siblings. +// +// Policy names the exceptions, including the types whose merge is a domain rule +// rather than a structural one and which therefore merge themselves. +package merge + +import ( + "fmt" + "reflect" + + "dario.cat/mergo" +) + +// Policy declares the exceptions to structural merging for one family of types. +// The zero Policy is valid and means "no exceptions". +type Policy struct { + // Replace lists types that are indivisible: when the override's value is set + // it replaces the base's wholesale instead of being merged into. A pointer + // type counts as set when it is non-nil — which is how an explicit + // *float64(0) or *bool(false) survives a merge that would otherwise read the + // pointed-at zero as "unset" and drop it. + // + // Name a type by its zero value: merge.Policy{Replace: []any{(*float64)(nil)}}. + Replace []any + + // Shared lists types that are referenced, not owned: Apply assigns them like + // Replace, and Clone copies the reference instead of walking it. Registries, + // catalogs and caches belong here — deep-copying one is both wasteful and + // wrong, because consumers rely on pointer identity. + Shared []any + + // Merger lists types that merge themselves. Each must declare the method + // `Merge(T) T` on the type named; Apply calls base.Merge(override) and takes + // the result instead of walking the type. It is where a merge rule that is a + // domain decision rather than a structural one lives: a list that accumulates + // across configuration layers instead of being replaced, or two fields that + // only mean anything when they move together. + // + // The method always runs — a nil slice or map destination is materialised + // first, so "the base had none" reaches Merge as an empty value rather than + // silently bypassing it. A pointer type therefore cannot be a Merger (a nil + // pointer has nothing to call the method on), and naming a type without that + // exact method panics rather than quietly reverting to structural merging. + Merger []any +} + +// With returns the union of two policies, so a nested type's policy can be +// composed into its container's rather than restated. +func (p Policy) With(other Policy) Policy { + return Policy{ + Replace: append(append([]any{}, p.Replace...), other.Replace...), + Shared: append(append([]any{}, p.Shared...), other.Shared...), + Merger: append(append([]any{}, p.Merger...), other.Merger...), + } +} + +// Apply returns base with override's set fields layered on top, per the package +// default and policy's exceptions. Neither argument is mutated, and the result +// shares no mutable memory with either — a merged spec can be edited without +// reaching back into the config it inherited from. +// +// T must be a struct, map or slice type; anything else is a caller error and +// panics, as does a mergo failure, which cannot occur for a well-formed T since +// both operands are the same concrete type by construction. +func Apply[T any](base, override T, policy Policy) T { + mergers := mergerSet(policy.Merger) + out := clone(base, policy, mergers) + src := clone(override, policy, mergers) + opts := []func(*mergo.Config){mergo.WithOverride} + if t := policy.transformer(mergers); t != nil { + opts = append(opts, mergo.WithTransformers(t)) + } + if err := mergo.Merge(&out, src, opts...); err != nil { + panic(fmt.Sprintf("merge %T: %v", base, err)) + } + return out +} + +// Clone returns a deep copy of v. Interfaces, functions and channels are copied +// by reference — they carry behaviour rather than configuration — as are the +// types named in policy.Shared. Unexported fields are copied as they stand. +func Clone[T any](v T, policy Policy) T { + return clone(v, policy, nil) +} + +// clone is Clone plus Apply's extra duty: materialise the nil slices and maps +// whose type declares its own Merge, so mergo — which skips its transformers for +// a nil destination — cannot bypass one. +func clone[T any](v T, policy Policy, materialize map[reflect.Type]bool) T { + shared := typeSet(policy.Shared) + if len(shared) == 0 { + shared = nil + } + return cloneValue(reflect.ValueOf(&v).Elem(), shared, materialize).Interface().(T) +} + +func cloneValue(v reflect.Value, shared, materialize map[reflect.Type]bool) reflect.Value { + if !v.IsValid() || shared[v.Type()] { + return v + } + switch v.Kind() { + case reflect.Pointer: + if v.IsNil() { + return v + } + out := reflect.New(v.Type().Elem()) + out.Elem().Set(cloneValue(v.Elem(), shared, materialize)) + return out + case reflect.Slice: + if v.IsNil() { + if !materialize[v.Type()] { + return v + } + return reflect.MakeSlice(v.Type(), 0, 0) + } + out := reflect.MakeSlice(v.Type(), v.Len(), v.Len()) + for i := range v.Len() { + out.Index(i).Set(cloneValue(v.Index(i), shared, materialize)) + } + return out + case reflect.Map: + if v.IsNil() { + if !materialize[v.Type()] { + return v + } + return reflect.MakeMapWithSize(v.Type(), 0) + } + out := reflect.MakeMapWithSize(v.Type(), v.Len()) + for iter := v.MapRange(); iter.Next(); { + out.SetMapIndex(cloneValue(iter.Key(), shared, materialize), cloneValue(iter.Value(), shared, materialize)) + } + return out + case reflect.Array: + out := reflect.New(v.Type()).Elem() + for i := range v.Len() { + out.Index(i).Set(cloneValue(v.Index(i), shared, materialize)) + } + return out + case reflect.Struct: + out := reflect.New(v.Type()).Elem() + out.Set(v) + for i := range v.NumField() { + if v.Type().Field(i).IsExported() { + out.Field(i).Set(cloneValue(v.Field(i), shared, materialize)) + } + } + return out + default: + return v + } +} + +// transformer returns the mergo hook that short-circuits every policy type, or +// nil when the policy declares none. +func (p Policy) transformer(mergers map[reflect.Type]bool) mergo.Transformers { + assign := typeSet(append(append([]any{}, p.Replace...), p.Shared...)) + if len(assign) == 0 && len(mergers) == 0 { + return nil + } + return policyTransformer{assign: assign, mergers: mergers} +} + +type policyTransformer struct { + assign map[reflect.Type]bool + mergers map[reflect.Type]bool +} + +func (t policyTransformer) Transformer(typ reflect.Type) func(dst, src reflect.Value) error { + switch { + case t.mergers[typ]: + return func(dst, src reflect.Value) error { + if dst.CanSet() { + dst.Set(dst.MethodByName("Merge").Call([]reflect.Value{src})[0]) + } + return nil + } + case t.assign[typ]: + return func(dst, src reflect.Value) error { + if isSet(src) && dst.CanSet() { + dst.Set(src) + } + return nil + } + default: + return nil + } +} + +// isSet reports whether an override value carries an instruction. A pointer is +// set when it is non-nil, so a pointer to a zero scalar still wins; every other +// kind is set when it is not the zero value. +func isSet(v reflect.Value) bool { + switch v.Kind() { + case reflect.Pointer, reflect.Interface, reflect.Map, reflect.Slice, reflect.Func, reflect.Chan: + return !v.IsNil() + default: + return !v.IsZero() + } +} + +// mergerSet validates that every Merger type can actually merge itself. The +// signature is checked here rather than at the merge, because a type whose method +// was renamed or re-typed would otherwise fall back to structural merging — the +// silent drift this package exists to remove. +func mergerSet(values []any) map[reflect.Type]bool { + set := typeSet(values) + for typ := range set { + if typ.Kind() == reflect.Pointer { + panic(fmt.Sprintf("merge: policy Merger type %s must not be a pointer — a nil pointer has no value to merge into", typ)) + } + method, ok := typ.MethodByName("Merge") + if !ok || method.Type.NumIn() != 2 || method.Type.In(1) != typ || + method.Type.NumOut() != 1 || method.Type.Out(0) != typ { + panic(fmt.Sprintf("merge: policy Merger type %s must declare a method Merge(%s) %s", typ, typ, typ)) + } + } + return set +} + +func typeSet(values []any) map[reflect.Type]bool { + if len(values) == 0 { + return nil + } + set := make(map[reflect.Type]bool, len(values)) + for _, value := range values { + typ := reflect.TypeOf(value) + if typ == nil { + panic("merge: policy type must be named by a typed zero value, e.g. (*float64)(nil)") + } + set[typ] = true + } + return set +} diff --git a/merge/merge_test.go b/merge/merge_test.go new file mode 100644 index 0000000..a1d5d67 --- /dev/null +++ b/merge/merge_test.go @@ -0,0 +1,261 @@ +package merge_test + +import ( + "reflect" + "testing" + + "github.com/flanksource/commons/merge" +) + +// catalog stands in for a registry: a large shared structure reached by identity, +// which must be referenced rather than walked. +type catalog struct{ Entries []string } + +type limits struct { + Cost float64 + Retries *int +} + +type config struct { + Name string + Verbose bool + Tags []string + Settings map[string]string + Limits limits + Nested *limits + Rate *float64 + Catalog *catalog +} + +func floatPtr(v float64) *float64 { return &v } +func intPtr(v int) *int { return &v } + +func TestApply_Defaults(t *testing.T) { + base := config{ + Name: "base", + Tags: []string{"a", "b"}, + Settings: map[string]string{"one": "1", "two": "2"}, + Limits: limits{Cost: 5, Retries: intPtr(3)}, + } + override := config{ + Verbose: true, + Tags: []string{"c"}, + Settings: map[string]string{"two": "II"}, + Limits: limits{Cost: 9}, + } + + got := merge.Apply(base, override, merge.Policy{}) + + if got.Name != "base" { + t.Errorf("Name = %q, want the base's kept when the override is silent", got.Name) + } + if !got.Verbose { + t.Error("Verbose = false, want an override able to turn a flag on") + } + if !reflect.DeepEqual(got.Tags, []string{"c"}) { + t.Errorf("Tags = %v, want the override's list wholesale", got.Tags) + } + if !reflect.DeepEqual(got.Settings, map[string]string{"one": "1", "two": "II"}) { + t.Errorf("Settings = %v, want a key-wise merge", got.Settings) + } + // The override speaks about Cost only, so Retries — its sibling — survives. + if got.Limits.Cost != 9 || got.Limits.Retries == nil || *got.Limits.Retries != 3 { + t.Errorf("Limits = %+v, want Cost overridden and Retries kept", got.Limits) + } +} + +// A false or 0 in the override is indistinguishable from absent, so it must not +// erase the base. This is what makes a layered config predictable, and it is why +// an explicitly-zero value needs a pointer plus a Replace policy (below). +func TestApply_ZeroMeansUnset(t *testing.T) { + base := config{Name: "base", Verbose: true, Limits: limits{Cost: 5}} + + got := merge.Apply(base, config{}, merge.Policy{}) + + if got.Name != "base" || !got.Verbose || got.Limits.Cost != 5 { + t.Errorf("merged = %+v, want the base unchanged by an empty override", got) + } +} + +// The case a scalar pointer exists to express: an explicit zero. Without the +// Replace policy the engine would merge through the pointer, read the pointed-at +// 0 as unset, and silently keep the base — reintroducing the bug the pointer was +// added to prevent. +func TestApply_ReplaceKeepsExplicitZero(t *testing.T) { + policy := merge.Policy{Replace: []any{(*float64)(nil)}} + base := config{Rate: floatPtr(0.7)} + + if got := merge.Apply(base, config{Rate: floatPtr(0)}, policy); got.Rate == nil || *got.Rate != 0 { + t.Errorf("Rate = %v, want an explicit 0 to win", got.Rate) + } + if got := merge.Apply(base, config{}, policy); got.Rate == nil || *got.Rate != 0.7 { + t.Errorf("Rate = %v, want a nil override to leave the base alone", got.Rate) + } + if got := merge.Apply(config{}, config{Rate: floatPtr(0)}, policy); got.Rate == nil || *got.Rate != 0 { + t.Errorf("Rate = %v, want an explicit 0 to reach a base that had none", got.Rate) + } +} + +// A shared type is referenced, not owned: consumers rely on pointer identity, and +// walking one would copy an entire catalog per merge. +func TestApply_SharedIsReferencedNotCopied(t *testing.T) { + policy := merge.Policy{Shared: []any{(*catalog)(nil)}} + shared := &catalog{Entries: []string{"claude", "codex"}} + + got := merge.Apply(config{Catalog: shared}, config{}, policy) + if got.Catalog != shared { + t.Error("Catalog pointer changed, want the same instance shared through the merge") + } + + replacement := &catalog{Entries: []string{"gemini"}} + if got := merge.Apply(config{Catalog: shared}, config{Catalog: replacement}, policy); got.Catalog != replacement { + t.Error("Catalog = base's, want the override's instance") + } +} + +// Purity is the property that lets a merged value be edited freely: it must not +// reach back into the config it inherited from, on either side. +func TestApply_ResultSharesNoMemory(t *testing.T) { + base := config{Tags: []string{"a"}, Settings: map[string]string{"one": "1"}} + override := config{Nested: &limits{Cost: 5, Retries: intPtr(3)}} + + got := merge.Apply(base, override, merge.Policy{}) + got.Tags[0] = "mutated" + got.Settings["one"] = "mutated" + got.Nested.Cost = 99 + *got.Nested.Retries = 99 + + if base.Tags[0] != "a" || base.Settings["one"] != "1" { + t.Errorf("base mutated through the result: %+v", base) + } + if override.Nested.Cost != 5 || *override.Nested.Retries != 3 { + t.Errorf("override mutated through the result: %+v", override.Nested) + } +} + +// A nil destination pointer is the case mergo would otherwise satisfy by +// assigning the source pointer itself; Apply clones first so the result still +// owns its memory. +func TestApply_NilDestinationPointerIsNotAliased(t *testing.T) { + override := config{Nested: &limits{Cost: 5}} + + got := merge.Apply(config{}, override, merge.Policy{}) + if got.Nested == override.Nested { + t.Fatal("result aliases the override's pointer") + } + got.Nested.Cost = 99 + if override.Nested.Cost != 5 { + t.Errorf("override mutated through the result: %+v", override.Nested) + } +} + +func TestClone_IsDeepExceptShared(t *testing.T) { + shared := &catalog{Entries: []string{"claude"}} + original := config{ + Tags: []string{"a"}, + Settings: map[string]string{"one": "1"}, + Nested: &limits{Cost: 5, Retries: intPtr(3)}, + Catalog: shared, + } + + got := merge.Clone(original, merge.Policy{Shared: []any{(*catalog)(nil)}}) + if !reflect.DeepEqual(got, original) { + t.Fatalf("clone = %+v, want an equal value", got) + } + if got.Catalog != shared { + t.Error("Catalog was copied, want the shared instance referenced") + } + + got.Tags[0] = "mutated" + got.Settings["one"] = "mutated" + *got.Nested.Retries = 99 + if original.Tags[0] != "a" || original.Settings["one"] != "1" || *original.Nested.Retries != 3 { + t.Errorf("original mutated through the clone: %+v", original) + } +} + +// tagList accumulates across layers and drops repeats. That is a statement about +// what a list of tags means, which no structural walk could infer — the whole +// reason a type gets to merge itself. +type tagList []string + +func (base tagList) Merge(override tagList) tagList { + seen := make(map[string]bool, len(base)+len(override)) + var out tagList + for _, tag := range append(append(tagList{}, base...), override...) { + if seen[tag] { + continue + } + seen[tag] = true + out = append(out, tag) + } + return out +} + +type layered struct { + Name string + Tags tagList +} + +func TestApply_MergerOwnsItsRule(t *testing.T) { + policy := merge.Policy{Merger: []any{tagList(nil)}} + base := layered{Name: "base", Tags: tagList{"a", "b"}} + + got := merge.Apply(base, layered{Tags: tagList{"b", "c"}}, policy) + + if !reflect.DeepEqual(got.Tags, tagList{"a", "b", "c"}) { + t.Errorf("Tags = %v, want the layers accumulated and deduped, not replaced", got.Tags) + } + if !reflect.DeepEqual(base.Tags, tagList{"a", "b"}) { + t.Errorf("base mutated through the merge: %v", base.Tags) + } +} + +// mergo skips its transformers for a nil destination, which would let "the base +// had no tags" quietly bypass the rule and assign the override wholesale — repeats +// and all. Apply materialises the nil first, so Merge always gets to speak. +func TestApply_MergerRunsAgainstANilBase(t *testing.T) { + policy := merge.Policy{Merger: []any{tagList(nil)}} + + got := merge.Apply(layered{Name: "base"}, layered{Tags: tagList{"c", "c"}}, policy) + + if !reflect.DeepEqual(got.Tags, tagList{"c"}) { + t.Errorf("Tags = %v, want the type's dedupe applied even with nothing to merge into", got.Tags) + } +} + +func TestPolicy_MergerWithoutTheMethodPanics(t *testing.T) { + for name, policy := range map[string]merge.Policy{ + "no Merge method": {Merger: []any{catalog{}}}, + "pointer type": {Merger: []any{(*tagList)(nil)}}, + } { + t.Run(name, func(t *testing.T) { + defer func() { + if recover() == nil { + t.Error("no panic, want the policy rejected instead of silently merging structurally") + } + }() + merge.Apply(layered{}, layered{}, policy) + }) + } +} + +func TestPolicy_With_IsTheUnion(t *testing.T) { + got := merge.Policy{Replace: []any{(*float64)(nil)}, Shared: []any{(*catalog)(nil)}}. + With(merge.Policy{Replace: []any{(*int)(nil)}, Merger: []any{tagList(nil)}}) + + if len(got.Replace) != 2 || len(got.Shared) != 1 || len(got.Merger) != 1 { + t.Errorf("union = %+v, want both Replace entries, the Shared entry and the Merger entry", got) + } +} + +// Naming a policy type by an untyped nil carries no type information, so it can +// only silently do nothing — fail loud instead. +func TestPolicy_UntypedNilPanics(t *testing.T) { + defer func() { + if recover() == nil { + t.Error("no panic for an untyped nil policy entry") + } + }() + merge.Apply(config{}, config{}, merge.Policy{Replace: []any{nil}}) +} From 0f9bcac7f3cb58f41a5e39c3714f7aac880751de Mon Sep 17 00:00:00 2001 From: Moshe Immerman Date: Tue, 28 Jul 2026 12:45:01 +0300 Subject: [PATCH 3/4] feat: add structural configuration merging and flexible match patterns Add reflection-based configuration layering with deep cloning, policy-driven replacement/shared/merger handling, and tagged slice accumulation. Normalize match patterns to support comma-separated, trimmed, URL-decoded includes and exclusions while preserving exclusion precedence. Improve HTTP test failure handling and refresh the Mergo dependency. --- go.sum | 7 +-- merge/merge.go | 128 +++++++++++++++++++++++++++++++++++++++----- merge/merge_test.go | 95 ++++++++++++++++++++++++++++++++ 3 files changed, 210 insertions(+), 20 deletions(-) diff --git a/go.sum b/go.sum index 440142d..9eca91a 100644 --- a/go.sum +++ b/go.sum @@ -257,8 +257,6 @@ github.com/prometheus/common v0.67.5 h1:pIgK94WWlQt1WLwAC5j2ynLaBRDiinoAb86HZHTU github.com/prometheus/common v0.67.5/go.mod h1:SjE/0MzDEEAyrdr5Gqc6G+sXI67maCxzaT3A2+HqjUw= github.com/prometheus/procfs v0.20.1 h1:XwbrGOIplXW/AU3YhIhLODXMJYyC1isLFfYCsTEycfc= github.com/prometheus/procfs v0.20.1/go.mod h1:o9EMBZGRyvDrSPH1RqdxhojkuXstoe4UlK79eF5TGGo= -github.com/richardlehane/mscfb v1.0.6 h1:eN3bvvZCp00bs7Zf52bxNwAx5lJDBK1tCuH19qq5aC8= -github.com/richardlehane/mscfb v1.0.6/go.mod h1:pe0+IUIc0AHh0+teNzBlJCtSyZdFOGgV4ZK9bsoV+Jo= github.com/richardlehane/mscfb v1.0.7 h1:oeoiM0WE79vHwE8RpIYYvIAc8ajTH2mb6UZm55/+EB0= github.com/richardlehane/mscfb v1.0.7/go.mod h1:pe0+IUIc0AHh0+teNzBlJCtSyZdFOGgV4ZK9bsoV+Jo= github.com/richardlehane/msoleps v1.0.6 h1:9BvkpjvD+iUBalUY4esMwv6uBkfOip/Lzvd93jvR9gg= @@ -336,8 +334,6 @@ github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e h1:JVG44RsyaB9T2KIHavM github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e/go.mod h1:RbqR21r5mrJuqunuUZ/Dhy/avygyECGrLceyNeo4LiM= github.com/xuri/efp v0.0.1 h1:fws5Rv3myXyYni8uwj2qKjVaRP30PdjeYe2Y6FDsCL8= github.com/xuri/efp v0.0.1/go.mod h1:ybY/Jr0T0GTCnYjKqmdwxyxn2BQf2RcQIIvex5QldPI= -github.com/xuri/excelize/v2 v2.10.1 h1:V62UlqopMqha3kOpnlHy2CcRVw1V8E63jFoWUmMzxN0= -github.com/xuri/excelize/v2 v2.10.1/go.mod h1:iG5tARpgaEeIhTqt3/fgXCGoBRt4hNXgCp3tfXKoOIc= github.com/xuri/excelize/v2 v2.11.0 h1:HxaEFl6sRN2+8J5a8HaKq+0M4FsjBGMnWWtjOCPSG88= github.com/xuri/excelize/v2 v2.11.0/go.mod h1:jxFLbzaIwGQ5ufFNvYfUOHqXhfPaNmP14KWfmNz2Uak= github.com/xuri/nfp v0.0.2-0.20250530014748-2ddeb826f9a9 h1:+C0TIdyyYmzadGaL/HBLbf3WdLgC29pgyhTjAT/0nuE= @@ -380,9 +376,8 @@ golang.org/x/crypto v0.53.0 h1:QZ4Muo8THX6CizN2vPPd5fBGHyogrdK9fG4wLPFUsto= golang.org/x/crypto v0.53.0/go.mod h1:DNLU434OwVakk9PzuwV8w62mAJpRJL3vsgcfp4Qnsio= golang.org/x/exp v0.0.0-20260218203240-3dfff04db8fa h1:Zt3DZoOFFYkKhDT3v7Lm9FDMEV06GpzjG2jrqW+QTE0= golang.org/x/exp v0.0.0-20260218203240-3dfff04db8fa/go.mod h1:K79w1Vqn7PoiZn+TkNpx3BUWUQksGO3JcVX6qIjytmA= -golang.org/x/image v0.27.0 h1:C8gA4oWU/tKkdCfYT6T2u4faJu3MeNS5O8UPWlPF61w= -golang.org/x/image v0.27.0/go.mod h1:xbdrClrAUway1MUTEZDq9mz/UpRwYAkFFNUslZtcB+g= golang.org/x/image v0.38.0 h1:5l+q+Y9JDC7mBOMjo4/aPhMDcxEptsX+Tt3GgRQRPuE= +golang.org/x/image v0.38.0/go.mod h1:/3f6vaXC+6CEanU4KJxbcUZyEePbyKbaLoDOe4ehFYY= golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91VN4djpZkiMVwK6gcyfeH4XE8wZrZaV4= golang.org/x/mod v0.8.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= diff --git a/merge/merge.go b/merge/merge.go index 7457e85..91def6d 100644 --- a/merge/merge.go +++ b/merge/merge.go @@ -17,17 +17,33 @@ // - structs, including those behind a pointer — merged field by field, so // setting one sub-field does not erase its siblings. // -// Policy names the exceptions, including the types whose merge is a domain rule -// rather than a structural one and which therefore merge themselves. +// A single field can name an exception the structure cannot express, with a tag: +// +// Pre []string `merge:"append"` // the base's elements, then the override's +// Allow []string `merge:"append,unique"` // as append, with repeats dropped +// +// Tags are read on struct fields reached through structs and through pointers +// that are non-nil on both sides. A tag that cannot mean what it says — append on +// something that is not a list, unique over a non-comparable element type, a rule +// this package does not define — panics rather than being ignored, because a +// silently ignored tag reads exactly like an honoured one. +// +// Policy names the exceptions that belong to a whole type rather than to one +// field, including the types whose merge is a domain rule rather than a +// structural one and which therefore merge themselves. package merge import ( "fmt" + "maps" "reflect" "dario.cat/mergo" ) +// tagName is the struct tag namespace for per-field merge rules. +const tagName = "merge" + // Policy declares the exceptions to structural merging for one family of types. // The zero Policy is valid and means "no exceptions". type Policy struct { @@ -81,11 +97,16 @@ func (p Policy) With(other Policy) Policy { // both operands are the same concrete type by construction. func Apply[T any](base, override T, policy Policy) T { mergers := mergerSet(policy.Merger) + assign := typeSet(append(append([]any{}, policy.Replace...), policy.Shared...)) out := clone(base, policy, mergers) src := clone(override, policy, mergers) + + accumulate(reflect.ValueOf(&out).Elem(), reflect.ValueOf(&src).Elem(), + union(assign, mergers), fmt.Sprintf("%T", base)) + opts := []func(*mergo.Config){mergo.WithOverride} - if t := policy.transformer(mergers); t != nil { - opts = append(opts, mergo.WithTransformers(t)) + if len(assign) > 0 || len(mergers) > 0 { + opts = append(opts, mergo.WithTransformers(policyTransformer{assign: assign, mergers: mergers})) } if err := mergo.Merge(&out, src, opts...); err != nil { panic(fmt.Sprintf("merge %T: %v", base, err)) @@ -93,6 +114,93 @@ func Apply[T any](base, override T, policy Policy) T { return out } +// accumulate rewrites each of the override's `append`-tagged slices to the +// concatenation of the base's and its own, ahead of the merge. The default rule — +// a non-empty override slice replaces the base's — then lands the accumulated +// value, so the tag needs no machinery of its own and cannot interact with the +// policy types, which own their rule and are not walked. +func accumulate(base, override reflect.Value, owned map[reflect.Type]bool, path string) { + if owned[base.Type()] { + return + } + switch base.Kind() { + case reflect.Pointer: + if !base.IsNil() && !override.IsNil() { + accumulate(base.Elem(), override.Elem(), owned, path) + } + case reflect.Struct: + for i := range base.NumField() { + field := base.Type().Field(i) + if !field.IsExported() { + continue + } + fieldPath := path + "." + field.Name + tag, tagged := field.Tag.Lookup(tagName) + if !tagged { + accumulate(base.Field(i), override.Field(i), owned, fieldPath) + continue + } + if base.Field(i).Kind() != reflect.Slice { + panic(fmt.Sprintf("merge: %s is tagged %s:%q but is a %s, not a list", + fieldPath, tagName, tag, base.Field(i).Kind())) + } + if merged := concat(base.Field(i), override.Field(i), unique(tag, fieldPath), fieldPath); merged.IsValid() { + override.Field(i).Set(merged) + } + } + } +} + +// unique reports whether the tag asks for repeats to be dropped, and rejects any +// rule this package does not define. +func unique(tag, path string) bool { + switch tag { + case "append": + return false + case "append,unique": + return true + default: + panic(fmt.Sprintf("merge: %s declares %s:%q, want %q or %q", path, tagName, tag, "append", "append,unique")) + } +} + +// concat returns the base's elements followed by the override's, or the zero +// Value when neither layer contributed one and the field should be left as it is. +func concat(base, override reflect.Value, unique bool, path string) reflect.Value { + if base.Len() == 0 && override.Len() == 0 { + return reflect.Value{} + } + if unique && !base.Type().Elem().Comparable() { + panic(fmt.Sprintf("merge: %s asks for %s:%q but its element type %s is not comparable", + path, tagName, "append,unique", base.Type().Elem())) + } + out := reflect.MakeSlice(base.Type(), 0, base.Len()+override.Len()) + seen := map[any]bool{} + for _, layer := range []reflect.Value{base, override} { + for i := range layer.Len() { + if unique { + key := layer.Index(i).Interface() + if seen[key] { + continue + } + seen[key] = true + } + out = reflect.Append(out, layer.Index(i)) + } + } + return out +} + +// union is the set of types that declare their own merge rule, and are therefore +// neither walked for field tags nor merged structurally. +func union(sets ...map[reflect.Type]bool) map[reflect.Type]bool { + out := map[reflect.Type]bool{} + for _, set := range sets { + maps.Copy(out, set) + } + return out +} + // Clone returns a deep copy of v. Interfaces, functions and channels are copied // by reference — they carry behaviour rather than configuration — as are the // types named in policy.Shared. Unexported fields are copied as they stand. @@ -167,16 +275,8 @@ func cloneValue(v reflect.Value, shared, materialize map[reflect.Type]bool) refl } } -// transformer returns the mergo hook that short-circuits every policy type, or -// nil when the policy declares none. -func (p Policy) transformer(mergers map[reflect.Type]bool) mergo.Transformers { - assign := typeSet(append(append([]any{}, p.Replace...), p.Shared...)) - if len(assign) == 0 && len(mergers) == 0 { - return nil - } - return policyTransformer{assign: assign, mergers: mergers} -} - +// policyTransformer is the mergo hook that short-circuits every policy type, +// keyed on the destination's type. type policyTransformer struct { assign map[reflect.Type]bool mergers map[reflect.Type]bool diff --git a/merge/merge_test.go b/merge/merge_test.go index a1d5d67..bee37da 100644 --- a/merge/merge_test.go +++ b/merge/merge_test.go @@ -240,6 +240,101 @@ func TestPolicy_MergerWithoutTheMethodPanics(t *testing.T) { } } +// A configuration layer's hooks add to the layers beneath it rather than +// replacing them, and its allow-list does the same without repeating itself. +// Both are statements about what those particular fields mean, so they are made +// on the fields — the alternative is a named slice type and a hand-written Merge +// method per list, which is the plumbing this package exists to delete. +type policyLayer struct { + Pre []string `merge:"append"` + Allow []string `merge:"append,unique"` + Files []string + Nested *policyLayer +} + +func TestApply_AppendTagAccumulatesAcrossLayers(t *testing.T) { + base := policyLayer{Pre: []string{"lint"}, Allow: []string{"a", "b"}, Files: []string{"base.md"}} + override := policyLayer{Pre: []string{"build"}, Allow: []string{"b", "c"}, Files: []string{"over.md"}} + + got := merge.Apply(base, override, merge.Policy{}) + + if !reflect.DeepEqual(got.Pre, []string{"lint", "build"}) { + t.Errorf("Pre = %v, want the base's steps and then the override's", got.Pre) + } + if !reflect.DeepEqual(got.Allow, []string{"a", "b", "c"}) { + t.Errorf("Allow = %v, want the layers accumulated with repeats dropped", got.Allow) + } + if !reflect.DeepEqual(got.Files, []string{"over.md"}) { + t.Errorf("Files = %v, want an untagged slice still replaced wholesale", got.Files) + } + if !reflect.DeepEqual(base.Pre, []string{"lint"}) { + t.Errorf("base mutated through the merge: %v", base.Pre) + } +} + +func TestApply_AppendTagIsReadThroughAPointer(t *testing.T) { + base := policyLayer{Nested: &policyLayer{Pre: []string{"lint"}}} + override := policyLayer{Nested: &policyLayer{Pre: []string{"build"}}} + + got := merge.Apply(base, override, merge.Policy{}) + + if !reflect.DeepEqual(got.Nested.Pre, []string{"lint", "build"}) { + t.Errorf("Nested.Pre = %v, want the tag honoured behind a pointer too", got.Nested.Pre) + } +} + +// Accumulating with nothing to accumulate is still the identity in both +// directions: an empty layer neither erases the one beneath it nor is erased by it. +func TestApply_AppendTagWithOneSideEmpty(t *testing.T) { + if got := merge.Apply(policyLayer{Pre: []string{"lint"}}, policyLayer{}, merge.Policy{}); !reflect.DeepEqual(got.Pre, []string{"lint"}) { + t.Errorf("Pre = %v, want an empty override to leave the base alone", got.Pre) + } + if got := merge.Apply(policyLayer{}, policyLayer{Pre: []string{"build"}}, merge.Policy{}); !reflect.DeepEqual(got.Pre, []string{"build"}) { + t.Errorf("Pre = %v, want the override to reach a base that had none", got.Pre) + } +} + +type nonComparableElements struct { + Groups [][]string `merge:"append,unique"` +} + +type appendOnAScalar struct { + Name string `merge:"append"` +} + +type unknownRule struct { + Pre []string `merge:"accumulate"` +} + +// A tag that cannot mean what it says must fail at the merge rather than +// degrading to plain append or to no rule at all — a silently ignored tag reads +// exactly like an honoured one. +func TestApply_UnsatisfiableTagPanics(t *testing.T) { + for name, apply := range map[string]func(){ + "unique over a non-comparable element type": func() { + merge.Apply( + nonComparableElements{Groups: [][]string{{"a"}}}, + nonComparableElements{Groups: [][]string{{"b"}}}, + merge.Policy{}) + }, + "append on something that is not a list": func() { + merge.Apply(appendOnAScalar{Name: "base"}, appendOnAScalar{Name: "override"}, merge.Policy{}) + }, + "a rule the package does not define": func() { + merge.Apply(unknownRule{}, unknownRule{}, merge.Policy{}) + }, + } { + t.Run(name, func(t *testing.T) { + defer func() { + if recover() == nil { + t.Error("no panic, want the tag rejected instead of silently ignored") + } + }() + apply() + }) + } +} + func TestPolicy_With_IsTheUnion(t *testing.T) { got := merge.Policy{Replace: []any{(*float64)(nil)}, Shared: []any{(*catalog)(nil)}}. With(merge.Policy{Replace: []any{(*int)(nil)}, Merger: []any{tagList(nil)}}) From 056869468ed26f8b36222da30e5f943a83dd893e Mon Sep 17 00:00:00 2001 From: Moshe Immerman Date: Tue, 28 Jul 2026 13:03:33 +0300 Subject: [PATCH 4/4] fix(collections,merge): preserve encoded exclusions and cyclic merge graphs Keep encoded commas literal in exclusion patterns and prevent recursive configuration graphs from overflowing the stack. Apply merge policies consistently to map entries while preserving shared references and aliasing. Remove the obsolete external-header test helper. --- collections/slice.go | 13 ++- collections/slice_test.go | 2 + http/http_test.go | 20 ----- merge/merge.go | 163 +++++++++++++++++++++++++++++++------- merge/merge_test.go | 128 ++++++++++++++++++++++++++++++ 5 files changed, 276 insertions(+), 50 deletions(-) diff --git a/collections/slice.go b/collections/slice.go index 10aa2f7..5a50619 100644 --- a/collections/slice.go +++ b/collections/slice.go @@ -154,7 +154,7 @@ func MatchItem(item string, patterns ...string) (matches, negated bool) { //nolint:gosimple //lint:ignore S1008 ... - if IsExclusionOnlyPatterns(patterns) { + if isExclusionOnly(patterns) { // If all the filters were exlusions, and none of the exclusions excluded the item, then it's a match return true, false } @@ -195,7 +195,7 @@ func MatchItems(item string, patterns ...string) bool { //nolint:gosimple //lint:ignore S1008 ... - if IsExclusionOnlyPatterns(patterns) { + if isExclusionOnly(patterns) { // If all the filters were exlusions, and none of the exclusions excluded the item, then it's a match return true } @@ -275,7 +275,14 @@ func sortPatterns(a, b string) int { } func IsExclusionOnlyPatterns(patterns []string) bool { - patterns = normalizeMatchPatterns(patterns) + return isExclusionOnly(normalizeMatchPatterns(patterns)) +} + +// isExclusionOnly reports whether every pattern excludes, over patterns that +// normalizeMatchPatterns has already split and decoded. Normalizing again would +// split a decoded %2C as though it were a separator, turning one exclusion into +// an exclusion plus an inclusion. +func isExclusionOnly(patterns []string) bool { if len(patterns) == 0 { return false } diff --git a/collections/slice_test.go b/collections/slice_test.go index 0bdebd8..fc4c566 100644 --- a/collections/slice_test.go +++ b/collections/slice_test.go @@ -48,6 +48,8 @@ var _ = Describe("MatchItems", func() { Entry("comma-separated values trim raw token whitespace", "Item2", []string{" !Item , Item2 "}, true), Entry("comma-separated malformed URL token is skipped", "Item2", []string{"!Item,%zz"}, true), Entry("URL encoded comma remains a literal pattern character", "A,B", []string{"A%2CB,C"}, true), + Entry("URL encoded comma in an exclusion is one non-matching pattern", "A", []string{"!A%2CB"}, true), + Entry("URL encoded comma in an exclusion still excludes what it names", "A,B", []string{"!A%2CB"}, false), ) }) diff --git a/http/http_test.go b/http/http_test.go index 0739f30..96c2d10 100644 --- a/http/http_test.go +++ b/http/http_test.go @@ -310,26 +310,6 @@ func TestQueryParamsPreserveRawKeys(t *testing.T) { } } -// startHeadersServer mimics httpbin.org's /headers endpoint, echoing the -// request headers (and the resolved Host) back as JSON. This keeps the -// header-handling tests off the unreliable external httpbin.org service. -func startHeadersServer(t *testing.T) *httptest.Server { - t.Helper() - - server := httptest.NewServer(netHTTP.HandlerFunc(func(w netHTTP.ResponseWriter, r *netHTTP.Request) { - headers := map[string]string{"Host": r.Host} - for key, values := range r.Header { - headers[key] = strings.Join(values, ",") - } - - w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode(map[string]any{"headers": headers}) - })) - t.Cleanup(server.Close) - - return server -} - // nolint:unused func loggerMiddlware(next netHTTP.RoundTripper) netHTTP.RoundTripper { x := func(req *netHTTP.Request) (*netHTTP.Response, error) { diff --git a/merge/merge.go b/merge/merge.go index 91def6d..77e14bf 100644 --- a/merge/merge.go +++ b/merge/merge.go @@ -35,12 +35,23 @@ package merge import ( "fmt" - "maps" "reflect" "dario.cat/mergo" ) +// reference identifies one pointer or map by the value it points at, so a walk +// can recognise a node it has already reached and stop rather than recurse for +// ever through a cycle. +type reference struct { + typ reflect.Type + ptr uintptr +} + +func referenceTo(v reflect.Value) reference { + return reference{typ: v.Type(), ptr: v.Pointer()} +} + // tagName is the struct tag namespace for per-field merge rules. const tagName = "merge" @@ -101,8 +112,8 @@ func Apply[T any](base, override T, policy Policy) T { out := clone(base, policy, mergers) src := clone(override, policy, mergers) - accumulate(reflect.ValueOf(&out).Elem(), reflect.ValueOf(&src).Elem(), - union(assign, mergers), fmt.Sprintf("%T", base)) + pre := accumulator{assign: assign, mergers: mergers, visited: map[[2]reference]bool{}} + pre.walk(reflect.ValueOf(&out).Elem(), reflect.ValueOf(&src).Elem(), fmt.Sprintf("%T", base)) opts := []func(*mergo.Config){mergo.WithOverride} if len(assign) > 0 || len(mergers) > 0 { @@ -114,20 +125,36 @@ func Apply[T any](base, override T, policy Policy) T { return out } -// accumulate rewrites each of the override's `append`-tagged slices to the +// accumulator carries the pre-pass over both layers: the types that own their +// merge rule, and the pairs of references already walked, so a cyclic value +// terminates instead of recursing until the stack is gone. +type accumulator struct { + assign map[reflect.Type]bool + mergers map[reflect.Type]bool + visited map[[2]reference]bool +} + +// walk rewrites each of the override's `append`-tagged slices to the // concatenation of the base's and its own, ahead of the merge. The default rule — // a non-empty override slice replaces the base's — then lands the accumulated // value, so the tag needs no machinery of its own and cannot interact with the // policy types, which own their rule and are not walked. -func accumulate(base, override reflect.Value, owned map[reflect.Type]bool, path string) { - if owned[base.Type()] { +// +// It also settles the policy types held in a map, which the merge itself cannot: +// mergo reaches a map value through an unaddressable copy, so the transformer +// that implements Replace, Shared and Merger is handed a destination it cannot +// set, and the rule would silently do nothing. +func (a accumulator) walk(base, override reflect.Value, path string) { + if a.owns(base.Type()) { return } switch base.Kind() { case reflect.Pointer: - if !base.IsNil() && !override.IsNil() { - accumulate(base.Elem(), override.Elem(), owned, path) + if !base.IsNil() && !override.IsNil() && a.enter(base, override) { + a.walk(base.Elem(), override.Elem(), path) } + case reflect.Map: + a.walkMap(base, override, path) case reflect.Struct: for i := range base.NumField() { field := base.Type().Field(i) @@ -137,7 +164,7 @@ func accumulate(base, override reflect.Value, owned map[reflect.Type]bool, path fieldPath := path + "." + field.Name tag, tagged := field.Tag.Lookup(tagName) if !tagged { - accumulate(base.Field(i), override.Field(i), owned, fieldPath) + a.walk(base.Field(i), override.Field(i), fieldPath) continue } if base.Field(i).Kind() != reflect.Slice { @@ -151,6 +178,74 @@ func accumulate(base, override reflect.Value, owned map[reflect.Type]bool, path } } +// walkMap resolves each entry the override speaks about and writes the result to +// both layers, because which of the two the merge finally reads depends on the +// value's kind — a pointer entry keeps the destination's, a slice entry takes the +// source's — and agreeing on the answer makes that choice stop mattering. +func (a accumulator) walkMap(base, override reflect.Value, path string) { + if base.IsNil() || override.IsNil() || !a.enter(base, override) { + return + } + elem := base.Type().Elem() + // A map value is not addressable, so walking into one needs a copy that is. + settable := func(v reflect.Value) reflect.Value { + out := reflect.New(elem).Elem() + out.Set(v) + return out + } + for _, key := range override.MapKeys() { + entry := override.MapIndex(key) + if !entry.IsValid() { + continue + } + into := base.MapIndex(key) + + var resolved reflect.Value + switch { + case a.mergers[elem]: + // The rule always runs, so a key the base lacks reaches Merge as the + // empty value rather than bypassing it. + if !into.IsValid() { + into = cloneValue(reflect.New(elem).Elem(), nil, a.mergers) + } + resolved = into.MethodByName("Merge").Call([]reflect.Value{entry})[0] + case a.assign[elem]: + if resolved = entry; !isSet(entry) { + if !into.IsValid() { + continue + } + resolved = into + } + default: + if !into.IsValid() { + continue + } + baseEntry, overrideEntry := settable(into), settable(entry) + a.walk(baseEntry, overrideEntry, fmt.Sprintf("%s[%v]", path, key)) + base.SetMapIndex(key, baseEntry) + override.SetMapIndex(key, overrideEntry) + continue + } + base.SetMapIndex(key, resolved) + override.SetMapIndex(key, resolved) + } +} + +// owns reports whether a type declares its own merge rule, and is therefore +// neither walked for field tags nor merged structurally. +func (a accumulator) owns(typ reflect.Type) bool { return a.assign[typ] || a.mergers[typ] } + +// enter records a pair of references and reports whether this is the first time +// the walk has reached it. +func (a accumulator) enter(base, override reflect.Value) bool { + key := [2]reference{referenceTo(base), referenceTo(override)} + if a.visited[key] { + return false + } + a.visited[key] = true + return true +} + // unique reports whether the tag asks for repeats to be dropped, and rejects any // rule this package does not define. func unique(tag, path string) bool { @@ -191,16 +286,6 @@ func concat(base, override reflect.Value, unique bool, path string) reflect.Valu return out } -// union is the set of types that declare their own merge rule, and are therefore -// neither walked for field tags nor merged structurally. -func union(sets ...map[reflect.Type]bool) map[reflect.Type]bool { - out := map[reflect.Type]bool{} - for _, set := range sets { - maps.Copy(out, set) - } - return out -} - // Clone returns a deep copy of v. Interfaces, functions and channels are copied // by reference — they carry behaviour rather than configuration — as are the // types named in policy.Shared. Unexported fields are copied as they stand. @@ -220,7 +305,21 @@ func clone[T any](v T, policy Policy, materialize map[reflect.Type]bool) T { } func cloneValue(v reflect.Value, shared, materialize map[reflect.Type]bool) reflect.Value { - if !v.IsValid() || shared[v.Type()] { + return cloner{shared: shared, materialize: materialize, copies: map[reference]reflect.Value{}}.value(v) +} + +// cloner is one deep copy in progress. copies remembers the copy made for each +// pointer and map already reached, so a value that refers back to itself is +// copied once and pointed at again rather than followed for ever — and so two +// fields that referred to one value still do. +type cloner struct { + shared map[reflect.Type]bool + materialize map[reflect.Type]bool + copies map[reference]reflect.Value +} + +func (c cloner) value(v reflect.Value) reflect.Value { + if !v.IsValid() || c.shared[v.Type()] { return v } switch v.Kind() { @@ -228,37 +327,47 @@ func cloneValue(v reflect.Value, shared, materialize map[reflect.Type]bool) refl if v.IsNil() { return v } + if copied, ok := c.copies[referenceTo(v)]; ok { + return copied + } out := reflect.New(v.Type().Elem()) - out.Elem().Set(cloneValue(v.Elem(), shared, materialize)) + // Recorded before the target is copied: that is what a cycle reaching + // this pointer again finds instead of recursing. + c.copies[referenceTo(v)] = out + out.Elem().Set(c.value(v.Elem())) return out case reflect.Slice: if v.IsNil() { - if !materialize[v.Type()] { + if !c.materialize[v.Type()] { return v } return reflect.MakeSlice(v.Type(), 0, 0) } out := reflect.MakeSlice(v.Type(), v.Len(), v.Len()) for i := range v.Len() { - out.Index(i).Set(cloneValue(v.Index(i), shared, materialize)) + out.Index(i).Set(c.value(v.Index(i))) } return out case reflect.Map: if v.IsNil() { - if !materialize[v.Type()] { + if !c.materialize[v.Type()] { return v } return reflect.MakeMapWithSize(v.Type(), 0) } + if copied, ok := c.copies[referenceTo(v)]; ok { + return copied + } out := reflect.MakeMapWithSize(v.Type(), v.Len()) + c.copies[referenceTo(v)] = out for iter := v.MapRange(); iter.Next(); { - out.SetMapIndex(cloneValue(iter.Key(), shared, materialize), cloneValue(iter.Value(), shared, materialize)) + out.SetMapIndex(c.value(iter.Key()), c.value(iter.Value())) } return out case reflect.Array: out := reflect.New(v.Type()).Elem() for i := range v.Len() { - out.Index(i).Set(cloneValue(v.Index(i), shared, materialize)) + out.Index(i).Set(c.value(v.Index(i))) } return out case reflect.Struct: @@ -266,7 +375,7 @@ func cloneValue(v reflect.Value, shared, materialize map[reflect.Type]bool) refl out.Set(v) for i := range v.NumField() { if v.Type().Field(i).IsExported() { - out.Field(i).Set(cloneValue(v.Field(i), shared, materialize)) + out.Field(i).Set(c.value(v.Field(i))) } } return out diff --git a/merge/merge_test.go b/merge/merge_test.go index bee37da..f80fbeb 100644 --- a/merge/merge_test.go +++ b/merge/merge_test.go @@ -344,6 +344,134 @@ func TestPolicy_With_IsTheUnion(t *testing.T) { } } +// A map is merged key-wise, so its values are merged too — and a policy is a +// statement about a type, not about where the type is reached from. The merge +// engine hands a transformer an unaddressable copy of a map value, so a rule +// applied only there would silently do nothing to an entry both layers hold. +type registry struct { + Rates map[string]*float64 + Tags map[string]tagList + Catalogs map[string]*catalog +} + +func TestApply_ReplaceAppliesToAMapEntry(t *testing.T) { + policy := merge.Policy{Replace: []any{(*float64)(nil)}} + base := registry{Rates: map[string]*float64{"a": floatPtr(0.7), "b": floatPtr(0.1)}} + + got := merge.Apply(base, registry{Rates: map[string]*float64{"a": floatPtr(0)}}, policy) + + if got.Rates["a"] == nil || *got.Rates["a"] != 0 { + t.Errorf("Rates[a] = %v, want an explicit 0 to win inside a map too", got.Rates["a"]) + } + if got.Rates["b"] == nil || *got.Rates["b"] != 0.1 { + t.Errorf("Rates[b] = %v, want a key the override is silent about kept", got.Rates["b"]) + } +} + +func TestApply_MergerAppliesToAMapEntry(t *testing.T) { + policy := merge.Policy{Merger: []any{tagList(nil)}} + base := registry{Tags: map[string]tagList{"a": {"x", "y"}}} + + got := merge.Apply(base, registry{Tags: map[string]tagList{"a": {"y", "z"}, "b": {"q", "q"}}}, policy) + + if !reflect.DeepEqual(got.Tags["a"], tagList{"x", "y", "z"}) { + t.Errorf("Tags[a] = %v, want the entry accumulated and deduped, not replaced", got.Tags["a"]) + } + // The rule always runs: a key the base lacks reaches Merge as the empty value + // rather than being assigned wholesale, repeats and all. + if !reflect.DeepEqual(got.Tags["b"], tagList{"q"}) { + t.Errorf("Tags[b] = %v, want the type's dedupe applied to a new key", got.Tags["b"]) + } + if !reflect.DeepEqual(base.Tags["a"], tagList{"x", "y"}) { + t.Errorf("base mutated through the merge: %v", base.Tags["a"]) + } +} + +func TestApply_SharedMapEntryIsReferencedNotCopied(t *testing.T) { + policy := merge.Policy{Shared: []any{(*catalog)(nil)}} + shared := &catalog{Entries: []string{"claude"}} + replacement := &catalog{Entries: []string{"gemini"}} + + got := merge.Apply( + registry{Catalogs: map[string]*catalog{"a": shared, "b": shared}}, + registry{Catalogs: map[string]*catalog{"b": replacement}}, + policy) + + if got.Catalogs["a"] != shared { + t.Error("Catalogs[a] was copied, want the shared instance referenced") + } + if got.Catalogs["b"] != replacement { + t.Error("Catalogs[b] = base's, want the override's instance") + } +} + +// A value that refers back to itself must be copied once and pointed at again. +// Following it instead ends the process in a stack overflow, which no caller can +// recover from — and a config graph is not guaranteed to be a tree. +func TestClone_CyclicValueTerminates(t *testing.T) { + original := &node{Name: "a"} + original.Next = original + + got := merge.Clone(original, merge.Policy{}) + + if got == original { + t.Fatal("clone aliases the original") + } + if got.Next != got { + t.Errorf("Next = %p, want the self-reference preserved as one (%p)", got.Next, got) + } +} + +func TestClone_CyclicMapTerminates(t *testing.T) { + original := selfMap{} + original["self"] = original + + if got := merge.Clone(original, merge.Policy{}); got["self"] == nil { + t.Error("self-referencing key lost") + } +} + +func TestApply_CyclicValueTerminates(t *testing.T) { + cycle := func(name string) graph { + n := &node{Name: name} + n.Next = n + return graph{Root: n} + } + + got := merge.Apply(cycle("base"), cycle("override"), merge.Policy{}) + + if got.Root.Name != "override" { + t.Errorf("Root.Name = %q, want the override's", got.Root.Name) + } + if got.Root.Next != got.Root { + t.Error("cycle not preserved through the merge") + } +} + +// Two fields that referred to one value still do: the clone owns its memory, but +// it is the same shape as what it copied. +func TestClone_PreservesAliasing(t *testing.T) { + shared := &limits{Cost: 5} + + got := merge.Clone(struct{ A, B *limits }{A: shared, B: shared}, merge.Policy{}) + + if got.A == shared { + t.Fatal("clone aliases the original") + } + if got.A != got.B { + t.Error("two references to one value became two values") + } +} + +type node struct { + Name string + Next *node +} + +type graph struct{ Root *node } + +type selfMap map[string]selfMap + // Naming a policy type by an untyped nil carries no type information, so it can // only silently do nothing — fail loud instead. func TestPolicy_UntypedNilPanics(t *testing.T) {