Upgrade jackson to 2.9.5 #713
Conversation
| Optional.of("baz"), | ||
| Paths.get("foo")); | ||
| Paths.get("/foo")); |
There was a problem hiding this comment.
This is explained in FasterXML/jackson-databind#1264 . Unfortunately previously java.nio.file.Path didn't round trip correctly. In order to fix it they changed the serialization format of Path to always write an absolute path. If you pass a relative path and serialize it will make it absolute which is hard to handle in tests
| @@ -46,6 +47,7 @@ | |||
| public final void run(Configuration config, final Environment env) throws Exception { | |||
| env.jersey().register(HttpRemotingJerseyFeature.INSTANCE); | |||
| env.jersey().register(new TestResource()); | |||
| env.jersey().register(new EmptyOptionalTo204ExceptionMapper()); | |||
There was a problem hiding this comment.
Dropwizard 1.x converts Optional.empty to 404 which breaks our tests. While 0.x version did nothing special about Optionals we need this now to preserve the expected behaviour of returning 204
There was a problem hiding this comment.
dropwizard/dropwizard#2350 added this class in dropwizard
There was a problem hiding this comment.
we'll need to minimally understand (and maybe deduplicate) this with Java8OptionalMessageBodyWriter and NoContentExceptionMapper
There was a problem hiding this comment.
(because it's not obvious how to use a HttpRemotingJerseyFeature.INSTANCE with dropwizard to get the desired behavior)
There was a problem hiding this comment.
but then I guess it's called JerseyFeature, not DropwizardFeature. not your fault, let's move on. #714
| @@ -6,6 +6,8 @@ server: | |||
| adminConnectors: | |||
| - type: http | |||
| port: 0 | |||
| gzip: | |||
There was a problem hiding this comment.
Dropwziards gzip filter gets in the way of remoting server compression filter. I think they should do the same thing but dropwizard returns different vary headers
There was a problem hiding this comment.
this is only for CompressionFilterTest, ja?
There was a problem hiding this comment.
yes, just for those tests
| @@ -52,6 +52,8 @@ | |||
| TestConstants.SERVER_KEY_STORE_JKS_PASSWORD), | |||
| ConfigOverride.config("server.applicationConnectors[0].trustStorePath", | |||
| TestConstants.CA_TRUST_STORE_PATH.toString()), | |||
| ConfigOverride.config("server.applicationConnectors[0].trustStorePassword", | |||
There was a problem hiding this comment.
As of jetty/jetty.project@a8d08df truststore password will default to keystore password which will fail upon reading the jks keystore. Internally we require client cert auth truststore to come with passwords
| @@ -15,4 +15,5 @@ dependencies { | |||
| testCompile "io.dropwizard:dropwizard-testing" | |||
| testCompile "junit:junit" | |||
| testCompile "org.hamcrest:hamcrest-all" | |||
| testCompile "org.mockito:mockito-core" | |||
There was a problem hiding this comment.
You have been relying on transitive mockito-core import from dropwizard that's no longer there
| .keyStoreType(SslConfiguration.StoreType.JKS) | ||
| .keyStoreKeyAlias("alias") | ||
| .keyStorePassword("password") | ||
| .build(); | ||
| String deserializedCamelCase = "{\"trustStorePath\":\"truststore.jks\",\"trustStoreType\":\"JKS\"," | ||
| + "\"keyStorePath\":\"keystore.jks\",\"keyStorePassword\":\"password\"," | ||
| String deserializedCamelCase = "{\"trustStorePath\":\"file:///truststore.jks\",\"trustStoreType\":\"JKS\"," |
There was a problem hiding this comment.
can old jackson versions deserialize this? (asking because some services (SMM?) may use the SslConfiguration class over the wire, I believe)
There was a problem hiding this comment.
I think I went overboard here with making behaviour the same in both cases. Let me revert that
There was a problem hiding this comment.
Added extra cases to show the compatibility guarantees
|
looks good modulo the path deserialization wire break question. |
|
@iamdanfox do you want to take a look, too, or are you good? |
| @@ -108,7 +108,6 @@ public static ObjectMapper newCborServerObjectMapper() { | |||
| public static ObjectMapper withDefaultModules(ObjectMapper mapper) { | |||
| return mapper | |||
| .registerModule(new GuavaModule()) | |||
| .registerModule(new ShimJdk7Module()) | |||
There was a problem hiding this comment.
If this is the only piece of non-test code that was changed then in theory if people hit sad times with the new jackson, they could conceivably force back to a lower version (and re-add this) right?
There was a problem hiding this comment.
So for this code, there are two arguments:
- This code makes remoting Jackson version independent. It's why some internal projects run 2.9. So leaving it in is nice because it gives you version independence.
- OTOH almost everything uses Jackson 2.7+ now, and we might want to use remoting to leverage that.
| "transitive": [ | ||
| "com.fasterxml.jackson.core:jackson-databind" | ||
| "com.fasterxml.jackson.core:jackson-databind", | ||
| "com.fasterxml.jackson.datatype:jackson-datatype-jsr310" |
There was a problem hiding this comment.
in 2.8 or 2.9 a lot of this stuff got renamed, no? specifically, why don't we have to update extras/jackson-support/build.gradle to reflect the new maven coordinates of the Guava and Java8 data formats?
|
@iamdanfox @uschi2000 turns out my debugging wasn't quite right - you can override path deserializer and the code we have just works. I mistakenly thought that what was actually a dropwizard misconfiguration was a jackson issue. I have reverted all the path related changes and instead adjusted DW used in test to use As far as I can tell the source repos moved around but artifacts didn't so we're all good (I just verified all java 8 related packages manually). |
| @@ -49,6 +50,15 @@ public Path getPath() { | |||
| return path; | |||
| } | |||
|
|
|||
| @Override | |||
| public String toString() { | |||
There was a problem hiding this comment.
Added for ease of debugging - you get useful assertions errors with this
|
thanks, @robert3005 |
No description provided.