ObjectReader readValue lacks Class<T> argument

[robinroos]

Since ObjectMappers are mutable, I am exposing ObjectReader and ObjectWriter through a factory.

Whereas ObjectMapper.readValue(String content, Class valueType) is of known Type =valueType, the equivalent methods on ObjectReader are declared as returning T but do not take a Class valueType parameter.

Even ObjectReader.forType(Class<?> valueType) does not return a parameterized instance of ObjectReader.

The impact is that, whereas with ObjectMapper.readValue(String,Class) I do not have to typecast the returned value, when I start using ObjectReader.readValue(String) or ObjectReader.forType(Class).readValue(String) I must explicitly typecast the result.

Surely either ObjectReader.readValue should be overloaded with Class valueType (presumably this is trivially easy), or ObjectReader.forType(Class valueType) should return a fully parameterized ObjectReader (more complicated as ObjectReader is not a parameterized class)?

[robinroos]

Some clarification is probably in order here.

The application is using Java 8.

The value resulting from ObjectReader.readValue() is not assigned to a variable, but passed to a method which is heavily overloaded.

[robinroos]

I submitted a PR #2637 which adds the necessary method overloadings. See: #2637

[robinroos]

As things stand I have (note the typecast):

return upstreamTransposer.transpose(
    (DriverDetails) downstreamReader.forType(DriverDetails.class).readValue(inputString)
);

With PR #2637 applied, I can then reduce my client code to:

return upstreamTransposer.transpose(
    downstreamReader.readValue(inputString, DriverDetails.class)
);

[robinroos]

I added a Test class which covers all of the added method overloadings.

[robinroos]

Noting that this just missed 2.10.3 (published yesterday), I'd appreciate it if the PR could form part of 2.10.4.

[cowtowncoder]

@robinroos API changes must go in minor release, so changes would need to go in 2.11.0 anyway.

[robinroos]

Ok.

[robinroos]

Please add the 2.11.0 label if you are now happy for this to be included.

[cowtowncoder]

Ok. I went back and forth with this a few times, but in the end I guess I can see the benefit, and that while it extends number of methods, it does not violate consistency -- partly due to existence of JsonParser taking variants.

I will merge this, make minor edits (add "since" tags), and it'll go in 2.11.0.rc1.

One question on testing: I can see why mocking is needed to get some coverage on, say, URL-taking version (without complexity from creating temporary file, URI for those etc). But I am not sure if that achieves much otherwise... since actual code is not called, right?
So just curious as to thoughts on that part.

[robinroos]

Thanks @cowtowncoder.

The tests show that the overloaded methods do indeed invoke the appropriate non-overloaded method (c/o verify), and illustrate that the returned type does not additionally require type-casting.

The actual code (the pre-existing non-overloaded methods) is mocked.

[robinroos]

In ObjectReaderTest, the added method testCanPassResultToOverloadedMethod() adds no value as it merely illustrates that javac can disambiguate based on <T> T.