Skip to content

Commit 85ba870

Browse files
golanglemonadegolanglemonade
authored
fix: nil pointer bug due to non comparable map (#2357)
Signed-off-by: Sarah Funkhouser <147884153+golanglemonade@users.noreply.github.com>
1 parent c2330b0 commit 85ba870

2 files changed

Lines changed: 275 additions & 36 deletions

File tree

‎pkg/slateparser/utils.go‎

Lines changed: 68 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ package slateparser
22

33
import (
44
"encoding/json"
5-
"maps"
5+
"strings"
66
)
77

88
// ContainsCommentsInTextJSON checks if the provided slate JSON elements contain any comments
@@ -20,8 +20,10 @@ func ContainsCommentsInTextJSON(elements []any) bool {
2020
return false
2121
}
2222

23+
// getChildrenFromSlateTextJSON recursively collects all leaf nodes (nodes with a "text" key)
24+
// from the slate element tree, handling both JSON string and map[string]any inputs
2325
func getChildrenFromSlateTextJSON(elements []any) []any {
24-
children := make([]any, 0)
26+
var leaves []any
2527
for _, elem := range elements {
2628
var m map[string]any
2729
switch v := elem.(type) {
@@ -34,67 +36,97 @@ func getChildrenFromSlateTextJSON(elements []any) []any {
3436
default:
3537
continue
3638
}
39+
collectLeafNodes(m, &leaves)
40+
}
41+
return leaves
42+
}
3743

38-
if c, ok := m["children"].([]any); ok {
39-
children = append(children, c...)
44+
// collectLeafNodes walks a slate node tree and appends leaf nodes (those with a "text" key) to leaves
45+
func collectLeafNodes(m map[string]any, leaves *[]any) {
46+
if _, hasText := m["text"]; hasText {
47+
*leaves = append(*leaves, m)
48+
return
49+
}
50+
51+
if children, ok := m["children"].([]any); ok {
52+
for _, child := range children {
53+
if childMap, ok := child.(map[string]any); ok {
54+
collectLeafNodes(childMap, leaves)
55+
}
4056
}
4157
}
58+
}
59+
60+
// valEqualBestEffort compares two any values for scalar JSON types without panicking on non-comparable types
61+
func valEqualBestEffort(a, b any) bool {
62+
switch av := a.(type) {
63+
case string:
64+
bv, ok := b.(string)
65+
return ok && av == bv
66+
case bool:
67+
bv, ok := b.(bool)
68+
return ok && av == bv
69+
case float64:
70+
bv, ok := b.(float64)
71+
return ok && av == bv
72+
case nil:
73+
return b == nil
74+
default:
75+
// non-comparable type (slice, nested map, etc.) — conservatively treat as not equal
76+
return false
77+
}
78+
}
4279

43-
return children
80+
func isCommentKey(key string) bool {
81+
return key == "comment" || strings.HasPrefix(key, "comment_")
4482
}
4583

4684
// OnlyCommentsAdded checks if the only changes between the old and new slate JSON elements are the addition of comments
4785
func OnlyCommentsAdded(oldText []any, newText []any) bool {
48-
// for each I want to see if the only change is the addition of a comment, if so return true, otherwise false
49-
oldChildren := getChildrenFromSlateTextJSON(oldText)
50-
newChildren := getChildrenFromSlateTextJSON(newText)
86+
oldLeaves := getChildrenFromSlateTextJSON(oldText)
87+
newLeaves := getChildrenFromSlateTextJSON(newText)
5188

52-
if len(oldChildren) != len(newChildren) {
89+
if len(oldLeaves) != len(newLeaves) || len(newLeaves) == 0 {
5390
return false
5491
}
5592

56-
// compare all children, and check the text is the same, comments are OK
57-
for i, oldChild := range oldChildren {
58-
// get the map
59-
oldChildMap, oldOK := oldChild.(map[string]any)
60-
newChildMap, newOK := newChildren[i].(map[string]any)
93+
for i, oldChild := range oldLeaves {
94+
oldLeaf, oldOK := oldChild.(map[string]any)
95+
newLeaf, newOK := newLeaves[i].(map[string]any)
6196

62-
// if its not a map, we can't compare, so we assume it's not just a comment change and return false
63-
if !oldOK || !newOK {
97+
if !oldOK || !newOK || oldLeaf == nil || newLeaf == nil {
6498
return false
6599
}
66100

67-
// if they are equal, continue to the next one
68-
if maps.Equal(oldChildMap, newChildMap) {
69-
continue
101+
// text must be unchanged
102+
oldTextStr, _ := oldLeaf["text"].(string)
103+
newTextStr, _ := newLeaf["text"].(string)
104+
if oldTextStr != newTextStr {
105+
return false
70106
}
71107

72-
allowedKeys := map[string]bool{
73-
"text": true,
74-
"comment": true,
75-
}
108+
// new leaf may only add comment-related keys; all other keys must exist in old with equal values
109+
for key, newVal := range newLeaf {
110+
if isCommentKey(key) {
111+
continue
112+
}
76113

77-
// if there are other keys besides text and comment, return false
78-
for key := range newChildMap {
79-
if !allowedKeys[key] {
114+
oldVal, exists := oldLeaf[key]
115+
if !exists || !valEqualBestEffort(oldVal, newVal) {
80116
return false
81117
}
82118
}
83119

84-
// if they are not equal, check the text is the same
85-
oldText, oldOK := oldChildMap["text"]
86-
newText, newOK := newChildMap["text"]
87-
88-
if oldOK && newOK {
89-
oldTextStr, oldOK := oldText.(string)
90-
newTextStr, newOK := newText.(string)
120+
// no non-comment keys should be removed from old
121+
for key := range oldLeaf {
122+
if isCommentKey(key) {
123+
continue
124+
}
91125

92-
// if they are strings and they are not equal, then it's not just a comment change, return false
93-
if oldOK && newOK && oldTextStr != newTextStr {
126+
if _, exists := newLeaf[key]; !exists {
94127
return false
95128
}
96129
}
97-
98130
}
99131

100132
return true

‎pkg/slateparser/utils_test.go‎

Lines changed: 207 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,17 @@ func TestOnlyCommentsAdded(t *testing.T) {
203203
assert.Check(t, slateparser.OnlyCommentsAdded(oldText, newText))
204204
})
205205

206+
t.Run("child with non-comparable value does not panic", func(t *testing.T) {
207+
// []any values can't be safely compared; best-effort returns false rather than panicking
208+
makeSlateWith := func(child map[string]any) []any {
209+
return []any{map[string]any{"type": "paragraph", "children": []any{child}}}
210+
}
211+
marks := []any{"bold"}
212+
oldText := makeSlateWith(map[string]any{"text": "hello", "marks": marks})
213+
newText := makeSlateWith(map[string]any{"text": "hello", "marks": marks})
214+
assert.Check(t, !slateparser.OnlyCommentsAdded(oldText, newText))
215+
})
216+
206217
t.Run("multiple children, extra key added", func(t *testing.T) {
207218
oldText := makeSlate(
208219
map[string]any{"text": "a"},
@@ -214,4 +225,200 @@ func TestOnlyCommentsAdded(t *testing.T) {
214225
)
215226
assert.Check(t, !slateparser.OnlyCommentsAdded(oldText, newText))
216227
})
228+
229+
// bold/italic/underline are stored as booleans on leaf nodes in Slate
230+
t.Run("bold mark unchanged, comment added", func(t *testing.T) {
231+
oldText := makeSlate(map[string]any{"text": "hello", "bold": true})
232+
newText := makeSlate(map[string]any{"text": "hello", "bold": true, "comment": true})
233+
assert.Check(t, slateparser.OnlyCommentsAdded(oldText, newText))
234+
})
235+
236+
t.Run("bold mark changed", func(t *testing.T) {
237+
oldText := makeSlate(map[string]any{"text": "hello", "bold": true})
238+
newText := makeSlate(map[string]any{"text": "hello", "bold": false})
239+
assert.Check(t, !slateparser.OnlyCommentsAdded(oldText, newText))
240+
})
241+
242+
t.Run("bold mark added (formatting change, not comment)", func(t *testing.T) {
243+
oldText := makeSlate(map[string]any{"text": "hello"})
244+
newText := makeSlate(map[string]any{"text": "hello", "bold": true})
245+
assert.Check(t, !slateparser.OnlyCommentsAdded(oldText, newText))
246+
})
247+
248+
t.Run("multiple marks unchanged, comment added", func(t *testing.T) {
249+
oldText := makeSlate(map[string]any{"text": "hello", "bold": true, "italic": true, "underline": true})
250+
newText := makeSlate(map[string]any{"text": "hello", "bold": true, "italic": true, "underline": true, "comment": true})
251+
assert.Check(t, slateparser.OnlyCommentsAdded(oldText, newText))
252+
})
253+
254+
// Slate adds both "comment" and "comment_<id>" keys when creating a comment
255+
t.Run("comment and comment_id keys added", func(t *testing.T) {
256+
oldText := makeSlate(map[string]any{"text": "hello"})
257+
newText := makeSlate(map[string]any{"text": "hello", "comment": true, "comment_MDHGnHfbfTfX": true})
258+
assert.Check(t, slateparser.OnlyCommentsAdded(oldText, newText))
259+
})
260+
261+
t.Run("multiple comment_id keys added", func(t *testing.T) {
262+
oldText := makeSlate(map[string]any{"text": "hello"})
263+
newText := makeSlate(map[string]any{"text": "hello", "comment": true, "comment_abc": true, "comment_xyz": true})
264+
assert.Check(t, slateparser.OnlyCommentsAdded(oldText, newText))
265+
})
266+
267+
t.Run("bold with comment_id added", func(t *testing.T) {
268+
oldText := makeSlate(map[string]any{"text": "hello", "bold": true})
269+
newText := makeSlate(map[string]any{"text": "hello", "bold": true, "comment": true, "comment_abc123": true})
270+
assert.Check(t, slateparser.OnlyCommentsAdded(oldText, newText))
271+
})
272+
273+
// Slate table elements have colSizes []float64 on the element node (not on leaf nodes),
274+
// and deeply nested children: table → tr → td → p → {text: "..."}
275+
t.Run("table with colSizes, no changes", func(t *testing.T) {
276+
makeTable := func() []any {
277+
return []any{map[string]any{
278+
"type": "table",
279+
"id": "tbl1",
280+
"colSizes": []any{0.0, 190.45703125, 296.11328125},
281+
"children": []any{
282+
map[string]any{
283+
"type": "tr", "id": "tr1",
284+
"children": []any{
285+
map[string]any{
286+
"type": "td", "id": "td1",
287+
"children": []any{
288+
map[string]any{
289+
"type": "p", "id": "p1",
290+
"children": []any{map[string]any{"text": "Version"}},
291+
},
292+
},
293+
},
294+
map[string]any{
295+
"type": "td", "id": "td2",
296+
"children": []any{
297+
map[string]any{
298+
"type": "p", "id": "p2",
299+
"children": []any{map[string]any{"text": "Date"}},
300+
},
301+
},
302+
},
303+
},
304+
},
305+
},
306+
}}
307+
}
308+
assert.Check(t, slateparser.OnlyCommentsAdded(makeTable(), makeTable()))
309+
})
310+
311+
t.Run("table with colSizes, comment added to leaf", func(t *testing.T) {
312+
makeTableLeaf := func(leaf map[string]any) []any {
313+
return []any{map[string]any{
314+
"type": "table",
315+
"id": "tbl1",
316+
"colSizes": []any{0.0, 190.45703125},
317+
"children": []any{
318+
map[string]any{
319+
"type": "tr", "id": "tr1",
320+
"children": []any{
321+
map[string]any{
322+
"type": "td", "id": "td1",
323+
"children": []any{
324+
map[string]any{
325+
"type": "p", "id": "p1",
326+
"children": []any{leaf},
327+
},
328+
},
329+
},
330+
},
331+
},
332+
},
333+
}}
334+
}
335+
oldText := makeTableLeaf(map[string]any{"text": "Version"})
336+
newText := makeTableLeaf(map[string]any{"text": "Version", "comment": true, "comment_abc": true})
337+
assert.Check(t, slateparser.OnlyCommentsAdded(oldText, newText))
338+
})
339+
340+
t.Run("table with colSizes, text changed in leaf", func(t *testing.T) {
341+
makeTableLeaf := func(text string) []any {
342+
return []any{map[string]any{
343+
"type": "table",
344+
"id": "tbl1",
345+
"colSizes": []any{0.0, 190.45703125},
346+
"children": []any{
347+
map[string]any{
348+
"type": "tr", "id": "tr1",
349+
"children": []any{
350+
map[string]any{
351+
"type": "td", "id": "td1",
352+
"children": []any{
353+
map[string]any{
354+
"type": "p", "id": "p1",
355+
"children": []any{map[string]any{"text": text}},
356+
},
357+
},
358+
},
359+
},
360+
},
361+
},
362+
}}
363+
}
364+
assert.Check(t, !slateparser.OnlyCommentsAdded(makeTableLeaf("Version"), makeTableLeaf("Changed")))
365+
})
366+
367+
// td cells in some Slate table plugins carry colSpan/rowSpan as float64
368+
t.Run("table cell with colSpan and rowSpan, comment added to leaf", func(t *testing.T) {
369+
makeCell := func(leaf map[string]any) []any {
370+
return []any{map[string]any{
371+
"type": "table", "id": "tbl1",
372+
"children": []any{
373+
map[string]any{
374+
"type": "tr", "id": "tr1",
375+
"children": []any{
376+
map[string]any{
377+
"type": "td", "id": "td1",
378+
"colSpan": float64(1), "rowSpan": float64(1),
379+
"children": []any{
380+
map[string]any{
381+
"type": "p", "id": "p1",
382+
"children": []any{leaf},
383+
},
384+
},
385+
},
386+
},
387+
},
388+
},
389+
}}
390+
}
391+
oldText := makeCell(map[string]any{"text": "hello"})
392+
newText := makeCell(map[string]any{"text": "hello", "comment": true})
393+
assert.Check(t, slateparser.OnlyCommentsAdded(oldText, newText))
394+
})
395+
396+
// list items use indent (float64) and listStyleType (string) on the element, not the leaf
397+
t.Run("list item with indent, no changes", func(t *testing.T) {
398+
makeList := func(leaf map[string]any) []any {
399+
return []any{map[string]any{
400+
"type": "p",
401+
"indent": float64(1),
402+
"listStyleType": "disc",
403+
"children": []any{leaf},
404+
}}
405+
}
406+
oldText := makeList(map[string]any{"text": "Confidentiality Policy"})
407+
newText := makeList(map[string]any{"text": "Confidentiality Policy"})
408+
assert.Check(t, slateparser.OnlyCommentsAdded(oldText, newText))
409+
})
410+
411+
t.Run("list item with indent, comment added to leaf", func(t *testing.T) {
412+
makeList := func(leaf map[string]any) []any {
413+
return []any{map[string]any{
414+
"type": "p",
415+
"indent": float64(1),
416+
"listStyleType": "disc",
417+
"children": []any{leaf},
418+
}}
419+
}
420+
oldText := makeList(map[string]any{"text": "Confidentiality Policy"})
421+
newText := makeList(map[string]any{"text": "Confidentiality Policy", "comment": true, "comment_xyz": true})
422+
assert.Check(t, slateparser.OnlyCommentsAdded(oldText, newText))
423+
})
217424
}

0 commit comments

Comments
 (0)