diff --git a/domain/values.go b/domain/values.go index 963a1d7..2e8f0d4 100644 --- a/domain/values.go +++ b/domain/values.go @@ -31,6 +31,11 @@ type ValuesFile struct { // Used to synthesize comment-only rows in the flattened entry list, and to // round-trip the text on save. nil when the source had none. FootComments map[string]string + + // RawBytes is the verbatim source captured at single-file load; nil for + // chart defaults, multi-file merges, and editor parse. Drives the + // byte-faithful save path when present. + RawBytes []byte } // ValuesEntry is a single key-value pair from a flattened YAML structure. diff --git a/service/FORMAT_PRESERVATION.md b/service/FORMAT_PRESERVATION.md new file mode 100644 index 0000000..9ecc575 --- /dev/null +++ b/service/FORMAT_PRESERVATION.md @@ -0,0 +1,200 @@ +# Source format preservation on save + +`gopkg.in/yaml.v3` is the only sane YAML library for Go, but it is a parse and re emit library, not a round trip library. Parsing the file into `yaml.Node`, then emitting that tree, normalizes everything: + +1. Blank lines between mapping entries are dropped. yaml.v3 does not track inter node whitespace at all. +2. Inline comment column alignment collapses. The source `cpu: 100m # cores` re emits as `cpu: 100m # cores`. +3. Folded scalars `>` get their logical value joined to a single line. yaml.v3 remembers what the value means but not where the user broke it across lines. +4. The merge key `<<:` gets an explicit `!!merge` tag prefix that the user never typed. +5. `!!set` collections lose the explicit key form. `? red` becomes `red:` because yaml.v3's emitter prefers the implicit key form. +6. `!!binary |` block scalars gain an extra blank line between the indicator and the body. +7. Per block indent variation is normalized to one file wide indent. +8. Inline comments on null leaves get dropped entirely (`key: ~ # explicit null` becomes `key: ~`). +9. Empty container placeholders (`key: []`, `key: {}`) sometimes get dropped from the override list and never re emitted. + +A clean save through plain yaml.v3 would produce a file with all nine of these normalizations applied, even on a save with zero user edits. + +## Architectural overview + +`PatchSourceText` is the save pipeline entry point. The controller calls it with the source bytes captured at load time, the parsed `yaml.Node` tree, the override entries the user wants to persist, and the doc level comment bundle. + +The function decides between three save paths in order of fidelity: + +1. **Byte verbatim short circuit.** When the column has no edits at all and the source bytes are still available, the controller writes the original bytes back to disk without any parsing or emitting. Zero diff guaranteed. This sits one level above `PatchSourceText` in the controller because it skips the entire service layer. + +2. **Leaf line splice.** For pure value edits to plain scalars, `planScalarSplice` walks the override entries, confirms every entry is safe to splice, and `SpliceScalarValues` rewrites just the value portion of each changed line. The rest of the file passes through byte for byte. One diff line per edit. This is the common case for any save where the user simply changed a value or two. + +3. **Encoder fallback with restoration.** When any entry blocks the splice (new keys, removed keys, comment edits, quoted or block scalar edits, alias target edits, container shape changes, nullify markers, or doc level comment edits) the pipeline falls through to the full `PatchNodeTree` encoder. Its output is then post processed by `PreserveSourceFormatting` to claw back as many bytes of source formatting as possible. The output is not byte identical with source for unchanged keys, but it is close. + +## Path 1: byte verbatim short circuit + +`onSaveColumnValues` checks four gates before doing anything else: + +1. `col.ValuesModified` is false (no editor change has fired). +2. `col.CustomValues.RawBytes` is non empty (the source bytes were retained at load). +3. `col.CustomFilePaths` has exactly one element (not a Save As, not a multi file merge). +4. `col.MergedFileCount` is one or less (single file load). + +If all four hold, `saveRawBytesToFile` writes `RawBytes` to the path without any further processing. `EncodeForFile` is deliberately skipped because the BOM and line endings are already baked into the source bytes. Routing through it would double the BOM and rewrite each already CRLF separator to `\r\r\n`. + +The external mtime check still runs before the short circuit fires, so if the file changed on disk since load the user gets the overwrite dialog. The dialog confirm path stores the raw bytes string in `overwritePendingYAML` and falls through the normal save handler, which is acceptable because the user explicitly chose to overwrite. + +## Path 2: leaf line splice + +`SpliceScalarValues` rewrites the value portion of one or more plain scalar leaves in raw bytes, preserving every other byte. Blank lines, inline comment column alignment, scalar styles, anchors, aliases, per block indent: all survive byte identical because the rest of the file is never re emitted. + +The viability check `planScalarSplice` runs first and gates the splice. Every entry in the override list must pass: + +* The key must physically exist in the source tree. New keys cannot be spliced because they have no source position to insert at. +* The target node must be a plain scalar with `Style == 0`. Quoted scalars need quote aware end detection that the simple plain scanner does not implement. Block scalars span multiple lines and need different handling. +* The entry's `HeadComment` and `LineComment` must match the tree's effective comments. A mismatch means a comment edit, which the splice cannot apply. +* The leaf must not be reached through an alias. Editing a value through an alias would silently change the anchor source's value, changing every other use site too. +* The entry must not be a container placeholder or a nullify marker. Container shape changes need the encoder. +* The new value must be plain safe, meaning yaml.v3 would not auto quote it. The check round trips the value through a single node encode and compares. +* `docsMatchSource` must hold: the doc level comment bundle the controller passed must match what `parseOrphanComments` extracts from raw. A mismatch means a banner, trailer, foot, or section head was edited, and the splice has no path to write those changes. + +If any entry fails, splice rejects, all or nothing. The hot path takes the boolean return and discards any diagnostic detail. The companion `describeSpliceBlocker` runs the same logic but formats a human readable description of the first failing check. It is used only by `FirstSpliceBlockerForTest` for cross package test diagnostics; the production save never pays the formatting cost. + +The per leaf splice in `spliceScalarValue` finds the value node, reads its `Line` and `Column`, locates the source line, computes the value end with `plainScalarLineEnd` (which scans forward until a ` #` inline comment marker or end of line), and replaces only the value substring. The leading indent, the key, the colon, the spacing between value and comment, and the comment itself are all preserved verbatim. + +## Path 3: encoder fallback with restoration + +When splice rejects, `PatchNodeTree` runs to produce a full re emitted YAML text. That text gets handed to `PreserveSourceFormatting`, which runs four restoration passes in order. + +Pass order matters. The first pass replaces multi line spans, which invalidates any pre computed line maps. Subsequent passes re parse the encoded output internally so they see the post substitution structure. + +### Strip the spurious `!!merge` tag + +yaml.v3's emitter writes `!!merge <<: *anchor` where the canonical form (and every hand authored source we have seen) writes `<<: *anchor`. A `bytes.ReplaceAll` drops the tag. The parser handles both forms identically, so the user perceives no behavior change, just the source spelling they originally had. + +### Byte range substitution for unchanged block subtrees + +`substituteBlockScalarRanges` handles two classes of nodes that the encoder fundamentally cannot round trip: + +1. **Block scalars** (literal `|`, folded `>`, plus chomp and tag variants like `!!binary |`). The encoder rejoins folded scalars, adds an extra blank between `!!binary |` and its body, and can shift the leading newline of any block scalar by one byte depending on the chomp indicator. + +2. **Tagged collections** (`!!set`, `!!omap`, user defined tags). The `!!set` form is the headline example: source `? red` becomes encoder `red:` because yaml.v3 always prefers the implicit key form for null valued mapping entries. + +For each such node, the pass computes its source byte range using the key node line and the indent based end detection in `refineBlockScalarEnd`. It then locates the same flat key in the encoder output, computes the encoded byte range the same way, and substitutes source bytes verbatim if the node content is structurally unchanged. Scalars compare by Value modulo leading and trailing newlines; non scalars compare by recursive structural equality through `nodesStructurallyEqual`. + +A structural mismatch (the user actually edited the block content) disables substitution for that node, so the user's edit survives. + +The Style bit field needs careful handling. `LiteralStyle` is bit 3, `TaggedStyle` is bit 0, so a `!!binary |` node carries `Style == 9` (bit 0 plus bit 3). Equality checks on Style would miss this, so `isBlockScalarNode` checks via bitmask AND, and `isTaggedCollectionNode` checks Tag against the default tag spellings (`!!map`, `!!seq`, and their URI forms) and treats anything else as substitutable. + +### Line for line substitution from source + +`substituteUnchangedLinesFromSource` is the bulk fidelity recovery pass. For each line in encoded that maps to a flat key (via `lineToFlatKey`), it finds the same flat key's line in source. If the two lines describe the same key, value, and inline comment modulo whitespace (via `linesSemanticallyMatch`), it substitutes the source line verbatim into the encoded output. + +This is what recovers inline comment column alignment for every unchanged leaf in the file. The encoder collapses `cpu: 100m # cores` to `cpu: 100m # cores`; the substitution restores the wide spacing from source. + +The equivalence check has one asymmetric loosening: if the source line has an inline comment but the encoded line lacks one, substitute anyway. This recovers the case where yaml.v3 drops inline comments on null leaves (the encoder emits `usePasswordFiles: ~` for source `usePasswordFiles: ~ # explicit null via tilde`). The loosening is intentionally one directional: an encoder line with content the source lacks is not overwritten, because that content might be an edit. + +Block scalar key lines are explicitly excluded from substitution. The key line `configuration: |` would match, but the body lines below it do not have per line flat keys, so substituting just the key line would leave the body in encoder form which is what Pass B is for. + +### Insert source lines for keys missing from encoded + +`insertMissingSourceLines` recovers single line entries that exist in source but were dropped from the encoder output. The headline case is empty container placeholders like `disableCommands: []`: the flattener emits a row for them so the UI can render them, but `collectOverrides` and `PatchNodeTree`'s container no op path together drop them somewhere between override list construction and encoder emission. + +For each source key that is in `insertableSingleLineKeys` but missing from the encoded `lineToFlatKey` map, the pass anchors on the previous source key that does exist in encoded, and emits the missing source line immediately after that anchor's encoded position. Anchoring on the previous key rather than the next key is critical: the next key approach landed insertions across section boundaries (an empty container at the tail of `auth:` would slip into `commonLabels:`'s first child position). + +Each missing key inserts exactly its own source line. Head comments and blank lines immediately above the missing key are not re attached. That trade off keeps adjacent missing keys from overlapping their source ranges and duplicating lines, which an earlier attempt at full context inclusion did do. + +Flow style mappings and sequences are explicitly skipped during `insertableSingleLineKeys` traversal because they put all entries on one source line. `lineToFlatKey` only records one key per line, so the per entry insertion logic would consider the others missing from encoded and try to insert them, producing nonsense. + +### Restore blank lines + +`restoreBlankLines` is the simplest pass conceptually but the most fragile in practice. It scans source for blank line runs, attributes each run to the previous keyed line as its `afterKey` anchor, and re inserts blanks after the same anchor's line in the encoded output. + +Two refinements keep it from misattributing: + +1. **Region awareness via `blockScalarRegions`.** Blanks inside block scalar bodies and tagged collection bodies are part of the value, not sibling separators. The block scalar and tagged collection regions are computed via `refineBlockScalarEnd` (indent based, YAML spec correct) and `collectBlankRuns` skips blanks inside any region. Tagged collections specifically must not use the sibling heuristic cap for region computation, because their child node lines appear after the collection's line and would mis cap the region at the first child. Block scalars are safe to use the sibling cap because their child node has no Line of its own. + +2. **Key adjacency tracking.** A blank only generates a run when no non blank non key line (typically a top level comment block attached as a HeadComment to some future node) intervenes between the last keyed line and the blank. Without this check, the blank between a top level comment wall and the next major section would get attributed to whichever leaf was most recently keyed, and `restoreBlankLines` would land the inserted blank in the wrong place (often inside a previous block scalar's body). + +The trade off: a user pattern of `# comment for X` followed by a blank followed by `X:` would lose the blank because the comment line breaks adjacency. This pattern is uncommon enough that we accept it. + +## State preservation across editor reparses + +### `RawBytes` carry forward + +Every keystroke in any cell editor fires `commitOverrideUpdate`, which rebuilds the entire column's YAML via `OverridesToYAML` and feeds the result to `ParseEditorContent`. `ParseEditorContent` returns a fresh `*FlatValues` from the rebuilt text. The controller's `pollEditorParse` then replaces `col.CustomValues` with that fresh value. + +`ParseEditorContent` has no canonical source file (it parses arbitrary editor text), so `RawBytes` on its result is always nil. Without explicit carry forward, the first keystroke wipes `col.CustomValues.RawBytes`, and from that point on every save falls through to the encoder path because `PatchSourceText` short circuits the splice when raw is empty. + +The fix: in `pollEditorParse`, after the new `FlatValues` is built but before it replaces `col.CustomValues`, copy `RawBytes` from the outgoing value to the incoming one. The source bytes never change as the user edits, so they survive any number of reparses. + +### Per entry `Comment` carry forward + +The same reparse cycle drops per entry `Comment` fields. The flattener's `attachComments` populates those at load time by parsing the raw bytes with `parseComments`, but `ParseEditorContent` does not run `parseComments` (the editor text is not the source file, and a parse on it might attribute differently). + +When `entry.Comment` is empty for a leaf that had a comment in the source, `loadFormForEditor` returns just the value without any `# ` prefix. The editor still holds its original text (with the `# ` prefix from load time), so the equality check in `collectOverrides`'s fast path fails. The slow path then runs, and `StripYAMLComments` greedily eats every line that starts with `#` at the start of the editor text, treating them all as head comments. + +For a single line value this is harmless. For a multi line block scalar whose body legitimately contains `#` lines (like `configuration: |` carrying a redis config snippet), this is catastrophic: every leading body line that starts with `#` gets stripped from the value and promoted to the entry's `HeadComment`. PatchNodeTree then writes the corrupted value into the in memory tree and the encoder emits a file where the `#` content lines now appear above the block scalar key instead of inside the body. + +The fix: in `pollEditorParse`, build a `key to Comment` map from the outgoing `col.CustomValues.Entries` and apply it to the incoming entries. Comments come from the source file, not the edited editor text, so the load time values remain authoritative. + +### `LoadAndMergeCustomValues` for single file loads + +The controller's load path always goes through `LoadAndMergeCustomValues`, even for single file opens. `ReadAndMergeCustomValues` intentionally leaves `RawBytes` nil because a multi file merge has no canonical source bytes. + +For single file loads this disabled the splice path globally. Service level tests passed because they called `ReadCustomValues` directly which does retain `RawBytes`; the production path diverged silently. + +The fix: in `ReadAndMergeCustomValues`, when the input path list has exactly one entry, read and retain that file's bytes on `vf.RawBytes`. Multi file merges still leave it nil so the encoder fallback runs. + +The e2e test now exercises `LoadAndMergeCustomValues` specifically to lock this in. + +## Tagged scalar precision in `ParseEditorContent` + +`ParseEditorContent` originally went through `chartutil.ReadValues` which routes through `sigs.k8s.io/yaml`. That library loses precision on tagged scalars: `!!binary` is decoded silently, `!!int "9007199254740993"` rounds to float64, large integers lose their literal text, and tagged set members are normalized. + +After the keystroke reparse, `col.CustomValues.NodeTree` is patched back from the load time tree (it carries source line positions and the literal scalar text yaml.v3 captured). But the per entry `Value` fields come from `ParseEditorContent`, which used the lossy decoder. + +For every tagged scalar in the file (the redis fixture has `!!binary`, `!!int "3"`, `!!str "42"`, several others), the entry Value disagreed with what `findEffectiveScalar` returned for the same path in the tree. `planScalarSplice` saw this disagreement as "value changed" and rejected the splice on every save. + +The fix: in `ParseYAMLText` (the shared parser under `ParseEditorContent`), re parse the same text through yaml.v3 directly, plant the result on `vf.NodeTree`, and run `rewriteValuesFromNodeTree` to swap each entry's Value with the literal scalar text from the new tree. The chartutil path keeps running for compatibility with the rest of the system, but the canonical scalar text comes from yaml.v3 and matches what the load time tree carries. + +## The bool switch add scenario + +A bool typed cell in the UI shows a switch widget. When the user toggles a switch on a chart only key (a key that exists in the chart defaults but the user has not yet overridden), `layoutBoolSwitchCell` writes the chart's default value into the override editor. This is intentional UX: the user sees the inherited value and can toggle without first typing anything. + +The save side effect: this looks like an add operation. The flat key goes through `collectOverrides` as a new override; the encoder inserts it into the tree at its conventional position; the encoder output has lines for it that source does not have. + +For splice this is fatal: `planScalarSplice` rejects on `findEffectiveScalar hasValue == false` for the new key. The encoder fallback runs. + +The four restoration passes in `PreserveSourceFormatting` were designed to make this fallback acceptable. The encoder writes the new key in its emitted position; the line for line substitution restores every other leaf to source form; the block range substitution restores any unchanged tagged or block scalars; the blank line restore re inserts source blanks. The user's resulting diff is the new key (two or three lines, depending on how nested the parent is) plus a small fixed residual of cosmetic differences (one head comment we cannot easily re attach, one trailing blank, one sequence comment relocation by one position). + +## Block scalar content as comment bug + +yaml.v3's parser treats `#` at a key column indent as a YAML comment, regardless of whether it lexically sits inside a block scalar body. For the redis fixture's `master.configuration: |` block, the source line ` # Pasted verbatim from redis.conf.` at column 4 is parsed as a HeadComment of the next mapping pair rather than as block scalar content. + +On clean re emit, the encoder writes the comment above `configuration:` and the block body no longer starts with that line. Subsequent saves preserve the corrupted form because parsing the corrupted file gives a HeadComment that matches the prior emission. + +This is the only sticky corruption in the pipeline. Once a file has been saved through a buggy intermediate, the comment placement does not self heal. The fix path requires reverting the file from git. The byte range substitution in Pass B specifically guards against introducing this corruption from a clean input: the block scalar's source bytes (including the `#` content line) are spliced verbatim, bypassing the parser's misattribution entirely. + +## BOM and CRLF round trip + +`EncodeForFile` prepends the UTF 8 BOM when the source file had one, and rewrites `\n` to `\r\n` when the source used CRLF. It is meant to run on yaml.v3 encoder output (which always emits UTF 8 LF). + +When `PatchSourceText` returns raw source bytes verbatim (either through the no edit short circuit or through the splice path), those bytes already carry the BOM and CRLF. Routing them through `EncodeForFile` doubles the BOM and rewrites each `\r\n` to `\r\r\n`, producing a corrupted file. + +The fix runs at two layers: + +1. The no edit short circuit in the controller writes raw bytes via `SaveRawBytes`, which skips `EncodeForFile` entirely. + +2. The splice path returns bytes that already carry the source's BOM and CRLF, so passing them through `EncodeForFile` with the column's source encoding labels would double encode. The `TestPatchSourceText_BOM_CRLF_RoundTripsThroughEncodeForFile` test in `yaml_format_test.go` pins this contract for both paths so any future regression at this seam fails loudly. + +## Known limitations + +The pipeline does not achieve byte identical round trip on every fixture. The remaining unintended diff lines on a clean save of the cornercase fixture with one added key are: + +1. **One head comment** for `auth.extraPolicies` is lost. `insertMissingSourceLines` re emits the empty container line but not the head comment that sits above it in source. Restoring the comment requires tracking head comment line ranges per insertable key and including them in the insertion span without overlapping adjacent missing keys' ranges. + +2. **One sequence comment** (`# - DEBUG` in `disableCommands`) shifts by one source position. yaml.v3 attaches the comment to a different node than where it lexically sits, and the encoder places it on the post attachment side of the blank line that separates it from the previous sequence item. + +3. **One trailing blank line** appears at the end of the `cornerCases.taggedCollections` subtree. The encoder writes one, `restoreBlankLines` writes another, and the deduplication does not currently detect the overlap. + +4. **Undo to original is not detected.** Typing a value, then typing the original back, leaves `ValuesModified` set to true. The encoder path runs even though the result would be source identical. A snapshot and diff at save time would catch this but the complexity is not warranted given the encoder fallback's restoration passes produce near identical output anyway. + +5. **A genuine block scalar body edit can lose the substitution.** `substituteBlockScalarRanges` compares scalar values modulo leading and trailing newlines but otherwise demands exact match. A user edit that touches a `|` block body (rare through the table UI, which does not surface block scalars as edit cells) will disable substitution for that block, falling back to encoder output for the body. + +6. **Doc level comment edits combined with value edits.** The splice rejects when `docsMatchSource` fails, but the encoder fallback writes doc comments through `applyDocFoots` and friends. The combination works, but the result is encoder formatted for the rest of the file too, so the value edit no longer benefits from the splice's byte fidelity. diff --git a/service/values_service.go b/service/values_service.go index 28ac5ca..a90d0c8 100644 --- a/service/values_service.go +++ b/service/values_service.go @@ -212,10 +212,15 @@ func (s *ValuesService) ReadCustomValues(ctx context.Context, filePath string) ( vf.FootComments = oc.Foots } - var doc yaml.Node - if parseErr := yaml.Unmarshal(rawData, &doc); parseErr == nil && len(doc.Content) > 0 { - vf.NodeTree = doc.Content[0] - } + // Retain source bytes so an unmodified save writes them back verbatim, + // preserving blanks the encoder would normalize. Only the single-file + // on-disk read populates this; merge and editor-parse leave it nil. + vf.RawBytes = rawData + } + + var doc yaml.Node + if err := yaml.Unmarshal(rawData, &doc); err == nil && len(doc.Content) > 0 { + vf.NodeTree = doc.Content[0] } rewriteValuesFromNodeTree(vf) @@ -280,6 +285,9 @@ func (s *ValuesService) ReadAndMergeCustomValues(ctx context.Context, paths []st // successful read. firstEnc = EncodingUTF8 firstEOL = LineEndingLF + // Retained for the splice path when len(paths)==1; multi-file merges + // leave it nil — no canonical source to splice against. + singleFileRaw []byte ) for i, path := range paths { @@ -298,6 +306,10 @@ func (s *ValuesService) ReadAndMergeCustomValues(ctx context.Context, paths []st if i == 0 { indent = DetectYAMLIndent(rawData) firstEnc, firstEOL = DetectFileEncoding(rawData) + + if len(paths) == 1 { + singleFileRaw = rawData + } } if comments, parseErr := parseComments(rawData); parseErr == nil { @@ -336,6 +348,7 @@ func (s *ValuesService) ReadAndMergeCustomValues(ctx context.Context, paths []st vf.FootComments = lastFoots vf.Encoding = firstEnc vf.LineEnding = firstEOL + vf.RawBytes = singleFileRaw attachComments(vf, mergedComments) rewriteValuesFromNodeTree(vf) @@ -368,6 +381,13 @@ func (s *ValuesService) ParseYAMLText(ctx context.Context, yamlText string) (*do vf := flattenValues("editor", vals) vf.RawValues = vals + var doc yaml.Node + if err := yaml.Unmarshal([]byte(yamlText), &doc); err == nil && len(doc.Content) > 0 { + vf.NodeTree = doc.Content[0] + } + + rewriteValuesFromNodeTree(vf) + return vf, nil } @@ -422,6 +442,21 @@ func (s *ValuesService) SaveValuesFile( return nil } +// SaveRawBytes writes raw verbatim to destPath, bypassing EncodeForFile. +// The bytes already carry the source BOM and line endings; routing through +// EncodeForFile would double the BOM and turn \r\n into \r\r\n. +func (s *ValuesService) SaveRawBytes(ctx context.Context, raw []byte, destPath string) error { + if ctx.Err() != nil { + return fmt.Errorf("save raw bytes: %w", ctx.Err()) + } + + if err := os.WriteFile(destPath, raw, saveFilePerm); err != nil { + return fmt.Errorf("save raw bytes to %s: %w", destPath, err) + } + + return nil +} + // CompareWithBaseline compares a current values file against baseline content (e.g. from git HEAD). // Returns a map of flat keys to their change status. Only added/modified keys are included. func (s *ValuesService) CompareWithBaseline( @@ -602,6 +637,43 @@ func newFlatValues(vf *domain.ValuesFile) *FlatValues { KeyPositions: buildFlatKeyPositions(vf.NodeTree), Encoding: vf.Encoding, LineEnding: vf.LineEnding, + RawBytes: vf.RawBytes, + } +} + +// MergeLoadedMetadata copies load-time metadata from loaded onto parsed +// (which ParseEditorContent leaves zero on those fields). Per-entry Comments +// are matched by FlatKey; loaded entries with empty Comment are skipped so +// the parsed side keeps any newly-typed comment text. +func MergeLoadedMetadata(parsed, loaded *FlatValues) { + if parsed == nil || loaded == nil { + return + } + + parsed.Indent = loaded.Indent + parsed.NodeTree = loaded.NodeTree + parsed.Anchors = loaded.Anchors + parsed.DocHeadComment = loaded.DocHeadComment + parsed.DocFootComment = loaded.DocFootComment + parsed.FootComments = loaded.FootComments + parsed.RawBytes = loaded.RawBytes + + if len(loaded.Entries) == 0 { + return + } + + loadedComments := make(map[string]string, len(loaded.Entries)) + + for _, e := range loaded.Entries { + if e.Comment != "" { + loadedComments[e.Key] = e.Comment + } + } + + for i := range parsed.Entries { + if c, ok := loadedComments[parsed.Entries[i].Key]; ok { + parsed.Entries[i].Comment = c + } } } diff --git a/service/values_types.go b/service/values_types.go index 38b3cda..1262bca 100644 --- a/service/values_types.go +++ b/service/values_types.go @@ -71,6 +71,10 @@ type FlatValues struct { // NodeTree was unavailable. Domain-layer Entries stays alphabetical so // ComputeDiff's two-pointer merge keeps working. KeyPositions map[string]int + + // RawBytes mirrors domain.ValuesFile.RawBytes — verbatim source for the + // byte-faithful save path; nil otherwise. + RawBytes []byte } // EntryKind identifies the role of a FlatValueEntry. Most entries are leaves diff --git a/service/yaml_blank_lines.go b/service/yaml_blank_lines.go new file mode 100644 index 0000000..88cd593 --- /dev/null +++ b/service/yaml_blank_lines.go @@ -0,0 +1,368 @@ +package service + +import ( + "bytes" + "slices" + "strconv" + + "gopkg.in/yaml.v3" + + "github.com/qdeck-app/qdeck/domain" +) + +// stripMergeTag removes the explicit `!!merge` tag yaml.v3 emits before +// merge keys. Canonical YAML `<<: *anchor` carries no tag. +func stripMergeTag(encoded []byte) []byte { + return bytes.ReplaceAll(encoded, []byte("!!merge <<:"), []byte("<<:")) +} + +// restoreBlankLines re-inserts blank lines that appeared in the original +// source after specific flat keys. Blanks inside block scalars are ignored — +// they're part of the scalar value and round-trip through the encoder. +func restoreBlankLines(raw []byte, origRoot *yaml.Node, encoded []byte) []byte { + var encDoc yaml.Node + if err := yaml.Unmarshal(encoded, &encDoc); err != nil || len(encDoc.Content) == 0 { + return encoded + } + + encRoot := encDoc.Content[0] + + origLineKey := lineToFlatKey(origRoot) + encLineKey := lineToFlatKey(encRoot) + + rawLines := bytes.Split(raw, []byte("\n")) + blockRegions := blockScalarRegions(origRoot, raw, len(rawLines)) + + runs := collectBlankRuns(rawLines, origLineKey, blockRegions) + if len(runs) == 0 { + return encoded + } + + encKeyLine := make(map[string]int, len(encLineKey)) + for line, key := range encLineKey { + encKeyLine[key] = line + } + + // Aggregate by encoded-line: a key that appears twice after deduplication + // shouldn't blow up. + inserts := make(map[int]int) + + for _, r := range runs { + if line, ok := encKeyLine[r.afterKey]; ok { + inserts[line] += r.count + } + } + + if len(inserts) == 0 { + return encoded + } + + encLines := bytes.Split(encoded, []byte("\n")) + + totalBlanks := 0 + for _, c := range inserts { + totalBlanks += c + } + + var out bytes.Buffer + + out.Grow(len(encoded) + totalBlanks) + + for i, line := range encLines { + out.Write(line) + + if i < len(encLines)-1 { + out.WriteByte('\n') + } + + if c := inserts[i+1]; c > 0 { + for range c { + out.WriteByte('\n') + } + } + } + + return out.Bytes() +} + +type blankRun struct { + afterKey string + count int +} + +// collectBlankRuns scans rawLines for blank-line runs outside block-scalar +// regions, attributing each run to the most recent keyed line. Comment-only +// lines don't update the anchor — comments re-emit as HeadComment on the +// next key, so blanks belong with the prior key. +func collectBlankRuns( + rawLines [][]byte, + lineKey map[int]string, + blockRegions [][2]int, +) []blankRun { + var ( + runs []blankRun + lastKey string + blanks int + keyAdjacent bool + ) + + for i, line := range rawLines { + lineNo := i + 1 + + if inSortedRegion(lineNo, blockRegions) { + // substituteBlockScalarRanges owns these bytes; don't generate runs. + blanks = 0 + + if k, ok := lineKey[lineNo]; ok { + lastKey = k + keyAdjacent = true + } + + continue + } + + if isBlankBytes(line) { + blanks++ + + continue + } + + if blanks > 0 { + // Only attribute blanks when no intervening non-key lines (top-level + // comment blocks become HeadComment on a future node — re-inserting + // after the prior key lands them in the wrong section). + if lastKey != "" && keyAdjacent { + runs = append(runs, blankRun{afterKey: lastKey, count: blanks}) + } + + blanks = 0 + } + + if k, ok := lineKey[lineNo]; ok { + lastKey = k + keyAdjacent = true + } else { + keyAdjacent = false + } + } + + return runs +} + +// lineToFlatKey maps 1-indexed source line numbers to the flat key whose +// declaration starts on that line. Keys are encoded via domain.EscapeSegment +// so literal '.' or '[' inside a map key doesn't collide with path separators. +func lineToFlatKey(root *yaml.Node) map[int]string { + if root == nil { + return nil + } + + out := make(map[int]string) + + var walk func(node *yaml.Node, path string) + + walk = func(node *yaml.Node, path string) { + switch node.Kind { + case yaml.MappingNode: + for i := 0; i+1 < len(node.Content); i += 2 { + key := node.Content[i] + val := node.Content[i+1] + child := joinFlatKey(path, domain.EscapeSegment(key.Value)) + + if key.Line > 0 { + out[key.Line] = child + } + + walk(val, child) + } + case yaml.SequenceNode: + for i, item := range node.Content { + child := path + "[" + strconv.Itoa(i) + "]" + + if item.Line > 0 { + out[item.Line] = child + } + + walk(item, child) + } + case yaml.DocumentNode: + for _, c := range node.Content { + walk(c, path) + } + } + } + + walk(root, "") + + return out +} + +// refineBlockScalarEnd narrows a sibling-heuristic block end via the YAML +// spec rule: the block extends while a line is blank or its indentation is +// >= the content column; it ends at the first non-blank line with smaller +// indentation. +func refineBlockScalarEnd(rawLines [][]byte, startLine, siblingEnd int) int { + if startLine < 1 || startLine > len(rawLines) { + return siblingEnd + } + + contentCol := -1 + + for i := startLine - 1; i < len(rawLines) && i < siblingEnd; i++ { + col := lineIndentColumn(rawLines[i]) + if col >= 0 { + contentCol = col + + break + } + } + + if contentCol < 0 { + return siblingEnd + } + + last := startLine - 1 + + for i := startLine - 1; i < len(rawLines) && i < siblingEnd; i++ { + line := rawLines[i] + col := lineIndentColumn(line) + + if col < 0 { + // Tentatively include blanks; trailing-blank clipping handles them. + last = i + 1 + + continue + } + + if col < contentCol { + break + } + + last = i + 1 + } + + return last +} + +// lineIndentColumn returns the 0-indexed column of the first non-whitespace +// byte, or -1 when the line is blank. Tabs count as 1 (yaml.v3 forbids tabs +// in indentation). +func lineIndentColumn(line []byte) int { + for i, c := range line { + if c != ' ' && c != '\t' && c != '\r' { + return i + } + } + + return -1 +} + +// blockScalarRegions returns sorted inclusive line ranges covering every +// block scalar and tagged collection body. Used to exclude blanks inside +// these regions from blank-run detection — substituteBlockScalarRanges +// emits them source-verbatim. +func blockScalarRegions(root *yaml.Node, raw []byte, totalLines int) [][2]int { + if root == nil { + return nil + } + + type entry struct { + line int + node *yaml.Node + } + + var ( + blocks []*yaml.Node + positions []entry + ) + + var walk func(n *yaml.Node) + + walk = func(n *yaml.Node) { + if n == nil { + return + } + + if n.Line > 0 { + positions = append(positions, entry{n.Line, n}) + } + + if isBlockScalarNode(n) || isTaggedCollectionNode(n) { + blocks = append(blocks, n) + } + + for _, c := range n.Content { + walk(c) + } + } + + walk(root) + + slices.SortFunc(positions, func(a, b entry) int { return a.line - b.line }) + + var rawLines [][]byte + if len(raw) > 0 { + rawLines = bytes.Split(raw, []byte("\n")) + } + + regions := make([][2]int, 0, len(blocks)) + + for _, b := range blocks { + startLine := b.Line + 1 + + // Sibling-heuristic cap valid only for block SCALARS: tagged + // collections have children with their own Line that would mis-cap + // the region. Rely on refineBlockScalarEnd's indent scan for those. + endLine := totalLines + + if isBlockScalarNode(b) { + for _, p := range positions { + if p.line > b.Line && p.node != b { + endLine = p.line - 1 + + break + } + } + } + + if rawLines != nil { + endLine = refineBlockScalarEnd(rawLines, startLine, endLine) + } + + if endLine >= startLine { + regions = append(regions, [2]int{startLine, endLine}) + } + } + + slices.SortFunc(regions, func(a, b [2]int) int { return a[0] - b[0] }) + + return regions +} + +// inSortedRegion reports whether line falls within any [start,end] range. +// regions must be sorted by start. +func inSortedRegion(line int, regions [][2]int) bool { + for _, r := range regions { + if r[0] > line { + break + } + + if line <= r[1] { + return true + } + } + + return false +} + +// isBlankBytes reports whether b contains only ASCII whitespace. Trailing CR +// is honored so CRLF files don't read as non-blank. +func isBlankBytes(b []byte) bool { + for _, c := range b { + if c != ' ' && c != '\t' && c != '\r' { + return false + } + } + + return true +} diff --git a/service/yaml_format.go b/service/yaml_format.go new file mode 100644 index 0000000..12f37c2 --- /dev/null +++ b/service/yaml_format.go @@ -0,0 +1,76 @@ +package service + +import ( + "bytes" + "fmt" + + "gopkg.in/yaml.v3" +) + +// PatchSourceText is the byte-faithful save entrypoint. It prefers leaf-line +// splicing — for value-only edits to plain scalars, the output differs from +// raw by exactly one line per edit, with everything else byte-identical. +// Falls back to the encoder + PreserveSourceFormatting path when splice +// preconditions don't hold for any entry. +func PatchSourceText( + raw []byte, + root *yaml.Node, + entries []OverrideEntry, + indent int, + docs DocComments, +) (string, error) { + // Strip BOM and convert CRLF→LF first: EncodeForFile re-applies them + // downstream based on stored encoding/line-ending labels, so leaving them + // would double the BOM and turn \r\n into \r\r\n. yaml.v3 also reports + // node Line/Column against the BOM-stripped, LF-normalized form, so the + // splice's byte math lines up. + raw = normalizeForEncodeForFile(raw) + + if root != nil && len(raw) > 0 { + if edits, ok := planScalarSplice(raw, root, entries, docs); ok { + if len(edits) == 0 { + return string(raw), nil + } + + if spliced, spliceOK := SpliceScalarValues(raw, root, edits); spliceOK { + return string(spliced), nil + } + } + } + + encoded, err := PatchNodeTree(root, entries, indent, docs) + if err != nil { + return "", fmt.Errorf("patch source: %w", err) + } + + return string(PreserveSourceFormatting(raw, root, []byte(encoded))), nil +} + +func normalizeForEncodeForFile(raw []byte) []byte { + raw = bytes.TrimPrefix(raw, bomUTF8) + if bytes.Contains(raw, []byte("\r\n")) { + raw = bytes.ReplaceAll(raw, []byte("\r\n"), []byte("\n")) + } + + return raw +} + +// PreserveSourceFormatting post-processes encoder output to recover source +// formatting yaml.v3 normalizes away: inter-node blank lines, the spurious +// !!merge tag on `<<:` keys, byte-verbatim block scalars and tagged +// collections, and semantically-equivalent leaves. Best-effort: each pass +// passes through on parse failure or structural divergence. +func PreserveSourceFormatting(raw []byte, origRoot *yaml.Node, encoded []byte) []byte { + encoded = stripMergeTag(encoded) + if len(raw) == 0 || origRoot == nil { + return encoded + } + + // Pass order matters: block-range substitution runs first because it + // replaces multi-line spans, invalidating any line-map built before it. + encoded = substituteBlockScalarRanges(raw, origRoot, encoded) + encoded = substituteUnchangedLinesFromSource(raw, origRoot, encoded) + encoded = insertMissingSourceLines(raw, origRoot, encoded) + + return restoreBlankLines(raw, origRoot, encoded) +} diff --git a/service/yaml_format_test.go b/service/yaml_format_test.go new file mode 100644 index 0000000..9fe3df5 --- /dev/null +++ b/service/yaml_format_test.go @@ -0,0 +1,264 @@ +package service + +import ( + "bytes" + "context" + "fmt" + "strings" + "testing" + + "gopkg.in/yaml.v3" +) + +// Shared fixture-row constants reused across save-path tests. The redis +// cornercase file's `architecture` key (with `replication`/`standalone` +// as before/after values) and the smaller `replicaCount` leaf are the +// canonical hand to swap for "single-field edit" scenarios. +const ( + fixtureKeyArchitecture = "architecture" + fixtureKeyReplicaCount = "replicaCount" + fixtureValReplication = "replication" + fixtureValStandalone = "standalone" +) + +// lineDiffs returns one descriptor per line index where a and b differ. +// Both inputs are right-trimmed of trailing newlines so a present-vs- +// absent final newline doesn't register. Lines beyond one side's length +// compare against the empty string, producing a diff entry per missing +// line. Test-only helper. +func lineDiffs(a, b string) []string { + aLines := strings.Split(strings.TrimRight(a, "\n"), "\n") + bLines := strings.Split(strings.TrimRight(b, "\n"), "\n") + + n := len(aLines) + if len(bLines) > n { + n = len(bLines) + } + + var diffs []string + + for i := range n { + var aL, bL string + if i < len(aLines) { + aL = aLines[i] + } + + if i < len(bLines) { + bL = bLines[i] + } + + if aL != bL { + diffs = append(diffs, fmt.Sprintf("line %d: %q -> %q", i+1, aL, bL)) + } + } + + return diffs +} + +// TestPatchSourceText_BOM_CRLF_RoundTripsThroughEncodeForFile pins down the +// full save contract for UTF-8-BOM + CRLF source files: after PatchSourceText +// returns and EncodeForFile re-applies the encoding labels stored on the +// column, the on-disk bytes must contain exactly one BOM and use \r\n line +// endings (not \r\r\n). +// +// The bug this guards against: PatchSourceText returning source bytes that +// already carry the BOM and CRLF, then EncodeForFile prepending a second +// BOM and rewriting each \n (which is already preceded by \r) to \r\n — +// producing a doubled-BOM file with \r\r\n separators. +// +// Both the splice-success path (value edit) and the no-edit fast path +// are checked because both routed source bytes verbatim through +// PatchSourceText before the fix. +func TestPatchSourceText_BOM_CRLF_RoundTripsThroughEncodeForFile(t *testing.T) { + t.Parallel() + + const utf8BOM = "\xEF\xBB\xBF" + + rawText := utf8BOM + "architecture: replication\r\nreplicaCount: 2\r\n" + raw := []byte(rawText) + + var doc yaml.Node + if err := yaml.Unmarshal(raw, &doc); err != nil { + t.Fatalf("yaml.Unmarshal: %v", err) + } + + if len(doc.Content) == 0 { + t.Fatal("empty doc after parse") + } + + root := doc.Content[0] + + cases := []struct { + name string + entries []OverrideEntry + wantValue string + }{ + { + name: "no edits — fast path", + entries: []OverrideEntry{ + {Key: fixtureKeyArchitecture, Value: fixtureValReplication, Type: typeString}, + {Key: fixtureKeyReplicaCount, Value: "2", Type: typeNumber}, + }, + wantValue: fixtureValReplication, + }, + { + name: "value edit — splice path", + entries: []OverrideEntry{ + {Key: fixtureKeyArchitecture, Value: fixtureValStandalone, Type: typeString}, + {Key: fixtureKeyReplicaCount, Value: "2", Type: typeNumber}, + }, + wantValue: fixtureValStandalone, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + got, err := PatchSourceText(raw, root, tc.entries, DefaultYAMLIndent, DocComments{}) + if err != nil { + t.Fatalf("PatchSourceText: %v", err) + } + + onDisk := EncodeForFile(got, EncodingUTF8BOM, LineEndingCRLF) + + if !bytes.HasPrefix(onDisk, []byte(utf8BOM)) { + t.Fatalf("on-disk bytes missing BOM prefix: %q", onDisk[:6]) + } + + if bytes.HasPrefix(onDisk[3:], []byte(utf8BOM)) { + t.Fatalf("doubled BOM in on-disk bytes: %q", onDisk[:9]) + } + + if bytes.Contains(onDisk, []byte("\r\r\n")) { + t.Fatalf("doubled CR in on-disk bytes: %q", onDisk) + } + + if !bytes.Contains(onDisk, []byte(tc.wantValue+"\r\n")) { + t.Fatalf("expected %q followed by CRLF in output, got:\n%q", tc.wantValue, onDisk) + } + }) + } +} + +// TestSingleFieldEditYieldsSingleLineDiff loads the cornercase fixture +// (../assets/test-data/redis-values-cornercases.yaml), changes one +// plain scalar value via the same save path the UI controller uses +// (PatchSourceText, which prefers leaf-line splice and falls back to +// PatchNodeTree + PreserveSourceFormatting), and asserts the result +// differs from the source by exactly one line. +// +// The build is realistic: it loads via ReadCustomValues to populate +// the same RawBytes / NodeTree / flat entry set the controller has at +// save time, copies every original leaf into the override list, and +// patches exactly one — mirroring "user opened a file, changed one +// field, hit save." +// +// This pins the production contract: a single-field edit through the +// real save path must produce a single-line diff. Failures here mean +// the splice precondition tightened or a regression has crept into +// PatchSourceText's encoder fallback. +func TestSingleFieldEditYieldsSingleLineDiff(t *testing.T) { + t.Parallel() + + svc := NewValuesService() + + vf, err := svc.ReadCustomValues(context.Background(), fixturePath) + if err != nil { + t.Skipf("fixture not present or unreadable: %v", err) + } + + if vf.NodeTree == nil { + t.Fatal("NodeTree is nil — fixture failed to parse as yaml.Node") + } + + if len(vf.RawBytes) == 0 { + t.Fatal("RawBytes empty — read path didn't retain source bytes") + } + + const ( + targetKey = fixtureKeyArchitecture + oldValue = fixtureValReplication + newValue = fixtureValStandalone + ) + + // Build the override list the controller would produce: every + // loaded leaf entry, with the target key's value swapped. The + // per-entry HeadComment carries the flattener's loaded comment + // text so PatchSourceText's splice-viability check sees the same + // comment shape PatchNodeTree would on the encoder path. + entries := make([]OverrideEntry, 0, len(vf.Entries)) + patched := false + + for _, e := range vf.Entries { + if e.Type == typeMap || e.Type == typeList { + continue + } + + value := e.Value + if string(e.Key) == targetKey { + if value != oldValue { + t.Fatalf("fixture changed: %s is %q, expected %q", targetKey, value, oldValue) + } + + value = newValue + patched = true + } + + entries = append(entries, OverrideEntry{ + Key: string(e.Key), + Value: value, + Type: e.Type, + HeadComment: e.Comment, + }) + } + + if !patched { + t.Fatalf("target key %q not present in flat entries", targetKey) + } + + // Pass the loaded doc-level comments — same path the controller + // takes via col.DocCommentsForSave(). The redis fixture has a + // banner, trailer, and per-leaf foots; an earlier version of this + // test passed DocComments{} and silently took the splice path that + // the production save couldn't, masking the bug where any file with + // a banner fell through to the encoder. + docs := DocComments{ + Head: vf.DocHeadComment, + Foot: vf.DocFootComment, + Foots: vf.FootComments, + } + + if !docsMatchSource(vf.RawBytes, docs) { + oc, _ := parseOrphanComments(vf.RawBytes) + t.Fatalf("docsMatchSource returned false for unedited load — splice will reject every save:\n"+ + " docs.Head len=%d vs oc.DocHead len=%d match=%v\n"+ + " docs.Foot len=%d vs oc.DocFoot len=%d match=%v\n"+ + " docs.Foots len=%d vs oc.Foots len=%d\n"+ + " docs.SectionHeads len=%d", + len(docs.Head), len(oc.DocHead), docs.Head == oc.DocHead, + len(docs.Foot), len(oc.DocFoot), docs.Foot == oc.DocFoot, + len(docs.Foots), len(oc.Foots), + len(docs.SectionHeads)) + } + + got, err := PatchSourceText(vf.RawBytes, vf.NodeTree, entries, vf.Indent, docs) + if err != nil { + t.Fatalf("PatchSourceText: %v", err) + } + + diffs := lineDiffs(string(vf.RawBytes), got) + if len(diffs) != 1 { + t.Fatalf("expected exactly 1 line of diff, got %d:\n%s", + len(diffs), strings.Join(diffs, "\n")) + } + + if !strings.Contains(diffs[0], oldValue) || !strings.Contains(diffs[0], newValue) { + t.Errorf("diff line doesn't describe the %s→%s rewrite:\n %s", + oldValue, newValue, diffs[0]) + } + + if !strings.Contains(diffs[0], targetKey) { + t.Errorf("diff line doesn't reference target key %q:\n %s", targetKey, diffs[0]) + } +} diff --git a/service/yaml_node_match.go b/service/yaml_node_match.go new file mode 100644 index 0000000..789c517 --- /dev/null +++ b/service/yaml_node_match.go @@ -0,0 +1,180 @@ +package service + +import ( + "strings" + + "gopkg.in/yaml.v3" +) + +// yaml.v3 default collection tags. Both the `!!short` and full URI +// spellings can appear in node.Tag depending on whether the source +// used an explicit tag directive. +const ( + tagMap = "!!map" + tagSeq = "!!seq" + tagMapURI = "tag:yaml.org,2002:map" + tagSeqURI = "tag:yaml.org,2002:seq" +) + +// isBlockScalarNode reports whether node is a scalar with a literal `|` or +// folded `>` style. Style is a bit-field — check via bitmask so combined +// styles (e.g. TaggedStyle | LiteralStyle) still match. +func isBlockScalarNode(node *yaml.Node) bool { + if node == nil || node.Kind != yaml.ScalarNode { + return false + } + + return node.Style&yaml.LiteralStyle != 0 || node.Style&yaml.FoldedStyle != 0 +} + +// isTaggedCollectionNode reports whether node is a mapping or sequence with a +// non-default tag (`!!set`, `!!omap`, user-defined tags). yaml.v3's emitter +// doesn't preserve presentation quirks (like `? key` for `!!set` members), so +// these classes round-trip safely only via byte-substitution. +func isTaggedCollectionNode(node *yaml.Node) bool { + if node == nil || node.Tag == "" { + return false + } + + switch node.Kind { + case yaml.MappingNode: + return node.Tag != tagMap && node.Tag != tagMapURI + case yaml.SequenceNode: + return node.Tag != tagSeq && node.Tag != tagSeqURI + } + + return false +} + +func isSingleLineNode(node *yaml.Node) bool { + if node == nil { + return false + } + + switch node.Kind { + case yaml.ScalarNode: + return !isBlockScalarNode(node) + case yaml.MappingNode: + return len(node.Content) == 0 + case yaml.SequenceNode: + return len(node.Content) == 0 + case yaml.AliasNode: + return true + } + + return false +} + +// blockScalarsEquivalent reports whether two substitutable nodes (block +// scalars or tagged collections) carry the same logical content. Style is +// intentionally NOT compared — the encoder may add the TaggedStyle bit-flag +// to a source that only had LiteralStyle. +func blockScalarsEquivalent(a, b *yaml.Node) bool { + if a == nil || b == nil { + return false + } + + if a.Kind != b.Kind || a.Tag != b.Tag { + return false + } + + if a.Kind == yaml.ScalarNode { + // Trim leading/trailing newlines: yaml.v3 can shift them when re-emitting + // depending on the chomp indicator; the substituted bytes carry the + // original whitespace shape. + return strings.Trim(a.Value, "\n") == strings.Trim(b.Value, "\n") + } + + return nodesStructurallyEqual(a, b) +} + +// nodesStructurallyEqual reports whether two yaml.Nodes have identical content +// trees (Kind, Tag, Value, recursive Content). Style and position are not +// compared. AliasNode equality is approximated by comparing the resolved +// Anchor — a true alias-graph walk would need cycle detection. +func nodesStructurallyEqual(a, b *yaml.Node) bool { + if a == nil || b == nil { + return a == b + } + + if a.Kind != b.Kind || a.Tag != b.Tag || a.Value != b.Value { + return false + } + + if a.Kind == yaml.AliasNode { + if a.Alias == nil || b.Alias == nil { + return a.Alias == b.Alias + } + + return a.Alias.Anchor == b.Alias.Anchor + } + + if len(a.Content) != len(b.Content) { + return false + } + + for i := range a.Content { + if !nodesStructurallyEqual(a.Content[i], b.Content[i]) { + return false + } + } + + return true +} + +// linesSemanticallyMatch reports whether two YAML lines describe the same +// key/value/inline-comment, ignoring whitespace differences. Skips multi-line +// scalars (literal `|` / folded `>`) — the line-level check can't see their +// body. Also widens for the asymmetric case where the encoder dropped an +// inline comment the source carried (most common on null-tagged leaves like +// `key: ~ # comment`). +func linesSemanticallyMatch(srcLine, encLine []byte) bool { + srcKV, srcCmt := splitKeyValueAndComment(srcLine) + encKV, encCmt := splitKeyValueAndComment(encLine) + + srcKVStr := strings.TrimRight(string(srcKV), " \t") + encKVStr := strings.TrimRight(string(encKV), " \t") + + if srcKVStr != encKVStr { + return false + } + + if strings.HasSuffix(srcKVStr, ": |") || strings.HasSuffix(srcKVStr, ": >") || + strings.HasSuffix(srcKVStr, ":|") || strings.HasSuffix(srcKVStr, ":>") || + strings.Contains(srcKVStr, ": |-") || strings.Contains(srcKVStr, ": >-") { + return false + } + + srcCmtTrim := strings.TrimSpace(string(srcCmt)) + encCmtTrim := strings.TrimSpace(string(encCmt)) + + if srcCmtTrim == encCmtTrim { + return true + } + + return encCmtTrim == "" && srcCmtTrim != "" +} + +// splitKeyValueAndComment partitions a YAML line into its key:value portion +// and its inline `# comment` portion. Quote-aware so a `#` inside a quoted +// scalar doesn't get treated as a comment marker. +func splitKeyValueAndComment(line []byte) (kv, comment []byte) { + var quote byte + + for i := 0; i < len(line); i++ { + c := line[i] + + switch { + case quote != 0: + if c == quote { + quote = 0 + } + case c == '\'' || c == '"': + quote = c + case (c == ' ' || c == '\t') && i+1 < len(line) && line[i+1] == '#': + return line[:i], line[i+1:] + } + } + + return line, nil +} diff --git a/service/yaml_splice.go b/service/yaml_splice.go new file mode 100644 index 0000000..515c715 --- /dev/null +++ b/service/yaml_splice.go @@ -0,0 +1,219 @@ +package service + +import ( + "bytes" + "strings" + + "gopkg.in/yaml.v3" +) + +type ScalarEdit struct { + FlatKey string + NewValue string +} + +// SpliceScalarValues rewrites the value portion of plain scalar leaves in raw, +// preserving every other byte. All-or-nothing: returns (raw, false) if any edit +// can't be safely applied so callers fall back to the encoder path uniformly. +func SpliceScalarValues(raw []byte, root *yaml.Node, edits []ScalarEdit) ([]byte, bool) { + if root == nil || len(raw) == 0 || len(edits) == 0 { + return raw, false + } + + out := raw + + for _, e := range edits { + next, ok := spliceScalarValue(out, root, e.FlatKey, e.NewValue) + if !ok { + return raw, false + } + + out = next + } + + return out, true +} + +func spliceScalarValue(raw []byte, root *yaml.Node, flatKey, newValue string) ([]byte, bool) { + valNode := findNodeSubtree(root, flatKey) + if valNode == nil || valNode.Kind != yaml.ScalarNode { + return raw, false + } + + // Style==0 is plain; any non-zero style needs quote-aware end detection. + if valNode.Style != 0 { + return raw, false + } + + if valNode.Line <= 0 || valNode.Column <= 0 { + return raw, false + } + + if !isYAMLPlainSafe(newValue) { + return raw, false + } + + lines := bytes.Split(raw, []byte("\n")) + if valNode.Line > len(lines) { + return raw, false + } + + line := lines[valNode.Line-1] + valueStart := valNode.Column - 1 + + if valueStart < 0 || valueStart >= len(line) { + return raw, false + } + + valueEnd := plainScalarLineEnd(line, valueStart) + + newLine := make([]byte, 0, len(line)+len(newValue)) + newLine = append(newLine, line[:valueStart]...) + newLine = append(newLine, []byte(newValue)...) + newLine = append(newLine, line[valueEnd:]...) + + lines[valNode.Line-1] = newLine + + return bytes.Join(lines, []byte("\n")), true +} + +// plainScalarLineEnd returns the offset at which a plain scalar ends: +// the first ` #` sequence (yaml.v3's inline-comment marker) or end of line. +// The offset points at the start of trailing whitespace so the splice +// preserves the spaces between value and comment. +func plainScalarLineEnd(line []byte, start int) int { + for i := start; i < len(line); i++ { + if line[i] != ' ' && line[i] != '\t' { + continue + } + + j := i + for j < len(line) && (line[j] == ' ' || line[j] == '\t') { + j++ + } + + if j < len(line) && line[j] == '#' { + return i + } + } + + return len(line) +} + +// isYAMLPlainSafe reports whether v survives encoding as a plain (unquoted) +// scalar without yaml.v3 promoting it to a quoted form. +func isYAMLPlainSafe(v string) bool { + var buf bytes.Buffer + + enc := yaml.NewEncoder(&buf) + enc.SetIndent(DefaultYAMLIndent) + + if err := enc.Encode(&yaml.Node{Kind: yaml.ScalarNode, Value: v}); err != nil { + return false + } + + _ = enc.Close() + + out := strings.TrimRight(buf.String(), "\n") + + return out == v +} + +// planScalarSplice decides whether the leaf-line splicer can apply all +// entries byte-faithfully. Returns (edits, true) when every entry is either +// a no-op or a plain-scalar value change with matching comments; returns +// (nil, false) when any entry would need the encoder fallback. +func planScalarSplice(raw []byte, root *yaml.Node, entries []OverrideEntry, docs DocComments) ([]ScalarEdit, bool) { + if !docsMatchSource(raw, docs) { + return nil, false + } + + var edits []ScalarEdit + + for _, e := range entries { + segments, err := parseKeySegments(e.Key) + if err != nil { + return nil, false + } + + if e.Type == typeMap || e.Type == typeList { + if treeNodeMatchesContainerType(root, segments, e.Type) { + continue + } + + return nil, false + } + + effective, hasValue := findEffectiveScalar(root, segments) + if !hasValue { + return nil, false + } + + effHead, effLine, viaAlias := effectiveComments(root, segments) + + if !viaAlias && !overrideCommentsMatch(e.HeadComment, e.LineComment, effHead, effLine) { + return nil, false + } + + if scalarsEquivalent(effective, e.Value, e.Type) { + continue + } + + if e.Type == TypeNull || viaAlias { + return nil, false + } + + valNode := findNodeSubtree(root, e.Key) + if valNode == nil || valNode.Kind != yaml.ScalarNode || valNode.Style != 0 { + return nil, false + } + + if !isYAMLPlainSafe(e.Value) { + return nil, false + } + + edits = append(edits, ScalarEdit{FlatKey: e.Key, NewValue: e.Value}) + } + + return edits, true +} + +// docsMatchSource reports whether docs equals the doc-level orphan comments +// currently in raw (banner, trailer, per-leaf foots) and that no section-head +// edit has been recorded. SectionHeads is a one-way edit flag: parseOrphanComments +// never populates it, so any non-empty map means a section head was touched +// and the encoder must run to place the change. +func docsMatchSource(raw []byte, docs DocComments) bool { + if len(docs.SectionHeads) > 0 { + return false + } + + oc, err := parseOrphanComments(raw) + if err != nil { + return false + } + + if docs.Head != oc.DocHead || docs.Foot != oc.DocFoot { + return false + } + + if len(docs.Foots) != len(oc.Foots) { + return false + } + + for k, v := range docs.Foots { + if oc.Foots[k] != v { + return false + } + } + + return true +} + +// PlanScalarSpliceForTest exposes planScalarSplice to cross-package tests. +// Production callers go through PatchSourceText. +func PlanScalarSpliceForTest( + raw []byte, root *yaml.Node, entries []OverrideEntry, docs DocComments, +) ([]ScalarEdit, bool) { + return planScalarSplice(raw, root, entries, docs) +} diff --git a/service/yaml_substitute.go b/service/yaml_substitute.go new file mode 100644 index 0000000..7639c2e --- /dev/null +++ b/service/yaml_substitute.go @@ -0,0 +1,385 @@ +package service + +import ( + "bytes" + "slices" + "strconv" + + "gopkg.in/yaml.v3" + + "github.com/qdeck-app/qdeck/domain" +) + +// substituteBlockScalarRanges replaces encoded byte ranges for block scalars +// and tagged collections with the source's verbatim bytes when the content is +// structurally unchanged. A value/structure mismatch keeps the encoded form +// so user edits survive. +func substituteBlockScalarRanges(raw []byte, origRoot *yaml.Node, encoded []byte) []byte { + if origRoot == nil || len(raw) == 0 { + return encoded + } + + var encDoc yaml.Node + if err := yaml.Unmarshal(encoded, &encDoc); err != nil || len(encDoc.Content) == 0 { + return encoded + } + + encRoot := encDoc.Content[0] + + rawLines := bytes.Split(raw, []byte("\n")) + encLines := bytes.Split(encoded, []byte("\n")) + + origBlocks := collectBlockScalarEntries(origRoot, rawLines) + encBlocks := collectBlockScalarEntries(encRoot, encLines) + + type substitution struct { + encStart, encEnd int + srcStart, srcEnd int + } + + var subs []substitution + + for flatKey, encInfo := range encBlocks { + origInfo, ok := origBlocks[flatKey] + if !ok { + continue + } + + if !blockScalarsEquivalent(origInfo.node, encInfo.node) { + continue + } + + subs = append(subs, substitution{ + encStart: encInfo.startLine, + encEnd: encInfo.endLine, + srcStart: origInfo.startLine, + srcEnd: origInfo.endLine, + }) + } + + if len(subs) == 0 { + return encoded + } + + slices.SortFunc(subs, func(a, b substitution) int { return a.encStart - b.encStart }) + + out := make([][]byte, 0, len(encLines)) + + cursor := 1 + + for _, s := range subs { + for i := cursor; i < s.encStart; i++ { + out = append(out, encLines[i-1]) + } + + for i := s.srcStart; i <= s.srcEnd; i++ { + if i >= 1 && i <= len(rawLines) { + out = append(out, rawLines[i-1]) + } + } + + cursor = s.encEnd + 1 + } + + for i := cursor; i <= len(encLines); i++ { + out = append(out, encLines[i-1]) + } + + return bytes.Join(out, []byte("\n")) +} + +type blockScalarEntry struct { + startLine int + endLine int + node *yaml.Node +} + +// collectBlockScalarEntries maps each flat key whose value is substitutable +// (block scalar or tagged collection) to its source line range. Recursion +// stops at substitutable nodes — the parent owns the whole byte range. +func collectBlockScalarEntries(root *yaml.Node, lines [][]byte) map[string]blockScalarEntry { + out := make(map[string]blockScalarEntry) + + var walk func(node *yaml.Node, path string) + + walk = func(node *yaml.Node, path string) { + if node == nil { + return + } + + switch node.Kind { + case yaml.MappingNode: + for i := 0; i+1 < len(node.Content); i += 2 { + key := node.Content[i] + val := node.Content[i+1] + child := joinFlatKey(path, domain.EscapeSegment(key.Value)) + + if isBlockScalarNode(val) || isTaggedCollectionNode(val) { + bodyStart := val.Line + 1 + if val.Line <= key.Line { + bodyStart = key.Line + 1 + } + + out[child] = blockScalarEntry{ + startLine: key.Line, + endLine: refineBlockScalarEnd(lines, bodyStart, len(lines)), + node: val, + } + + continue + } + + walk(val, child) + } + case yaml.SequenceNode: + for i, item := range node.Content { + child := path + "[" + strconv.Itoa(i) + "]" + + if isBlockScalarNode(item) || isTaggedCollectionNode(item) { + out[child] = blockScalarEntry{ + startLine: item.Line, + endLine: refineBlockScalarEnd(lines, item.Line+1, len(lines)), + node: item, + } + + continue + } + + walk(item, child) + } + case yaml.DocumentNode: + for _, c := range node.Content { + walk(c, path) + } + } + } + + walk(root, "") + + return out +} + +// substituteUnchangedLinesFromSource replaces each encoded line whose +// corresponding source line is semantically equivalent (same key, value, and +// inline comment modulo whitespace) with the source bytes. Recovers +// inline-comment column alignment for keys the encoder reformatted as a +// side-effect of re-emitting a different leaf. +func substituteUnchangedLinesFromSource(raw []byte, origRoot *yaml.Node, encoded []byte) []byte { + var encDoc yaml.Node + if err := yaml.Unmarshal(encoded, &encDoc); err != nil || len(encDoc.Content) == 0 { + return encoded + } + + encRoot := encDoc.Content[0] + + origLineKey := lineToFlatKey(origRoot) + encLineKey := lineToFlatKey(encRoot) + + if len(origLineKey) == 0 || len(encLineKey) == 0 { + return encoded + } + + origKeyLine := make(map[string]int, len(origLineKey)) + for line, key := range origLineKey { + origKeyLine[key] = line + } + + rawLines := bytes.Split(raw, []byte("\n")) + encLines := bytes.Split(encoded, []byte("\n")) + + for i := range encLines { + encLineNo := i + 1 + + key, ok := encLineKey[encLineNo] + if !ok { + continue + } + + srcLineNo, ok := origKeyLine[key] + if !ok { + continue + } + + if srcLineNo < 1 || srcLineNo > len(rawLines) { + continue + } + + srcLine := rawLines[srcLineNo-1] + if linesSemanticallyMatch(srcLine, encLines[i]) { + encLines[i] = srcLine + } + } + + return bytes.Join(encLines, []byte("\n")) +} + +// insertMissingSourceLines re-inserts single-line source nodes (scalar leaves +// and empty containers) whose keys are missing from the encoded output. +// Typically recovers `key: []` / `key: {}` placeholders upstream layers +// skipped as no-op container entries. +func insertMissingSourceLines(raw []byte, origRoot *yaml.Node, encoded []byte) []byte { + if origRoot == nil || len(raw) == 0 { + return encoded + } + + var encDoc yaml.Node + if err := yaml.Unmarshal(encoded, &encDoc); err != nil || len(encDoc.Content) == 0 { + return encoded + } + + encRoot := encDoc.Content[0] + origLineKey := lineToFlatKey(origRoot) + encLineKey := lineToFlatKey(encRoot) + + encKeyPresent := make(map[string]bool, len(encLineKey)) + for _, key := range encLineKey { + encKeyPresent[key] = true + } + + encKeyLine := make(map[string]int, len(encLineKey)) + for line, key := range encLineKey { + encKeyLine[key] = line + } + + insertable := insertableSingleLineKeys(origRoot) + + sourceLines := make([]int, 0, len(origLineKey)) + for line := range origLineKey { + sourceLines = append(sourceLines, line) + } + + slices.Sort(sourceLines) + + // Anchor each insertion on the PREVIOUS source key that exists in encoded; + // insert AFTER that key's line. Anchoring on the next key would land the + // insertion across a section boundary. + type insertion struct { + afterEncLine int // 1-indexed; 0 means "insert at top of file" + srcLine int // 1-indexed source line to emit + } + + var insertions []insertion + + for i, srcLine := range sourceLines { + key := origLineKey[srcLine] + if encKeyPresent[key] { + continue + } + + if !insertable[key] { + continue + } + + var prevEncLine int + + for j := i - 1; j >= 0; j-- { + candidateKey := origLineKey[sourceLines[j]] + if encLine, ok := encKeyLine[candidateKey]; ok { + prevEncLine = encLine + + break + } + } + + insertions = append(insertions, insertion{ + afterEncLine: prevEncLine, + srcLine: srcLine, + }) + } + + if len(insertions) == 0 { + return encoded + } + + rawLines := bytes.Split(raw, []byte("\n")) + encLines := bytes.Split(encoded, []byte("\n")) + + buckets := make(map[int][]insertion) + for _, ins := range insertions { + buckets[ins.afterEncLine] = append(buckets[ins.afterEncLine], ins) + } + + out := make([][]byte, 0, len(encLines)) + + emitBucket := func(target int) { + for _, ins := range buckets[target] { + if ins.srcLine >= 1 && ins.srcLine <= len(rawLines) { + out = append(out, rawLines[ins.srcLine-1]) + } + } + } + + emitBucket(0) + + for i, encLine := range encLines { + encLineNo := i + 1 + + out = append(out, encLine) + + emitBucket(encLineNo) + } + + return bytes.Join(out, []byte("\n")) +} + +// insertableSingleLineKeys returns flat keys whose source representation +// fits on one line — scalar leaves and empty containers. +func insertableSingleLineKeys(root *yaml.Node) map[string]bool { + if root == nil { + return nil + } + + out := make(map[string]bool) + + var walk func(node *yaml.Node, path string) + + walk = func(node *yaml.Node, path string) { + if node == nil { + return + } + + switch node.Kind { + case yaml.MappingNode: + // Flow-style mappings put all entries on one line; lineToFlatKey can + // only record one key per line. The parent's single-line entry + // covers the whole mapping. + if node.Style == yaml.FlowStyle { + return + } + + for i := 0; i+1 < len(node.Content); i += 2 { + key := node.Content[i] + val := node.Content[i+1] + child := joinFlatKey(path, domain.EscapeSegment(key.Value)) + + if isSingleLineNode(val) { + out[child] = true + } else { + walk(val, child) + } + } + case yaml.SequenceNode: + if node.Style == yaml.FlowStyle { + return + } + + for i, item := range node.Content { + child := path + "[" + strconv.Itoa(i) + "]" + + if isSingleLineNode(item) { + out[child] = true + } else { + walk(item, child) + } + } + case yaml.DocumentNode: + for _, c := range node.Content { + walk(c, path) + } + } + } + + walk(root, "") + + return out +} diff --git a/service/yaml_tree.go b/service/yaml_tree.go index a34ab9b..6f49f75 100644 --- a/service/yaml_tree.go +++ b/service/yaml_tree.go @@ -746,10 +746,10 @@ func setLeaf(current *yaml.Node, seg keySegment, value any) error { func containerForNextSegment(next keySegment) *yaml.Node { if next.isIndex { - return &yaml.Node{Kind: yaml.SequenceNode, Tag: "!!seq"} + return &yaml.Node{Kind: yaml.SequenceNode, Tag: tagSeq} } - return &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"} + return &yaml.Node{Kind: yaml.MappingNode, Tag: tagMap} } // deletePath removes the physical key at the flat path. Aliases along the way diff --git a/ui/page/values_controller.go b/ui/page/values_controller.go index 8e1261e..343920a 100644 --- a/ui/page/values_controller.go +++ b/ui/page/values_controller.go @@ -101,12 +101,16 @@ type ValuesController struct { lastRenderMode renderMode // Overwrite confirmation dialog (shown when file changed on disk). + // overwritePendingWrite captures the dispatch the user is about to + // confirm — either an EncodeForFile-routed save or a verbatim raw- + // bytes write — closed over so the dialog handler doesn't need to + // branch on save-shape. overwriteDialog customwidget.ConfirmDialog overwriteDialogYes widget.Clickable overwriteDialogNo widget.Clickable overwriteDialogActive bool overwritePendingCol int - overwritePendingYAML string + overwritePendingWrite func() // focusSaver debounces per-chart cell-focus writes to avoid rewriting the // whole AppData JSON on every arrow-key frame. @@ -399,43 +403,7 @@ func (vc *ValuesController) pollEditorParse() { } else { col.EditorParseError = "" - // Preserve detected YAML indent from the originally loaded file. - // Also preserve the parsed yaml.Node tree: it represents the - // on-disk structure (anchors, aliases, comments, styles) and - // stays authoritative across edits — ParseEditorContent does - // not reparse it, and PatchNodeTree works against a deep copy - // so successive saves start from the same original tree. - // Doc-level orphan comments (banner/trailer/per-leaf foots) - // are similarly load-time metadata: ParseEditorContent only - // sees the editor's value text, so it can't recover them — - // carry them across so the banner strip and comment-row - // renderers don't lose their data on every keystroke. - if col.CustomValues != nil { - if col.CustomValues.Indent > 0 { - res.Value.Indent = col.CustomValues.Indent - } - - if col.CustomValues.NodeTree != nil { - res.Value.NodeTree = col.CustomValues.NodeTree - } - - if col.CustomValues.Anchors != nil { - res.Value.Anchors = col.CustomValues.Anchors - } - - if col.CustomValues.DocHeadComment != "" { - res.Value.DocHeadComment = col.CustomValues.DocHeadComment - } - - if col.CustomValues.DocFootComment != "" { - res.Value.DocFootComment = col.CustomValues.DocFootComment - } - - if col.CustomValues.FootComments != nil { - res.Value.FootComments = col.CustomValues.FootComments - } - } - + service.MergeLoadedMetadata(res.Value, col.CustomValues) col.CustomValues = res.Value // Rebuild the unified entry list: cell edits can introduce diff --git a/ui/page/values_controller_actions.go b/ui/page/values_controller_actions.go index 4d826e2..a54fdf4 100644 --- a/ui/page/values_controller_actions.go +++ b/ui/page/values_controller_actions.go @@ -239,20 +239,32 @@ func (vc *ValuesController) onSaveColumnValues(colIdx int) { col := &vc.State.Columns[colIdx] - var ( - tree *yaml.Node - docs service.DocComments - loadedValues map[string]string - ) + // Byte-verbatim short-circuit for unmodified single-file loads. Future + // UI handlers that toggle indent/line-ending without touching values + // MUST set col.ValuesModified=true or their changes won't take effect. + if !col.ValuesModified && + col.CustomValues != nil && + len(col.CustomValues.RawBytes) > 0 && + len(col.CustomFilePaths) == 1 && + col.MergedFileCount <= 1 { + path := col.CustomFilePaths[0] + raw := col.CustomValues.RawBytes + write := func() { vc.saveRawBytesToFile(raw, path) } + + if vc.stashIfExternallyModified(colIdx, path, write) { + return + } - if col.CustomValues != nil { - tree = col.CustomValues.NodeTree - docs = col.DocCommentsForSave() - loadedValues = state.LoadedValuesMap(col.CustomValues) + write() + + return } + tree, docs, loadedValues, rawSource := col.SaveInputs() + yamlText, err := state.OverridesToYAML( - vc.State.Entries, col.OverrideEditors, col.YAMLIndent(), tree, docs, loadedValues, col.NullifiedKeys, + vc.State.Entries, col.OverrideEditors, col.YAMLIndent(), + tree, docs, loadedValues, col.NullifiedKeys, rawSource, ) if err != nil { vc.NotifState.Show(err.Error(), state.NotificationError, time.Now()) @@ -263,20 +275,14 @@ func (vc *ValuesController) onSaveColumnValues(colIdx int) { // If a file is already loaded, overwrite it directly without a save dialog. if len(col.CustomFilePaths) > 0 { path := col.CustomFilePaths[0] + enc, eol := columnFileEncoding(col) + write := func() { vc.saveToFile(yamlText, path, enc, eol) } - // Check if the file was modified externally since we loaded it. - if !col.FileModTime.IsZero() { - if info, err := os.Stat(path); err == nil && info.ModTime().After(col.FileModTime) { - vc.overwriteDialogActive = true - vc.overwritePendingCol = colIdx - vc.overwritePendingYAML = yamlText - - return - } + if vc.stashIfExternallyModified(colIdx, path, write) { + return } - enc, eol := columnFileEncoding(col) - vc.saveToFile(yamlText, path, enc, eol) + write() return } @@ -316,6 +322,37 @@ func (vc *ValuesController) saveToFile(yamlText, path, encoding, lineEnding stri }) } +func (vc *ValuesController) saveRawBytesToFile(raw []byte, path string) { + vc.ExportRunner.RunWithTimeout(config.FileExportOperation, func(ctx context.Context) (string, error) { + if err := vc.ValuesService.SaveRawBytes(ctx, raw, path); err != nil { + return "", fmt.Errorf("save raw bytes: %w", err) + } + + return path, nil + }) +} + +// stashIfExternallyModified stashes write for the overwrite dialog when the +// on-disk file is newer than the column's load-time snapshot. Returns true +// when the caller should bail; false to proceed with the save. +func (vc *ValuesController) stashIfExternallyModified(colIdx int, path string, write func()) bool { + col := &vc.State.Columns[colIdx] + if col.FileModTime.IsZero() { + return false + } + + info, err := os.Stat(path) + if err != nil || !info.ModTime().After(col.FileModTime) { + return false + } + + vc.overwriteDialogActive = true + vc.overwritePendingCol = colIdx + vc.overwritePendingWrite = write + + return true +} + // columnFileEncoding returns the source-file encoding/line-ending preserved on // the column's loaded CustomValues so SaveValuesFile can round-trip the // original BOM and CRLF shape. Returns ("", "") when the column has nothing @@ -336,7 +373,7 @@ func (vc *ValuesController) IsOverwriteDialogActive() bool { // DismissOverwriteDialog hides the overwrite confirmation dialog without saving. func (vc *ValuesController) DismissOverwriteDialog() { vc.overwriteDialogActive = false - vc.overwritePendingYAML = "" + vc.overwritePendingWrite = nil } // HandleOverwriteDialog processes confirm/cancel clicks on the overwrite-changes dialog. @@ -349,15 +386,13 @@ func (vc *ValuesController) HandleOverwriteDialog(gtx layout.Context) { case customwidget.ConfirmYes: vc.overwriteDialogActive = false - col := &vc.State.Columns[vc.overwritePendingCol] - if len(col.CustomFilePaths) > 0 { + if vc.overwritePendingWrite != nil { vc.pendingSave = saveValues vc.saveColumnIdx = vc.overwritePendingCol - enc, eol := columnFileEncoding(col) - vc.saveToFile(vc.overwritePendingYAML, col.CustomFilePaths[0], enc, eol) + vc.overwritePendingWrite() } - vc.overwritePendingYAML = "" + vc.overwritePendingWrite = nil case customwidget.ConfirmNo: vc.DismissOverwriteDialog() } @@ -1013,21 +1048,11 @@ func (vc *ValuesController) severAnchorsInSubtree(col *state.CustomColumnState, // editor changes — so the parse-and-republish state machine stays in // sync after a controller-initiated mutation. func (vc *ValuesController) publishColumnYAML(col *state.CustomColumnState, colIdx int) { - var ( - 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) - } + tree, docs, loadedValues, rawSource := col.SaveInputs() yamlText, err := state.OverridesToYAML( vc.State.Entries, col.OverrideEditors, col.YAMLIndent(), - tree, docs, loadedValues, col.NullifiedKeys, + tree, docs, loadedValues, col.NullifiedKeys, rawSource, ) vc.onColumnOverrideChanged(colIdx, yamlText, err) } diff --git a/ui/state/editor_state.go b/ui/state/editor_state.go index 798dde1..92f0768 100644 --- a/ui/state/editor_state.go +++ b/ui/state/editor_state.go @@ -67,13 +67,14 @@ func OverridesToYAML( docs service.DocComments, loadedValues map[string]string, nullified map[string]bool, + raw []byte, ) (string, error) { overrides := collectOverrides(entries, editors, loadedValues, nullified) if len(overrides) == 0 && docs.Head == "" && docs.Foot == "" && len(docs.Foots) == 0 { return "", nil } - yamlText, err := service.PatchNodeTree(tree, overrides, indent, docs) + yamlText, err := service.PatchSourceText(raw, tree, overrides, indent, docs) if err != nil { return "", fmt.Errorf("overrides to YAML: %w", err) } diff --git a/ui/state/save_redis_e2e_test.go b/ui/state/save_redis_e2e_test.go new file mode 100644 index 0000000..ecda027 --- /dev/null +++ b/ui/state/save_redis_e2e_test.go @@ -0,0 +1,344 @@ +package state + +import ( + "context" + "fmt" + "strings" + "testing" + + "gioui.org/widget" + + "github.com/qdeck-app/qdeck/service" +) + +const redisFixturePath = "../../assets/test-data/redis-values-cornercases.yaml" + +// TestSaveRedisFixture_SingleFieldEdit_OneLineDiff is the production-faithful +// end-to-end test for the save path. It loads the cornercase fixture through +// the SAME entrypoint the controller uses (LoadAndMergeCustomValues), then +// reparses the bytes through ParseEditorContent to mimic the keystroke-time +// onChange cycle — preserving load-time metadata via the carry-over the +// controller performs at ui/page/values_controller.go pollEditorParse — +// and finally runs the result through OverridesToYAML exactly as +// onSaveColumnValues does. +// +// Why the load path matters: LoadAndMergeCustomValues routes through +// ReadAndMergeCustomValues even for a single file. An earlier iteration +// only retained RawBytes in ReadCustomValues, so production loads +// silently lost the source bytes the splice path needs. This test calls +// the real load path so that regression can't slip back in. +func TestSaveRedisFixture_SingleFieldEdit_OneLineDiff(t *testing.T) { + t.Parallel() + + svc := service.NewValuesService() + + loaded, err := svc.LoadAndMergeCustomValues(context.Background(), []string{redisFixturePath}) + if err != nil { + t.Skipf("fixture not present or unreadable: %v", err) + } + + if len(loaded.RawBytes) == 0 { + t.Fatal("LoadAndMergeCustomValues didn't retain RawBytes for single-file load — splice will always fail in production") + } + + // Mimic the controller's editor-reparse cycle: ParseEditorContent + // returns a fresh FlatValues from raw text, and the controller's + // pollEditorParse calls MergeLoadedMetadata to carry forward + // load-time data (NodeTree, Indent, doc comments, RawBytes, + // per-entry head comments) from the previous CustomValues. + flat, err := svc.ParseEditorContent(context.Background(), string(loaded.RawBytes)) + if err != nil { + t.Fatalf("ParseEditorContent: %v", err) + } + + service.MergeLoadedMetadata(flat, loaded) + + const ( + targetKey = "architecture" + oldValue = "replication" + newValue = "standalone" + ) + + loadedValues := LoadedValuesMap(flat) + + editors := make([]widget.Editor, len(flat.Entries)) + + patched := false + + for i := range flat.Entries { + e := flat.Entries[i] + + var text string + + switch { + case !e.IsFocusable(): + text = e.Comment + case e.Key == targetKey: + if e.Value != oldValue { + t.Fatalf("fixture changed: %s is %q, expected %q", targetKey, e.Value, oldValue) + } + + text = loadFormForEditor(e.Comment, newValue) + patched = true + default: + text = loadFormForEditor(e.Comment, loadedValues[e.Key]) + } + + editors[i].SetText(text) + } + + if !patched { + t.Fatalf("target key %q not present in flat entries", targetKey) + } + + docs := service.DocComments{ + Head: flat.DocHeadComment, + Foot: flat.DocFootComment, + Foots: flat.FootComments, + SectionHeads: flat.SectionHeads, + } + + // First OverridesToYAML — same as the keystroke-time onChange path + // at ui/widget/override_table_events.go: rebuilds yaml from the + // editor state and feeds it to ParseEditorContent. Any reformatting + // that creeps in here gets baked into the post-keystroke col state. + keystrokeYAML, err := OverridesToYAML( + flat.Entries, editors, flat.Indent, flat.NodeTree, docs, loadedValues, nil, flat.RawBytes, + ) + if err != nil { + t.Fatalf("keystroke OverridesToYAML: %v", err) + } + + // Sanity check: the keystroke output must already be a one-line diff — + // otherwise the reparse cycle bakes encoder normalization into the + // post-keystroke state and the save call can never recover it. + keystrokeDiffs := lineDiffs(string(flat.RawBytes), keystrokeYAML) + if len(keystrokeDiffs) != 1 { + overrides := collectOverrides(flat.Entries, editors, loadedValues, nil) + + _, ok := service.PlanScalarSpliceForTest(flat.RawBytes, flat.NodeTree, overrides, docs) + t.Fatalf("keystroke YAML has %d diff lines (expected 1); splice viable: %v", + len(keystrokeDiffs), ok) + } + + // Simulate the EditorParseRunner cycle: ParseEditorContent reads + // keystrokeYAML and returns a fresh FlatValues; the controller's + // pollEditorParse then patches load-time metadata back onto it. + reparsed, err := svc.ParseEditorContent(context.Background(), keystrokeYAML) + if err != nil { + t.Fatalf("ParseEditorContent of keystroke YAML: %v", err) + } + + service.MergeLoadedMetadata(reparsed, loaded) + + // Re-build editors against the reparsed entries. In production the + // editors aren't reset on reparse — they keep the user's text — but + // the unified entry list IS rebuilt, so the index/key alignment + // drifts. Re-binding here mirrors that. + postKeystrokeEditors := make([]widget.Editor, len(reparsed.Entries)) + postKeystrokeLoaded := LoadedValuesMap(reparsed) + + for i := range reparsed.Entries { + e := reparsed.Entries[i] + + var text string + + switch { + case !e.IsFocusable(): + text = e.Comment + case e.Key == targetKey: + text = loadFormForEditor(e.Comment, newValue) + default: + text = loadFormForEditor(e.Comment, postKeystrokeLoaded[e.Key]) + } + + postKeystrokeEditors[i].SetText(text) + } + + postKeystrokeDocs := service.DocComments{ + Head: reparsed.DocHeadComment, + Foot: reparsed.DocFootComment, + Foots: reparsed.FootComments, + SectionHeads: reparsed.SectionHeads, + } + + // Save call — second OverridesToYAML. This is what hits disk. + got, err := OverridesToYAML( + reparsed.Entries, postKeystrokeEditors, reparsed.Indent, reparsed.NodeTree, + postKeystrokeDocs, postKeystrokeLoaded, nil, reparsed.RawBytes, + ) + if err != nil { + t.Fatalf("save OverridesToYAML: %v", err) + } + + diffs := lineDiffs(string(loaded.RawBytes), got) + if len(diffs) != 1 { + overrides := collectOverrides(reparsed.Entries, postKeystrokeEditors, postKeystrokeLoaded, nil) + + _, ok := service.PlanScalarSpliceForTest(reparsed.RawBytes, reparsed.NodeTree, overrides, postKeystrokeDocs) + t.Fatalf("expected exactly 1 line of diff, got %d; splice viable: %v\n%s", + len(diffs), ok, strings.Join(diffs, "\n")) + } + + if !strings.Contains(diffs[0], oldValue) || !strings.Contains(diffs[0], newValue) { + t.Errorf("diff line doesn't describe the %s→%s rewrite:\n %s", + oldValue, newValue, diffs[0]) + } +} + +// TestSaveRedisFixture_AddingChartDefaultKey_NoUnrelatedDiff covers the +// scenario the user hit in production: toggling a bool switch on a +// chart-default-only key copies the chart default into the override +// editor, producing an OverrideEntry for a path that isn't in the +// custom file's tree. Splice can't handle adds, so the encoder runs, +// and earlier the encoder normalized inline-comment column alignment +// across every other leaf in the file. +// +// After substituteUnchangedLinesFromSource lands, the diff should be +// limited to the newly-added lines — every other line must be byte- +// identical with the source even though the encoder re-emitted them. +// "Limited to newly-added lines" means: every removed-line in the diff +// is a blank line we can re-insert, OR the removed lines correspond to +// folded scalars that the encoder canonicalizes; every added-line is +// either a folded scalar's single-line form or the genuinely new key. +// +// The test asserts a much looser bound than the value-edit test: at +// most a small number of diff lines, dominated by encoder-canonicalized +// folded scalars (a separate yaml.v3 limitation) plus the new key +// itself. If we regress on inline-comment alignment, the count balloons +// and the test fails. +func TestSaveRedisFixture_AddingChartDefaultKey_NoUnrelatedDiff(t *testing.T) { + t.Parallel() + + svc := service.NewValuesService() + + loaded, err := svc.LoadAndMergeCustomValues(context.Background(), []string{redisFixturePath}) + if err != nil { + t.Skipf("fixture not present or unreadable: %v", err) + } + + loadedValues := LoadedValuesMap(loaded) + + editors := make([]widget.Editor, len(loaded.Entries)+1) + + for i := range loaded.Entries { + e := loaded.Entries[i] + + var text string + + switch { + case !e.IsFocusable(): + text = e.Comment + default: + text = loadFormForEditor(e.Comment, loadedValues[e.Key]) + } + + editors[i].SetText(text) + } + + // Simulate the user toggling a bool switch on a chart-only key — + // the cell handler writes the chart default into the override + // editor (see ui/widget/override_table_cells.go layoutBoolSwitchCell). + // We model this by appending an entry to the override list with the + // editor pre-populated. + const addedKey = "global.security.allowInsecureImages" + + entries := make([]service.FlatValueEntry, 0, len(loaded.Entries)+1) + entries = append(entries, loaded.Entries...) + entries = append(entries, service.FlatValueEntry{ + Key: addedKey, + Value: yamlTrueLiteralForTest, + Type: "bool", + }) + editors[len(loaded.Entries)].SetText(yamlTrueLiteralForTest) + + docs := service.DocComments{ + Head: loaded.DocHeadComment, + Foot: loaded.DocFootComment, + Foots: loaded.FootComments, + SectionHeads: loaded.SectionHeads, + } + + got, err := OverridesToYAML( + entries, editors, loaded.Indent, loaded.NodeTree, docs, loadedValues, nil, loaded.RawBytes, + ) + if err != nil { + t.Fatalf("OverridesToYAML: %v", err) + } + + // Set-based check: count source lines that DON'T appear anywhere in + // the output. Insertion of the added key shifts line positions, so a + // pure line-by-line diff would balloon with false positives — what + // matters here is whether the post-process preserved every source + // line somewhere in the output, not whether positions exactly align. + unmatched := unmatchedSourceLines(string(loaded.RawBytes), got) + + // Allow a small budget for yaml.v3 emitter quirks we don't fix: + // folded scalars collapse, !!set form converts, !!binary blank line. + // The strict goal is ≤ 1 (only the empty `disableCommands: []` + // could legitimately go missing if B's insertion misses an edge case). + const maxAllowedUnmatched = 8 + if unmatched > maxAllowedUnmatched { + t.Fatalf("expected ≤%d source lines missing from output, got %d", maxAllowedUnmatched, unmatched) + } +} + +// lineDiffs returns one descriptor per line index where a and b differ. +// Both inputs are right-trimmed of trailing newlines so a present-vs- +// absent final newline doesn't register. Lines beyond one side's length +// compare against the empty string, producing a diff entry per missing +// line. +func lineDiffs(a, b string) []string { + aLines := strings.Split(strings.TrimRight(a, "\n"), "\n") + bLines := strings.Split(strings.TrimRight(b, "\n"), "\n") + + n := max(len(bLines), len(aLines)) + + var diffs []string + + for i := range n { + var aL, bL string + if i < len(aLines) { + aL = aLines[i] + } + + if i < len(bLines) { + bL = bLines[i] + } + + if aL != bL { + diffs = append(diffs, fmt.Sprintf("line %d: %q -> %q", i+1, aL, bL)) + } + } + + return diffs +} + +// unmatchedSourceLines returns the count of source lines that don't +// appear (as exact string matches) anywhere in output. Used by the +// add-scenario test to assess source preservation without sensitivity +// to line-position shifts introduced by the encoder's added content. +func unmatchedSourceLines(src, out string) int { + outLines := strings.Split(out, "\n") + outCount := make(map[string]int, len(outLines)) + + for _, l := range outLines { + outCount[l]++ + } + + unmatched := 0 + + for srcLine := range strings.SplitSeq(src, "\n") { + if outCount[srcLine] > 0 { + outCount[srcLine]-- + + continue + } + + unmatched++ + } + + return unmatched +} + +const yamlTrueLiteralForTest = "true" diff --git a/ui/state/values_state.go b/ui/state/values_state.go index 8a56a63..a337bf0 100644 --- a/ui/state/values_state.go +++ b/ui/state/values_state.go @@ -8,6 +8,7 @@ import ( "time" "gioui.org/widget" + "gopkg.in/yaml.v3" "github.com/qdeck-app/qdeck/domain" "github.com/qdeck-app/qdeck/service" @@ -36,6 +37,24 @@ func (c *CustomColumnState) YAMLIndent() int { return service.DefaultYAMLIndent } +// SaveInputs bundles c.CustomValues fields OverridesToYAML needs at each save +// call site. Zero values when CustomValues is nil. +func (c *CustomColumnState) SaveInputs() ( + tree *yaml.Node, + docs service.DocComments, + loadedValues map[string]string, + rawSource []byte, +) { + if c == nil || c.CustomValues == nil { + return nil, service.DocComments{}, nil, nil + } + + return c.CustomValues.NodeTree, + c.DocCommentsForSave(), + LoadedValuesMap(c.CustomValues), + c.CustomValues.RawBytes +} + // DocCommentsForSave returns the doc-level orphan comments — banner, trailer, // and per-leaf foot blocks — that the save path writes back via the // DocumentNode wrapper and applyDocFoots. The fields are kept in their diff --git a/ui/widget/override_table_events.go b/ui/widget/override_table_events.go index 16fe1b3..ddfe404 100644 --- a/ui/widget/override_table_events.go +++ b/ui/widget/override_table_events.go @@ -14,7 +14,6 @@ import ( "gioui.org/layout" "gioui.org/op/clip" "gioui.org/widget" - "gopkg.in/yaml.v3" "github.com/qdeck-app/qdeck/service" "github.com/qdeck-app/qdeck/ui/state" @@ -144,25 +143,19 @@ func (t *OverrideTable) commitOverrideUpdate( indent := service.DefaultYAMLIndent - var ( - tree *yaml.Node - docs service.DocComments - loadedValues map[string]string - nullified map[string]bool - ) + var nullified map[string]bool - if cs := t.ColumnStates[c]; cs != nil { + cs := t.ColumnStates[c] + if cs != nil { indent = cs.YAMLIndent() nullified = cs.NullifiedKeys - - 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, loadedValues, nullified) + tree, docs, loadedValues, rawSource := cs.SaveInputs() + + yamlText, yamlErr := state.OverridesToYAML( + entries, editors, indent, tree, docs, loadedValues, nullified, rawSource, + ) t.OnChanged(c, yamlText, yamlErr) }