Skip to content

Conversation

rohanlopes20
Copy link

Create new PR based on @cowtowncoder comments

#5064 (comment)

@yawkat
Copy link
Member

yawkat commented Sep 9, 2025

i havent followed the discussion, i dont know whether the name is settled yet, and i dont feel strongly about it, but how about 'readAutomatic' instead? it makes it clear that additional magic is happening, 'automatic' is similar to C++ auto, and it is still short enough to not be cumbersome.

@rohanlopes20
Copy link
Author

i havent followed the discussion, i dont know whether the name is settled yet, and i dont feel strongly about it, but how about 'readAutomatic' instead? it makes it clear that additional magic is happening, 'automatic' is similar to C++ auto, and it is still short enough to not be cumbersome.

@cowtowncoder can you suggest.

@cowtowncoder
Copy link
Member

I'll have to think about this: I do not think readObject() works.

My initial thinking is/was that we would just have a method for resolving type to create ObjectReader; but I am not sure what would be good name for that method.

Maybe ObjectReader forAutomaticType(...) or forDetectedType(...)?

@rohanlopes20
Copy link
Author

I'll have to think about this: I do not think readObject() works.

My initial thinking is/was that we would just have a method for resolving type to create ObjectReader; but I am not sure what would be good name for that method.

Maybe ObjectReader forAutomaticType(...) or forDetectedType(...)?

Something like this ? @cowtowncoder. Also, are we going to remove method added for readObject?

    public <T> ObjectReader forAutomaticType(T... reified) {
        Type type = getClassOf(reified);
        _assertNotNull("type", type);
        return _newReader(deserializationConfig(), _typeFactory.constructType(type), null,
                null, _injectableValues);
    }

and test

        final String JSON = "[1]";
        JsonParser p = MAPPER.createParser(JSON);
        Object ob = MAPPER.forAutomaticType()
                .readValue(p);
        p.close();
        assertTrue(ob instanceof List<?>);

@cowtowncoder
Copy link
Member

Yes, remove readObject(). And test should probably use target types other than java.lang.Object (although that can also be used) to ensure actual type introspection -- Object is "untyped" target, which maps JSON array as List anyway.

@cowtowncoder
Copy link
Member

cowtowncoder commented Sep 12, 2025

@rohanlopes20 just realized something: trying to make ObjectReader readerForAutomatic() (or similar) probably cannot work as there isn't actual type information included -- ObjectReader is not generic (unlike result value for ObjectMapper.readValue()).

I am not sure how to go about that, then -- I do NOT like either overloading dozens of readValue() methods, nor adding dozens of differently named ones. Nor changing signatures of readValue.

So not sure how to proceeed.

The only thing I can think of is to use this trick on creating JavaType maybe. But... would that be any less code from caller than just passing type (class) or TypeReference? (probably not).

EDIT: or maybe it does work? Test would need to use a POJO type, for sure.

EDIT/2: test I added shows it fails the way I originally feared: no type info passed since ObjectReader is non-generic. But maybe it could be changed... hmmm.

{
final String JSON = "[1]";
JsonParser p = MAPPER.createParser(JSON);
List<Integer> ob = MAPPER.forAutomaticType()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of (or in addition to), really needs to use POJO type, to ensure type capture works.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... which, alas, proves failure. :-(

@cowtowncoder cowtowncoder changed the title Renamed method to readObject and taking base from 3.x Implement #5064 (automatic type detection) Sep 12, 2025
* Factory method for constructing {@link ObjectReader} that will
* read or update instances automatically
*/
public <T> ObjectReader forAutomaticType(T... reified) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong location, should be next to other factory methods; I'll move.

Copy link
Member

@cowtowncoder cowtowncoder Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also need name to align with readerFor() somehow

EDIT: chose readerForDetectedType()

@cowtowncoder
Copy link
Member

cowtowncoder commented Sep 12, 2025

Ok; edited PR to show how things fail with naive approach.

Not sure there is a way to fix this.

@rohanlopes20
Copy link
Author

Ok; edited PR to show how things fail with naive approach.

Not sure there is a way to fix this.

It indeed fails :(. Let me check if any other way possible or not. Currently MAPPER.readerForDetectedType().readValue fails to detect Point class as ObjectReader is not generic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants