Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added support for useObjectId #38

Merged
merged 7 commits into from Aug 30, 2015
Merged

added support for useObjectId #38

merged 7 commits into from Aug 30, 2015

Conversation

@diabolicallabs
Copy link
Contributor

diabolicallabs commented Aug 29, 2015

Fixed issue 36 to add support for useObjectId in the configuration. If set to true, the _id field will be saved as an ObjectId in native MongoDB format. When read, it will be returned to the reader as a hex string rather than a compound $oid field. If the _id is set by the user prior to insert, it should be as a valid hex string.

I suggest merging sooner rather than later. It's a fairly core change and should be tested well prior to release in 3.1 I'm using the 3.1 SNAPSHOT with this change in an important project and it seems to be fine so far.

@purplefox purplefox added the to review label Aug 29, 2015
@karianna

This comment has been minimized.

Copy link
Contributor

karianna commented Aug 29, 2015

See #36

@@ -17,6 +17,7 @@
package io.vertx.groovy.ext.mongo;
import groovy.transform.CompileStatic
import io.vertx.lang.groovy.InternalHelper
import io.vertx.core.json.JsonObject

This comment has been minimized.

Copy link
@karianna

karianna Aug 29, 2015

Contributor

redundant import?

@@ -68,6 +70,7 @@ public MongoClientImpl(Vertx vertx, JsonObject config, String dataSourceName) {
this.vertx = vertx;
this.holder = lookupHolder(dataSourceName, config);
this.mongo = holder.mongo();
useObjectId = config.getBoolean("useObjectId", false);

This comment has been minimized.

Copy link
@karianna

karianna Aug 29, 2015

Contributor

use this. as convention

@@ -629,7 +627,7 @@ private void doTestFind(int numDocs, JsonObject query, FindOptions options, Cons
await();
}

@Test
//@Test

This comment has been minimized.

Copy link
@karianna

karianna Aug 29, 2015

Contributor

Not sure about commenting out tests here and below...

@@ -111,12 +122,24 @@ public void close() {

boolean id = document.containsKey(ID_FIELD);

if (id) {
if (useObjectId) {
if (document.getValue(ID_FIELD) instanceof String) {

This comment has been minimized.

Copy link
@karianna

karianna Aug 29, 2015

Contributor

Could combine if and extract 3 conditions to a single method

if (useObjectId) {
if (query.containsKey(ID_FIELD)) {
if (query.getValue(ID_FIELD) instanceof String) {
query.put(ID_FIELD, new JsonObject().put(OID_FIELD, query.getString(ID_FIELD)));

This comment has been minimized.

Copy link
@karianna

karianna Aug 29, 2015

Contributor

Could combine if conditions into a named method.

result.put(OID_FIELD, reader.readObjectId().toHexString());
return result;
if (reader.getCurrentName().equals(ID_FIELD)) return reader.readObjectId().toHexString();
return new JsonObject().put(OID_FIELD, reader.readObjectId().toHexString());

This comment has been minimized.

Copy link
@karianna

karianna Aug 29, 2015

Contributor

I personally prefer returns on the next line, not 100% sure of vertx convention

assertTrue(true);
testComplete();
await();
}

This comment has been minimized.

Copy link
@karianna

karianna Aug 29, 2015

Contributor

This seems like a test with always true logic...

This comment has been minimized.

Copy link
@diabolicallabs

diabolicallabs Aug 30, 2015

Author Contributor

That overrides a test in the base class that doesn't make sense when useObjectId = true. A Long _id isn't valid

This comment has been minimized.

Copy link
@karianna

karianna Aug 30, 2015

Contributor

Ah my bad!

@karianna

This comment has been minimized.

Copy link
Contributor

karianna commented Aug 29, 2015

I've made mostly nit pick type review comments. Still needs a 'is this the right thing to do' review from @johnoliver (who's going to join this project any day now.)

@karianna karianna self-assigned this Aug 29, 2015
@diabolicallabs

This comment has been minimized.

Copy link
Contributor Author

diabolicallabs commented Aug 30, 2015

Thank you for the thorough review. Kindly review the changes made since your initial review.

@johnoliver

This comment has been minimized.

Copy link
Contributor

johnoliver commented Aug 30, 2015

Having had a look through and test I think this all looks good, I will make a few more small style comments but other than that, have you signed the Eclipse CLA:

https://github.com/eclipse/vert.x/blob/master/CONTRIBUTING.md

Once done should be good to merge

* `useObjectId`:: Toggle this option to support persisting and retrieving ObjectId's as strings. Defaults to `false`.
* `useObjectId`:: Toggle this option to support persisting and retrieving ObjectId's as strings. If `true', hex-strings will
* be saved as native Mongodb ObjectId types in the document collection. This will allow the sorting of documents based on creation
* time. You can also derive the creation time from the hex-string using ObjectId::getDate(). Set to `false' for other types of your choosing.

This comment has been minimized.

Copy link
@johnoliver

johnoliver Aug 30, 2015

Contributor

I would add that by default false will generate hex strings if the user does not provide an _id

} else {
coll.replaceOne(wrap(new JsonObject().put(ID_FIELD, document.getValue(ID_FIELD))), document, convertCallback(resultHandler, result -> null));
JsonObject filter = new JsonObject();
encodeKeyWhenUseObjectId(document);

This comment has been minimized.

Copy link
@johnoliver

johnoliver Aug 30, 2015

Contributor

Since these methods return the document again, I would do:

document=encodeKeyWhenUseObjectId(document);

To make it obvious that document is being modified.

If you can also do this on all uses of decodeKeyWhenUseObjectId and encodeKeyWhenUseObjectId

This comment has been minimized.

Copy link
@diabolicallabs

diabolicallabs Aug 30, 2015

Author Contributor

Sadly, we can't do that within a lambda. "document" as to be final or effectively final. You can't assign anything back to it. It seems ridiculous since you can mutate the object anyway, like I am doing. If you know a way to make the assignment within a lambda, I would surely like to know it! It's the thing I dislike most about Java 8. It's pushing me back toward Groovy. :-)

I return the JsonObject from the method in the event that someone else wants to use it fluently.

This comment has been minimized.

Copy link
@johnoliver

johnoliver Aug 30, 2015

Contributor

Ergh, yeah, there is no good option on this one, you can use an AtomicReference to get around the effectively final thing, but here its more trouble than its worth.

It sucks that mongo is returning its ids by mutating its arguments in the first place, this has already bitten me once on other stuff I have been working on, I am tempted with all of the vertx-mongo-client stuff to do a copy of all of the arguments first so that we don't mutate objects that come in as args to avoid the issues I had.

You have 4 uses of encodeKeyWhenUseObjectId, only this one cant be fixed as the object is used in a lambda, so I would fix the others.

You could assign to a new variable, i.e:

    JsonObject idEncodedDocument = encodeKeyWhenUseObjectId(document);

    MongoCollection<JsonObject> coll = getCollection(collection, writeOption);
    coll.insertOne(idEncodedDocument, convertCallback(resultHandler, wr -> {
      if (id) {
        return null;
      } else {
        JsonObject documentWithId = decodeKeyWhenUseObjectId(idEncodedDocument);
        return documentWithId.getString(ID_FIELD);
      }
    }));
    return this;

But it is up to you if you want to do that, I am not too bothered as mongo has already screwed us on this one.

@@ -54,12 +54,14 @@
private static final UpdateOptions DEFAULT_UPDATE_OPTIONS = new UpdateOptions();
private static final FindOptions DEFAULT_FIND_OPTIONS = new FindOptions();
private static final String ID_FIELD = "_id";
private static final String OID_FIELD = "$oid";

This comment has been minimized.

Copy link
@johnoliver

johnoliver Aug 30, 2015

Contributor

this is duplicated from JsonObjectCodec, you could refer to the one there

@@ -7,6 +7,7 @@
import io.vertx.core.json.JsonObject;
import io.vertx.test.core.TestUtils;
import org.bson.types.ObjectId;
import org.jruby.RubyProcess;

This comment has been minimized.

Copy link
@johnoliver

johnoliver Aug 30, 2015

Contributor

unused import

@johnoliver

This comment has been minimized.

Copy link
Contributor

johnoliver commented Aug 30, 2015

Oh yeah, also the commits should be "signed off" as per:

https://github.com/eclipse/vert.x/blob/master/CONTRIBUTING.md

I am sure I managed to sign-off commits in the past after they have been made by rebaseing and squashing them into a single signed off commit, will have to try to remember how to do this.

@johnoliver

This comment has been minimized.

Copy link
Contributor

johnoliver commented Aug 30, 2015

if you add the script at:

http://stackoverflow.com/questions/25570947/how-to-use-git-interactive-rebase-for-signing-off-a-series-of-commits

You can sign off by

git signoff-rebase <hash of commit before yours>
@diabolicallabs

This comment has been minimized.

Copy link
Contributor Author

diabolicallabs commented Aug 30, 2015

I did sign the CLA

@diabolicallabs

This comment has been minimized.

Copy link
Contributor Author

diabolicallabs commented Aug 30, 2015

Hmm. I though signoff was only required for the vertx-core project. That's why I don't contribute to vertx-core. I didn't want to figure it out. :-)

David Bush and others added 4 commits Aug 30, 2015
Signed-off-by: David Bush <david.bush@process-intellect.com>
Signed-off-by: David Bush <david.bush@process-intellect.com>
@diabolicallabs

This comment has been minimized.

Copy link
Contributor Author

diabolicallabs commented Aug 30, 2015

I think it's signed-off now. I'm not sure how to check. I used the provided script anyhow.

@johnoliver

This comment has been minimized.

Copy link
Contributor

johnoliver commented Aug 30, 2015

Not sure, but as you say it doesn’t seem like others are signing off on this project so yeah, I would just fix the uses of encodeKeyWhenUseObjectId that can be fixed and we are done here

@diabolicallabs

This comment has been minimized.

Copy link
Contributor Author

diabolicallabs commented Aug 30, 2015

I added variables for encoded or decoded document to avoid the 'effectively final' error and still make an obvious assignment in the code.

@johnoliver

This comment has been minimized.

Copy link
Contributor

johnoliver commented Aug 30, 2015

Great, looks good. Thanks for the contribution, a much needed feature.

johnoliver added a commit that referenced this pull request Aug 30, 2015
@johnoliver johnoliver merged commit bf7de6b into vert-x3:master Aug 30, 2015
@purplefox purplefox removed the to review label Aug 30, 2015
@karianna karianna assigned johnoliver and unassigned karianna Aug 30, 2015
@karianna

This comment has been minimized.

Copy link
Contributor

karianna commented Aug 30, 2015

@diabolicallab Thanks again, great contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.