Skip to content

Commit f531346

Browse files
committed
fix: return error on path collisions when unflattening manifests
setNestedValue used an unchecked type assertion that panicked when flattened paths disagreed about whether a node is a scalar or an object (e.g. "a.b" alongside "a.b.c"). Replace the assertion with the comma-ok form and propagate ErrInvalidManifest. Also catch the mirror case where a leaf would silently overwrite an existing nested object.
1 parent 6f49023 commit f531346

2 files changed

Lines changed: 53 additions & 7 deletions

File tree

internal/pkg/manifest/merge.go

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"maps"
77

88
"github.com/slackapi/slack-cli/internal/shared/types"
9+
"github.com/slackapi/slack-cli/internal/slackerror"
910
)
1011

1112
// Resolution represents which side the user chose for a given field.
@@ -101,7 +102,10 @@ func MergeAllFrom(local, remote types.AppManifest, diffs *DiffResult, strategy M
101102
// unflatten converts a flat dot-notation map back into a nested structure,
102103
// then marshals/unmarshals into AppManifest.
103104
func unflatten(flat map[string]any) (types.AppManifest, error) {
104-
nested := unflattenToMap(flat)
105+
nested, err := unflattenToMap(flat)
106+
if err != nil {
107+
return types.AppManifest{}, err
108+
}
105109
data, err := json.Marshal(nested)
106110
if err != nil {
107111
return types.AppManifest{}, fmt.Errorf("failed to marshal merged manifest: %w", err)
@@ -114,29 +118,43 @@ func unflatten(flat map[string]any) (types.AppManifest, error) {
114118
}
115119

116120
// unflattenToMap reconstructs a nested map from dot-notation paths.
117-
func unflattenToMap(flat map[string]any) map[string]any {
121+
func unflattenToMap(flat map[string]any) (map[string]any, error) {
118122
result := make(map[string]any)
119123
for path, value := range flat {
120-
setNestedValue(result, path, value)
124+
if err := setNestedValue(result, path, value); err != nil {
125+
return nil, err
126+
}
121127
}
122-
return result
128+
return result, nil
123129
}
124130

125-
func setNestedValue(m map[string]any, path string, value any) {
131+
func setNestedValue(m map[string]any, path string, value any) error {
126132
parts := splitPath(path)
127133
current := m
128134
for i, part := range parts {
129135
if i == len(parts)-1 {
136+
if existing, exists := current[part]; exists {
137+
if _, isMap := existing.(map[string]any); isMap {
138+
return slackerror.New(slackerror.ErrInvalidManifest).
139+
WithMessage("Conflicting types at manifest path %q: leaf value would overwrite a nested object", path)
140+
}
141+
}
130142
current[part] = value
131-
return
143+
return nil
132144
}
133145
next, exists := current[part]
134146
if !exists {
135147
next = make(map[string]any)
136148
current[part] = next
137149
}
138-
current = next.(map[string]any)
150+
nextMap, ok := next.(map[string]any)
151+
if !ok {
152+
return slackerror.New(slackerror.ErrInvalidManifest).
153+
WithMessage("Conflicting types at manifest path %q: cannot descend into a non-object value", path)
154+
}
155+
current = nextMap
139156
}
157+
return nil
140158
}
141159

142160
func splitPath(path string) []string {

internal/pkg/manifest/merge_test.go

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"testing"
55

66
"github.com/slackapi/slack-cli/internal/shared/types"
7+
"github.com/slackapi/slack-cli/internal/slackerror"
78
"github.com/stretchr/testify/assert"
89
"github.com/stretchr/testify/require"
910
)
@@ -99,6 +100,33 @@ func Test_Merge(t *testing.T) {
99100
}
100101
}
101102

103+
func Test_unflatten_PathCollision(t *testing.T) {
104+
tests := map[string]struct {
105+
flat map[string]any
106+
}{
107+
"leaf at parent path collides with deeper path": {
108+
flat: map[string]any{
109+
"a.b": "scalar",
110+
"a.b.c": "deep",
111+
},
112+
},
113+
"deeper path collides with leaf at parent": {
114+
flat: map[string]any{
115+
"a.b.c": "deep",
116+
"a.b": "scalar",
117+
},
118+
},
119+
}
120+
for name, tc := range tests {
121+
t.Run(name, func(t *testing.T) {
122+
_, err := unflatten(tc.flat)
123+
require.Error(t, err, "expected path-collision error, got nil")
124+
slackErr := slackerror.ToSlackError(err)
125+
assert.Equal(t, slackerror.ErrInvalidManifest, slackErr.Code)
126+
})
127+
}
128+
}
129+
102130
func Test_MergeAllFrom(t *testing.T) {
103131
local := types.AppManifest{
104132
DisplayInformation: types.DisplayInformation{Name: "Local", Description: "Local desc"},

0 commit comments

Comments
 (0)