[JAVA-4874] BsonDocument.clone should return a mutable deep copy, or we need a different mechanism of achieving that Created: 08/Feb/23 Updated: 28/Oct/23 Resolved: 13/Feb/23 |
|
| Status: | Closed |
| Project: | Java Driver |
| Component/s: | BSON |
| Affects Version/s: | None |
| Fix Version/s: | 4.10.0 |
| Type: | Bug | Priority: | Minor - P4 |
| Reporter: | Valentin Kavalenka | Assignee: | Valentin Kavalenka |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Backwards Compatibility: | Fully Compatible |
| Documentation Changes: | Not Needed |
| Documentation Changes Summary: | 1. What would you like to communicate to the user about this feature? |
| Description |
|
Currently we have at least two places (that I know of) where we rely on BsonDocument.clone returning not only a deep copy, but mutable deep copy: 1. AbstractConstructibleBson.newMerged ross@mongodb.com pointed out that RawBsonDocument breaks the "mutable" part and can come from users. |
| Comments |
| Comment by Githook User [ 13/Feb/23 ] |
|
Author: {'name': 'Valentin Kovalenko', 'email': 'valentin.kovalenko@mongodb.com', 'username': 'stIncMale'}Message: Introduce `BsonUtil.mutableDeepCopy` (#1081)
|
| Comment by Valentin Kavalenka [ 09/Feb/23 ] |
If performance matters, I wonder why we clone bytes in RawBsonDocument.clone, given that RawBsonDocument is documented to be immutable.
I am not sure users really need something like this, otherwise we would have had requests from them by now. Let's introduce a mutableDeepCopy(BsonDocument) internal utility method. And if users ever request something like that, we can expose it via BsonDocument. |
| Comment by Jeffrey Yemin [ 09/Feb/23 ] |
|
The original commit that added support for cloning provides some more context on the intent. Specifically:
The idea here was to make it as efficient as possible for the driver to include the command and command reply in the events, but still provide a way for event listeners to efficiently copy those documents for later use, in the event they are needed outside the scope of the event listener method. Hence the Javadoc for CommandStartedEvent#getCommand:
It should probably also indicate that the document is read-only, and in practice it is, because it's actually an instance of com.mongodb.internal.connection.ByteBufBsonDocument. I don't think we should change the clone method of that class to return a mutable BsonDocument, as it would change the performance characteristics of the method significantly. The same goes for RawBsonDocument. Could we instead say that, if you need an actual instance of BsonDocument that is a mutable copy of the source, it's your responsibility to do so using a BsonDocument constructor? Currently, it doesn't seem like there is a convenient one to use, but we could add one. |