Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 25 additions & 18 deletions fieldpath/serialize.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package fieldpath
import (
"bytes"
"io"
"slices"
"unsafe"

jsoniter "github.com/json-iterator/go"
Expand Down Expand Up @@ -204,34 +205,40 @@ func readIterV1(iter *jsoniter.Iterator) (children *Set, isMember bool) {
if children == nil {
children = &Set{}
}
m := &children.Members.members
// Since we expect that most of the time these will have been
// serialized in the right order, we just verify that and append.
appendOK := len(*m) == 0 || (*m)[len(*m)-1].Less(pe)
if appendOK {
*m = append(*m, pe)
} else {
children.Members.Insert(pe)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does append + sort do the same thing as Insert (here) and Descend (below)?

it looks like Insert can be a no-op if the item already exists in the set:

if s.members[loc].Equals(pe) {
return
}

and Descend can return the existing set if the path already exists

if s.members[loc].pathElement.Equals(pe) {
return s.members[loc].set
}

It looks like switching these to unconditional appends can produce duplicates where that was impossible before?

If so, can you add tests that ensure this isn't changing behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I added tests for duplicate members and child sets (the members of the last duplicate child set is kept) and fixed it. This made the bulk grandchildren sort a little more expensive, because it had to preserve the accumulated order. It still pays off in the worst case as inputs get larger than about a couple hundred kB (well within the default apiserver request size limit).

}
children.Members.members = append(children.Members.members, pe)
}
if grandchildren != nil {
if children == nil {
children = &Set{}
}
// Since we expect that most of the time these will have been
// serialized in the right order, we just verify that and append.
m := &children.Children.members
appendOK := len(*m) == 0 || (*m)[len(*m)-1].pathElement.Less(pe)
if appendOK {
*m = append(*m, setNode{pe, grandchildren})
} else {
*children.Children.Descend(pe) = *grandchildren
}
children.Children.members = append(children.Children.members, setNode{pe, grandchildren})
}
return true
})
if children == nil {
isMember = true
} else {
slices.SortFunc(children.Members.members, func(a, b PathElement) int {
return a.Compare(b)
})
children.Members.members = slices.CompactFunc(children.Members.members, func(a, b PathElement) bool {
return a.Equals(b)
})
if len(children.Children.members) > 1 {
slices.SortStableFunc(children.Children.members, func(a, b setNode) int {
return a.pathElement.Compare(b.pathElement)
})
// Compact duplicates, keeping the last occurrence to preserve historical behavior:
var dst int
for src := 1; src < len(children.Children.members); src++ {
if !children.Children.members[dst].pathElement.Equals(children.Children.members[src].pathElement) {
dst++
}
children.Children.members[dst] = children.Children.members[src]
}
clear(children.Children.members[dst+1:])
children.Children.members = children.Children.members[:dst+1]
}
}

return children, isMember
Expand Down
53 changes: 53 additions & 0 deletions fieldpath/serialize_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package fieldpath_test
import (
"bytes"
"fmt"
"os"
"strings"
"testing"

Expand Down Expand Up @@ -330,3 +331,55 @@ func TestDropUnknown(t *testing.T) {
t.Errorf("Failed;\ngot: %s\nwant: %s\n", b, expect)
}
}

func TestDuplicateMembers(t *testing.T) {
s := NewSet()
if err := s.FromJSON(strings.NewReader(`{"f:":{},"f:":{}}`)); err != nil {
t.Fatal(err)
}
if size := s.Members.Size(); size != 1 {
t.Errorf("want 1 member, got %d", size)
}
}

func TestDuplicateChildren(t *testing.T) {
d := NewSet()
if err := d.FromJSON(strings.NewReader(`{"f:":{"f:":{".":{},"f:1":{}}},"f:":{"f:":{".":{},"f:2":{}}}}`)); err != nil {
t.Fatal(err)
}
s := NewSet()
if err := s.FromJSON(strings.NewReader(`{"f:":{"f:":{".":{},"f:2":{}}}}`)); err != nil {
t.Fatal(err)
}
if !d.Equals(s) {
// The last occurrence of the duplicated child set is preserved.
t.Fatalf("want:\n%s\ngot:\n%s", s, d)
}
}

func BenchmarkSetFromJSON(b *testing.B) {
for _, tc := range []struct {
name string
fixture string
}{
{"children in ascending order", "testdata/set_ascending.json"},
{"children in descending order", "testdata/set_descending.json"},
{"grandchildren in ascending order", "testdata/set_ascending_grandchildren.json"},
{"grandchildren in descending order", "testdata/set_descending_grandchildren.json"},
} {
b.Run(tc.name, func(b *testing.B) {
data, err := os.ReadFile(tc.fixture)
if err != nil {
b.Fatal(err)
}

b.ResetTimer()
for range b.N {
x := NewSet()
if err := x.FromJSON(bytes.NewReader(data)); err != nil {
b.Fatal(err)
}
}
})
}
}
1 change: 1 addition & 0 deletions fieldpath/testdata/set_ascending.json

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions fieldpath/testdata/set_ascending_grandchildren.json

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions fieldpath/testdata/set_descending.json

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions fieldpath/testdata/set_descending_grandchildren.json

Large diffs are not rendered by default.