(regression) Factory method generic type resolution does not use Class-bound type parameter

[marcospassos]

Describe the bug
We're currently using Jackson 2.11.2, updating to the latest version has broken several tests. After some investigation, we discovered that the bug was introduced in version 2.11.3, as the reproduction test shows.

The bug is somewhat related to static factory methods (stamp method in the example) – the same does not happen to constructor methods.

Version information
Introduced in 2.11.3

To Reproduce
#3221

Expected behavior
No error

Additional context
Add any other context about the problem here.

[marcospassos]

@cowtowncoder, this issue is blocking our whole system from receiving updates. It also affects our Kafka applications (Avro serialization), and I believe there is nothing to do except report it here and track down the issue.
Is there anything else I can do?

[cowtowncoder]

@marcospassos This is literally on top of my to-evaluate list (https://github.com/FasterXML/jackson-future-ideas/wiki/Jackson-Work-in-Progress) so I hope to look into it tonight or tomorrow.

[marcospassos]

Thank you for the feedback!

[cowtowncoder]

Looking at 2.11.3, guessing this might be related to #2795 or possibly #2846.
If it wasn't for something in a patch I would have guessed that there was some optimization going on that accidentally prevented application of mix-ins.

[cowtowncoder]

Hmmh. Some bad news here: I am not sure how this should ever have worked. A fundamental flaw is this:

    static class Timestamped<T> {
        public static <T> Timestamped<T> stamp(T value, int timestamp) {
            return new Timestamped<>(value, timestamp);
        }
   }

wherein it appears assumed that T declared for the factory method is somehow bound to class type variable T. This is not the case, as we might as well have:

    static class Timestamped<T> {
        public static <XYZ> Timestamped<XYZ> stamp(XYZ value, int timestamp) {
            return new Timestamped<>(value, timestamp);
        }
   }

which would show the problem -- the two type parameters T are completely unrelated in the first example.
They do not refer to the same type; and the one for factory method really is effectively declared like:

        public Timestamped<?> stamp(Object value, int timestamp) {
            return new Timestamped<>(value, timestamp);
        }

Static methods and fields are not really "part" of the class, and specifically they do not get bound type parameters -- if they did, there would be no need to add that <T> between static and Timestamped.

What I do not understand, tho, is how this would ever have worked. I don't think it really should.
And it is very unfortunate if it appeared too, simply due to naming.

[marcospassos]

So, if we rename the factory name generic parameter to anything other than T is it supposed to work?

[marcospassos]

Apparently not. Even renaming the generic parameter to avoid conflict, the issue persists. So I believe it should be something else.

[cowtowncoder]

No, it's not conflict, it is assumption that class type parameter T is related to static-method-local type parameter T: they are not.

Still, there was issue #921 in which such extra logic was added to help with problem similar to this. So I wonder if @vjkoskela might have an idea of what could be done here.
I am also puzzled at change being in patch version; obviously this is a side-effect of a fix but usually type resolution fixes are done in minor versions since they tend to have non-trivial regression risk.

[marcospassos]

Do you know any possible workaround? We're completely stuck depending on updating several projects that rely on Jackson. I can't see what we can do at our end to fix It somehow temporarily and gain time.

[cowtowncoder]

I wish I had a good answer. I will try to dig into this more and perhaps find a solution to make binding work.

One other thing that would help is figuring out the exact commit that changed the behavior as that might give a clue if something could be changed in a way that reverts this particular behavior without breaking anything else.

[marcospassos]

One other thing that would help is figuring out the exact commit that changed the behavior as that might give a clue if something could be changed in a way that reverts this particular behavior without breaking anything else.

It took some doing, but I found it.

[cowtowncoder]

Thank you @marcospassos! Not sure if it's good or bad but it definitely is an explicit change. I'll try reverting that, see if anything else breaks; if not, I should be able to make a patch. May need to make that all the way to 2.11, given how this can definitely break a lot of existing usage; making it necessary to cut another 2.11 release.

For 3.0 will need to think about this a bit; I might want to add a MapperFeature for optionally enabling behavior (but defaulting to false) but first things first.

[vjkoskela]

Sorry I'm late to the party. For the builder in #921 we needed to infer the generic types to instantiate the builder class from a type reference to the actual class.

I'm not familiar with mix-ins in Jackson, but I could see a similar strategy here if the mix-in binding via addMixIn inferred the type bounds. However, unlike builders where there is some expectation of 1:1 generic types between the target class and builder class, I don't believe this is true with mix-ins.

[cowtowncoder]

@vjkoskela No problem; issue is not actually mix-ins, but a change that removed earlier passing of class-variable-bindings for static type resolution. I think @marcospassos found the specific change and now I need to think of how to handle this issue.
In short term I hope to effectively revert the change but need to ensure this will not regress something else.

[cowtowncoder]

@marcospassos Interestingly enough, looks as if this was fixed in 2.11.4 via #2894. At least my test fails for 2.11.3 but not 2.11.4. This more as a sidenote; also simplifies things as I won't need to worry about 2.11.5 patch (was not planning such release).

But I'll check 2.12 where it probably re-regressed, then.

[marcospassos]

Thank you, @cowtowncoder. Any guess on when 2.12.5 will be out?

[cowtowncoder]

@marcospassos I'll have to see -- maybe I can figure it out by the next weekend.