Java - Jackson #6157
Java - Jackson #6157
Conversation
|
Does this break existing API? If so then this should be optional, maybe triggered via Otherwise it looks great, it could potentially greatly simplify using POJOs. |
| } | ||
| public static ObjectMapper getObjectMapper(){ | ||
| if (mapper != null) return mapper; | ||
| return new ObjectMapper(); |
There was a problem hiding this comment.
If we're going to create a new mapper, we should probably assign it to mapper before returning it.
There was a problem hiding this comment.
Also, I think we should verify the pojo tests work, and possibly write some new tests using the @id annotation
|
@PeterKnego I'm a bit torn on this. So in an ideal world, we'd be backwards compatible for a while. In practice though, this is a big maintenance burden and obviously now there's less manpower available for it. Given that Jackson is better at this stuff than the hand-rolled system we have now, I think it's reasonable to replace it, even if it results in some small changes in client code. |
|
Right, I agree on POJO tests, I will run some. I can also just set the object mapper to a new instance and then return that. |
Tested and then fixed the @id annotation
|
Update: ID annotation fixed and tested. POJO's tested. ObjectMapper get and set methods revised. @deontologician its ready |
|
Ok, this looks pretty good, but I'm going to run the tests before merging, which might take a little time since I haven't got it set up on my laptop here. Bear with me |
| /** | ||
| * The Singleton to use to begin interacting with RethinkDB Driver | ||
| */ | ||
| public static final RethinkDB r = new RethinkDB(); |
There was a problem hiding this comment.
This should be re-added
|
As noted: this will break stuff for a lot of people. You should not break APIs on the minor version, ideally even major versions should be backwards compatible. |
| ); | ||
| T t = RethinkDB.getObjectMapper().convertValue(map,pojoClass); | ||
| return t; | ||
| } |
There was a problem hiding this comment.
The indentation is F'd up here.
There was a problem hiding this comment.
And the bracing and comma style is inconsistent with preexisting code, everywhere.
|
Can somebody please explain for a Java-noob like me what the benefit of this patch is? Edit: Thank you. |
|
@srh Current Java driver has a limited ability do map values provided by DB (basically a JSON) to Java objects (you don't wan to marshall/demarshall data coming/going to DB by hand, right?). This patch would provide a much better mapping abilities, as provided by Jackson, IMO the best JSON-to-Java-object mapper there is. So this is a good patch that would provide a much better functionality. However, this will change current API (the data returned/sent to driver will be different) and will break things for current users. As a basic rule minor updates should not change API contract, even major updates should be backwards compatible. My suggestion would be to make this optional via a config setting and announce this change, than later change make it default when major number is bumped. |
My understanding was that the Java driver was still kind of in beta. It's also relatively new, and missing features like async responses which means that many people aren't able to adopt it. I suspect that the number of people who would be affected and wouldn't have the engineering resources to update this would be low.
That's a good general principle, but may not work with the RethinkDB drivers, as they are versioned against the RethinkDB version, rather than using semver on their own. The RethinkDB protocol is also very stable and RethinkDB is backwards compatible with older protocols, so there is nothing stopping people from relying on the older driver while they migrate. Especially considering the reduced manpower available for RethinkDB, anything that can reduce maintenance burden is a big win in my book. |
|
I am conducting tests and I have the requested changes ready to commit. |
Fixed rethinkdb. Added checks to prevent against new fields in pojos.
|
Please see srh@891ed5f for a style fix commit. |
|
@srh Sorry, I didn't see any official style guid :P. Personally, I use allman but I didn't know what here. anyway, can you explain to me why this matters? I am a bit of a starter when it comes to coding conventions. Oh and the Java doc was intentional. |
|
@mcjp78 When you add code to an existing codebase you will want to match the existing conventions so all your code looks and reads the same as all the code around it. Doing so will make the codebase one cohesive unit. You'll find this in every open source project, and the majority of closed source projects as well. I believe RethinkDB has an official style guide though I'm not sure it is public at the moment. In lieu of an official style guide, it should be easy to match the other code already existing in the repository. |
|
I thought that Javadoc comments don't mean anything (to the Javadoc tool) inside a function body. At least a quick google suggested that. I'm not aware of a Java-specific RethinkDB style guide, but having the statements in a block be at the same level of indentation is a more or less universal standard for the same reason that they're indented in the first place. Regarding spacing between tokens, one of the following happens: Somebody goes and cleans it up later, polluting git blame history. Or, before that, somebody reading the code will see the weird style, think to themselves, "Oh what is up with this, I better check this out," and start scrutinizing it for quality problems. So now they're doing work that doesn't need to be done, because of a false bad code smell. Good code should smell good, so that bad-smelling code is a more reliable signal. |
|
Ah I see, I will fix the styling then |
|
@srh Would you mind checking again to make sure that I fixed everything? |
|
LGTM - @srh or @deontologician can you guys glance over this once more? |
|
LGTM |
|
Looks good to me but my opinion on Java code isn't worth much. |
|
Ok merging |
This is on a different branch than the [Python/Ruby SNI mods][1] just to make testing easier for myself. I.e. I had code that worked with the 2.3.3 driver where as the recent [Jackson changes][2] broke that. However, the underlying SNI changes will work with the Next branch. This includes some commented out printlns that ultimately need removing, but they were for my debugging and I don't want to forget that. Enabling SNI itself is easy, the tricky bit for me was modifying a test app to work with it. You have to use the SSLContext option similar to [here][3]. For testing I've just used a null sslContext: private static final String DEFAULT_SSL_PROTOCOL = "TLSv1.2"; ... Connection conn = null; Connection.Builder builder = r.connection(). hostname(host). port(port). authKey(authKey); final SSLContext sslContext = SSLContext.getInstance(DEFAULT_SSL_PROTOCOL); sslContext.init(null, null, null); conn = builder.sslContext(sslContext).connect(); [1]: https://github.com/atomicules/rethinkdb/commits/sni [2]: rethinkdb#6157 [3]: https://github.com/pires/rethinkdb-ssl-test/blob/master/src/main/java/com/github/pires/App.java#L70
Enabling SNI itself is easy, the tricky bit for me was modifying a test app to work with it. For that I [applied these changes][1] on a different branch based off the 2.3.3 driver since I had connection code that would work with that - the recent [Jackson changes][2] here broke my connection example. You have to use the SSLContext option similar to [here][3]: private static final String DEFAULT_SSL_PROTOCOL = "TLSv1.2"; ... Connection conn = null; Connection.Builder builder = r.connection(). hostname(host). port(port). authKey(authKey); final SSLContext sslContext = SSLContext.getInstance(DEFAULT_SSL_PROTOCOL); sslContext.init(null, null, null); conn = builder.sslContext(sslContext).connect(); Init with all null options here is valid. It will use the default/system trust manager for verification. [1]: 771f34d [2]: rethinkdb#6157 [3]: https://github.com/pires/rethinkdb-ssl-test/blob/master/src/main/java/com/github/pires/App.java#L70
Now uses Jackson (Json parser) for the java driver