[GODRIVER-2252] Unmarshalling null into a pointer field always creates a value if the type implements bson.Unmarshaler or bson.ValueUnmarshaler Created: 06/Dec/21 Updated: 28/Oct/23 Resolved: 02/Feb/22 |
|
| Status: | Closed |
| Project: | Go Driver |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | 1.9.0, 1.8.5 |
| Type: | Bug | Priority: | Unknown |
| Reporter: | Joël Gähwiler | Assignee: | Matt Dale |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||
| Description |
|
I'm not sure, but have the feeling that the following code should not allocate an object for the field "Baz" and instead set it to nil: https://go.dev/play/p/eRTWs2Y9sDe.
|
| Comments |
| Comment by Githook User [ 22/Mar/22 ] | ||||
|
Author: {'name': 'Matt Dale', 'email': '9760375+matthewdale@users.noreply.github.com', 'username': 'matthewdale'}Message: | ||||
| Comment by Matt Dale [ 02/Feb/22 ] | ||||
|
Hey joel.gaehwiler@gmail.com, the PR was merged and is scheduled for release with the next minor version (v1.9.0). There's no scheduled date for the v1.9.0 driver release, but it will likely be in the next month. I'm closing this ticket since the issue will be resolved in the next release. Please comment here again if you continue to have issues. Thanks! | ||||
| Comment by Githook User [ 13/Jan/22 ] | ||||
|
Author: {'name': 'Matt Dale', 'email': '9760375+matthewdale@users.noreply.github.com', 'username': 'matthewdale'}Message: | ||||
| Comment by Joël Gähwiler [ 11/Jan/22 ] | ||||
|
Thanks for the update and I'm looking forward to the PR being merged! | ||||
| Comment by Matt Dale [ 11/Jan/22 ] | ||||
|
Hey joel.gaehwiler@gmail.com, thanks for the feedback! We're currently reviewing a change to skip allocating a value and skip calling UnmarshalBSON in the case where the receiver is a pointer and the corresponding BSON field value is empty (check it out here). | ||||
| Comment by Joël Gähwiler [ 21/Dec/21 ] | ||||
|
Thanks for your detailed investigation! I was actually stumbling over this issue while implementing a custom type in a library. The "omitempty" hack certainly works for prototyping the functionality, but the correct behaviour of setting the pointer to nil if there is a null value would be most welcome from a user perspective. If changing the behaviour ist not an option I could image that adding a sentinel error like "ErrSetNil" may be returned from the unmarshaling function to force a nil value? Perhaps, this might even open other use cases where a non-null value is unmarshalled, but the custom unmarshalling function would like to set a nil pointer. | ||||
| Comment by Matt Dale [ 20/Dec/21 ] | ||||
|
joel.gaehwiler@gmail.com thank you for the example code, it was really helpful to reproduce the behavior! The unexpected behavior is an inconsistency between how the BSON unmarshaler works and how the "encoding/json" unmarshaler works when the destination field is a pointer, has a custom unmarshal function, and the associated value in the marshalled document is null/empty. The "encoding/json" unmarshaler doesn't allocate a value or call UnmarshalJSON in that case (check out this helpful Stackoverflow answer for more context on why). However, the BSON unmarshaler does allocate a value and call UnmarshalBSON in that case. I'm currently investigating if making the BSON unmarshaler match the "encoding/json" unmarshaler behavior makes sense and is safe (i.e. won't break any existing use cases). A short-term fix is to add the omitempty directive to the struct tag to instruct the BSON marshaler to omit those empty fields from the output document. If the field is not included in the BSON document, the BSON unmarshaler for that field won't be called and no value will be allocated. E.g. foo with omitempty
Does that fix resolve your issue or are there already-encoded documents that you need to read? | ||||
| Comment by Esha Bhargava [ 07/Dec/21 ] | ||||
|
joel.gaehwiler@gmail.com Thank you for reporting this issue! We'll look into it and get back to you soon. |