Skip to content

Commit 1c8eb85

Browse files
authored
Fix config sync when nested node is added to non-existing parent (#4378)
## Changes Fix the missing parent issue when adding new tags on the remote. When we do the addition operation to the config, we look for the error trace and see what intermediate nodes we should create. We do string matching, which is less robust, but acceptance tests should cover potential issues One potential caveat is that the created structure might be incomplete from a config perspective, but these should be filtered out early on the bundle plan level ## Why <!-- Why are these changes needed? Provide the context that the reviewer might be missing. For example, were there any decisions behind the change that are not reflected in the code itself? --> Example issue - add a tag when tags are not yet defined ## Tests Updated acceptance to capture the incorrect behavior <!-- If your PR needs to be included in the release notes for next release, add a separate entry in NEXT_CHANGELOG.md as part of your PR. -->
1 parent 1391e21 commit 1c8eb85

File tree

8 files changed

+190
-93
lines changed

8 files changed

+190
-93
lines changed

acceptance/bundle/config-remote-sync/job_fields/databricks.yml.tmpl

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,6 @@ resources:
1616
periodic:
1717
interval: 1
1818
unit: DAYS
19-
tags:
20-
env: dev
2119
environments:
2220
- environment_key: default
2321
spec:
@@ -32,3 +30,7 @@ resources:
3230
spark_version: $DEFAULT_SPARK_VERSION
3331
node_type_id: $NODE_TYPE_ID
3432
num_workers: 1
33+
34+
targets:
35+
default:
36+
mode: development

acceptance/bundle/config-remote-sync/job_fields/output.txt

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ Updating deployment state...
55
Deployment complete!
66

77
=== Modify job fields remotely
8+
89
=== Detect and save changes
910
Detected changes in 1 resource(s):
1011

@@ -27,7 +28,7 @@ Resource: resources.jobs.my_job
2728
-
2829
resources:
2930
jobs:
30-
@@ -8,15 +7,21 @@
31+
@@ -8,12 +7,17 @@
3132
on_success:
3233
- success@example.com
3334
+ no_alert_for_skipped_runs: true
@@ -49,8 +50,12 @@ Resource: resources.jobs.my_job
4950
- interval: 1
5051
+ interval: 2
5152
unit: DAYS
52-
tags:
53-
env: dev
54-
+ team: data
5553
environments:
56-
- environment_key: default
54+
@@ -31,5 +35,6 @@
55+
node_type_id: [NODE_TYPE_ID]
56+
num_workers: 1
57+
-
58+
+ tags:
59+
+ team: data
60+
targets:
61+
default:

acceptance/bundle/config-remote-sync/job_fields/script

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,14 @@ job_id="$(read_id.py my_job)"
88

99

1010
title "Modify job fields remotely"
11+
echo
1112
edit_resource.py jobs $job_id <<EOF
1213
r["email_notifications"]["on_failure"] = ["failure@example.com"]
1314
r["email_notifications"]["no_alert_for_skipped_runs"] = True
1415
r["parameters"].append({"name": "region", "default": "us-east-1"})
1516
r["trigger"]["periodic"]["interval"] = 2
17+
if "tags" not in r:
18+
r["tags"] = {}
1619
r["tags"]["team"] = "data"
1720
EOF
1821

acceptance/bundle/config-remote-sync/pipeline_fields/databricks.yml.tmpl

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,3 +17,7 @@ resources:
1717
libraries:
1818
- notebook:
1919
path: /Users/{{workspace_user_name}}/notebook
20+
21+
targets:
22+
default:
23+
mode: development

acceptance/bundle/config-remote-sync/pipeline_fields/output.txt

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ Resource: resources.pipelines.my_pipeline
1212
notifications[0].alerts: update
1313
notifications[0].email_recipients: update
1414
schema: update
15+
tags['foo']: update
1516

1617

1718
=== Configuration changes
@@ -25,7 +26,7 @@ Resource: resources.pipelines.my_pipeline
2526
-
2627
resources:
2728
pipelines:
28-
@@ -7,12 +6,15 @@
29+
@@ -7,16 +6,20 @@
2930
name: test-pipeline-[UNIQUE_NAME]
3031
catalog: main
3132
- schema: default
@@ -42,3 +43,9 @@ Resource: resources.pipelines.my_pipeline
4243
+ - on-update-failure
4344
libraries:
4445
- notebook:
46+
path: /Users/{{workspace_user_name}}/notebook
47+
-
48+
+ tags:
49+
+ foo: bar
50+
targets:
51+
default:

acceptance/bundle/config-remote-sync/pipeline_fields/script

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,9 @@ r["configuration"]["key2"] = "value2"
1212
r["notifications"][0]["email_recipients"].append("admin@example.com")
1313
r["notifications"][0]["alerts"].append("on-update-failure")
1414
r["schema"] = "prod"
15+
if "tags" not in r:
16+
r["tags"] = {}
17+
r["tags"]["foo"] = "bar"
1518
EOF
1619

1720
# TODO add support for permissions

bundle/configsync/patch.go

Lines changed: 136 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package configsync
33
import (
44
"context"
55
"encoding/json"
6+
"errors"
67
"fmt"
78
"os"
89
"reflect"
@@ -95,6 +96,11 @@ func ApplyChangesToYAML(ctx context.Context, b *bundle.Bundle, planChanges map[s
9596
return result, nil
9697
}
9798

99+
type parentNode struct {
100+
path yamlpatch.Path
101+
missingPath yamlpatch.Path
102+
}
103+
98104
// applyChanges applies all field changes to a YAML
99105
func applyChanges(ctx context.Context, filePath string, changes resolvedChanges, targetName string) (string, error) {
100106
content, err := os.ReadFile(filePath)
@@ -110,15 +116,15 @@ func applyChanges(ctx context.Context, filePath string, changes resolvedChanges,
110116

111117
for _, fieldPath := range fieldPaths {
112118
changeDesc := changes[fieldPath]
113-
jsonPointer, err := strPathToJSONPointer(fieldPath)
119+
fieldJsonPointer, err := strPathToJSONPointer(fieldPath)
114120
if err != nil {
115121
return "", fmt.Errorf("failed to convert field path %q to JSON pointer: %w", fieldPath, err)
116122
}
117123

118-
jsonPointers := []string{jsonPointer}
124+
jsonPointers := []string{fieldJsonPointer}
119125
if targetName != "" {
120126
targetPrefix := "/targets/" + targetName
121-
jsonPointers = append(jsonPointers, targetPrefix+jsonPointer)
127+
jsonPointers = append(jsonPointers, targetPrefix+fieldJsonPointer)
122128
}
123129

124130
hasConfigValue := changeDesc.Old != nil || changeDesc.New != nil
@@ -128,63 +134,93 @@ func applyChanges(ctx context.Context, filePath string, changes resolvedChanges,
128134

129135
success := false
130136
var lastErr error
131-
var lastPointer string
137+
var parentNodesToCreate []parentNode
138+
139+
normalizedRemote, err := normalizeValue(changeDesc.Remote)
140+
if err != nil {
141+
return "", fmt.Errorf("failed to normalize remote value for %s: %w", fieldJsonPointer, err)
142+
}
132143

133144
for _, jsonPointer := range jsonPointers {
134145
path, err := yamlpatch.ParsePath(jsonPointer)
135146
if err != nil {
136147
return "", fmt.Errorf("failed to parse JSON Pointer %s: %w", jsonPointer, err)
137148
}
138149

139-
var testOp yamlpatch.Operation
150+
patcher := gopkgv3yamlpatcher.New(gopkgv3yamlpatcher.IndentSpaces(2))
151+
var modifiedContent []byte
152+
var patchErr error
153+
140154
if isRemoval {
141-
testOp = yamlpatch.Operation{
155+
modifiedContent, patchErr = patcher.Apply(content, yamlpatch.Patch{yamlpatch.Operation{
142156
Type: yamlpatch.OperationRemove,
143157
Path: path,
144-
}
158+
}})
145159
} else if isReplacement {
146-
normalizedRemote, err := normalizeValue(changeDesc.Remote)
147-
if err != nil {
148-
return "", fmt.Errorf("failed to normalize replacement value for %s: %w", jsonPointer, err)
149-
}
150-
testOp = yamlpatch.Operation{
160+
modifiedContent, patchErr = patcher.Apply(content, yamlpatch.Patch{yamlpatch.Operation{
151161
Type: yamlpatch.OperationReplace,
152162
Path: path,
153163
Value: normalizedRemote,
154-
}
164+
}})
155165
} else if isAddition {
156-
normalizedRemote, err := normalizeValue(changeDesc.Remote)
157-
if err != nil {
158-
return "", fmt.Errorf("failed to normalize addition value for %s: %w", jsonPointer, err)
159-
}
160-
testOp = yamlpatch.Operation{
166+
modifiedContent, patchErr = patcher.Apply(content, yamlpatch.Patch{yamlpatch.Operation{
161167
Type: yamlpatch.OperationAdd,
162168
Path: path,
163169
Value: normalizedRemote,
170+
}})
171+
172+
// Collect parent path errors for later retry
173+
if patchErr != nil && isParentPathError(patchErr) {
174+
if missingPath, extractErr := extractMissingPath(patchErr); extractErr == nil {
175+
parentNodesToCreate = append(parentNodesToCreate, parentNode{path, missingPath})
176+
}
164177
}
165178
} else {
166179
return "", fmt.Errorf("unknown operation type for field %s", fieldPath)
167180
}
168181

169-
patcher := gopkgv3yamlpatcher.New(gopkgv3yamlpatcher.IndentSpaces(2))
170-
modifiedContent, err := patcher.Apply(content, yamlpatch.Patch{testOp})
171-
if err == nil {
182+
if patchErr == nil {
172183
content = modifiedContent
173-
log.Debugf(ctx, "Applied %s change to %s", testOp.Type, jsonPointer)
184+
log.Debugf(ctx, "Applied changes to %s", jsonPointer)
174185
success = true
186+
lastErr = nil
175187
break
176-
} else {
177-
log.Debugf(ctx, "Failed to apply change to %s: %v", jsonPointer, err)
178-
lastErr = err
179-
lastPointer = jsonPointer
188+
}
189+
190+
log.Debugf(ctx, "Failed to apply change to %s: %v", jsonPointer, patchErr)
191+
lastErr = patchErr
192+
}
193+
194+
// If all attempts failed with parent path errors, try creating nested structures
195+
if !success && len(parentNodesToCreate) > 0 {
196+
for _, errInfo := range parentNodesToCreate {
197+
nestedValue := buildNestedMaps(errInfo.path, errInfo.missingPath, normalizedRemote)
198+
199+
patcher := gopkgv3yamlpatcher.New(gopkgv3yamlpatcher.IndentSpaces(2))
200+
modifiedContent, patchErr := patcher.Apply(content, yamlpatch.Patch{yamlpatch.Operation{
201+
Type: yamlpatch.OperationAdd,
202+
Path: errInfo.missingPath,
203+
Value: nestedValue,
204+
}})
205+
206+
if patchErr == nil {
207+
content = modifiedContent
208+
lastErr = nil
209+
log.Debugf(ctx, "Created nested structure at %s", errInfo.missingPath.String())
210+
break
211+
}
212+
lastErr = patchErr
213+
log.Debugf(ctx, "Failed to create nested structure at %s: %v", errInfo.missingPath.String(), patchErr)
180214
}
181215
}
182-
if !success {
183-
if lastErr != nil {
184-
return "", fmt.Errorf("failed to apply change %s: %w", lastPointer, lastErr)
216+
217+
if lastErr != nil {
218+
if (isRemoval || isReplacement) && isPathNotFoundError(lastErr) {
219+
return "", fmt.Errorf("failed to apply change %s: field not found in YAML configuration: %w", fieldJsonPointer, lastErr)
185220
}
186-
return "", fmt.Errorf("failed to apply change for field %s: no valid target found", fieldPath)
221+
return "", fmt.Errorf("failed to apply change %s: %w", fieldJsonPointer, lastErr)
187222
}
223+
188224
}
189225

190226
return string(content), nil
@@ -208,10 +244,12 @@ func getResolvedFieldChanges(ctx context.Context, b *bundle.Bundle, planChanges
208244

209245
// If field has no location, find the parent resource's location to then add a new field
210246
if filePath == "" {
211-
filePath = findResourceFileLocation(ctx, b, resourceKey)
247+
resourceLocation := b.Config.GetLocation(resourceKey)
248+
filePath = resourceLocation.File
212249
if filePath == "" {
213-
continue
250+
return nil, fmt.Errorf("failed to find location for resource %s for a field %s", resourceKey, fieldPath)
214251
}
252+
215253
log.Debugf(ctx, "Field %s has no location, using resource location: %s", fullPath, filePath)
216254
}
217255

@@ -225,6 +263,72 @@ func getResolvedFieldChanges(ctx context.Context, b *bundle.Bundle, planChanges
225263
return resolvedChangesByFile, nil
226264
}
227265

266+
// isParentPathError checks if error indicates missing parent path.
267+
func isParentPathError(err error) bool {
268+
if err == nil {
269+
return false
270+
}
271+
msg := err.Error()
272+
return strings.Contains(msg, "parent path") && strings.Contains(msg, "does not exist")
273+
}
274+
275+
// isPathNotFoundError checks if error indicates the path itself does not exist.
276+
func isPathNotFoundError(err error) bool {
277+
if err == nil {
278+
return false
279+
}
280+
msg := err.Error()
281+
return strings.Contains(msg, "does not exist")
282+
}
283+
284+
// extractMissingPath extracts the missing path from error message like:
285+
// "op add /a/b/c/d: parent path /a/b does not exist"
286+
// Returns: "/a/b"
287+
func extractMissingPath(err error) (yamlpatch.Path, error) {
288+
msg := err.Error()
289+
start := strings.Index(msg, "parent path ")
290+
if start == -1 {
291+
return nil, errors.New("could not find 'parent path' in error message")
292+
}
293+
start += len("parent path ")
294+
295+
end := strings.Index(msg[start:], " does not exist")
296+
if end == -1 {
297+
return nil, errors.New("could not find 'does not exist' in error message")
298+
}
299+
300+
pathStr := msg[start : start+end]
301+
return yamlpatch.ParsePath(pathStr)
302+
}
303+
304+
// buildNestedMaps creates a nested map structure from targetPath to missingPath.
305+
// Example:
306+
//
307+
// targetPath: /a/b/c/d/e
308+
// missingPath: /a/b
309+
// leafValue: "foo"
310+
//
311+
// Returns: {c: {d: {e: "foo"}}}
312+
func buildNestedMaps(targetPath, missingPath yamlpatch.Path, leafValue any) any {
313+
missingLen := len(missingPath)
314+
targetLen := len(targetPath)
315+
316+
if missingLen >= targetLen {
317+
// Missing path is not a parent of target path
318+
return leafValue
319+
}
320+
321+
// Build nested structure from leaf to missing parent
322+
result := leafValue
323+
for i := targetLen - 1; i >= missingLen; i-- {
324+
result = map[string]any{
325+
targetPath[i]: result,
326+
}
327+
}
328+
329+
return result
330+
}
331+
228332
// strPathToJSONPointer converts a structpath string to JSON Pointer format.
229333
// Example: "resources.jobs.test[0].name" -> "/resources/jobs/test/0/name"
230334
func strPathToJSONPointer(pathStr string) (string, error) {
@@ -253,21 +357,3 @@ func strPathToJSONPointer(pathStr string) (string, error) {
253357
}
254358
return "/" + strings.Join(parts, "/"), nil
255359
}
256-
257-
// findResourceFileLocation finds the file where a resource is defined.
258-
// It checks both the root resources and target-specific overrides,
259-
// preferring the target override if it exists.
260-
func findResourceFileLocation(_ context.Context, b *bundle.Bundle, resourceKey string) string {
261-
targetName := b.Config.Bundle.Target
262-
263-
if targetName != "" {
264-
targetPath := "targets." + targetName + "." + resourceKey
265-
loc := b.Config.GetLocation(targetPath)
266-
if loc.File != "" {
267-
return loc.File
268-
}
269-
}
270-
271-
loc := b.Config.GetLocation(resourceKey)
272-
return loc.File
273-
}

0 commit comments

Comments
 (0)