Remove ObjectMapper::copy() from QueryResource#1092
Conversation
There was a problem hiding this comment.
This seems to be happening because we are storing a copy of the ObjectMapper, can we instead just not try to store a copy?
I think we are doing this so that we can set the config below this, but from looking at the code, I don't think we need to do that even. If closing the OutputStream we are being handed in the streaming response breaks something, then we can wrap that object such that the close() method doesn't actually do anything instead of setting this parameter.
There was a problem hiding this comment.
That could break if the current behavior relies on something other than ObjectMapper to do the closing. I'll look at it more when I have fresh eyes.
There was a problem hiding this comment.
From what I can tell, nothing is actually closing the OutputStream, which makes me think that it is the Jersey framework that is taking on that responsibility.
If that is the case, then I really think that it is the Jersey framework's job to make sure that calling close() on the OutputStream doesn't screw with things. But if they haven't done that, it should be easy enough for us to ensure that close() doesn't get called.
There was a problem hiding this comment.
I agree with that. As a matter of fact, the only closable stream the objectMapper seems to be used on is the input stream where it parses the Query datatype. After trying a few quick examples it seems to function fine without the close. I'm going to try a few more tests then get rid of the copy completely.
There was a problem hiding this comment.
After investigating the code and running a few tests, there doesn't seem to be any side effect of using the injected ObjectMappers. I'm uploading a new commit which removes the copy. I'll change the PR title when that is up.
|
It is worth noting some of the changes were from a blanket reformat code... applied to the files that were touched. |
There was a problem hiding this comment.
This line is the actual change
drcrallen
force-pushed
the
jacksonObjectMapperCopyFix
branch
2 times, most recently
from
a6ad941 to
8134daf
Compare
|
TravisCI failed on unrelated code. |
drcrallen
force-pushed
the
jacksonObjectMapperCopyFix
branch
from
8134daf to
1febc0a
Compare
drcrallen
changed the title
There was a problem hiding this comment.
Since we don't need to inject InjectableValues (which is in itself a very confusing thing), we can probably remove this method entirely or at least remove Guice annotations and make it private.
drcrallen
force-pushed
the
jacksonObjectMapperCopyFix
branch
from
1febc0a to
a6b3fa9
Compare
…ected ObjectMapper directly * Workaround for FasterXML/jackson-databind#696
drcrallen
force-pushed
the
jacksonObjectMapperCopyFix
branch
from
a6b3fa9 to
d51b37c
Compare
|
LGTM, any reason not to merge? |
|
I was just waiting for Charles to remove the unnecessary Guice stuff, looks like that's done now. |
Remove ObjectMapper::copy() from QueryResource
3 participants
Ran into this one while working on Query-time Lookups.
Essentially anything annotated with
@JacksonInjectthat's in the query codepath does not inject at all currently.The configuration codepath still works as intended as far as I know.
This now uses the default configuration for
ObjectMapper(@Jsonand@Smile) which may auto-close the input/outputs.