Ignore ConstructorDetector.SingleArgConstructor.PROPERTIES preference for Records.

[yihtserns]

To fix #3968.

NOTE: I also wanted to add test case to see what happens when the canonical constructor is @JsonIgnore-ed, but apparently that's (currently) not allowed because it will throw exception:

jackson-databind/src/main/java/com/fasterxml/jackson/databind/jdk14/JDK14Util.java

Lines 206 to 209 in 2cea7c9

	if (primary == null) {
	throw new IllegalArgumentException("Failed to find the canonical Record constructor of type "
	+ClassUtil.getTypeDescription(_recordClass.getType()));
	}

[cowtowncoder]

Is this based on semantics needing changing, or just that it happens to solve the failure case?
My understanding is that for Records, default of Mode.PROPERTIES is something we would want, not Mode.DELEGATING?

[yihtserns]

My understanding is that for Records, default of Mode.PROPERTIES is something we would want, not Mode.DELEGATING?

For when the constructor has single-arg, canonical constructor will already be recognised as properties-based creator by:

jackson-databind/src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java

Lines 1025 to 1033 in a1e6208

	if (propDef != null) {
	// One more thing: if implicit name matches property with a getter
	// or field, we'll consider it property-based as well
	String implName = propDef.getName();
	if (implName != null && !implName.isEmpty()) {
	if (propDef.couldSerialize()) {
	return true;
	}
	}

...because only a canonical constructor will have an implicit constructor + propDef with implicit name.

This ConstructorDetector config would only affect non-canonical constructors (which currently are/should only be considered for delegating creators).

Additional notes: Currently the only way to "implicitly" make canonical constructor a delegating creator is via @JsonValue:

jackson-databind/src/test-jdk14/java/com/fasterxml/jackson/databind/records/RecordImplicitCreatorsTest.java

Lines 34 to 44 in a1e6208

	record RecordWithSingleValueConstructorWithJsonValue(@JsonValue int id) {
	}
	record RecordWithSingleValueConstructorWithJsonValueAccessor(int id) {
	@JsonValue
	@Override
	public int id() {
	return id;
	}
	}

[cowtowncoder]

You mean, implicitly? And this because by default, canonical constructor is already considered (usually) Properties-based?

[yihtserns]

Yes, canonical constructor will be recognised as properties-based creator by:

1. jackson-databind/src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java

   Lines 1025 to 1033 in a1e6208

   	if (propDef != null) {
   	// One more thing: if implicit name matches property with a getter
   	// or field, we'll consider it property-based as well
   	String implName = propDef.getName();
   	if (implName != null && !implName.isEmpty()) {
   	if (propDef.couldSerialize()) {
   	return true;
   	}
   	}

2. jackson-databind/src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java

   Lines 542 to 548 in a1e6208

   	if ((propDef != null)
   	// [databind#3724]: Record canonical constructor will have implicitly named propDef
   	&& (propDef.isExplicitlyNamed() || beanDesc.isRecordType())) {
   	++explicitNameCount;
   	properties[i] = constructCreatorProperty(ctxt, beanDesc, name, i, param, injectable);
   	continue;
   	}

...because only a canonical constructor will have an implicit constructor + propDef with implicit name.

[cowtowncoder]

Ok. I think I understand how this works around the issue, although it still seems like it shouldn't be needed (but is due to... combination of factors). But since it actually resolves an actual problem & does not break other tests, I think I should merge this. And revisit whole thing for 2.16; but not block 2.15.x.

[cowtowncoder]

Ok, added some questions -- I think I understand the challenge, just seems tricky to combine defaulting of creator Mode (where Records definitely should default to Properties). But I understand that the issue then is just the conflict between 2 alternative Creators of same type.

However... for non-annotated ("implicit") case the Canonical constructor should win, I think; and in case of annotated (explicit) that one.

So I just want to try to make sure logic does not get even more fragile with various extra checks.

I hope to review this within 24 hours or so, but wanted add a quick note first.

[JooHyukKim]

In this case, perhaps the unification of POJO handling (POJOs do not have canonical constructors) and Records caused more problems than it's worth... again. :-/

After seeing a couple of additional checks to fix regressions, I'm considering the need to discuss risks, backup plans, or at least warnings for users, especially for those who use record class heavily.

• Warning : should users be informed?
• Back-up, is there any back-up plan? revert? another refactor?

NOTE: Maybe this is not a right place to share this?

[cowtowncoder]

@JooHyukKim Alas, when we have all existing issues, I am not sure how much users will benefit from more pro-active communication outside of issue updates (if I understood this correctly). It's very tricky situation of course, esp. with patch releases... we don't have mechanism for pre-releases (nor do users really use those even if we did). SNAPSHOT builds exist, but are rarely used too. Test coverage works for regressions but not use cases users have -- and users rarely contribute tests for theirs, not until they break :-( :-( :-(

[cowtowncoder]

For what it is worth I do plan on now finally tackling introspection rewrite for 2.16. It may be risky but at least we do have much improved test suite, as of 2.15.x. And -- I hope -- better understanding of challenges.

[cowtowncoder]

Thank you @yihtserns !