[GODRIVER-2137] empty/nil slices create null field, which can't be used by various functions Created: 22/Aug/21 Updated: 26/Jul/23 |
|
| Status: | Backlog |
| Project: | Go Driver |
| Component/s: | BSON |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Improvement | Priority: | Minor - P4 |
| Reporter: | Liam Stanley | Assignee: | Unassigned |
| Resolution: | Unresolved | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Attachments: |
|
||||||||||||||||||||||||
| Issue Links: |
|
||||||||||||||||||||||||
| Backwards Compatibility: | Major Change | ||||||||||||||||||||||||
| Description |
|
As similarly expressed in
The primary issue with this, is that things like $push, $in, $addToSet, etc do not work as expected when the value is set to null.
The two current solutions are:
Given the field being written as an empty array via bson, can be converted back to nil on the Go side, I don't believe there is a downside in taking the mgocompat approach, and bringing this in as the default. This would also ensure standard methods are prone to work as expected, play better with folks who are using validation, and seems more obvious.
At a minimum, I wish this would be re-visited, given the nature of how Go arrays have a nil zero value, which isn't necessarily common with some of the other languages which are supported, and thus I think may warrant an exception to the standard "take exactly what we're given" bson encoding semantics. |
| Comments |
| Comment by Matt Dale [ 02/Mar/23 ] | ||||
|
Here's what we're currently planning to do for various use cases. Go structs as MongoDB documentsTo support the use case of encoding Go structs as MongoDB documents, we will add an omitnil BSON struct tag that only omits nil values but includes empty values. The $push and $addToSet update operators work the same when modifying document fields that are empty arrays and document fields that do not exist (the only problem is document fields that are BSON null), so the behavior of omitnil maps well to that use case. Additionally, other MongoDB drivers (e.g. Java and Rust) offer a similar behaviors. See GODRIVER-2765 for more details. Make configuring the MongoDB Client BSON marshaling behavior much easierTo support the use case of changing Client-wide BSON marshaling behavior, we will move all of the current configuration options that are buried in the bsoncodec package onto the bson.Encoder and bson.Decoder types. For example, instead of having to create a SliceCodec and set EncodeNilAsEmpty, you only have to create a bson.Encoder and then call SetNilAsEmpty. See me@liamstanley.io Does that sound like it will resolve some of your original concerns? | ||||
| Comment by Benji Rewis (Inactive) [ 02/Dec/22 ] | ||||
|
karthickcseapitam@gmail.com I've responded to your community forum post; that's probably the best place to continue this conversation. Thanks! | ||||
| Comment by karthick d [ 02/Dec/22 ] | ||||
|
benji.rewis@mongodb.com
how can I add also NewRespectNilValuesRegistryBuilder().Build() in the above code ,? Also us emptyarray BSON tag implemented by any chance ?
| ||||
| Comment by Benji Rewis (Inactive) [ 29/Sep/21 ] | ||||
|
All good, me@liamstanley.io! I'll reopen this ticket for now and mark it with fix version 2.0. We can reconsider changing our default behavior at that point in time. For now, it seems like using the mgocompat registry is a viable workaround if you want both empty-initialized and nil arrays to marshal and unmarshal to and from empty BSON arrays. I'll bring up the idea of a new emptyarray BSON tag with the rest of the team, too. | ||||
| Comment by Liam Stanley [ 27/Sep/21 ] | ||||
|
I'm sorry for the delay. You are correct, I thought I tested that specific scenario previously, but I guess I didn't test properly.
I guess that does change things a bit, though I do think the default for nil arrays being changed should be something that is discussed. I'm not confident in if it should be the standard, but it does seem to make sense to me. I wonder if a tag value (similar to omitempty), would be better (not as much of a potential breaking change), that when present, tells bson that it:
Something like `bson:",emptyarray"` or similar, on the field.
I still think the default changing makes sense for most users, since nil is harder to work with (e.g. $addToSet and $push fail, so you'd have to convert first, then use those methods). If we were to change the default, we could use tag fields to keep the original behavior if for some reason someone wanted to continue to write nil's. | ||||
| Comment by Backlog - Core Eng Program Management Team [ 15/Sep/21 ] | ||||
|
There hasn't been any recent activity on this ticket, so we're resolving it. Thanks for reaching out! Please feel free to comment on this if you're able to provide more information. | ||||
| Comment by Benji Rewis (Inactive) [ 31/Aug/21 ] | ||||
|
Thanks again for your improvement suggestion me@liamstanley.io! If I’m not mistaken, Go arrays that are empty but initialized will Marshal to an empty BSON array. Go arrays that are nil will become null when encoded to BSON. Please see the emptyarraymarshal_test.go file I’ve attached. Were you referring to a different form of “empty” slice? In any case, modifying the default behavior of the default registry used by the Go BSON library in order to encode nil Go arrays as empty BSON arrays would be a backwards-breaking API change. Users are likely relying on the existing behavior. I do see an argument for encoding nil Go arrays to empty BSON arrays, but that change would have to be done in a major release (like the upcoming Go driver 2.0). I also agree that neither the mgocompat registry nor the omitempty struct tag are immediately obvious solutions for this use-case. So, we could certainly Open this ticket for consideration in 2.0. | ||||
| Comment by Benji Rewis (Inactive) [ 30/Aug/21 ] | ||||
|
Hello me@liamstanley.io and apologies for the delayed response. Thanks for your improvement suggestions; we're investigating now! |