Simplify usage of user provided aggregation operations. #4059
Conversation
Introduce Aggregation.stage which allows to use a plain JSON String or any valid Bson representation to be used within an aggregation pipeline stage, without having to implement AggregationOperation directly. The change allows to make use of driver native builder API for aggregates.
|
|
||
| @Override | ||
| default CodecRegistry getCodecRegistry() { | ||
| return MongoClientSettings.getDefaultCodecRegistry(); |
There was a problem hiding this comment.
In practice, when (if ever) will this default implementation be used? The underlying driver never uses this method, because it doesn't take into account important user configuration (most notably, the UUID representation).
There was a problem hiding this comment.
Since it’s public API that users may have implemented, defaulting here eases the pain when upgrading to the new version. Internal implementations pick up the driver registry.
Are custom codecs heavily used?
There was a problem hiding this comment.
The main thing is that even the default registry is changed in the MongoClient constructor in order to apply the uuidRepresentation to the registry. The default registry will throw when trying to encode a UUID value, because the default UuidRepresentation is UNSPECIFIED
There was a problem hiding this comment.
Just a note: MongoClientSettings.getDefaultCodecRegistry() is also used explicitly in the default implementation of the MongoConverter.getCodecRegistry method.
There was a problem hiding this comment.
true, same reason for those upgrading. As long as the user does not provide a custom implementation of the converter the codec registry (with all the user settings regarding UUID codec,...) will be used.
| public static Map<String, Object> asMap(@Nullable Bson bson, CodecRegistry codecRegistry) { | ||
|
|
||
| if (bson == null) { | ||
| return Collections.emptyMap(); | ||
| } | ||
| if (bson instanceof BasicDBObject) { | ||
| return ((BasicDBObject) bson); | ||
|
|
||
| if (bson instanceof Document document) { | ||
| return document; | ||
| } | ||
| if (bson instanceof DBObject) { | ||
| return ((DBObject) bson).toMap(); | ||
| if (bson instanceof BasicDBObject dbo) { | ||
| return dbo; | ||
| } | ||
| if (bson instanceof DBObject dbo) { | ||
| return dbo.toMap(); | ||
| } | ||
|
|
||
| return (Map) bson.toBsonDocument(Document.class, MongoClientSettings.getDefaultCodecRegistry()); | ||
| return new Document((Map) bson.toBsonDocument(Document.class, codecRegistry)); |
There was a problem hiding this comment.
- Should
org.bson.BsonDocumentbe also included as a special case? It is aMapwithStringkeys, just likeDocumentandBasicDBObject. - Should
BasicDBObjectbe replaced with its supertypeorg.bson.BasicBSONObject?BasicBSONObjectis aMapwithStringkeys. bson.toBsonDocument(Document.class, codecRegistry)is aMap<String, BsonValue>. Couldn't it be returned without shallowly copying its content in a newDocument? The methodBsonUtils.asDocumentwill still do this copying, but at least it will be done exactly where it is needed, instead of being done when there may be no need, which may be the case whenasMapis called not fromasDocument.
There was a problem hiding this comment.
the additional Document wrapping can be removed, yes.
The BasicDBObject is still in there from 2.x generation and likely to be completely removed for 4.x.
| if (value instanceof Document document) { | ||
| return document; | ||
| } | ||
|
|
||
| if (value instanceof Bson bson) { | ||
| return BsonUtils.asDocument(bson, context.getCodecRegistry()); | ||
| } | ||
|
|
||
| if (value instanceof Map map) { | ||
| return new Document(map); | ||
| } |
There was a problem hiding this comment.
BsonUtil.asMap does not allow itself a liberty of just checking instanceof Map. I think that's because this check does not guarantee that the map keys are of the String type. For this reason, I don't think that
if (value instanceof Map map) {
return new Document(map);
}is a type-safe code. Am I missing something?
Given the above, and that BsonUtil.asMap and BsonUtil.asDocument already check for special cases when some work may be skipped, can't the three if blocks be replaced with
if (value instanceof Bson bson) {
return BsonUtils.asDocument(bson, context.getCodecRegistry());
}?
| if (bson instanceof Document document) { | ||
| return document; | ||
| } | ||
|
|
||
| Map<String, Object> map = asMap(bson); | ||
| Map<String, Object> map = asMap(bson, codecRegistry); |
There was a problem hiding this comment.
Given that asMap already does the instanceof Document check, this check may be safely removed here.
|
|
||
| @Override | ||
| default CodecRegistry getCodecRegistry() { | ||
| return MongoClientSettings.getDefaultCodecRegistry(); |
There was a problem hiding this comment.
Just a note: MongoClientSettings.getDefaultCodecRegistry() is also used explicitly in the default implementation of the MongoConverter.getCodecRegistry method.
Introduce Aggregation.stage which allows to use a plain JSON String or any valid Bson representation to be used within an aggregation pipeline stage, without having to implement AggregationOperation directly. The change allows to make use of driver native builder API for aggregates. Original pull request: #4059. Closes #4038
|
That's merged and polished now. |
Introduce
Aggregation.stagewhich allows to use a plain JSON String or any validBsonrepresentation to be used within an aggregation pipeline stage, without having to implementAggregationOperationdirectly.The change allows to make use of driver native builder API for aggregates.