Skip to content

Commit 2f4b886

Browse files
AsterDYAsterDY
committed
fix: only assign leaf nodes
1 parent 7bb7cb9 commit 2f4b886

File tree

2 files changed

+178
-19
lines changed

2 files changed

+178
-19
lines changed

‎trim/assign.go‎

Lines changed: 93 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -322,13 +322,39 @@ func assignValueToField(desc *Descriptor, src interface{}, fieldValue reflect.Va
322322
// Handle pointer fields - allocate if needed
323323
if fieldValue.Kind() == reflect.Ptr {
324324
if fieldValue.IsNil() {
325+
// Skip allocating for empty non-leaf sources to avoid clobbering existing data
326+
if isEmptyNonLeaf(desc, src) {
327+
return nil
328+
}
329+
325330
fieldValue.Set(reflect.New(fieldValue.Type().Elem()))
326331
}
327332
return assignValue(desc, src, fieldValue.Elem(), opt, stack)
328333
}
329334
return assignValue(desc, src, fieldValue, opt, stack)
330335
}
331336

337+
// isEmptyNonLeaf returns true if src is an empty composite value for the given descriptor.
338+
// Used to avoid allocating new nested structs or slices when there is nothing to assign.
339+
func isEmptyNonLeaf(desc *Descriptor, src interface{}) bool {
340+
if desc == nil || src == nil {
341+
return false
342+
}
343+
344+
switch desc.Kind {
345+
case TypeKind_Struct, TypeKind_StrMap:
346+
if m, ok := src.(map[string]interface{}); ok {
347+
return len(m) == 0
348+
}
349+
case TypeKind_List:
350+
if s, ok := src.([]interface{}); ok {
351+
return len(s) == 0
352+
}
353+
}
354+
355+
return false
356+
}
357+
332358
// assignStrMap handles TypeKind_StrMap assignment
333359
func assignStrMap(desc *Descriptor, src interface{}, destValue reflect.Value, opt *AssignOptions, stack *pathStack) error {
334360
srcMap, ok := src.(map[string]interface{})
@@ -355,24 +381,55 @@ func assignStrMap(desc *Descriptor, src interface{}, destValue reflect.Value, op
355381
elemType := destValue.Type().Elem()
356382

357383
for key, value := range srcMap {
358-
// Create a new element
384+
keyValue := reflect.ValueOf(key)
385+
existing := destValue.MapIndex(keyValue)
359386
elemValue := reflect.New(elemType).Elem()
387+
if existing.IsValid() {
388+
elemValue.Set(existing)
389+
}
390+
391+
// Find the appropriate descriptor for this entry
392+
var childDesc *Descriptor
393+
if wildcardDesc != nil {
394+
childDesc = wildcardDesc
395+
} else if child, ok := keyDescMap[key]; ok {
396+
childDesc = child.Desc
397+
}
398+
399+
// Skip assigning empty non-leaf nodes to avoid overwriting existing values
400+
if childDesc != nil && isEmptyNonLeaf(childDesc, value) {
401+
if !existing.IsValid() {
402+
continue
403+
}
404+
// keep existing value untouched
405+
continue
406+
}
360407

361408
if value == nil {
362-
// Set nil value in map (zero value for the element type, e.g., nil pointer)
363-
destValue.SetMapIndex(reflect.ValueOf(key), elemValue)
409+
// Preserve existing entry if present; otherwise set zero value
410+
if existing.IsValid() {
411+
destValue.SetMapIndex(keyValue, existing)
412+
}
364413
continue
365414
}
366415

367416
// Push map key onto stack
368417
stack.push(TypeKind_StrMap, key, 0)
369418

370-
// Find the appropriate descriptor
371419
var err error
372-
if wildcardDesc != nil {
373-
err = assignValueToField(wildcardDesc, value, elemValue, opt, stack)
374-
} else if child, ok := keyDescMap[key]; ok && child.Desc != nil {
375-
err = assignValueToField(child.Desc, value, elemValue, opt, stack)
420+
if childDesc != nil {
421+
if elemType.Kind() == reflect.Ptr {
422+
ptrValue := elemValue
423+
if elemValue.IsNil() {
424+
ptrValue = reflect.New(elemType.Elem())
425+
}
426+
err = assignValue(childDesc, value, ptrValue.Elem(), opt, stack)
427+
if err == nil {
428+
elemValue = ptrValue
429+
}
430+
} else {
431+
err = assignValue(childDesc, value, elemValue, opt, stack)
432+
}
376433
} else {
377434
err = assignLeaf(reflect.ValueOf(value), elemValue)
378435
}
@@ -384,7 +441,7 @@ func assignStrMap(desc *Descriptor, src interface{}, destValue reflect.Value, op
384441
return err
385442
}
386443

387-
destValue.SetMapIndex(reflect.ValueOf(key), elemValue)
444+
destValue.SetMapIndex(keyValue, elemValue)
388445
}
389446

390447
return nil
@@ -440,14 +497,27 @@ func assignList(desc *Descriptor, src interface{}, destValue reflect.Value, opt
440497
return nil
441498
}
442499

443-
// Handle slice (dynamic size)
500+
// Handle slice (dynamic size). Reuse existing slice when possible; only allocate when growing.
444501
elemType := destValue.Type().Elem()
445-
newSlice := reflect.MakeSlice(destValue.Type(), srcLen, srcLen)
502+
destLen := destValue.Len()
503+
useSlice := destValue
504+
if destLen < srcLen {
505+
useSlice = reflect.MakeSlice(destValue.Type(), srcLen, srcLen)
506+
if destLen > 0 {
507+
reflect.Copy(useSlice, destValue)
508+
}
509+
}
446510

447511
for i := 0; i < srcLen; i++ {
448-
elemValue := newSlice.Index(i)
512+
elemValue := useSlice.Index(i)
513+
514+
// Skip empty non-leaf sources to avoid overwriting existing elements
515+
if wildcardDesc != nil && isEmptyNonLeaf(wildcardDesc, srcSlice[i]) {
516+
continue
517+
}
518+
449519
if srcSlice[i] == nil {
450-
// Keep as zero value
520+
// Preserve existing value
451521
continue
452522
}
453523

@@ -456,12 +526,15 @@ func assignList(desc *Descriptor, src interface{}, destValue reflect.Value, opt
456526

457527
var err error
458528
if wildcardDesc != nil {
459-
// Handle pointer element type
529+
// Handle pointer element type without dropping existing value
460530
if elemType.Kind() == reflect.Ptr {
461-
newElem := reflect.New(elemType.Elem())
462-
err = assignValue(wildcardDesc, srcSlice[i], newElem.Elem(), opt, stack)
531+
targetPtr := elemValue
532+
if elemValue.IsNil() {
533+
targetPtr = reflect.New(elemType.Elem())
534+
}
535+
err = assignValue(wildcardDesc, srcSlice[i], targetPtr.Elem(), opt, stack)
463536
if err == nil {
464-
elemValue.Set(newElem)
537+
elemValue.Set(targetPtr)
465538
}
466539
} else {
467540
err = assignValue(wildcardDesc, srcSlice[i], elemValue, opt, stack)
@@ -478,7 +551,9 @@ func assignList(desc *Descriptor, src interface{}, destValue reflect.Value, opt
478551
}
479552
}
480553

481-
destValue.Set(newSlice)
554+
if useSlice.Pointer() != destValue.Pointer() || destLen != useSlice.Len() {
555+
destValue.Set(useSlice)
556+
}
482557
return nil
483558
}
484559

‎trim/assign_test.go‎

Lines changed: 85 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424

2525
"github.com/bytedance/sonic"
2626
"github.com/cloudwego/dynamicgo/proto/binary"
27+
"github.com/stretchr/testify/require"
2728
"google.golang.org/protobuf/proto"
2829
"google.golang.org/protobuf/reflect/protodesc"
2930
"google.golang.org/protobuf/reflect/protoreflect"
@@ -232,6 +233,89 @@ func TestAssignAny_Map(t *testing.T) {
232233
}
233234
}
234235

236+
func TestAssignAny_OnlyAssignLeafNodes(t *testing.T) {
237+
src := map[string]interface{}{
238+
"field_b": []interface{}{
239+
map[string]interface{}{},
240+
map[string]interface{}{
241+
"field_a": 10,
242+
"field_b": []interface{}{},
243+
},
244+
map[string]interface{}{
245+
"field_a": 10,
246+
},
247+
},
248+
"field_c": map[string]interface{}{
249+
"empty": map[string]interface{}{},
250+
"non_empty": map[string]interface{}{
251+
"field_a": 10,
252+
"field_c": map[string]interface{}{},
253+
},
254+
"add": map[string]interface{}{
255+
"field_a": 10,
256+
},
257+
},
258+
"field_d": map[string]interface{}{},
259+
}
260+
var desc = new(Descriptor)
261+
*desc = Descriptor{
262+
Kind: TypeKind_Struct,
263+
Type: "SampleAssign",
264+
Children: []Field{
265+
{Name: "field_a", ID: 1, Desc: &Descriptor{Kind: TypeKind_Leaf, Type: "INT32"}},
266+
{Name: "field_b", ID: 2, Desc: &Descriptor{
267+
Kind: TypeKind_List,
268+
Type: "LIST",
269+
Children: []Field{{Name: "*", Desc: desc}},
270+
}},
271+
{Name: "field_c", ID: 3, Desc: &Descriptor{
272+
Kind: TypeKind_StrMap,
273+
Type: "MAP",
274+
Children: []Field{{Name: "*", Desc: desc}},
275+
}},
276+
{
277+
Name: "field_d",
278+
ID: 4,
279+
Desc: desc,
280+
},
281+
},
282+
}
283+
284+
dest := &sampleAssign{
285+
FieldB: []*sampleAssign{
286+
{FieldA: 1, FieldE: "should not be cleared"},
287+
{FieldA: 1, FieldB: []*sampleAssign{{FieldA: 1}}, FieldE: "should not be cleared"},
288+
},
289+
FieldC: map[string]*sampleAssign{
290+
"empty": {FieldA: 1, FieldE: "should not be cleared"},
291+
"non_empty": {FieldA: 1, FieldC: map[string]*sampleAssign{"a": {FieldA: 1}}, FieldE: "should not be cleared"},
292+
},
293+
FieldD: &sampleAssign{FieldA: 1, FieldE: "should not be cleared"},
294+
}
295+
296+
assigner := &Assigner{}
297+
err := assigner.AssignAny(desc, src, dest)
298+
if err != nil {
299+
t.Fatalf("AssignAny failed: %v", err)
300+
}
301+
302+
expected := &sampleAssign{
303+
FieldB: []*sampleAssign{
304+
{FieldA: 1, FieldE: "should not be cleared"},
305+
{FieldA: 10, FieldB: []*sampleAssign{{FieldA: 1}}, FieldE: "should not be cleared"},
306+
{FieldA: 10},
307+
},
308+
FieldC: map[string]*sampleAssign{
309+
"empty": {FieldA: 1, FieldE: "should not be cleared"},
310+
"non_empty": {FieldA: 10, FieldC: map[string]*sampleAssign{"a": {FieldA: 1}}, FieldE: "should not be cleared"},
311+
"add": {FieldA: 10},
312+
},
313+
FieldD: &sampleAssign{FieldA: 1, FieldE: "should not be cleared"},
314+
}
315+
316+
require.Equal(t, expected, dest)
317+
}
318+
235319
// TestAssignAny_UnknownFields tests that the converted sample
236320
// with XXX_unrecognized can be correctly serialized and deserialized
237321
func TestAssignAny_UnknownFields(t *testing.T) {
@@ -1656,7 +1740,7 @@ func TestAssignAny_ListWithSpecificIndices(t *testing.T) {
16561740
}
16571741

16581742
// Wildcard should completely replace with source length
1659-
if len(dest.FieldB) != 2 {
1743+
if len(dest.FieldB) != 3 {
16601744
t.Fatalf("field_b: expected length 2 (from source), got %d", len(dest.FieldB))
16611745
}
16621746

0 commit comments

Comments
 (0)