Improve ObjectId support

[meggarr]

Improve the ObjectId support in read/write object id from JsonObject. Also it adds protection if the "_id" field provided is not a valid ObjectId string.

[karianna]

@johnoliver Isn't this already effectively done? (Alternative implementation)

[karianna]

Could extract this piece of assert code? It's mostly repeated above

[meggarr]

Yes, I can wrap it to a method together with other comments if any.

[karianna]

I've got no further code comments, good quality patch :-) - but @johnoliver
will need to check to make sure this isn't an alternative implementation of
a previously accepted patch.

On Monday, 14 September 2015, meggarr notifications@github.com wrote:

In
vertx-mongo-client/src/test/java/io/vertx/ext/mongo/MongoClientTestBase.java
#41 (comment)
:

@@ -691,7 +696,12 @@ public void testReplaceUpsert2() {
mongoClient.find(collection, new JsonObject(), onSuccess(list -> {
assertNotNull(list);
assertEquals(1, list.size());

•      assertEquals(id, list.get(0).getString("_id"));
  

•      Object id_value = list.get(0).getValue("_id");
  

•      if (id_value instanceof JsonObject) {
  

•        assertEquals(id, ((JsonObject) id_value).getString("$oid"));
  

•      } else {
  

•        assertEquals(id, (String) id_value);
  

•      }
  

Yes, I can wrap it to a method together with other comments if any.

—
Reply to this email directly or view it on GitHub
https://github.com/vert-x3/vertx-mongo-client/pull/41/files#r39390711.

Cheers, Martijn (Sent from Gmail Mobile)

[meggarr]

The main purpose of the change is in two aspects

1. When "useObjectId" is true, and if the "_id" provided cannot be convert to a ObjectId, it will not do the conversion, but leave it as it is. The API use should clearly know that its "_id" string is not a ObjectId
2. When "useObjectId" is true, if the existing document in MongoDB has a "_id" field but that is not an ObjectId, it will not read as ObjectId/JsonObject. After this change, it will not have the error when reading exiting document.

[johnoliver]

I want to merge this as I think it is needed, only problem is this does change the current API slightly. Currently a document will be returned as:

{"_id":"5603e9ef6edd3843424db95b",...}

With this patch it will be:
{"_id":{"$oid":"5603e9ef6edd3843424db95b"},...}

I think this is an improvement, I am just a bit worried about breaking API compatibility here.

@karianna are we able to break this given the next release is a 3.1.0? Given _id support was only added after the last release, I am inclined to say yes.

[karianna]

If we're following semantic versioning I'd say technically no (a breaking change should really force a major version number bump, i.e. 4.x). That said, we're pinned by the overall vertx release train version numbering so it kind of doesn't really matter! I'd say go for it and we'll make sure there's a serious warning label in the release notes.

[karianna]

@johnoliver I've missed the last week or so's vertx traffic BTW, I assume 3.1.0 is still to go out yeah?

[cescoffier]

3.1.0 is planned for next week (end of September).

[johnoliver]

@meggarr Thank you for the contribution

[meggarr]

@johnoliver Glad to contribute my code. :)

During experience of MongoClient of Vert.X, I agree with your concern about my patch changes the content - {"_id":{"$oid":"5603e9ef6edd3843424db95b"},...}, this is not convenient for user to always handle the $oid, so I think it may make the "$oid":"5603e9ef6edd3843424db95b" automatically be translated to String in return content. What do you think?

(ObjectId string is already translated to Json with $oid when useObjectId == true)

[johnoliver]

I agree I think it would be a good idea to translate it back to a string when it is returned.

[johnoliver]

I also think we need to go through all of the methods in MongoClientImpl, as I think there are some objects that should have encodeKeyWhenUseObjectId called on them that currently do not, updateWithOptions for instance does not call it on the update, which I believe means you cant pass in an object to update with an ObjectId _id.

[karianna]

@johnoliver - Can you split that into a separate issue?