Seeming parameter name consideration in jackson-module-scala 2.12.0+ causes new exceptions

[blast-hardcheese]

Related PR that triggered this investigation: guardrail-dev/guardrail#927
Reproducing repo: https://github.com/blast-hardcheese/jackson-module-scala-bug#first-issue

README from reproducing repo

Repo to show ObjectMapper with DefaultScalaModule 2.12.0+ causes some new and creative runtime errors.

First issue

In the first case, methods annotated with @JsonCreator with the same name but different types now require that the parameter names be different, otherwise

Conflicting property-based creators: already had explicitly marked creator [method regression.ConflictingJsonCreator#apply(long)],
encountered another: [method regression.ConflictingJsonCreator#apply(java.lang.String)]

Second issue

In the second case, all @JsonCreator's must have different parameter names from the member of the value class, otherwise:

Cannot construct instance of `regression.ConflictingMember` (although at least one Creator exists):
no int/Int-argument constructor/factory method to deserialize from Number value (10)

[cowtowncoder]

Quick note/question: reference to "property-based Creator" suggests that use of mode might avoid collision:

@JsonCreator(more = JsonCreator.Mode.DELEGATING)

[pjfanning]

this issue seems to be in 2.11 too - I just added a test to the 2.11 suite that fails - based on the PositiveLong scenario. -- b643f57

[pjfanning]

I see zero evidence that jackson-module-scala ever supported JsonCreator annotations on companion objects -- these are the only tested use cases - https://github.com/FasterXML/jackson-module-scala/blob/2.12/src/test/scala/com/fasterxml/jackson/module/scala/deser/CreatorTest.scala

[blast-hardcheese]

@pjfanning If you alter build.sbt to bump the jackson version back down to 2.11.4 all three of the test cases pass:

sbt:regression> run
[info] Compiling 1 Scala source to /private/tmp/jackson-module-scala-bug/target/scala-2.12/classes ...
[info] running regression.Bug
Success(PositiveLong(10))
Success(ConflictingJsonCreator(10))
Success(ConflictingMember(10))
[success] Total time: 3 s, completed Feb 24, 2021 4:21:33 PM

[blast-hardcheese]

unless I've done something profoundly silly, I believe my initial claim still stands -- it is interesting that this seems to be unintended behaviour (unexpected feature?) though.

[pjfanning]

@blast-hardcheese could you review #506? - it fails on jackson 2.11

[blast-hardcheese]

@blast-hardcheese could you review #506? - it fails on jackson 2.11

On it

[pjfanning]

@blast-hardcheese I got the test to pass when I moved it out of the CreatorTest object - pretty strange

[blast-hardcheese]

@pjfanning This appears to be due to nesting PositiveLong inside object CreatorTest. I created a package creatorTest and moved those definitions into that package, then import creatorTest._ and got

[info] - should support multiple creator annotations *** FAILED ***
[info]   PositiveLong(10) did not equal PositiveLong(10) (CreatorTest.scala:176)

which seems to be more of an issue with shouldEqual than the deserialization.

[pjfanning]

test is merged to 2.11 and a #507 is prepared for 2.12 branch -- so far seems to pass (when expected to fail)

[blast-hardcheese]

what you have in that PR is the known good behaviour -- altering the two apply methods to be

def apply(value: Long): PositiveLong = ...
def apply(value: String): PositiveLong = ...

would exercise the first regression behaviour. long: Long/str: String is the known good construction.

[blast-hardcheese]

The second would be

class PositiveLong private (val value: Long)
object PositiveLong {
  @JsonCreator
  def apply(value: Long): PositiveLong = new PositiveLong(value)
}

where the value in def apply(value: Long) would conflict with the value in private (val value: Long) in the class.

[blast-hardcheese]

If you'd like I can add the failing tests to 2.12, I have a pretty good handle on things at this time

[blast-hardcheese]

OK, so

• 2.11 branch, passes, as expected: Fully representing regression test for #505 #508
• 2.12 branch, fails, as expected: Issue 505 test (2.12) #509

It seems as though after 7fce4ca, having both class ConflictingJsonCreator private (val value: Long) as well as class ConflictingMember private (val value: Long) seems to be unnecessary, as prior to that commit there were two distinct exceptions thrown, but after that commit only one error is thrown. Presumably there was an upstream bugfix in jackson itself. If you'd like me to alter either of those PRs I'm happy to.

Thanks again for your effort through all of this

[cowtowncoder]

One quick note from core Jackson side: I am thinking of releasing 2.12.2 relatively soon, but if there's a chance this might get addressed would be happy to wait until that happens (or it becomes clear there's a blocker). So I'll add this to things I'll keep an eye on (https://github.com/FasterXML/jackson-future-ideas/wiki/Jackson-Work-in-Progress).

[blast-hardcheese]

I've got a bisect running, I'll see where things stand in the morning

[pjfanning]

I merged a change that seems to fix @blast-hardcheese test cases

[pjfanning]

when I forward fitted to master/jackson3 branch, one of the new tests is broken there - I've marked it as ignored for time being because jackson 3 is not yet planned for release and it is one of 4 or 5 tests that fail in that branch (that pass in jackson 2 branches)

[blast-hardcheese]

Confirmed the fix, thank you!