Skip to content

Conversation

@qingyang-hu
Copy link
Collaborator

@qingyang-hu qingyang-hu commented May 29, 2025

GODRIVER-3574

Summary

  1. Align BSON decoding for interface slice with json package.
    • Fix the inconsistent behavior from v1 when the pointed target value contains pre-assigned concrete values.
    • Extend the targeted slice when the capacity is insufficient.
  2. Clarify the error message in the case of an unsettable value.

Background & Motivation

@qingyang-hu qingyang-hu requested a review from a team as a code owner May 29, 2025 22:45
Copy link
Member

@prestonvasquez prestonvasquez left a comment

Choose a reason for hiding this comment

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

@qingyang-hu We should add the test described in the ticket to to the mongo/ package. Is there a way to update the test to be specific to the bson package (i.e extend TestDefaultValueDecoders)?

@qingyang-hu
Copy link
Collaborator Author

qingyang-hu commented May 30, 2025

@qingyang-hu We should add the test described in the ticket to to the mongo/ package. Is there a way to update the test to be specific to the bson package (i.e extend TestDefaultValueDecoders)?

Sorry, I forgot to label this PR in draft. It's still in progress. I also need to update the error message wording.

@qingyang-hu qingyang-hu marked this pull request as draft May 30, 2025 18:35
@qingyang-hu qingyang-hu marked this pull request as ready for review June 4, 2025 15:44
@prestonvasquez prestonvasquez added the review-priority-urgent High Priority PR for Review: review immediately! label Jun 4, 2025
prestonvasquez
prestonvasquez previously approved these changes Jun 4, 2025
Copy link
Member

@prestonvasquez prestonvasquez left a comment

Choose a reason for hiding this comment

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

Nice work 👍

},
},
{
name: "struct with interface slice containing concrete values",
Copy link
Member

Choose a reason for hiding this comment

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

Could we rename this to "overwriting prepopulated slice?"

},
},
{
name: "struct with interface slice containing concrete values",
Copy link
Member

Choose a reason for hiding this comment

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

Could we rename this to "overwriting prepopulated slice?"

if err != nil {
return nil, err
}
if !elem.CanSet() {
Copy link
Member

Choose a reason for hiding this comment

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

This change is really nuanced. Can we add a comment here noting why we do this? "If an element is unsettable and allocated, it must be overwritten."

Copy link
Member

Choose a reason for hiding this comment

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

I also suggest making the following changes to make this more readable:

isInterfaceSlice := eType.Kind() == reflect.Interface && val.Len() > 0

// If this is not an interface slice with pre-populated elements, we can look
// up the decoder for eType once.
var vDecoder ValueDecoder
if !isInterfaceSlice {
	vDecoder, err = dc.LookupDecoder(eType)
	if err != nil {
		return nil, err
	}
}

for {
	// ...
	if isInterfaceSlice && idx < val.Len() {
		// Decode into an existing interface{} slot.
		
		// ...
	} else {
		// For non-interface slices, or if we've exhuasted the pre-populated
		// slots, we create a fresh value.
		if vDecoder == nil {
			// ...
		}

		//...
	}
}

@qingyang-hu qingyang-hu merged commit ad5468b into mongodb:master Jun 4, 2025
26 of 32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review-priority-urgent High Priority PR for Review: review immediately!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants