Fix unintended change of explicit creators priority (including primary constructor of record class )

[JooHyukKim]

fixes #4724

[cowtowncoder]

I am not sure I follow this comment... does _addExplicitDelegatingCreator override possible formerly added ones or... ?

[JooHyukKim]

does _addExplicitDelegatingCreator override possible formerly added ones

Yeah, via BasicDeserializer._handleSingleArgumentCreator() -> CreatorCollector.addStringCreator().
Due to both constructor and @JsonCreator-annotated method being single-arg with same type.

[cowtowncoder]

Hmmmh. I would have thought a conflict should arise if multiple explicit creators of same type exist. I'll need to think this through; I am not comfortable with change I don't understand. :)

[JooHyukKim]

if multiple explicit creators of same type exist

This may be a problem as well. There seems to be no forcing deduplication.

Or could also be wrong with current issue where record classs primary constructor and creator method are being considered. Ideally we should just know which one to choose.

[cowtowncoder]

There is intended logic; but precedence only works within category... so it might not be considering choices as being similar enough to collide.

But the basic rule is that an explicitly annotated Creator does override (and not conflict) default one; both Record case and POJOs. Default one does similarly have precedence over implicit ones; implicit ones do not conflict (if multiple exist, none chosen).
But explicitly annotated constructors should conflict -- except there's Properties- vs Delegating which are not conflicting.

I'll need to check out this example again to see if I agree with intended resolution.

[JooHyukKim]

The basic rule is that an explicitly annotated Creator overrides the default one, for both Records and POJOs.

After reviewing your explanation, I think we could benefit from introducing the concept of "default-ness" to creators. Currently, the primary constructor of a record class is treated as just another "explicitDelegatingConstructor." At the point of setting the creator, we can’t use default-ness to select the right one.

By using "default-ness," we could establish a clearer ordering without relying on for-loop direction, as we are now.

[JooHyukKim]

Forgot to mention where to exactly use the "default-ness"? Possibly within CreatorCollector.verifyNonDup() when CreatorCollector.addStringCreator() is called.

[cowtowncoder]

Quick note: I think the term should be "default creator" but in some places went with "Primary Creator", like in POJOPropertiesCollector._addCreators(). But I will change that now.

The default constructor of the Record class is absolutely not explicit one since there is no annotation. So if there are true explicit creators, it should be ignored.
Note, too, that POJOs (and Kotlin/Scala equivalents) can introduce Primary creators as well; via `AnnotationIntrospector.

Conversely tho it has higher precedence than implicit ones: so three levels really:

1. Explicit creators -- annotations (or anything AnnotationIntrospector indicates)
2. Default Creator
3. Implicit Creators

[cowtowncoder]

So, basically "default-ness" is not carried along BasicDeserializerFactory and it should not need to.
But it is considered during POJOPropertiesCollector._addCreators().

It looks like somehow default constructor is found as-is, but also as explicit one.
This latter part seems odd... I'll see if PotentialCreator might be constructed incorrectly or something.

[cowtowncoder]

One other note: "default creator" (or "default constructor") can be confusing wrt JsonCreator.Mode -- DEFAULT for that is completely unrelated (rather that DEFAULT basically means mode is unspecified/undefined).

[cowtowncoder]

Ok, I think this from POJOPropertiesCollector:

   protected void _addCreators(Map<String, POJOPropertyBuilder> props)
   {
        //...

        if (defaultCreator != null) {
            if (!creators.hasPropertiesBased()) {
                // ... but only process if still included as a candidate
                if (constructors.remove(defaultCreator)
                        || factories.remove(defaultCreator)) {
                    // But wait! Could be delegating
                    if (_isDelegatingConstructor(defaultCreator)) {
                        creators.addExplicitDelegating(defaultCreator);
                    } else {
                        creators.setPropertiesBased(_config, defaultCreator, "Primary");
                    }
                }
            }
        }

is what is wrong since defaultCreator is indeed NOT annotated, hence NOT explicit.

[JooHyukKim]

is what is wrong since defaultCreator is indeed NOT annotated, hence NOT explicit.

Thank you for digging into it @cowtowncoder ! Lemme see how we can approach this.

[cowtowncoder]

is what is wrong since defaultCreator is indeed NOT annotated, hence NOT explicit.

Thank you for digging into it @cowtowncoder ! Lemme see how we can approach this.

Yup, see #4740.