Skip to content

Commit 9ddb189

Browse files
fix(reformat): reformat overwrites content (#4672)
This pull request fixes an issue where the current reformat logic could unintentionally overwrite content following the `query` field in an artifact. This most commonly affected artifacts that define additional fields such as `notebook` or `column_types`. For example, given the following artifact with a `notebook` field: ```yaml name: Notebook sources: - query: | SELECT A,B,C FROM scope() notebook: - name: Test type: vql_suggestion template: | SELECT * FROM scope() ``` Running the reformat command (either via CLI or the GUI) would remove the `notebook:` line. Subsequent reformats would continue stripping content, producing an invalid artifact such as: ```yaml name: Notebook sources: - query: | SELECT A, B, C FROM scope() - name: Test type: vql_suggestion template: | SELECT * FROM scope() ``` > Error: While parsing VQL at line 8: unexpected token "-" The root cause was an off-by-one error in the mutation logic applied to the `query` block, which caused the reformat operation to extend beyond the intended range. While addressing this, I also identified an issue with how `original_end_line` was calculated for chomping indicator nodes (`|-`). Because these nodes omit the trailing newline, splitting on `\n` resulted in an incorrect end line calculation. Additionally, the `current_mu.err` value was never checked when applying mutations that only had a length of one. This has also been resolved by adding an additional check at the beginning of the line iteration. This change corrects these issues and adds regression tests to verify that: - Content following a `query` block is preserved. - Running reformat multiple times produces stable, idempotent output. Please let me know if you'd like further clarification, or if there are any changes you'd like made. I am also considering a refactor of the `artifacts reformat` to call a `reformat` VQL function which wraps this functionality, following the pattern that CLI commands are just wrappers around VQL. Let me know if this is also something you'd be interested in me contributing. 😃
1 parent 8d84532 commit 9ddb189

File tree

3 files changed

+90
-7
lines changed

3 files changed

+90
-7
lines changed

services/repository/fixtures/TestReformat.golden

Lines changed: 36 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,14 +23,16 @@
2323
"- query: SELECT * FROM info()",
2424
"- query: |",
2525
" SELECT A",
26-
" FROM scope()"
26+
" FROM scope()",
27+
""
2728
],
2829
"export reformatted": [
2930
"",
3031
"name: Exported",
3132
"export: |",
3233
" SELECT *",
33-
" FROM scope()"
34+
" FROM scope()",
35+
""
3436
],
3537
"preconditions": [
3638
"",
@@ -39,6 +41,37 @@
3941
"sources:",
4042
"- precondition: |",
4143
" SELECT *",
42-
" FROM info()"
44+
" FROM info()",
45+
""
46+
],
47+
"notebook": [
48+
"",
49+
"name: Notebook",
50+
"sources:",
51+
"- query: |",
52+
" SELECT A,",
53+
" B,",
54+
" C",
55+
" FROM scope()",
56+
" notebook:",
57+
" - name: Test",
58+
" type: vql_suggestion",
59+
" template: |",
60+
" SELECT * FROM scope()",
61+
""
62+
],
63+
"column types": [
64+
"",
65+
"name: Column Types",
66+
"sources:",
67+
"- query: |",
68+
" SELECT A,",
69+
" B,",
70+
" C",
71+
" FROM scope()",
72+
"column_types:",
73+
" - name: A",
74+
" type: string",
75+
""
4376
]
4477
}

services/repository/reformat.go

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -85,11 +85,16 @@ func reformatNode(vql_node yaml.NodeContext) (m mutation, err error) {
8585
indented = append(indented, ind+l)
8686
}
8787

88+
// Check if node style is chomping (no final new line)
89+
length := len(strings.Split(vql_node.Value, "\n"))
90+
if strings.HasSuffix(vql_node.Value, "\n") {
91+
length--
92+
}
93+
8894
return mutation{
8995
original_start_line: vql_node.Line,
90-
original_end_line: vql_node.Line +
91-
len(strings.Split(vql_node.Value, "\n")) - 1,
92-
replacement: indented,
96+
original_end_line: vql_node.Line + length,
97+
replacement: indented,
9398
}, nil
9499
}
95100

@@ -103,6 +108,10 @@ func applyMutations(text string, mu []mutation) (string, error) {
103108
current_mu_idx := 0
104109

105110
for i := 0; i < len(lines); {
111+
if current_mu.err != nil {
112+
return text, current_mu.err
113+
}
114+
106115
if i < current_mu.original_start_line {
107116
result = append(result, lines[i])
108117
i++
@@ -111,10 +120,15 @@ func applyMutations(text string, mu []mutation) (string, error) {
111120

112121
if i == current_mu.original_start_line {
113122
result = append(result, current_mu.replacement...)
123+
114124
i = current_mu.original_end_line
125+
if current_mu.original_end_line == current_mu.original_start_line {
126+
i++
127+
}
128+
115129
if current_mu_idx+1 >= len(mu) {
116130
// No more mutations, just copy the rest and return
117-
result = append(result, lines[i+1:]...)
131+
result = append(result, lines[i:]...)
118132
return strings.Join(result, "\n"), nil
119133
}
120134
current_mu_idx++

services/repository/reformat_test.go

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,30 @@ precondition: SELECT * FROM info()
4747
sources:
4848
- precondition: |
4949
SELECT * FROM info()
50+
`}, {
51+
name: "notebook",
52+
in: `
53+
name: Notebook
54+
sources:
55+
- query: |
56+
SELECT A,B,C
57+
FROM scope()
58+
notebook:
59+
- name: Test
60+
type: vql_suggestion
61+
template: |
62+
SELECT * FROM scope()
63+
`}, {
64+
name: "column types",
65+
in: `
66+
name: Column Types
67+
sources:
68+
- query: |
69+
SELECT A,B,C
70+
FROM scope()
71+
column_types:
72+
- name: A
73+
type: string
5074
`},
5175
}
5276

@@ -59,3 +83,15 @@ func TestReformat(t *testing.T) {
5983
}
6084
goldie.AssertJson(t, "TestReformat", golden)
6185
}
86+
87+
// Test that when VQL is reformatted multiple times it doesn't change.
88+
func TestReformatMultiple(t *testing.T) {
89+
golden := ordereddict.NewDict()
90+
for _, c := range reformatCases {
91+
first, err := reformatVQL(c.in)
92+
second, err := reformatVQL(first)
93+
assert.NoError(t, err)
94+
golden.Set(c.name, strings.Split(second, "\n"))
95+
}
96+
goldie.AssertJson(t, "TestReformat", golden)
97+
}

0 commit comments

Comments
 (0)