From 997993a375015cb7b9a87541da4db3141a1d03f7 Mon Sep 17 00:00:00 2001 From: xugangqiang Date: Tue, 10 Nov 2026 19:07:53 +0800 Subject: [PATCH] refactor(ingestion): collapse duplicated extractor-param normalization and drop redundant deep-copy helper (#22204) --- .../ingestion/pipeline/component_params.go | 11 +- .../normalize_extractor_params_test.go | 168 ++++++++++++++++++ .../ingestion/pipeline/pipeline_params.go | 52 ++---- 3 files changed, 179 insertions(+), 52 deletions(-) create mode 100644 internal/ingestion/pipeline/normalize_extractor_params_test.go diff --git a/internal/ingestion/pipeline/component_params.go b/internal/ingestion/pipeline/component_params.go index f566ee4983..9b0767e31e 100644 --- a/internal/ingestion/pipeline/component_params.go +++ b/internal/ingestion/pipeline/component_params.go @@ -98,15 +98,6 @@ func deepCopyValue(v any) any { } } -// deepCopyMapStr deep-copies a map[string]any so the caller can safely mutate the result. -func deepCopyMapStr(src map[string]any) map[string]any { - cp := make(map[string]any, len(src)) - for k, v := range src { - cp[k] = deepCopyValue(v) - } - return cp -} - // ComponentParamsDefaults extracts component params from DSL JSON and returns // them in the component_params storage format {cpnID: {param: value}}. // Values are deep-copied from the DSL so callers can safely mutate the result. @@ -117,7 +108,7 @@ func ComponentParamsDefaults(dslJSON []byte) (map[string]map[string]any, error) } out := make(map[string]map[string]any, len(schemas)) for _, s := range schemas { - out[s.CpnID] = deepCopyMapStr(s.ParamsDefaults) + out[s.CpnID] = deepCopyValue(s.ParamsDefaults).(map[string]any) } return out, nil } diff --git a/internal/ingestion/pipeline/normalize_extractor_params_test.go b/internal/ingestion/pipeline/normalize_extractor_params_test.go new file mode 100644 index 0000000000..45a8521bf8 --- /dev/null +++ b/internal/ingestion/pipeline/normalize_extractor_params_test.go @@ -0,0 +1,168 @@ +package pipeline + +import ( + "encoding/json" + "reflect" + "testing" +) + +// TestNormalizeExtractorParams_NilReturnsNonNilEmpty locks that a nil input +// yields a usable (non-nil) empty map, not nil — callers range/assign into it. +func TestNormalizeExtractorParams_NilReturnsNonNilEmpty(t *testing.T) { + out := NormalizeExtractorParams(nil) + if out == nil { + t.Fatal("nil input must yield a non-nil empty map") + } + if len(out) != 0 { + t.Errorf("expected empty map, got %#v", out) + } +} + +// TestNormalizeExtractorParams_KnownKeyMapIsDistinctCopy locks that each of the +// five modular keys (keywords/questions/tags/summary/metadata), when its value is +// a map, is replaced by a fresh copy so callers can mutate it without touching +// the caller's request map. +func TestNormalizeExtractorParams_KnownKeyMapIsDistinctCopy(t *testing.T) { + for _, key := range []string{"keywords", "questions", "tags", "summary", "metadata"} { + t.Run(key, func(t *testing.T) { + sub := map[string]any{"top_n": 5} + raw := map[string]any{key: sub} + out := NormalizeExtractorParams(raw) + got, ok := out[key].(map[string]any) + if !ok { + t.Fatalf("%s: expected map, got %T", key, out[key]) + } + if mapDataPtr(out[key]) == mapDataPtr(raw[key]) { + t.Errorf("%s: normalized map must be a distinct copy, not the input map", key) + } + // Mutating the normalized copy must not leak into the input. + got["top_n"] = 999 + if raw[key].(map[string]any)["top_n"] != 5 { + t.Errorf("%s: mutation of normalized copy leaked into the input map", key) + } + }) + } +} + +// TestNormalizeExtractorParams_UnknownMapKeyIsShared locks the precise contract +// that ONLY the five modular keys get copied. Any other map-typed value (e.g. a +// legacy/nested key) must stay shared by reference. This is the adversarial guard +// against a refactor that loosens the loop to "copy every map value" — that +// would be a behavior change and must fail. +func TestNormalizeExtractorParams_UnknownMapKeyIsShared(t *testing.T) { + custom := map[string]any{"a": 1} + raw := map[string]any{"custom_map": custom} + out := NormalizeExtractorParams(raw) + if mapDataPtr(out["custom_map"]) != mapDataPtr(raw["custom_map"]) { + t.Errorf("non-modular map key must remain shared by reference, got %#v", out["custom_map"]) + } +} + +// mapDataPtr returns the underlying data pointer for a map (or 0 for nil / +// non-map). It lets tests assert map identity without the `==` operator, which +// panics on uncomparable map values. +func mapDataPtr(v any) uintptr { + if v == nil { + return 0 + } + return reflect.ValueOf(v).Pointer() +} + +// TestNormalizeExtractorParams_TopLevelScalarShared locks that non-map top-level +// values are carried over by reference (the function only clones the five +// modular sub-maps, never the scalars). +func TestNormalizeExtractorParams_TopLevelScalarShared(t *testing.T) { + raw := map[string]any{"llm_id": "llm-a", "temperature": 0.7} + out := NormalizeExtractorParams(raw) + if out["llm_id"] != "llm-a" { + t.Errorf("llm_id = %v, want llm-a", out["llm_id"]) + } + if out["temperature"] != 0.7 { + t.Errorf("temperature = %v, want 0.7", out["temperature"]) + } +} + +// TestNormalizeExtractorParams_OneLevelCopyOfKnownMap locks that the copy of a +// modular sub-map is one level deep: the sub-map's own entries are new keys, but +// nested values (slices/maps) are shared by reference. This is the exact +// behavior the refactor must preserve. +func TestNormalizeExtractorParams_OneLevelCopyOfKnownMap(t *testing.T) { + inner := []map[string]any{{"key": "cat", "type": "string"}} + sub := map[string]any{"metadata": inner} + raw := map[string]any{"metadata": sub} + out := NormalizeExtractorParams(raw) + got, ok := out["metadata"].(map[string]any) + if !ok { + t.Fatalf("expected metadata map, got %T", out["metadata"]) + } + gotInner, ok := got["metadata"].([]map[string]any) + if !ok { + t.Fatalf("expected metadata slice, got %T", got["metadata"]) + } + if &gotInner[0] != &inner[0] { + t.Error("nested slice element inside a modular map must be shared by reference (one-level copy)") + } +} + +// TestComponentParamsDefaults_DeepCopyIsRecursive locks that the per-component +// params returned by ComponentParamsDefaults are recursively deep-copied (not +// shallow). This guards the S5 cleanup: deepCopyMapStr is removed in favor of +// deepCopyValue, which must keep the recursive deep-copy semantics so a mutation +// of a nested value in the result cannot leak into a subsequent call's result. +func TestComponentParamsDefaults_DeepCopyIsRecursive(t *testing.T) { + dsl := map[string]any{ + "components": map[string]any{ + "Extractor:X": map[string]any{ + "obj": map[string]any{ + "component_name": "Extractor", + "params": map[string]any{ + "metadata": map[string]any{ + "metadata": []map[string]any{{"key": "cat", "type": "string"}}, + }, + }, + }, + }, + }, + } + dslJSON, err := json.Marshal(dsl) + if err != nil { + t.Fatalf("marshal dsl: %v", err) + } + result, err := ComponentParamsDefaults(dslJSON) + if err != nil { + t.Fatalf("ComponentParamsDefaults: %v", err) + } + comp := result["Extractor:X"] + meta, ok := comp["metadata"].(map[string]any) + if !ok { + t.Fatalf("expected metadata map, got %T", comp["metadata"]) + } + list, ok := meta["metadata"].([]any) + if !ok { + t.Fatalf("expected metadata slice, got %T", meta["metadata"]) + } + entry, ok := list[0].(map[string]any) + if !ok { + t.Fatalf("expected metadata entry map, got %T", list[0]) + } + // Mutate the nested map inside the returned copy. + entry["key"] = "changed" + + // A fresh call must not see the mutation — proof of recursive deep copy. + result2, err := ComponentParamsDefaults(dslJSON) + if err != nil { + t.Fatalf("ComponentParamsDefaults (2nd): %v", err) + } + meta2 := result2["Extractor:X"]["metadata"].(map[string]any) + list2, ok := meta2["metadata"].([]any) + if !ok { + t.Fatalf("expected metadata slice (2nd), got %T", meta2["metadata"]) + } + entry2, ok := list2[0].(map[string]any) + if !ok { + t.Fatalf("expected metadata entry map (2nd), got %T", list2[0]) + } + if entry2["key"] != "cat" { + t.Errorf("recursive deep copy broken: nested map leaked mutation, got key=%q", entry2["key"]) + } +} diff --git a/internal/ingestion/pipeline/pipeline_params.go b/internal/ingestion/pipeline/pipeline_params.go index 355ffb0a52..454799348f 100644 --- a/internal/ingestion/pipeline/pipeline_params.go +++ b/internal/ingestion/pipeline/pipeline_params.go @@ -305,49 +305,17 @@ func NormalizeExtractorParams(raw map[string]any) map[string]any { out[k] = v } - // 1. Keywords - if kwRaw, ok := out["keywords"].(map[string]any); ok { - kw := make(map[string]any, len(kwRaw)) - for k, v := range kwRaw { - kw[k] = v + // The five modular extractor sub-objects are cloned one level deep so the + // caller can mutate them without aliasing the request map. Any other value + // (scalars, unknown nested maps) is carried over by reference. + for _, key := range []string{"keywords", "questions", "tags", "summary", "metadata"} { + if sub, ok := out[key].(map[string]any); ok { + cp := make(map[string]any, len(sub)) + for k, v := range sub { + cp[k] = v + } + out[key] = cp } - out["keywords"] = kw - } - - // 2. Questions - if qRaw, ok := out["questions"].(map[string]any); ok { - q := make(map[string]any, len(qRaw)) - for k, v := range qRaw { - q[k] = v - } - out["questions"] = q - } - - // 3. Tags - if tagRaw, ok := out["tags"].(map[string]any); ok { - tag := make(map[string]any, len(tagRaw)) - for k, v := range tagRaw { - tag[k] = v - } - out["tags"] = tag - } - - // 4. Summary - if sumRaw, ok := out["summary"].(map[string]any); ok { - sum := make(map[string]any, len(sumRaw)) - for k, v := range sumRaw { - sum[k] = v - } - out["summary"] = sum - } - - // 5. Metadata - if metaRaw, ok := out["metadata"].(map[string]any); ok { - meta := make(map[string]any, len(metaRaw)) - for k, v := range metaRaw { - meta[k] = v - } - out["metadata"] = meta } return out