Jackson Scala module build from`master` (v3) fails

[cowtowncoder]

Jackson Scala module build from`master` (v3) fails

[pjfanning]

master was working about 3 weeks ago and no changes have been made to jackson-module-scala master branch since

https://github.com/FasterXML/jackson-module-scala/actions/runs/9211075159

[pjfanning]

@cowtowncoder enabling MapperFeature.ALLOW_FINAL_FIELDS_AS_MUTATORS fixes the tests. The tests use immutable fields and this is the Scala norm - so hacking the tests to make them mutable isn't a good idea as far as I am concerned.

[cowtowncoder]

cc @JooHyukKim -- we were just discussing this.

[pjfanning]

@cowtowncoder I fixed one test where the exception type has changed but the 8 remaining failures are all in https://github.com/FasterXML/jackson-module-scala/blob/master/src/test/scala/tools/jackson/module/scala/deser/MergeTest.scala

All the tests in the test class fail with the same issue.

No fallback setter/field defined for creator property 'field1' (of `tools.jackson.module.scala.deser.ClassWithLists`)
 at [No location information] (through reference chain: tools.jackson.module.scala.deser.ClassWithLists["field1"])
tools.jackson.databind.exc.InvalidDefinitionException: No fallback setter/field defined for creator property 'field1' (of `tools.jackson.module.scala.deser.ClassWithLists`)
 at [No location information] (through reference chain: tools.jackson.module.scala.deser.ClassWithLists["field1"])
	at tools.jackson.databind.exc.InvalidDefinitionException.from(InvalidDefinitionException.java:65)
	at tools.jackson.databind.deser.CreatorProperty._reportMissingSetter(CreatorProperty.java:307)
	at tools.jackson.databind.deser.CreatorProperty._verifySetter(CreatorProperty.java:290)
	at tools.jackson.databind.deser.CreatorProperty.deserializeAndSet(CreatorProperty.java:220)
	at tools.jackson.databind.deser.bean.BeanDeserializer.deserialize(BeanDeserializer.java:289)
	at tools.jackson.databind.deser.DeserializationContextExt.readRootValue(DeserializationContextExt.java:267)
	at tools.jackson.databind.ObjectReader._bindAndClose(ObjectReader.java:1647)
	at tools.jackson.databind.ObjectReader.readValue(ObjectReader.java:1206)
	at tools.jackson.module.scala.deser.MergeTest.updateValue(MergeTest.scala:114)
	at tools.jackson.module.scala.deser.MergeTest.$anonfun$new$1(MergeTest.scala:29)

The tests are trying to use ObjectMapper.updateValue with a class that has an @JsonMerge annotation on one of the fields. This test has always passed and only recent jackson-databind changes have broken it.

I didn't write this test and it is testing non-core functionality - I am not 100% sure why this functionality was regarded as important to test. Simple readValue calls work for these classes and JSON inputs but something has busted updateValue.

[pjfanning]

The class that is being tested is
case class ClassWithLists(field1: List[String], @JsonMerge field2: List[String])

Scala case classes are a lot like Java records. The class instance is immutable. There are no setters to update the field values.
If you want to mutate an instance - you create a new instance with the new values.

This all works hunky dory in Jackson 2.

[cowtowncoder]

@pjfanning Hmmmh. Oh, one thing that might explain these is the change to MapperFeature.ALLOW_FINAL_FIELDS_AS_MUTATORS default (changed to false):

FasterXML/jackson-databind#4552

so there are 2 ways to go about it:

1. Change MapperFeature.ALLOW_FINAL_FIELDS_AS_MUTATORS for mapper used by tests (so it's like 2.x)
2. Change test not to rely on ability to forcibly set final fields (it's a quirk of JDK, presumably required by JDK serialization)

for jackson-databind I did (2) but (1) is quicker.

[cowtowncoder]

Ok, if it's that issue it's a good catch I think. And dependent builds should make it much easier to catch these (not quite prevent, but catch).

Although... need to resolve Sonatype publishing problem ASAP. Did file a ticket against their support, instructions not quite sufficient (I had to do DNS proof for 3.0 a while ago, but apparently need a ticket to refer to for validation or so). But really hoping they could just transfer publishing rights along accounts; not sure why they did not -- guessing it's to force sort of "reset" so that legacy publishing would be less likely to be abused.

[cowtowncoder]

@pjfanning Right, more likely tests should use Constructor-passed properties since that's what is more likely real usage. But that's more work and if tests are of questionable value better solve the immediate issue.
Even Java usage has over time moved more and more to using Constructors and immutable classes (which makes sense), or lately, records.

This is why I think rewriting property introspection to better handle Creator properties was so important: focus is definitely for that side, not for direct field assignment or (even less) setters.

[pjfanning]

@cowtowncoder approximately what sort of annotations would be needed on

case class ClassWithLists(field1: List[String], @JsonMerge field2: List[String])

to convince jackson-databind to use the constructor instead of trying to use a setter?

[cowtowncoder]

@pjfanning This is where the idea of callback to "canonical" Creator would come in: databind would ask which (if any) of detential potential properties-based Creators is the canonical one if any -- list of PotentialCreators is the one used by databind for all processing.
So module would indicate this as THE candidate.

But aside from that idea, if there is just one constructor in the class, and parameter names (implicit names) are detectable, it would (in 2.18) be selected as implicit Creator, without any annotations.
I think Scala module does provide implicit names. So it should be auto-detected... unless there also happens to be no-args constructor?
If it is not auto-detected, I am not quite sure why: this is a case tested with 2.18 unit tests.

But if all of above fail, any of:

1. @JsonCreator (with or without mode)
2. @JsonProperty on at least one parameter

(@JsonProperty would be needed on all parameter if implicit names not detected)

[pjfanning]

I tried to find an equivalent test in jackson-databind for records and found

https://github.com/FasterXML/jackson-databind/blob/master/src/test-jdk17/java/tools/jackson/databind/failing/RecordUpdate3079FailingTest.java

This test is broken. The testDirectRecordUpdate is the closest to what the MergeTest in this module is trying to do.

[cowtowncoder]

@pjfanning Oh.... updateValue() is a whole another problem. If that is part of the problem, yeah, there's no good solution I'm afraid. Records/case/data classes are immutable, and then the only way to support things (aside from yucky force-changing-of-immutable fields) would be to do "convert" approach -- try to access state of existing value, override with data, deserialize. And mixing serialization side (access to existing state) with deserialization is working against design where ser/deser do not have full access across (that is, from deserialization workflow there is no way to invoke serialization, or vice versa). ObjectMapper.convertValue() works because ObjectMapper can access both sides.

So, for updateValue() case it either has to force setting of final Fields, or just fails.

[cowtowncoder]

Apologies for missing updateValue() in there (it is visible now that I looked).
That @JsonMerge is a tell I suppose. But I am not sure we should even pretend to support @JsonMerge via Creator parameter, due to inability to make that work.

EDIT: thinking about this bit further, added a new note on FasterXML/jackson-databind#3079 -- basically, I think it would be theoretically possibly to implement update because although JsonDeserializer cannot indeed access JsonSerializer(s) for conversion, there is access (from POJOPropertiesCollector) to underlying accessors (getter-method, field). So one could conceivably construct a "merging deserializer" that would access properties of instance to "update" (merge), and then use those for new instance as needed (if input does not contain value).

It would probably a big undertaking to actually implement, so for time being it is safe to say we just do not support merging of immutable types. But good to know conceptually it could be supported.

[cowtowncoder]

@pjfanning I think change for FasterXML/jackson-databind#4572 -- defaulting to "sort alphabetically" for properties -- also added couple more failures. But we are close to green build for 3.0.0-SNAPSHOT I think (I fixed modules other than Scala and Kotlin so nothing else fails as of now).

Sonatype auth issues are resolved as well: have to use different Nexus user token b/w Jackson 2.x and 3.0 -- and thereby different Github Secrets. master has CI_DEPLOY_USERNAME3 and CI_DEPLOY_PASSWORD3; I updated Scala module's .github/workflows/ci.yml appropriate (just for master).

[pjfanning]

@cowtowncoder I don't think that "sort alphabetically" change would be popular with Scala users. For me, I prefer the JSON to serialize with the fields in the defined order of the case class fields. Java Records are very similar to Scala Case Classes and again for me if I have record Person(String name, int age) - I would like to see name appear before age in the JSON.

[cowtowncoder]

@pjfanning As of now, this is merely change to default settings that user controls.

But I do agree that personally, for me, declaration order of Records/Case/Data classes seems like a sensible default ordering (in absence of explicit @JsonPropertyOrder).
This is why I created

FasterXML/jackson-databind#4580

when realizing that although Creator properties are -- by default, once again -- sorted before other kinds of properties, within that group alphabetic-sorting is still used with current code (2.18/master).

But it need not be: the main question is that of configurability; otherwise I think implementation is easy, it's all in POJOPropertiesCollector method _sortProperties().