AWS DocumentDB support through OP_MSG and handling different OK type #2616
Conversation
There was a problem hiding this comment.
The part of handling int in addition to double looks okay, I'd just factor the expression into a private function to avoid code duplication*. Using an empty string to mean "admin" doesn't feel very clean, but I also can't think of a better approach for now, so that's okay, too.
If possible, I'd ask you to separate these two into a separate PR that can be merged first, so that we can concentrate on the OpCode.Msg part here, as I feel that there might be a few more hidden issues not caught by the existing tests.
* e.g. void enforceReplyOK(Bson reply, string failure_message)
| @@ -306,26 +308,50 @@ final class MongoConnection { | |||
| { | |||
| scope(failure) disconnect(); | |||
| foreach (d; documents) if (d["_id"].isNull()) d["_id"] = Bson(BsonObjectID.generate()); | |||
| send(OpCode.Insert, -1, cast(int)flags, collection_name, documents); | |||
| string collection = collection_name.canFind(".") ? collection_name.split(".")[1] : collection_name; | |||
There was a problem hiding this comment.
What would be the semantic of the first part of collection'_name here, the database name? Since it is unused, would that result in the expected behavior if it mismatches?
| "$db": Bson(m_settings.database == string.init ? "admin" : m_settings.database), | ||
| "documents": Bson(documents) | ||
| ]); | ||
| auto id = send(OpCode.Msg, -1, cast(int)0, cast(ubyte)0, operation); |
There was a problem hiding this comment.
This uses 0 instead of the flags argument that was used for OpCode.insert - I'm not sure how the semantics translate to OpCode.Msg, though, probably they have to be added as fields to operation?
| if (!returnFieldSelector.isNull) | ||
| op["projection"] = returnFieldSelector; | ||
|
|
||
| if (flags && QueryFlags.tailableCursor) |
There was a problem hiding this comment.
| if (flags && QueryFlags.tailableCursor) | |
| if (flags & QueryFlags.tailableCursor) |
|
|
||
| if (flags && QueryFlags.tailableCursor) | ||
| op["tailable"] = Bson(true); | ||
| if (flags && QueryFlags.awaitData) |
There was a problem hiding this comment.
| if (flags && QueryFlags.awaitData) | |
| if (flags & QueryFlags.awaitData) |
| op["awaitData"] = Bson(true); | ||
|
|
||
| Bson operation = op; | ||
| id = send(OpCode.Msg, -1, cast(int)0, cast(ubyte)0, operation); |
There was a problem hiding this comment.
There are a few more query flags that are unhandled here: slaveOk, oplogReplay, noCursorTimeout, exhaust, partial
Also, the collection_name parameter is now unused.
| } | ||
|
|
||
| void getMore(T)(string collection_name, int nret, long cursor_id, scope ReplyDelegate on_msg, scope DocDelegate!T on_doc) | ||
| { | ||
| scope(failure) disconnect(); | ||
| auto id = send(OpCode.GetMore, -1, cast(int)0, collection_name, nret, cursor_id); | ||
| string collection = collection_name.canFind(".") ? collection_name.split(".")[1] : collection_name; |
There was a problem hiding this comment.
Same question as for insert here
|
I've implemented this as well in #2691 now, although I more strictly separated the OP_MSG implementation from OP_QUERY. My implementation checks if OP_MSG can be used as per driver spec and then only continues to use that from that point on. There are a few changes that are crucial for that imo, which aren't in this PR, like changing how I have upgraded most basic queries (find, update, etc) to OP_MSG there, but have kept incompatible ones (e.g. return selector no longer exists afaics) using the legacy OP_QUERY in. I have also deprecated our custom non-conformant CRUD APIs there and replaced them with fully configurable driver-spec conformant versions. |
I wouldn't suggest merging this directly as is, as it could probably break compatibility with older mongodb <3.6 i believe is the cutoff here. But I thought i'd share the changes here as this was what I had to do to get vibe-d mongo driver to work with AWS DocumentDB. This is because even though docdb is mongo compliant it only has implemented OP_MSG for the operations i modified below. Also docdb doesn't support $external.$cmd for handshaking and so with this fork you MUST specify a database in the client settings so that it can auth against it.
I hope this can help you find a cleaner way to integrate these changes where they won't break non-docdb use cases. This was a quick and dirty way of getting compatibility working.