From 420c0407f98ca108f864946ce9ef327f031ecc00 Mon Sep 17 00:00:00 2001 From: Eugene Date: Sun, 3 May 2026 21:37:44 +0300 Subject: [PATCH] bug: preserve unusual keys, empty containers, big ints, emoji, and aliases across the save round-trip --- domain/flatkey.go | 269 +++++++++++++++- domain/flatkey_test.go | 150 +++++++++ service/comment_parser.go | 4 +- service/fixture_test.go | 2 +- service/round_trip_fixture_test.go | 183 ++++++----- service/values_service.go | 80 ++++- service/yaml_serialize.go | 262 +++++++++++++++- service/yaml_serialize_test.go | 450 ++++++++++++++++++++++++++- service/yaml_tree.go | 212 +++++++++---- ui/page/values_controller.go | 9 +- ui/page/values_controller_actions.go | 10 +- ui/page/values_page_anchors.go | 2 +- ui/state/editor_state.go | 156 +++++++++- ui/state/editor_state_test.go | 348 +++++++++++++++++++++ ui/state/values_state.go | 22 ++ ui/widget/flat_key.go | 33 +- ui/widget/override_table_anchors.go | 3 +- ui/widget/override_table_events.go | 8 +- 18 files changed, 1998 insertions(+), 205 deletions(-) create mode 100644 domain/flatkey_test.go create mode 100644 ui/state/editor_state_test.go diff --git a/domain/flatkey.go b/domain/flatkey.go index 78d8254..581577b 100644 --- a/domain/flatkey.go +++ b/domain/flatkey.go @@ -1,12 +1,198 @@ +// Package domain ... +// +// # FlatKey encoding +// +// A FlatKey is a dot-separated path into a YAML document where map keys live +// between '.' separators and sequence indices live in '[i]' suffixes. Three +// characters are special inside a map-key segment and must be backslash-escaped +// to round-trip safely: +// +// \. literal '.' +// \[ literal '[' +// \\ literal '\' +// +// An empty-string map key is encoded as a zero-character segment, so the +// boundary '.' alone carries the structure (e.g. "parent." is the segment +// "parent" followed by an empty-string child key). Sequence indices ('[i]') +// are unaffected by escaping; '[' inside a map key must be escaped to avoid +// being parsed as the start of an index group. +// +// EscapeSegment / UnescapeSegment apply and reverse the encoding for one +// segment at a time. The string-walking helpers (Depth, Parent, LastSegment) +// honour escapes — they only treat unescaped '.' or '[' as boundaries. package domain -import "strings" +import ( + "fmt" + "iter" + "strings" +) // FlatKey represents a dot-separated path into a YAML document. -// Example: "service.ports[0].name" +// Example: "service.ports[0].name". See package doc for the escape scheme +// covering map-key segments that contain '.', '[', '\', or are empty. type FlatKey string -// Depth returns the nesting depth (number of dot segments). +// escapeGrowSlack is the extra capacity reserved on the strings.Builder used +// by EscapeSegment to absorb a handful of escape backslashes without a +// reallocation. Tuned for typical k8s label keys (1–3 dots). +const escapeGrowSlack = 4 + +// EscapeSegment returns name with the three FlatKey escape sequences applied. +// Empty input maps to empty output — an empty-string map key needs no escape; +// the surrounding '.' boundaries already encode the empty segment. +func EscapeSegment(name string) string { + if !strings.ContainsAny(name, `.[\`) { + return name + } + + var b strings.Builder + + b.Grow(len(name) + escapeGrowSlack) + + for i := 0; i < len(name); i++ { + c := name[i] + if c == '.' || c == '[' || c == '\\' { + b.WriteByte('\\') + } + + b.WriteByte(c) + } + + return b.String() +} + +// UnescapeSegment reverses EscapeSegment. Returns an error when the input +// contains a dangling '\' or an unrecognised escape (anything other than +// \., \[, or \\). +func UnescapeSegment(seg string) (string, error) { + if !strings.ContainsRune(seg, '\\') { + return seg, nil + } + + var b strings.Builder + + b.Grow(len(seg)) + + for i := 0; i < len(seg); i++ { + c := seg[i] + if c != '\\' { + b.WriteByte(c) + + continue + } + + if i+1 >= len(seg) { + return "", fmt.Errorf("dangling backslash at end of segment %q", seg) + } + + next := seg[i+1] + if next != '.' && next != '[' && next != '\\' { + return "", fmt.Errorf("invalid escape \\%c in segment %q", next, seg) + } + + b.WriteByte(next) + + i++ + } + + return b.String(), nil +} + +// MapSegments returns the dot-separated segments of the key, decoded back to +// their literal form. Sequence indices stay attached to their preceding map +// segment (e.g. "foo.bar[0]" → ["foo", "bar[0]"]). Returns nil for the empty +// key. Used by display code that needs the human-readable path components; +// not for tree navigation, which goes through parseKeySegments. +// +// On a malformed escape inside a segment that segment is returned encoded +// (no error) so display paths don't have to handle errors. +func (k FlatKey) MapSegments() []string { + if k == "" { + return nil + } + + parts := SplitEncoded(string(k)) + out := make([]string, len(parts)) + + for i, p := range parts { + decoded, err := UnescapeSegment(p) + if err != nil { + out[i] = p + + continue + } + + out[i] = decoded + } + + return out +} + +// unescapedBytes yields (index, byte) pairs of s that are not part of an +// escape sequence. Backslash-escape pairs `\X` are silently consumed — +// neither byte is yielded — so callers see only "real" boundary candidates. +// A trailing unmatched `\` is yielded as itself. Shared by every +// escape-aware walker so the skip-pair logic lives in one place. +func unescapedBytes(s string) iter.Seq2[int, byte] { + return func(yield func(int, byte) bool) { + for i := 0; i < len(s); { + c := s[i] + if c == '\\' && i+1 < len(s) { + i += 2 + + continue + } + + if !yield(i, c) { + return + } + + i++ + } + } +} + +// SplitEncoded splits s on unescaped '.' boundaries, returning the parts in +// their still-encoded form (callers unescape per segment). A trailing '.' +// produces a final empty part; consecutive ".." produce an empty middle part. +// Used by MapSegments and by the service-layer flat-key parser. +func SplitEncoded(s string) []string { + if s == "" { + return nil + } + + out := make([]string, 0, strings.Count(s, ".")+1) + start := 0 + + for i, c := range unescapedBytes(s) { + if c == '.' { + out = append(out, s[start:i]) + start = i + 1 + } + } + + out = append(out, s[start:]) + + return out +} + +// IndexUnescaped returns the index of the first unescaped occurrence of b in +// s, or -1 when none exists. Used by the service-layer parser to find the +// first real '[' (start of an index group) inside an already-split flat-key +// segment without misreading an escaped literal `\[`. +func IndexUnescaped(s string, b byte) int { + for i, c := range unescapedBytes(s) { + if c == b { + return i + } + } + + return -1 +} + +// Depth returns the nesting depth measured by unescaped '.' separators. +// Empty key returns 0; a key with no separators returns 1. func (k FlatKey) Depth() int { if k == "" { return 0 @@ -14,8 +200,8 @@ func (k FlatKey) Depth() int { count := 1 - for i := range len(string(k)) { - if k[i] == '.' { + for _, c := range unescapedBytes(string(k)) { + if c == '.' { count++ } } @@ -28,35 +214,90 @@ func (k FlatKey) Depth() int { // "[i]", so the parent of "foo.bar[0].baz" is "foo.bar[0]", and the parent // of "foo.bar[0]" is "foo.bar" (the list header). Splitting only on '.' // would skip the list-header level and break ancestor walks. +// +// Honours backslash escapes — an escaped '\.' or '\[' inside a segment is +// not treated as a boundary. func (k FlatKey) Parent() FlatKey { - s := string(k) - - dotIdx := strings.LastIndexByte(s, '.') - bracketIdx := strings.LastIndexByte(s, '[') + idx := lastBoundary(string(k)) + if idx < 0 { + return "" + } - idx := max(bracketIdx, dotIdx) + return k[:idx] +} +// ParentMapPath returns the prefix before the last unescaped '.', or "" if +// none exists. Unlike Parent it ignores '[' boundaries — used by UI code +// that groups rows by their enclosing mapping and treats list-header levels +// as part of the leaf, not a separate parent row. +func (k FlatKey) ParentMapPath() FlatKey { + idx := lastUnescapedDot(string(k)) if idx < 0 { return "" } - return FlatKey(s[:idx]) + return k[:idx] } -// LastSegment returns the final segment of the key. +// LastSegment returns the final segment of the key, decoded back to its +// literal form (escapes removed). // // Note: unlike Parent this splits only on '.'; it does NOT treat "[i]" as a // separator, so LastSegment("foo.bar[0]") is "bar[0]" (the list field plus // its index), not "[0]". That matches what UI code wants to display as the // leaf label and is why the two helpers intentionally diverge. If you need // the index-only form, strip it explicitly from the result. +// +// On a malformed escape the encoded tail is returned unchanged so callers +// don't have to plumb error handling through display paths. func (k FlatKey) LastSegment() string { - s := string(k) + s := lastSegmentRaw(string(k)) - idx := strings.LastIndexByte(s, '.') + decoded, err := UnescapeSegment(s) + if err != nil { + return s + } + + return decoded +} + +// lastSegmentRaw returns the tail after the last unescaped '.', preserving +// any escape sequences. Used internally where the caller wants to compare +// against another encoded form. +func lastSegmentRaw(s string) string { + idx := lastUnescapedDot(s) if idx < 0 { return s } return s[idx+1:] } + +// lastBoundary returns the byte index of the last unescaped '.' or '[' in s, +// or -1 when none exists. The boundary char itself is at the returned index; +// callers slice with s[:idx] to drop it. +func lastBoundary(s string) int { + last := -1 + + for i, c := range unescapedBytes(s) { + if c == '.' || c == '[' { + last = i + } + } + + return last +} + +// lastUnescapedDot returns the byte index of the last unescaped '.' in s, +// or -1. Used by LastSegment, which does not treat '[' as a boundary. +func lastUnescapedDot(s string) int { + last := -1 + + for i, c := range unescapedBytes(s) { + if c == '.' { + last = i + } + } + + return last +} diff --git a/domain/flatkey_test.go b/domain/flatkey_test.go new file mode 100644 index 0000000..7ab951c --- /dev/null +++ b/domain/flatkey_test.go @@ -0,0 +1,150 @@ +package domain + +import "testing" + +const ( + tkSingle = "single" + tkAB = "a.b" + tkABC = "a.b.c" + tkBackslash = `\` + + tkPlain = "plainKey" + tkSpaces = "with spaces" + tkColon = "key:value" + tkHash = "value with # not a comment" + tkSlashKey = "costcenter/team" + tkK8sLabel = "app.kubernetes.io/part-of" + tkK8sLabelE = `app\.kubernetes\.io/part-of` + tkLabelsKey = `commonLabels.app\.kubernetes\.io/part-of` + tkEscDot = `a\.b` + tkTrailing = "trailing." + tkFooBar0 = "foo.bar[0]" +) + +func TestEscapeUnescapeRoundTrip(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + in string + want string + }{ + {"plain", tkPlain, tkPlain}, + {"empty", "", ""}, + {"only dot", ".", `\.`}, + {"only backslash", tkBackslash, `\\`}, + {"only bracket", "[", `\[`}, + {"k8s label", tkK8sLabel, tkK8sLabelE}, + {"slash key", tkSlashKey, tkSlashKey}, + {"with spaces", tkSpaces, tkSpaces}, + {"colon", tkColon, tkColon}, + {"hash", tkHash, tkHash}, + {"mixed special", `a.b\c[d`, `a\.b\\c\[d`}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + got := EscapeSegment(tc.in) + if got != tc.want { + t.Errorf("EscapeSegment(%q) = %q, want %q", tc.in, got, tc.want) + } + + back, err := UnescapeSegment(got) + if err != nil { + t.Fatalf("UnescapeSegment(%q): %v", got, err) + } + + if back != tc.in { + t.Errorf("round-trip: got %q, want %q", back, tc.in) + } + }) + } +} + +func TestUnescapeSegmentErrors(t *testing.T) { + t.Parallel() + + bad := []string{ + tkBackslash, // dangling + `abc\`, // dangling at end + `\x`, // unknown escape + `\n`, // unknown escape (we don't recognise C-style escapes) + } + + for _, in := range bad { + if _, err := UnescapeSegment(in); err == nil { + t.Errorf("UnescapeSegment(%q): expected error, got nil", in) + } + } +} + +func TestFlatKeyDepth(t *testing.T) { + t.Parallel() + + cases := []struct { + key FlatKey + want int + }{ + {"", 0}, + {tkSingle, 1}, + {tkAB, 2}, + {tkABC, 3}, + {tkEscDot, 1}, + {`a\.b.c`, 2}, + {tkLabelsKey, 2}, + {tkTrailing, 2}, + {"foo[0].bar", 2}, + {`a\\b.c`, 2}, + } + + for _, tc := range cases { + got := tc.key.Depth() + if got != tc.want { + t.Errorf("FlatKey(%q).Depth() = %d, want %d", tc.key, got, tc.want) + } + } +} + +// flatKeyMethodCase drives both Parent and LastSegment table tests through +// one shared loop so the two table-test bodies don't trip the dupl linter +// while still letting each method assert against its own expected output. +type flatKeyMethodCase struct { + key FlatKey + parent FlatKey + lastSeg string + skipPart bool // skip Parent assertion (case is LastSegment-specific) + skipLast bool // skip LastSegment assertion (case is Parent-specific) +} + +func TestFlatKeyParentAndLastSegment(t *testing.T) { + t.Parallel() + + cases := []flatKeyMethodCase{ + {key: "", parent: "", lastSeg: ""}, + {key: tkSingle, parent: "", lastSeg: tkSingle}, + {key: tkAB, parent: "a", lastSeg: "b"}, + {key: tkABC, parent: tkAB, lastSeg: "c"}, + {key: FlatKey(tkFooBar0), parent: "foo.bar", lastSeg: "bar[0]"}, + {key: "foo.bar[0].baz", parent: FlatKey(tkFooBar0), skipLast: true}, + {key: tkLabelsKey, parent: "commonLabels", lastSeg: tkK8sLabel}, + {key: tkEscDot, parent: "", lastSeg: "a.b"}, + {key: FlatKey(tkTrailing), parent: "trailing", lastSeg: ""}, + {key: `a.b\\c`, skipPart: true, lastSeg: `b\c`}, + } + + for _, tc := range cases { + if !tc.skipPart { + if got := tc.key.Parent(); got != tc.parent { + t.Errorf("FlatKey(%q).Parent() = %q, want %q", tc.key, got, tc.parent) + } + } + + if !tc.skipLast { + if got := tc.key.LastSegment(); got != tc.lastSeg { + t.Errorf("FlatKey(%q).LastSegment() = %q, want %q", tc.key, got, tc.lastSeg) + } + } + } +} diff --git a/service/comment_parser.go b/service/comment_parser.go index ade3c80..ff433f3 100644 --- a/service/comment_parser.go +++ b/service/comment_parser.go @@ -96,7 +96,7 @@ func walkFootComments(item commentStackItem, stack *[]commentStackItem, foots ma case yaml.MappingNode: for i := 0; i < len(content)-1; i += 2 { keyNode, valNode := content[i], content[i+1] - if keyNode == nil || valNode == nil || keyNode.Value == "" { + if keyNode == nil || valNode == nil { continue } @@ -199,7 +199,7 @@ func walkCommentChildren(item commentStackItem, stack *[]commentStackItem, comme case yaml.MappingNode: for i := 0; i < len(content)-1; i += 2 { keyNode, valNode := content[i], content[i+1] - if keyNode == nil || valNode == nil || keyNode.Value == "" { + if keyNode == nil || valNode == nil { continue } diff --git a/service/fixture_test.go b/service/fixture_test.go index 4753276..65523bc 100644 --- a/service/fixture_test.go +++ b/service/fixture_test.go @@ -14,7 +14,7 @@ func TestLoadCustomFixture_BannerSurvives(t *testing.T) { svc := NewValuesService() - domainVF, err := svc.ReadCustomValues(context.Background(), "../test-data/redis-values-cornercases.yaml") + domainVF, err := svc.ReadCustomValues(context.Background(), fixturePath) if err != nil { t.Skipf("fixture not present or unreadable: %v", err) } diff --git a/service/round_trip_fixture_test.go b/service/round_trip_fixture_test.go index cfa77a5..286ac21 100644 --- a/service/round_trip_fixture_test.go +++ b/service/round_trip_fixture_test.go @@ -1,6 +1,7 @@ package service import ( + "os" "strings" "testing" ) @@ -9,81 +10,82 @@ import ( // service test files so goconst doesn't flag duplication. const yamlTrueLiteral = "true" -// TestRoundTripCornerCases is the headline regression for the comment + -// anchor preservation work. The inline source below is a condensed -// counterpart to test-data/redis-values-cornercases.yaml — it exercises the -// features qdeck must round-trip on save: file-level banner, anchors with -// alias references, single-source merge keys (`<<: *base`), multi-source -// merge keys (`<<: [*a, *b]`), per-leaf foot blocks, deeply nested mappings, -// and quoted keys. -// -// The inline form is preferred over loading the on-disk fixture because the -// parser doesn't yet support every YAML construct the fixture contains -// (notably empty-string keys produce flat keys like `parent.` that -// parseKeySegments rejects). When that limitation is fixed, this test can -// be ported to load the fixture directly and the assertions transferred. +// fixturePath is the on-disk corner-case fixture loaded by both +// TestRoundTripCornerCases and (via ReadCustomValues) TestLoadCustomFixture_*. +// Path is relative to the service/ directory, where `go test` runs. +const fixturePath = "../assets/test-data/redis-values-cornercases.yaml" + +// TestRoundTripCornerCases is the headline regression for comment + anchor + +// unusual-key preservation. It loads the on-disk fixture +// assets/test-data/redis-values-cornercases.yaml — which exercises file-level +// banners, anchors with alias references, single-source merge keys +// (`<<: *base`), multi-source merge keys (`<<: [*a, *b]`), per-leaf foot +// blocks, deep nesting, AND the two unusual-key cases the FlatKey escape +// scheme handles: empty-string map keys (`"": v`) and keys containing +// literal '.' (`app.kubernetes.io/part-of`). // // The test patches a single primitive scalar so PatchNodeTree's edit path -// runs end-to-end — without an edit, the path would just shortcut through -// "nothing changed" and miss serializer regressions. +// runs end-to-end — without an edit the path would shortcut through "nothing +// changed" and miss serializer regressions. func TestRoundTripCornerCases(t *testing.T) { t.Parallel() - src := `# ============================================================================ -# Banner block — must survive as DocumentNode.HeadComment. -# ============================================================================ - -x-defaults: &defaults - enabled: true - replicas: 1 - -x-extra: &extra - tier: gold - -global: - imageRegistry: "" - storageClass: "standard-rwo" - # foot block on storageClass + raw, err := os.ReadFile(fixturePath) + if err != nil { + t.Skipf("fixture not present or unreadable: %v", err) + } -primary: - <<: *defaults - resources: - cpu: 100m - memory: 128Mi + src := string(raw) -replica: - <<: [*defaults, *extra] - enabled: false + tree, docs := loadTreeAndDocs(t, src) -deep: - level1: - level2: - level3: - leaf: deep-value -` + svc := NewValuesService() - tree, docs := loadTreeAndDocs(t, src) + parsed, err := svc.ParseYAMLText(t.Context(), src) + if err != nil { + t.Fatalf("ParseYAMLText: %v", err) + } - // Build entries from the parsed tree. Filter to physical keys the - // parser handles — anchors / merge sources are physical leaves but - // merge keys themselves aren't user-edited, so we don't add them to - // `want`. A real save flow does the same via collectOverrides. const ( - fixtureRegistry = "my-registry.example.com" - fixtureDeepValue = "deep-value" - fixtureFootCheck = "foot block on storageClass" + fixtureRegistry = "my-registry.example.com" + fixtureBanner = "Bitnami Redis" + fixtureAnchor = "&defaults" + fixtureMergeAlpha = "<<:" + fixtureAlias = "*defaults" + // Empty-string key in cornerCases.quotedKeys (line 420 of fixture). + fixtureEmptyKeyValue = "empty key" ) + // fixtureDottedLabelValue is the value paired with the literal-dot + // k8s label key in commonLabels (line 79 of fixture). Lifted to + // package scope as dottedLabelValue so the serialise tests can reuse + // the same literal. + fixtureDottedLabelValue := dottedLabelValue + + // Build override entries from the parsed flat-key list, patching one + // scalar so the edit path runs. + entries := make([]OverrideEntry, 0, len(parsed.Entries)) + patched := false + + for _, e := range parsed.Entries { + if e.Type == typeMap || e.Type == typeList { + continue + } + + value := e.Value + if !patched && string(e.Key) == "image.registry" { + value = fixtureRegistry + patched = true + } + + entries = append(entries, OverrideEntry{ + Key: string(e.Key), + Value: value, + Type: e.Type, + }) + } - entries := []OverrideEntry{ - {Key: "x-defaults.enabled", Value: yamlTrueLiteral, Type: typeBool}, - {Key: "x-defaults.replicas", Value: "1", Type: "int"}, - {Key: "x-extra.tier", Value: "gold", Type: typeString}, - {Key: "global.imageRegistry", Value: fixtureRegistry, Type: typeString}, - {Key: "global.storageClass", Value: "standard-rwo", Type: typeString}, - {Key: "primary.resources.cpu", Value: "100m", Type: typeString}, - {Key: "primary.resources.memory", Value: "128Mi", Type: typeString}, - {Key: "replica.enabled", Value: "false", Type: typeBool}, - {Key: "deep.level1.level2.level3.leaf", Value: fixtureDeepValue, Type: typeString}, + if !patched { + t.Fatal("did not find image.registry in parsed entries; fixture changed?") } got, err := PatchNodeTree(tree, entries, DefaultYAMLIndent, docs) @@ -96,19 +98,58 @@ deep: contains string }{ {"patched value", fixtureRegistry}, - {"banner", "Banner block"}, - {"anchor &defaults", "&defaults"}, - {"anchor &extra", "&extra"}, - {"merge key directive", "<<:"}, - {"alias reference *defaults", "*defaults"}, - {"alias reference *extra", "*extra"}, - {"foot block on storageClass", fixtureFootCheck}, - {"deep nesting leaf", fixtureDeepValue}, + {"banner", fixtureBanner}, + {"anchor &defaults", fixtureAnchor}, + {"merge key directive", fixtureMergeAlpha}, + {"alias reference *defaults", fixtureAlias}, + {"empty-string-key value", fixtureEmptyKeyValue}, + {"literal-dot label value", fixtureDottedLabelValue}, + {"literal-dot key preserved as single map key", "app.kubernetes.io/part-of"}, + {"another literal-dot key", "kubernetes.io/change-cause"}, } for _, c := range checks { if !strings.Contains(got, c.contains) { - t.Errorf("%s missing from output (looked for %q):\n%s", c.name, c.contains, got) + t.Errorf("%s missing from output (looked for %q)", c.name, c.contains) } } + + // Guard against the silent-corruption shape: a literal-dot key being + // split into nested mappings would produce a top-level " app:" line + // inside commonLabels. + commonLabelsBlock := extractMappingBlock(got, "commonLabels:") + if strings.Contains(commonLabelsBlock, "\n app:\n") { + t.Errorf("literal-dot label key got split into nested mappings:\n%s", commonLabelsBlock) + } +} + +// extractMappingBlock returns the substring of yaml from the line beginning +// with header through the next blank line or top-level key. Used to scope +// substring assertions to a specific mapping block. +func extractMappingBlock(yaml, header string) string { + idx := strings.Index(yaml, header) + if idx < 0 { + return "" + } + + rest := yaml[idx:] + + // Stop at the first non-indented, non-empty line after the header line. + lines := strings.Split(rest, "\n") + if len(lines) <= 1 { + return rest + } + + end := 1 + + for end < len(lines) { + line := lines[end] + if line != "" && !strings.HasPrefix(line, " ") && !strings.HasPrefix(line, "#") { + break + } + + end++ + } + + return strings.Join(lines[:end], "\n") } diff --git a/service/values_service.go b/service/values_service.go index 8bc176c..c984e5e 100644 --- a/service/values_service.go +++ b/service/values_service.go @@ -212,9 +212,40 @@ func (s *ValuesService) ReadCustomValues(ctx context.Context, filePath string) ( } } + rewriteValuesFromNodeTree(vf) + return vf, nil } +// rewriteValuesFromNodeTree replaces each entry's Value with the literal +// scalar text from vf.NodeTree, when one is reachable. chartutil.ReadValuesFile +// routes through sigs.k8s.io/yaml, which decodes integers larger than 2^53 +// to float64 and silently loses precision (e.g. `9007199254740993` becomes +// `9.007199254740992e+15`). yaml.v3's parser keeps the literal text on each +// scalar node verbatim, so once we've parsed the file into NodeTree we can +// use it as the source of truth for scalar leaves. Aliases and merge keys +// resolve through the same code path PatchNodeTree uses, so the rewritten +// Value matches what a YAML consumer (Helm, kubectl) would see at that key. +// +// No-op when NodeTree is unavailable (e.g. ReadValuesFile succeeded but the +// raw-bytes re-parse failed). +func rewriteValuesFromNodeTree(vf *domain.ValuesFile) { + if vf == nil || vf.NodeTree == nil { + return + } + + for i := range vf.Entries { + e := &vf.Entries[i] + if e.Type == typeMap || e.Type == typeList { + continue + } + + if literal, ok := EffectiveScalarAt(vf.NodeTree, string(e.Key)); ok { + e.Value = literal + } + } +} + // ReadAndMergeCustomValues loads multiple values files and deep-merges them. // Later files override earlier ones for both data and per-leaf head/line // comments. The file-level shape — DocHeadComment, DocFootComment, @@ -301,6 +332,7 @@ func (s *ValuesService) ReadAndMergeCustomValues(ctx context.Context, paths []st vf.LineEnding = firstEOL attachComments(vf, mergedComments) + rewriteValuesFromNodeTree(vf) return vf, nil } @@ -607,7 +639,7 @@ func buildFlatKeyPositions(root *yaml.Node) map[string]int { k := node.Content[i] v := node.Content[i+1] - if k == nil || v == nil || k.Value == "" { + if k == nil || v == nil { continue } @@ -780,9 +812,19 @@ func flattenValues(source string, vals map[string]any) *domain.ValuesFile { } default: + value := fmt.Sprintf("%v", typedVal) + // fmt.Sprintf renders nil as "" — convertValue would then + // pass that literal string through to the YAML encoder, replacing + // the original `null` / `~` with the four-character "". + // Empty-string Value with Type=null round-trips through + // convertValue's typeNull case back to a real nil. + if typedVal == nil { + value = "" + } + vf.Entries = append(vf.Entries, domain.ValuesEntry{ Key: domain.FlatKey(item.prefix), - Value: fmt.Sprintf("%v", typedVal), + Value: value, Type: inferType(typedVal), }) } @@ -795,12 +837,40 @@ func flattenValues(source string, vals map[string]any) *domain.ValuesFile { return vf } +// buildKey appends segment to prefix, applying flat-key escaping so that map +// keys containing '.', '[', '\', or empty strings round-trip safely. prefix +// is already encoded (it was returned by an earlier buildKey call); only +// segment needs escaping at construction time. +// +// Use joinFlatKey instead when both arguments are already encoded paths — +// passing an encoded path through buildKey's segment slot would +// double-escape its embedded backslashes. func buildKey(prefix, segment string) string { + encoded := domain.EscapeSegment(segment) if prefix == "" { - return segment + return encoded + } + + return prefix + "." + encoded +} + +// joinFlatKey concatenates two already-encoded flat-key paths with the '.' +// separator. Neither argument is re-escaped — both must be in the FlatKey +// escape form already (e.g. produced by an earlier flatten call or +// buildKey). Used when prefixing a subchart's per-leaf keys with the +// subchart name, where both halves are encoded paths and re-running +// EscapeSegment on either would double-escape any backslashes. +// +// Empty prefix returns the suffix verbatim (root-level join). Empty suffix +// is preserved as a trailing '.' — that's how the FlatKey encoding +// represents an empty-string child key, so dropping it here would collapse +// the leaf out of existence. +func joinFlatKey(prefixEncoded, suffixEncoded string) string { + if prefixEncoded == "" { + return suffixEncoded } - return prefix + "." + segment + return prefixEncoded + "." + suffixEncoded } func inferType(v any) string { @@ -905,7 +975,7 @@ func collectDependencyValues(ch *chart.Chart, parentPrefix string) []domain.Valu for _, e := range subVF.Entries { all = append(all, domain.ValuesEntry{ - Key: domain.FlatKey(buildKey(fullPrefix, string(e.Key))), + Key: domain.FlatKey(joinFlatKey(fullPrefix, string(e.Key))), Value: e.Value, Type: e.Type, Comment: e.Comment, diff --git a/service/yaml_serialize.go b/service/yaml_serialize.go index e7cb1e0..cb06f4a 100644 --- a/service/yaml_serialize.go +++ b/service/yaml_serialize.go @@ -259,12 +259,35 @@ func PatchNodeTree(root *yaml.Node, entries []OverrideEntry, indent int, docs Do continue } + // flattenValues emits "{}"/"[]" placeholder entries for empty + // containers so the UI has a row to render. The deep-copied tree + // already carries the correct (possibly populated) container at + // that path; treating the placeholder as a scalar edit would + // overwrite the container with a literal "{}"/"[]" string. Skip + // when the tree already has a matching container kind. + if e.Type == typeMap || e.Type == typeList { + if treeNodeMatchesContainerType(workingTree, segments, e.Type) { + continue + } + } + effective, hasValue := findEffectiveScalar(workingTree, segments) - valueUnchanged := hasValue && effective == e.Value + valueUnchanged := hasValue && scalarsEquivalent(effective, e.Value, e.Type) - effHead, effLine := effectiveComments(workingTree, segments) + effHead, effLine, viaAlias := effectiveComments(workingTree, segments) commentsUnchanged := overrideCommentsMatch(e.HeadComment, e.LineComment, effHead, effLine) + // When the leaf is reached through an alias, the comments yaml.v3 + // reports at this position actually live on the anchor target. + // Writing comments here breaks the alias for no good reason — the + // user can only edit comments at the anchor source (where they're + // physically authored). Treat alias-resolved comments as + // effectively unchanged so an empty entry.HeadComment/LineComment + // doesn't trigger a clear that would expand the alias. + if viaAlias { + commentsUnchanged = true + } + if valueUnchanged && commentsUnchanged { continue } @@ -287,6 +310,24 @@ func PatchNodeTree(root *yaml.Node, entries []OverrideEntry, indent int, docs Do return encodeWithDocComments(workingTree, indent, docs.Head, docs.Foot) } +// scalarsEquivalent reports whether the tree's effective scalar text and the +// override entry's value mean the same thing for round-trip purposes. For +// typeNull the three YAML representations (`null`, `~`, blank `""`) all encode +// the same value, so treating them as equal here keeps PatchNodeTree's unedited +// branch from rewriting (and re-styling) a null leaf the user didn't touch. +// All other types fall back to literal string equality. +func scalarsEquivalent(treeValue, entryValue, entryType string) bool { + if entryType == typeNull && isYAMLNullLiteral(treeValue) && isYAMLNullLiteral(entryValue) { + return true + } + + return treeValue == entryValue +} + +func isYAMLNullLiteral(s string) bool { + return s == "" || s == "~" || s == typeNull +} + // applyDocFoots writes each (leafKey -> footText) pair as the FootComment of // the value node identified by leafKey. Missing keys are skipped silently // (the source file may have changed since the foot was captured); empty foots @@ -330,6 +371,185 @@ func applyTextByKey( return nil } +// Hex parsing parameters for parseHexEscape — named so the bit-shift and +// digit-offset arithmetic don't trip mnd; the values themselves are +// intrinsic to base-16 number representation. +const ( + hexNibbleBits = 4 + hexDigitOffset = 10 // value of 'a' in base-16 (and 'A') + yamlEscPrefix = 2 // bytes consumed by `\U` / `\u` before the digits + yamlEscWide = 8 // hex digits in a `\U` 32-bit escape + yamlEscNarrow = 4 // hex digits in a `\u` 16-bit escape +) + +// unescapeYAMLSupplementaryRunes rewrites yaml.v3's `\Unnnnnnnn` / +// `\unnnn` escape sequences back to their literal Unicode characters +// inside double-quoted scalars. yaml.v3's emitter classifies any code point +// above the Basic Multilingual Plane (> U+FFFF — emojis, skin-tone +// modifiers, lots of CJK Extension blocks) as "unprintable" and forces +// double-quoted style with `\U` escapes regardless of the requested Style. +// Without this pass, every save would silently rewrite an emoji like 🎉 +// (U+1F389) to the literal seven-character text `\U0001F389`. +// +// State machine walks the rendered text tracking double-quoted-string +// boundaries; escapes are only converted while we're inside a `"..."` +// scalar so a `\U…` sequence appearing in a comment, single-quoted scalar, +// or block scalar passes through untouched. `\\U…` (backslash-escaped +// backslash before `U`) is left alone — that's a literal `\U` in the +// user's value. +func unescapeYAMLSupplementaryRunes(s string) string { + if !strings.ContainsRune(s, '\\') { + return s + } + + var b strings.Builder + + b.Grow(len(s)) + + const ( + stateText = iota // outside any scalar + stateDouble // inside a "..." scalar + stateSingle // inside a '...' scalar + stateLineComment // after an unquoted '#' to end of line + ) + + state := stateText + + for i := 0; i < len(s); { + c := s[i] + + switch state { + case stateText: + switch c { + case '"': + state = stateDouble + + b.WriteByte(c) + + i++ + case '\'': + state = stateSingle + + b.WriteByte(c) + + i++ + case '#': + state = stateLineComment + + b.WriteByte(c) + + i++ + default: + b.WriteByte(c) + + i++ + } + + case stateLineComment: + b.WriteByte(c) + + if c == '\n' { + state = stateText + } + + i++ + + case stateSingle: + b.WriteByte(c) + + i++ + + if c == '\'' { + state = stateText + } + + case stateDouble: + if c != '\\' { + b.WriteByte(c) + + if c == '"' { + state = stateText + } + + i++ + + continue + } + + // Escape sequence inside double-quoted scalar. + if i+1 >= len(s) { + b.WriteByte(c) + + i++ + + continue + } + + next := s[i+1] + switch next { + case 'U': + if r, ok := parseHexEscape(s, i+yamlEscPrefix, yamlEscWide); ok { + b.WriteRune(r) + + i += yamlEscPrefix + yamlEscWide + + continue + } + case 'u': + if r, ok := parseHexEscape(s, i+yamlEscPrefix, yamlEscNarrow); ok { + b.WriteRune(r) + + i += yamlEscPrefix + yamlEscNarrow + + continue + } + } + + // Any other escape (including `\\`, `\"`, `\n`, etc.) passes + // through verbatim — yaml.v3 will re-parse them on next load. + b.WriteByte(c) + b.WriteByte(next) + + i += 2 + } + } + + return b.String() +} + +// parseHexEscape parses width hex digits at s[off:] and returns the rune +// they encode plus a true ok flag. Returns (0, false) when the slice is +// short or any character isn't a hex digit, leaving the caller to emit the +// escape sequence verbatim. +func parseHexEscape(s string, off, width int) (rune, bool) { + if off+width > len(s) { + return 0, false + } + + var r rune + + for i := range width { + c := s[off+i] + + var d rune + + switch { + case c >= '0' && c <= '9': + d = rune(c - '0') + case c >= 'a' && c <= 'f': + d = rune(c-'a') + hexDigitOffset + case c >= 'A' && c <= 'F': + d = rune(c-'A') + hexDigitOffset + default: + return 0, false + } + + r = r< 0 { - segments = append(segments, keySegment{name: part[:bracketIdx]}) + name, err := domain.UnescapeSegment(part[:bracketIdx]) + if err != nil { + return nil, fmt.Errorf("decode segment %q: %w", part, err) + } + + segments = append(segments, keySegment{name: name}) } - // Extract all [N] indices from this part. + // Index groups are pure numeric; no escape handling needed. rest := part[bracketIdx:] for len(rest) > 0 { if rest[0] != '[' { @@ -94,62 +109,87 @@ func parseKeySegments(key string) ([]keySegment, error) { return segments, nil } -// parseArraySegment splits "key[0]" into ("key", 0, true) or "[0]" into ("", 0, true). -// For plain keys like "host" it returns ("host", 0, false). -func parseArraySegment(seg string) (string, int, bool) { - bracketIdx := strings.IndexByte(seg, '[') - if bracketIdx < 0 { - return seg, 0, false +// findNodeSubtree supports array index segments like "servers[0].host" by +// navigating into SequenceNode children. Honours flat-key escapes — a key +// segment like "app\.kubernetes\.io/x" is treated as a single map key with a +// literal '.' rather than three nested segments. +func findNodeSubtree(root *yaml.Node, keyPath string) *yaml.Node { + segs, err := parseKeySegments(keyPath) + if err != nil { + return nil } - closeIdx := strings.IndexByte(seg[bracketIdx:], ']') - if closeIdx < 0 { - return seg, 0, false - } + current := root + + for _, seg := range segs { + if seg.isIndex { + if current.Kind != yaml.SequenceNode || seg.index < 0 || seg.index >= len(current.Content) { + return nil + } - idxStr := seg[bracketIdx+1 : bracketIdx+closeIdx] + current = current.Content[seg.index] - idx, err := strconv.Atoi(idxStr) - if err != nil { - return seg, 0, false + continue + } + + current = findMappingChild(current, seg.name) + if current == nil { + return nil + } } - return seg[:bracketIdx], idx, true + return current } -// findNodeSubtree supports array index segments like "servers[0].host" by -// navigating into SequenceNode children. -func findNodeSubtree(root *yaml.Node, keyPath string) *yaml.Node { - segments := strings.Split(keyPath, ".") - current := root +// treeNodeMatchesContainerType reports whether the tree node at segments is +// a MappingNode (matching typeMap) or SequenceNode (matching typeList). +// Walks read-only — alias nodes are resolved but never broken — so callers +// can probe the tree's shape without mutating shared anchor targets. Used +// by PatchNodeTree to skip the "{}"/"[]" placeholder entries flattenValues +// emits for empty containers, which would otherwise overwrite the existing +// container with a literal string scalar on save. +func treeNodeMatchesContainerType(root *yaml.Node, segments []keySegment, typ string) bool { + current := resolveAlias(root) for _, seg := range segments { - // Handle array index segments like "items[0]" or bare "[0]". - mapKey, idx, hasIndex := parseArraySegment(seg) + if current == nil { + return false + } - if mapKey != "" { - current = findMappingChild(current, mapKey) - if current == nil { - return nil + if seg.isIndex { + if current.Kind != yaml.SequenceNode || seg.index < 0 || seg.index >= len(current.Content) { + return false } + + current = resolveAlias(current.Content[seg.index]) + + continue } - if hasIndex { - if current.Kind != yaml.SequenceNode || idx < 0 || idx >= len(current.Content) { - return nil - } + if current.Kind != yaml.MappingNode { + return false + } - current = current.Content[idx] - } else if mapKey == "" { - // Plain key segment — navigate into mapping. - current = findMappingChild(current, seg) - if current == nil { - return nil - } + next := mappingValueWithMerge(current, seg.name) + if next == nil { + return false } + + current = resolveAlias(next) } - return current + if current == nil { + return false + } + + switch typ { + case typeMap: + return current.Kind == yaml.MappingNode + case typeList: + return current.Kind == yaml.SequenceNode + } + + return false } func findMappingChild(node *yaml.Node, key string) *yaml.Node { @@ -247,6 +287,72 @@ func resolvePhysicalForMutation(tree *yaml.Node, segments []keySegment) (*yaml.N return current, nil } +// findParentSlotReadOnly is the non-mutating variant of findParentSlot used +// by callers that only read state (e.g. effectiveComments). Alias nodes +// encountered along the walk are resolved through but never broken, so +// probing the tree's shape doesn't expand aliases as a side effect — the +// mutation paths still go through findParentSlot, which intentionally +// breaks aliases before writing so edits stay local. +// +// The returned viaAlias flag reports whether any segment along the path was +// resolved through an alias. Callers (PatchNodeTree's commentsUnchanged +// check) use this to skip comment-clearing on aliased leaves: the comments +// the tree exposes at an aliased position actually live at the anchor +// source, so an empty entry.HeadComment / LineComment doesn't mean the +// user wants those source-side comments cleared — applying that as a +// write would break the alias for no good reason. +func findParentSlotReadOnly(tree *yaml.Node, segments []keySegment) (*yaml.Node, int, bool, error) { + if len(segments) == 0 { + return nil, 0, false, errors.New("empty segments") + } + + current := tree + viaAlias := false + + for i := 0; i < len(segments)-1; i++ { + if current.Kind == yaml.AliasNode { + viaAlias = true + current = resolveAlias(current) + } + + // stepPhysical's alias-breaking branch is unreachable here: we + // resolveAlias above before each step, so current is always a + // non-alias node when stepPhysical inspects it. Reusing it keeps + // the index-bounds and mapping-lookup error messages in one place. + next, err := stepPhysical(current, segments[i], i) + if err != nil { + return nil, 0, false, err + } + + current = next + } + + if current.Kind == yaml.AliasNode { + viaAlias = true + current = resolveAlias(current) + } + + last := segments[len(segments)-1] + if last.isIndex { + if current.Kind != yaml.SequenceNode || last.index >= len(current.Content) { + return nil, 0, false, fmt.Errorf("final segment: index %d out of range", last.index) + } + + return current, last.index, viaAlias, nil + } + + if current.Kind != yaml.MappingNode { + return nil, 0, false, fmt.Errorf("final segment (%q): parent is not a mapping", last.name) + } + + idx := mappingKeyIndex(current, last.name) + if idx < 0 { + return nil, 0, false, fmt.Errorf("final segment (%q): key not present in local mapping", last.name) + } + + return current, idx + 1, viaAlias, nil +} + // findParentSlot walks to the parent of the final segment and returns the // parent node plus the Content index holding the final child's value, so the // caller can swap the child (e.g. replace it with an AliasNode). For mappings @@ -347,9 +453,11 @@ func forEachMappingChild(n *yaml.Node, prefix string, visit func(childPrefix str continue } - childPrefix := keyNode.Value + encoded := domain.EscapeSegment(keyNode.Value) + + childPrefix := encoded if prefix != "" { - childPrefix = prefix + "." + keyNode.Value + childPrefix = prefix + "." + encoded } visit(childPrefix, n.Content[i+1]) @@ -603,7 +711,7 @@ func setLeaf(current *yaml.Node, seg keySegment, value any) error { for len(current.Content) <= seg.index { current.Content = append(current.Content, &yaml.Node{ - Kind: yaml.ScalarNode, Tag: yamlTagNull, Value: "null", + Kind: yaml.ScalarNode, Tag: yamlTagNull, Value: typeNull, }) } diff --git a/ui/page/values_controller.go b/ui/page/values_controller.go index 44b521c..60579bf 100644 --- a/ui/page/values_controller.go +++ b/ui/page/values_controller.go @@ -634,7 +634,14 @@ func (vc *ValuesController) populateColumnOverrides(colIdx int) { commentMap := make(map[string]string, len(col.CustomValues.Entries)) for _, e := range col.CustomValues.Entries { - if e.Value != "" { + // Include scalar-leaf entries unconditionally — even when Value is + // the empty string. Skipping empty values used to fall through to + // SerializeNodeSubtree, which YAML-encodes the scalar (`""` for an + // empty string) and writes that quoted form into the editor; on + // save, those literal quotes round-tripped as the four-character + // string `""` and yaml.v3 emitted them as `"\"\""`. Section and + // comment rows still skip — they're not editable scalar cells. + if !e.IsSection() && !e.IsComment() { customMap[e.Key] = e.Value } diff --git a/ui/page/values_controller_actions.go b/ui/page/values_controller_actions.go index 93aaf3c..9efa808 100644 --- a/ui/page/values_controller_actions.go +++ b/ui/page/values_controller_actions.go @@ -239,16 +239,20 @@ func (vc *ValuesController) onSaveColumnValues(colIdx int) { col := &vc.State.Columns[colIdx] var ( - tree *yaml.Node - docs service.DocComments + tree *yaml.Node + docs service.DocComments + loadedValues map[string]string ) if col.CustomValues != nil { tree = col.CustomValues.NodeTree docs = col.DocCommentsForSave() + loadedValues = state.LoadedValuesMap(col.CustomValues) } - yamlText, err := state.OverridesToYAML(vc.State.Entries, col.OverrideEditors, col.YAMLIndent(), tree, docs) + yamlText, err := state.OverridesToYAML( + vc.State.Entries, col.OverrideEditors, col.YAMLIndent(), tree, docs, loadedValues, + ) if err != nil { vc.NotifState.Show(err.Error(), state.NotificationError, time.Now()) diff --git a/ui/page/values_page_anchors.go b/ui/page/values_page_anchors.go index d645ed1..6b2db44 100644 --- a/ui/page/values_page_anchors.go +++ b/ui/page/values_page_anchors.go @@ -476,7 +476,7 @@ func (p *ValuesPage) resolveFocusedCell() (int, string, bool) { // Falls back to "anchor" when the sanitized result is empty and prefixes // "_" when the first char would be a digit. func suggestAnchorName(flatKey string) string { - parts := strings.Split(flatKey, ".") + parts := domain.FlatKey(flatKey).MapSegments() tail := parts if len(parts) > 2 { //nolint:mnd // last-two segments, not a tunable. diff --git a/ui/state/editor_state.go b/ui/state/editor_state.go index 21532db..4b66967 100644 --- a/ui/state/editor_state.go +++ b/ui/state/editor_state.go @@ -10,26 +10,58 @@ import ( "github.com/qdeck-app/qdeck/service" ) +// LoadedValuesMap projects fv.Entries into a flat-key → value map used by +// collectOverrides to scope the empty-value round-trip path to keys that +// were actually present in the loaded file. Returns nil for a nil +// FlatValues, so callers can pass column state without a nil-guard. +// +// Comment-row entries have an empty Key and are skipped — the map stays a +// scalar/section-leaf index keyed by flat-key strings, matching what +// FlatValueEntry.Key carries on the UI side. +func LoadedValuesMap(fv *service.FlatValues) map[string]string { + if fv == nil { + return nil + } + + out := make(map[string]string, len(fv.Entries)) + + for _, e := range fv.Entries { + if e.Key == "" { + continue + } + + out[e.Key] = e.Value + } + + return out +} + // OverridesToYAML builds YAML text from non-empty override editors. When tree // is non-nil, it is used as a template: anchors, aliases, comments, and scalar // styles from the originally loaded file are preserved for subtrees the user // did not edit. When tree is nil (no file loaded yet), the result is rebuilt // from scratch via FlatEntriesToYAML. // -// Only entries with non-empty editor text are included. Section headers and -// orphan-comment rows are skipped. indent controls the number of spaces per -// nesting level in the output. docs carries doc-level orphan comments -// (banner/trailer/per-leaf foots) so they round-trip on save — a zero -// DocComments leaves the output without any banner or foot blocks. Returns -// empty string with nil error when no overrides AND no doc comments are present. +// loadedValues maps flat key → value as parsed from the column's loaded file +// (nil when no file was loaded, e.g. user just typed into a fresh column). +// It scopes the round-trip of explicit empty/null scalars to keys actually +// present in the loaded file: without this, every empty/null chart default +// would leak into the saved file as an override the user never made. +// Section headers and orphan-comment rows are skipped. indent controls the +// number of spaces per nesting level in the output. docs carries doc-level +// orphan comments (banner/trailer/per-leaf foots) so they round-trip on +// save — a zero DocComments leaves the output without any banner or foot +// blocks. Returns empty string with nil error when no overrides AND no doc +// comments are present. func OverridesToYAML( entries []service.FlatValueEntry, editors []widget.Editor, indent int, tree *yaml.Node, docs service.DocComments, + loadedValues map[string]string, ) (string, error) { - overrides := collectOverrides(entries, editors) + overrides := collectOverrides(entries, editors, loadedValues) if len(overrides) == 0 && docs.Head == "" && docs.Foot == "" && len(docs.Foots) == 0 { return "", nil } @@ -48,7 +80,11 @@ func OverridesToYAML( // into separate OverrideEntry fields so the serializer can write the block // above the key and the inline comment next to the value, matching the style // the user typed. -func collectOverrides(entries []service.FlatValueEntry, editors []widget.Editor) []service.OverrideEntry { +func collectOverrides( + entries []service.FlatValueEntry, + editors []widget.Editor, + loadedValues map[string]string, +) []service.OverrideEntry { count := 0 for i := range entries { @@ -56,7 +92,11 @@ func collectOverrides(entries []service.FlatValueEntry, editors []widget.Editor) break } - if StripYAMLComments(editors[i].Text()) != "" && entries[i].IsFocusable() { + if !entries[i].IsFocusable() { + continue + } + + if entryHasContent(StripYAMLComments(editors[i].Text()), entries[i].Key, loadedValues) { count++ } } @@ -74,12 +114,54 @@ func collectOverrides(entries []service.FlatValueEntry, editors []widget.Editor) raw := editors[i].Text() + // Round-trip fast path: when the raw editor text matches what + // the load step would have written (head comment block followed + // by the loaded value), the user hasn't edited the cell — use + // entry.Comment / entry.Value verbatim. This is essential for + // multi-line literal-block values whose own content starts with + // `#` (e.g. `configuration: |` carrying a redis.conf comment), + // where StripYAMLComments would otherwise eat the literal `#` + // line as if it were a YAML head comment. + loaded, hasLoaded := loadedValues[entry.Key] + if hasLoaded && raw == loadFormForEditor(entry.Comment, loaded) { + if !entryHasContent(loaded, entry.Key, loadedValues) { + // Mirrors entryHasContent's "drop empty cell with no + // loaded content" rule. The fast path can't bypass it. + continue + } + + result = append(result, service.OverrideEntry{ + Key: entry.Key, + Value: loaded, + Type: entry.Type, + HeadComment: entry.Comment, + }) + + continue + } + val := StripYAMLComments(raw) - if val == "" || !entry.IsFocusable() { + if !entry.IsFocusable() || !entryHasContent(val, entry.Key, loadedValues) { continue } - cleanVal, inline := SplitInlineComment(val) + // SplitInlineComment is ambiguous when the value itself contains + // " #" (e.g. a quoted YAML scalar like `"s3cr3t # not really a + // comment"`). When the editor text matches the value as parsed from + // the loaded file, the user hasn't edited the cell and we use the + // full text as the value (no split). Splitting in that case would + // mis-cut the literal '#' out of the value, break the surrounding + // YAML alias on save, and produce a phantom inline comment. + var ( + cleanVal string + inline string + ) + + if hasLoaded && val == loaded { + cleanVal = val + } else { + cleanVal, inline = SplitInlineComment(val) + } result = append(result, service.OverrideEntry{ Key: entry.Key, @@ -93,6 +175,58 @@ func collectOverrides(entries []service.FlatValueEntry, editors []widget.Editor) return result } +// loadFormForEditor reconstructs the exact text the load step writes into an +// editor cell for (comment, value): each comment line prefixed `# ` and +// terminated by `\n`, then value verbatim. Mirror of +// ui/page.formatCommentForEditor — duplicated here so collectOverrides can +// detect "user hasn't edited this cell" without crossing the page→state +// boundary. Empty comment yields just value (no leading `# ...\n`). +func loadFormForEditor(comment, value string) string { + if comment == "" { + return value + } + + var b strings.Builder + + b.Grow(len(comment) + len(value) + commentLineOverheadGuess) + + for _, line := range strings.Split(comment, "\n") { + b.WriteString("# ") + b.WriteString(line) + b.WriteByte('\n') + } + + b.WriteString(value) + + return b.String() +} + +// commentLineOverheadGuess is the per-line overhead Builder.Grow reserves — +// "# " plus newline. Tuned for the typical single-line comment block. +const commentLineOverheadGuess = 4 + +// entryHasContent reports whether collectOverrides should treat the cell as a +// real entry to round-trip on save. +// +// A non-empty stripped editor is always content. An empty stripped editor is +// only treated as content when the key is present in loadedValues with an +// empty loaded value — i.e. the source file had this key explicitly set to +// `""` / `null` / `~`. Without this round-trip path, PatchNodeTree's +// deletion phase drops the leaf, even though the source file had it. +// +// All other empty-editor cases — chart defaults the loaded file doesn't +// override, unfilled cells in a fresh column, user-cleared overrides — +// return false and the entry is dropped. +func entryHasContent(strippedEditor, key string, loadedValues map[string]string) bool { + if strippedEditor != "" { + return true + } + + loaded, ok := loadedValues[key] + + return ok && loaded == "" +} + // StripYAMLComments removes leading lines starting with # from editor text, // returning only the value portion. If the text contains only comment lines, // an empty string is returned. diff --git a/ui/state/editor_state_test.go b/ui/state/editor_state_test.go new file mode 100644 index 0000000..63bbaaa --- /dev/null +++ b/ui/state/editor_state_test.go @@ -0,0 +1,348 @@ +package state + +import ( + "testing" + + "gioui.org/widget" + + "github.com/qdeck-app/qdeck/service" +) + +// typeNull / typeString mirror the canonical values the service package +// stores in FlatValueEntry.Type. Test-only flat keys are also lifted into +// constants here so goconst stays quiet. +const ( + typeNull = "null" + typeString = "string" + + flatKeyImageRegistry = "global.imageRegistry" + flatKeyUsePasswordFls = "auth.usePasswordFiles" + flatKeyImageTag = "image.tag" + flatKeyImageTagValue = "1.2.3" + flatKeyAuthPwd = "auth.password" +) + +// TestCollectOverrides_HashInsideStringNotSplit covers the bug where a YAML +// value containing a literal `#` (e.g. `"s3cr3t # not really a comment"`) +// was being mis-split into value + inline comment by SplitInlineComment on +// save, breaking the surrounding YAML alias and producing a phantom inline +// comment. When the editor text matches the loaded entry's Value verbatim +// (no edit) the splitter must be skipped. +func TestCollectOverrides_HashInsideStringNotSplit(t *testing.T) { + t.Parallel() + + const literal = "s3cr3t # not really a comment" + + entries := []service.FlatValueEntry{ + {Key: flatKeyAuthPwd, Value: literal, Type: typeString, Kind: service.EntryKindLeaf}, + } + + editors := make([]widget.Editor, 1) + editors[0].SetText(literal) + + loaded := map[string]string{flatKeyAuthPwd: literal} + + overrides := collectOverrides(entries, editors, loaded) + + if len(overrides) != 1 { + t.Fatalf("expected 1 override, got %d", len(overrides)) + } + + if overrides[0].Value != literal { + t.Errorf("Value got split: want %q, got %q", literal, overrides[0].Value) + } + + if overrides[0].LineComment != "" { + t.Errorf("phantom LineComment: want empty, got %q", overrides[0].LineComment) + } +} + +// TestCollectOverrides_RoundTripEmptyAndNullValues verifies that legitimately +// empty YAML values (`""`, `null`/`~`) survive an unedited save. Without this +// path, collectOverrides drops every entry whose stripped editor text is +// empty — and the deletion phase in PatchNodeTree then removes the leaf, +// even when the source file had the key with an explicit empty value. +func TestCollectOverrides_RoundTripEmptyAndNullValues(t *testing.T) { + t.Parallel() + + entries := []service.FlatValueEntry{ + {Key: flatKeyImageRegistry, Value: "", Type: typeString, Kind: service.EntryKindLeaf}, + {Key: flatKeyUsePasswordFls, Value: "", Type: typeNull, Kind: service.EntryKindLeaf}, + } + + editors := make([]widget.Editor, 2) + // Loaded editor text mirrors what formatCommentForEditor produces for + // these cases — empty Value with no head comment yields empty editor text. + editors[0].SetText("") + editors[1].SetText("") + + loaded := map[string]string{ + flatKeyImageRegistry: "", + flatKeyUsePasswordFls: "", + } + + overrides := collectOverrides(entries, editors, loaded) + + if len(overrides) != 2 { + t.Fatalf("expected 2 overrides, got %d", len(overrides)) + } + + keys := make([]string, len(overrides)) + for i, o := range overrides { + keys[i] = o.Key + } + + if !contains(keys, flatKeyImageRegistry) { + t.Errorf("empty-string entry dropped: %v", keys) + } + + if !contains(keys, flatKeyUsePasswordFls) { + t.Errorf("null entry dropped: %v", keys) + } +} + +// TestCollectOverrides_EmptyEditorWithNonEmptyDefaultDropped verifies the +// inverse: when the user clears a cell that had a non-empty loaded value, +// the entry IS dropped (matching the prior unfilled-override-cell semantics). +// Without this, every chart default would round-trip as an override even +// when the user never typed anything. +func TestCollectOverrides_EmptyEditorWithNonEmptyDefaultDropped(t *testing.T) { + t.Parallel() + + entries := []service.FlatValueEntry{ + {Key: flatKeyImageTag, Value: flatKeyImageTagValue, Type: typeString, Kind: service.EntryKindLeaf}, + } + + editors := make([]widget.Editor, 1) + editors[0].SetText("") + + // loadedValues has a non-empty value for image.tag — empty editor + // against a non-empty loaded value reads as "user cleared this", which + // should still drop the entry. + loaded := map[string]string{flatKeyImageTag: flatKeyImageTagValue} + + overrides := collectOverrides(entries, editors, loaded) + + if len(overrides) != 0 { + t.Errorf("cleared cell should drop entry; got %d overrides", len(overrides)) + } +} + +// TestCollectOverrides_ChartMergedAliasPreserved simulates the bug where the +// unified entries list mixes chart defaults with a loaded override file. The +// chart's default for an aliased key (`auth.password`) holds a different value +// than what the loaded file resolved the alias to. Without the loadedValues +// signal, the editor text (loaded value) doesn't match entries[i].Value (chart +// default), the splitter cuts on " #", and PatchNodeTree breaks the alias. +// +// With loadedValues passed through, the comparison happens against the loaded +// value — which matches the editor text — so the splitter is skipped. +func TestCollectOverrides_ChartMergedAliasPreserved(t *testing.T) { + t.Parallel() + + const ( + loadedAliased = "s3cr3t # not really a comment" + chartDefault = "supersecret" + ) + + entries := []service.FlatValueEntry{ + // entries[i].Value is the chart default (RebuildUnifiedEntries + // keeps defaults' Value when both sources have the key). + {Key: flatKeyAuthPwd, Value: chartDefault, Type: typeString, Kind: service.EntryKindLeaf}, + } + + editors := make([]widget.Editor, 1) + editors[0].SetText(loadedAliased) + + // loadedValues reflects what the user's loaded file actually contains + // at this key (the alias-resolved value). + loaded := map[string]string{flatKeyAuthPwd: loadedAliased} + + overrides := collectOverrides(entries, editors, loaded) + + if len(overrides) != 1 { + t.Fatalf("expected 1 override, got %d", len(overrides)) + } + + if overrides[0].Value != loadedAliased { + t.Errorf("alias-aware comparison failed: want %q, got %q", loadedAliased, overrides[0].Value) + } + + if overrides[0].LineComment != "" { + t.Errorf("phantom LineComment: %q", overrides[0].LineComment) + } +} + +// TestCollectOverrides_EmptyDefaultDropped verifies the chart-default leak +// fix: when an entry has empty editor text AND the key is NOT in the +// loaded-values map (e.g. a chart default the user's file doesn't override), +// the entry is dropped — without this, every empty/null chart default would +// flood the saved file as a phantom override. +func TestCollectOverrides_EmptyDefaultDropped(t *testing.T) { + t.Parallel() + + entries := []service.FlatValueEntry{ + {Key: "auth.acl.userSecret", Value: "", Type: typeString, Kind: service.EntryKindLeaf}, + {Key: "master.persistence.medium", Value: "", Type: typeString, Kind: service.EntryKindLeaf}, + } + + editors := make([]widget.Editor, 2) + editors[0].SetText("") + editors[1].SetText("") + + // loadedValues has neither key — these are chart defaults the loaded + // file doesn't touch. + loaded := map[string]string{"some.other.key": "x"} + + overrides := collectOverrides(entries, editors, loaded) + + if len(overrides) != 0 { + t.Errorf("chart defaults shouldn't leak into save; got %d overrides", len(overrides)) + } +} + +func contains(ss []string, want string) bool { + for _, s := range ss { + if s == want { + return true + } + } + + return false +} + +// TestCollectOverrides_MultiLineLiteralBlockPreservesHashContent guards the +// editor-text round-trip for multi-line literal-block values whose own +// content starts with `#` — e.g. a `configuration: |` cell carrying a +// pasted redis.conf block. StripYAMLComments greedily eats every leading +// line that starts with `#` from the editor text, so without the +// raw==loadForm fast path it would consume the literal first line of the +// block as a YAML head comment, then PatchNodeTree would write the +// truncated value back AND hoist the eaten line to a YAML head comment +// outside the literal block. +// +// The expected behaviour: when the editor text equals exactly what the +// load step wrote (head comment block, then the loaded value), the saved +// entry uses entry.Comment / entry.Value verbatim with no stripping. +func TestCollectOverrides_MultiLineLiteralBlockPreservesHashContent(t *testing.T) { + t.Parallel() + + const ( + key = "master.configuration" + // Literal-block content — note both leading and mid lines start + // with `#`. These are part of the string, not YAML comments. + literalValue = "# Pasted verbatim from redis.conf.\n" + + "maxmemory-policy allkeys-lru\n" + + "# Comment inside a block scalar is part of the string, not YAML.\n" + + "appendonly yes\n" + + "save \"\"\n" + headComment = "Block literal: preserves every newline and comment inside exactly." + ) + + entries := []service.FlatValueEntry{ + {Key: key, Value: literalValue, Type: typeString, Comment: headComment, Kind: service.EntryKindLeaf}, + } + + // Editor text mirrors what populateColumnOverrides → resolveCustomValueWithComments + // writes: "# {comment}\n{value}" via formatCommentForEditor. + loadForm := "# " + headComment + "\n" + literalValue + + editors := make([]widget.Editor, 1) + editors[0].SetText(loadForm) + + loaded := map[string]string{key: literalValue} + + overrides := collectOverrides(entries, editors, loaded) + + if len(overrides) != 1 { + t.Fatalf("expected 1 override, got %d", len(overrides)) + } + + if overrides[0].Value != literalValue { + t.Errorf("literal-block content lost or truncated:\ngot %q\nwant %q", overrides[0].Value, literalValue) + } + + if overrides[0].HeadComment != headComment { + t.Errorf("head comment garbled:\ngot %q\nwant %q", overrides[0].HeadComment, headComment) + } + + if overrides[0].LineComment != "" { + t.Errorf("phantom LineComment: %q", overrides[0].LineComment) + } +} + +// TestRebuildUnifiedEntries_CustomOnlyEmptyContainerPreserved is the +// regression for the custom-only empty-container drop. RebuildUnifiedEntries +// blanks Value on custom-only entries so the defaults-side cell renders +// empty — but for empty-container leaves (`Value="{}"` / `Value="[]"`) that +// blanking turns the row into a section-shaped entry (`Value=""` with +// typeMap/typeList), which IsSection() then misclassifies as a populated +// section header. collectOverrides drops sections, and PatchNodeTree's +// deletion phase removes the leaf from the saved file. +// +// The fix preserves Value when it's the empty-container placeholder; this +// test guards both the typeMap and typeList shapes. +func TestRebuildUnifiedEntries_CustomOnlyEmptyContainerPreserved(t *testing.T) { + t.Parallel() + + const ( + mapKey = "cornerCases.quotedKeys.extraPolicies" + listKey = "cornerCases.sequences.flowEmpty" + ) + + s := &ValuesPageState{ + DefaultValues: &service.FlatValues{ + Entries: []service.FlatValueEntry{ + {Key: "image.tag", Value: flatKeyImageTagValue, Type: typeString, Kind: service.EntryKindLeaf}, + }, + }, + ColumnCount: 1, + } + + s.Columns[0].CustomValues = &service.FlatValues{ + Entries: []service.FlatValueEntry{ + {Key: mapKey, Value: emptyMapValue, Type: "map", Kind: service.EntryKindLeaf}, + {Key: listKey, Value: emptyListValue, Type: "list", Kind: service.EntryKindLeaf}, + }, + } + + s.RebuildUnifiedEntries() + + var ( + mapEntry *service.FlatValueEntry + listEntry *service.FlatValueEntry + ) + + for i := range s.Entries { + switch s.Entries[i].Key { + case mapKey: + mapEntry = &s.Entries[i] + case listKey: + listEntry = &s.Entries[i] + } + } + + if mapEntry == nil { + t.Fatalf("custom-only empty map dropped from unified entries") + } + + if listEntry == nil { + t.Fatalf("custom-only empty list dropped from unified entries") + } + + if mapEntry.Value != emptyMapValue { + t.Errorf("empty-map placeholder lost: got Value=%q, want %q", mapEntry.Value, emptyMapValue) + } + + if listEntry.Value != emptyListValue { + t.Errorf("empty-list placeholder lost: got Value=%q, want %q", listEntry.Value, emptyListValue) + } + + if mapEntry.IsSection() { + t.Errorf("empty-container leaf misclassified as section: %+v", *mapEntry) + } + + if listEntry.IsSection() { + t.Errorf("empty-container leaf misclassified as section: %+v", *listEntry) + } +} diff --git a/ui/state/values_state.go b/ui/state/values_state.go index 3b8fc6e..86b7039 100644 --- a/ui/state/values_state.go +++ b/ui/state/values_state.go @@ -63,6 +63,15 @@ func (c *CustomColumnState) DocCommentsForSave() service.DocComments { const helmInstallPrefix = "helm install" +// emptyMapValue / emptyListValue mirror the canonical placeholder Values +// flattenValues emits for empty mapping / sequence leaves. Duplicated here +// so the unified-entry merge can detect the leaf-shaped empty-container +// rows without exposing the service package's full value-type taxonomy. +const ( + emptyMapValue = "{}" + emptyListValue = "[]" +) + // CustomColumnState holds per-column widget state for an editable override column. type CustomColumnState struct { // Values data @@ -551,8 +560,21 @@ func (s *ValuesPageState) RebuildUnifiedEntries() (prev []service.FlatValueEntry continue } + // Carry the original Value through for empty-container leaves + // (Value="{}" / "[]"). Without this, IsSection() on the unified + // row would classify them as section headers (Value=="" with + // typeMap/typeList), collectOverrides would filter them out, and + // PatchNodeTree's deletion phase would drop the empty-container + // leaf from the saved file. Populated sections still arrive with + // Value="" from flattenValues, so they keep IsSection() == true. + value := "" + if e.Value == emptyMapValue || e.Value == emptyListValue { + value = e.Value + } + customOnly[e.Key] = service.FlatValueEntry{ Key: e.Key, + Value: value, Type: e.Type, Depth: e.Depth, Comment: e.Comment, diff --git a/ui/widget/flat_key.go b/ui/widget/flat_key.go index 345779e..6f04f88 100644 --- a/ui/widget/flat_key.go +++ b/ui/widget/flat_key.go @@ -1,30 +1,19 @@ package widget -import "strings" +import "github.com/qdeck-app/qdeck/domain" -// lastSegment returns the portion after the last dot in a dot-separated flat -// key. Used by the override table to derive the leaf label shown in the key -// column. +// lastSegment returns the portion after the last unescaped dot in a flat key, +// decoded back to its literal form. Used by the override table to derive the +// leaf label shown in the key column. func lastSegment(key string) string { - idx := strings.LastIndexByte(key, '.') - - if idx < 0 { - return key - } - - return key[idx+1:] + return domain.FlatKey(key).LastSegment() } -// parentPath returns the portion before the last dot, or "" for root-level -// keys. Unlike domain.FlatKey.Parent this splits only on '.', which matches -// what the override table uses for grouping rows by their enclosing mapping -// (list-header levels aren't shown as separate rows there). +// parentPath returns the portion before the last unescaped dot, or "" for +// root-level keys. Unlike domain.FlatKey.Parent this splits only on '.', not +// on '[', which matches what the override table uses for grouping rows by +// their enclosing mapping (list-header levels aren't shown as separate rows +// there). func parentPath(key string) string { - idx := strings.LastIndexByte(key, '.') - - if idx < 0 { - return "" - } - - return key[:idx] + return string(domain.FlatKey(key).ParentMapPath()) } diff --git a/ui/widget/override_table_anchors.go b/ui/widget/override_table_anchors.go index fc4c579..b2f45bf 100644 --- a/ui/widget/override_table_anchors.go +++ b/ui/widget/override_table_anchors.go @@ -17,6 +17,7 @@ import ( "gioui.org/widget" "gioui.org/widget/material" + "github.com/qdeck-app/qdeck/domain" "github.com/qdeck-app/qdeck/service" "github.com/qdeck-app/qdeck/ui/state" "github.com/qdeck-app/qdeck/ui/theme" @@ -215,7 +216,7 @@ func appendAnchorMemberships( out = append(out, anchorMembership{ name: info.Name, - depth: strings.Count(k, "."), + depth: domain.FlatKey(k).Depth() - 1, source: source, }) } diff --git a/ui/widget/override_table_events.go b/ui/widget/override_table_events.go index c9faac1..9bc9af6 100644 --- a/ui/widget/override_table_events.go +++ b/ui/widget/override_table_events.go @@ -129,8 +129,9 @@ func (t *OverrideTable) commitOverrideUpdate( indent := service.DefaultYAMLIndent var ( - tree *yaml.Node - docs service.DocComments + tree *yaml.Node + docs service.DocComments + loadedValues map[string]string ) if cs := t.ColumnStates[c]; cs != nil { @@ -138,10 +139,11 @@ func (t *OverrideTable) commitOverrideUpdate( if cs.CustomValues != nil { tree = cs.CustomValues.NodeTree docs = cs.DocCommentsForSave() + loadedValues = state.LoadedValuesMap(cs.CustomValues) } } - yamlText, yamlErr := state.OverridesToYAML(entries, editors, indent, tree, docs) + yamlText, yamlErr := state.OverridesToYAML(entries, editors, indent, tree, docs, loadedValues) t.OnChanged(c, yamlText, yamlErr) }