Skip to content

Commit 6364af7

Browse files
denikclaude
andauthored
internal: add PathNode.HasPrefix() and use it instead of string-based HasPrefix (#4387)
## Why I want more code to use *PathNode instead of strings and this change facilitates that. We also will replace HasPrefix with MatchesPattern and this changes prepares for that. ## Tests Existing tests, new unit tests. --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 18ab5c5 commit 6364af7

File tree

4 files changed

+102
-31
lines changed

4 files changed

+102
-31
lines changed

bundle/deployplan/plan.go

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,8 +121,17 @@ func (c *Changes) HasChange(fieldPath string) bool {
121121
return false
122122
}
123123

124+
fieldPathNode, err := structpath.Parse(fieldPath)
125+
if err != nil {
126+
return false
127+
}
128+
124129
for field := range *c {
125-
if structpath.HasPrefix(field, fieldPath) {
130+
fieldNode, err := structpath.Parse(field)
131+
if err != nil {
132+
continue
133+
}
134+
if fieldNode.HasPrefix(fieldPathNode) {
126135
return true
127136
}
128137
}

bundle/direct/bundle_plan.go

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -383,7 +383,7 @@ func addPerFieldActions(ctx context.Context, adapter *dresources.Adapter, change
383383
// Empty struct in config should not cause drift when remote has values
384384
ch.Action = deployplan.Skip
385385
ch.Reason = deployplan.ReasonEmptyStruct
386-
} else if action := shouldIgnore(cfg, pathString); action != deployplan.Undefined {
386+
} else if action := shouldIgnore(cfg, path); action != deployplan.Undefined {
387387
ch.Action = action
388388
ch.Reason = deployplan.ReasonBuiltinRule
389389
} else if ch.New == nil && ch.Old == nil && ch.Remote != nil && path.IsDotString() {
@@ -393,7 +393,7 @@ func addPerFieldActions(ctx context.Context, adapter *dresources.Adapter, change
393393
// Note, we only consider struct fields here. Adding/removing elements to/from maps and slices should trigger updates.
394394
ch.Action = deployplan.Skip
395395
ch.Reason = deployplan.ReasonServerSideDefault
396-
} else if action := shouldUpdateOrRecreate(cfg, pathString); action != deployplan.Undefined {
396+
} else if action := shouldUpdateOrRecreate(cfg, path); action != deployplan.Undefined {
397397
ch.Action = action
398398
ch.Reason = deployplan.ReasonBuiltinRule
399399
} else {
@@ -419,29 +419,29 @@ func addPerFieldActions(ctx context.Context, adapter *dresources.Adapter, change
419419
return nil
420420
}
421421

422-
func shouldIgnore(cfg *dresources.ResourceLifecycleConfig, pathString string) deployplan.ActionType {
422+
func shouldIgnore(cfg *dresources.ResourceLifecycleConfig, path *structpath.PathNode) deployplan.ActionType {
423423
if cfg == nil {
424424
return deployplan.Undefined
425425
}
426426
for _, p := range cfg.IgnoreRemoteChanges {
427-
if structpath.HasPrefix(pathString, p.String()) {
427+
if path.HasPrefix(p) {
428428
return deployplan.Skip
429429
}
430430
}
431431
return deployplan.Undefined
432432
}
433433

434-
func shouldUpdateOrRecreate(cfg *dresources.ResourceLifecycleConfig, pathString string) deployplan.ActionType {
434+
func shouldUpdateOrRecreate(cfg *dresources.ResourceLifecycleConfig, path *structpath.PathNode) deployplan.ActionType {
435435
if cfg == nil {
436436
return deployplan.Undefined
437437
}
438438
for _, p := range cfg.RecreateOnChanges {
439-
if structpath.HasPrefix(pathString, p.String()) {
439+
if path.HasPrefix(p) {
440440
return deployplan.Recreate
441441
}
442442
}
443443
for _, p := range cfg.UpdateIDOnChanges {
444-
if structpath.HasPrefix(pathString, p.String()) {
444+
if path.HasPrefix(p) {
445445
return deployplan.UpdateWithID
446446
}
447447
}

libs/structs/structpath/path.go

Lines changed: 45 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -669,40 +669,64 @@ func (p *PathNode) Prefix(n int) *PathNode {
669669
return current
670670
}
671671

672-
// HasPrefix tests whether the path string s begins with prefix.
673-
// Unlike strings.HasPrefix, this function is path-aware and correctly handles
674-
// path component boundaries. For example:
675-
// - HasPrefix("a.b", "a") returns true (matches field "a")
676-
// - HasPrefix("aa", "a") returns false (field "aa" is different from "a")
677-
// - HasPrefix("a[0]", "a") returns true (matches field "a")
678-
// - HasPrefix("config.name", "config") returns true
679-
func HasPrefix(s, prefix string) bool {
680-
// Handle edge cases
681-
if prefix == "" {
672+
// HasPrefix tests whether the path begins with the given prefix PathNode.
673+
// Returns true if all components of prefix match the first components of this path.
674+
// A nil prefix (root) always returns true.
675+
//
676+
// Examples:
677+
// - "a.b".HasPrefix("a") returns true
678+
// - "a.b".HasPrefix("a.b.c") returns false (prefix is longer)
679+
// - "items[0].name".HasPrefix("items[0]") returns true
680+
// - "items[0].name".HasPrefix("items[1]") returns false (different index)
681+
func (p *PathNode) HasPrefix(prefix *PathNode) bool {
682+
// Nil prefix (root) is always a prefix
683+
if prefix.IsRoot() {
682684
return true
683685
}
684-
if s == "" {
686+
687+
// Nil path (root) can only have nil prefix
688+
if p.IsRoot() {
685689
return false
686690
}
687-
if s == prefix {
688-
return true
689-
}
690691

691-
// Check if s starts with prefix using string comparison
692-
if !strings.HasPrefix(s, prefix) {
692+
// Get lengths
693+
pLen := p.Len()
694+
prefixLen := prefix.Len()
695+
696+
// Prefix cannot be longer than the path
697+
if prefixLen > pLen {
693698
return false
694699
}
695700

696-
// Ensure the match is at a path boundary
697-
// The character after the prefix must be a path separator: '.', '[', or end of string
698-
if len(s) > len(prefix) {
699-
nextChar := s[len(prefix)]
700-
return nextChar == '.' || nextChar == '['
701+
// Get to the position in p that corresponds to the last node of prefix
702+
pAtPrefixLen := p.Prefix(prefixLen)
703+
704+
// Walk both paths backwards and compare each node
705+
currentP := pAtPrefixLen
706+
currentPrefix := prefix
707+
708+
for currentPrefix != nil {
709+
if !nodesEqual(currentP, currentPrefix) {
710+
return false
711+
}
712+
currentP = currentP.prev
713+
currentPrefix = currentPrefix.prev
701714
}
702715

703716
return true
704717
}
705718

719+
// nodesEqual compares two PathNodes for equality (without comparing prev pointers).
720+
func nodesEqual(a, b *PathNode) bool {
721+
if a == nil && b == nil {
722+
return true
723+
}
724+
if a == nil || b == nil {
725+
return false
726+
}
727+
return a.key == b.key && a.index == b.index && a.value == b.value
728+
}
729+
706730
// MarshalYAML implements yaml.Marshaler for PathNode.
707731
func (p *PathNode) MarshalYAML() (any, error) {
708732
return p.String(), nil

libs/structs/structpath/path_test.go

Lines changed: 40 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -744,12 +744,50 @@ func TestHasPrefix(t *testing.T) {
744744
prefix: "a[*]",
745745
expected: false,
746746
},
747+
748+
// Exact component matching - array indices, bracket keys, and key-value notation
749+
{
750+
name: "prefix longer than path",
751+
s: "a.b",
752+
prefix: "a.b.c",
753+
expected: false,
754+
},
755+
{
756+
name: "different array indices",
757+
s: "items[0].name",
758+
prefix: "items[1]",
759+
expected: false,
760+
},
761+
{
762+
name: "different bracket keys",
763+
s: "config['spark.conf']",
764+
prefix: "config['other.conf']",
765+
expected: false,
766+
},
767+
{
768+
name: "key-value in prefix",
769+
s: "tasks[task_key='my_task'].notebook_task.source",
770+
prefix: "tasks[task_key='my_task']",
771+
expected: true,
772+
},
773+
{
774+
name: "different key-value values",
775+
s: "tasks[task_key='my_task']",
776+
prefix: "tasks[task_key='other_task']",
777+
expected: false,
778+
},
747779
}
748780

749781
for _, tt := range tests {
750782
t.Run(tt.name, func(t *testing.T) {
751-
result := HasPrefix(tt.s, tt.prefix)
752-
assert.Equal(t, tt.expected, result, "HasPrefix(%q, %q)", tt.s, tt.prefix)
783+
path, err := Parse(tt.s)
784+
require.NoError(t, err)
785+
786+
prefix, err := Parse(tt.prefix)
787+
require.NoError(t, err)
788+
789+
result := path.HasPrefix(prefix)
790+
assert.Equal(t, tt.expected, result, "path.HasPrefix(prefix) where path=%q, prefix=%q", tt.s, tt.prefix)
753791
})
754792
}
755793
}

0 commit comments

Comments
 (0)