fix 3.0.0-SNAPSHOT build issues

[pjfanning]

Compilation fails due to major changes in jackson-databind

https://travis-ci.org/FasterXML/jackson-module-scala/jobs/588055923

[cowtowncoder]

Yeah, this will require quite a bit of work, likely. Let me know if I can help: Kotlin module has some challenges too, but I did help with some changes there.

[cowtowncoder]

@pjfanning One question/suggestion, more related to 2.12 (but eventually to 3.0/master too): I created this project:

https://github.com/FasterXML/jackson-integration-tests

to allow basic sanity/smoke tests; something that would run simple tests to ensure that a set (like 2.12.0-SNAPSHOT, 3.0.0-SNAPSHOT) of components in their latest publish snapshots versions still works. I have added tests for dataformats, some of datatypes (joda, guava), as well as certain combinations (Joda + XML/CSV/Properties wrt timestamp values).

I am not sure exactly how, but I wonder if there was a way to invoke some Scala mapper sanity tests from Java, built by Maven, to do simplest of validations that a scala-supporting object mapper can be constructed and used on something that exercises its functionality? I suspect the main challenge is that of building scala types for testing; perhaps integration-tests needs to be split to either have a support package(s) of value types (Kotlin no doubt needs something similar), or even just fully separate sub-projects (making current main-level project a sub-project).

At any rate, I would really like to quickly find out if changes I am making break Scala module: I just integrated PR for

https://github.com/FasterXML/jackson-databind/issues/1296

and doing refactoring for it. I will try to keep things backwards compatible for 2.12 obviously, but sometimes there are dependencies I do not see (Scala and Kotlin typically, although of course there are many 3rd party extensions beyond this). And with 3.x I will simply remove deprecated methods so there is more work anyway.

[pjfanning]

I'm not sure - all that can be done is to copy lots of the code and tests from jackson-module-scala into jackson-integration-tests - I'd be reluctant to maintain 2 copies of the code.

I have nightly builds running jackson-module-scala on travis but recently my travis account has had a problem that doesn't let me set up new builds for github orgs that I am member of. I need to either follow up with travis or try something else like github actions.

I'm afraid master version of jackson-module-scala was well broken before I even had a chance to set up builds. I have it compiling and tests running but many are broken - many tests are probably broken for similar reasons.

[pjfanning]

@cowtowncoder a lot of the broken tests appear to be because the DeserializerResolvers in master do not appear to have calls made to findReferenceDeserializer - only to findBeanDeserializer.

Tests that pass in v2.12 and that fail in master have lots of calls to findReferenceDeserializer in 2.12 but none in master.

https://github.com/FasterXML/jackson-module-scala/blob/master/src/main/scala/com/fasterxml/jackson/module/scala/deser/EitherDeserializer.scala#L115

[cowtowncoder]

@pjfanning Quick clarification wrt jackson-integration-tests: the idea would be a very small set of tests, similar to "hello world" style, even just one read, one write or such. To catch catastrophic cases. I wouldn't want to maintain separate sets of tests either. One other benefit is ability to test combinations (like Scala + CSV, Scala + XML), without adding dependencies across components. Those tests would not be duplicated FWTW. But it is up to you of course, I will mention this as something I plan to make use of and wanted to mention.

[cowtowncoder]

@pjfanning On findReferenceDeserializer: that sounds odd, as if anything, 3.0 should be making more accurate calls -- although without some of fallbacks, which may be the reason for breakage.

I would guess 2 likely sources of issues:

1. Some calls that relied on fallback (first check reference deserializer, if not found, try bean deserializer?) do not implement fallbacks any more
2. Type refinement is somehow not working so calls are being made to wrong lookup methods

Would it be possible to test to see if calls expected to go to findReferenceDeserializer() end up on findBeanDeserializer() or vice versa? Databind itself has only one reference type (for AtomicReference) and while that is tested is not extensively tested.

[pjfanning]

I'm not sure when I'll next have chance to spend time on this.

With reference types, wouldn't java.util.Optional be a reference type (akin to scala.Option and the master tests are failing for scala.Option)?

[cowtowncoder]

Yes, java.util.Optional is a reference type for sure; refined as such by jackson-module-java8 (in 2.x), and jackson-databind itself in master. And I think scala.Option should become one: but it is up to module to make sure that happens with TypeModifier implementation. But I think this is done by Scala module, both for 2.x and 3.0, and should also be tested that this occurs (by resolving via TypeFactory on which appropriate TypeModifier has been registered).

[pjfanning]

https://github.com/FasterXML/jackson-module-scala/blob/master/src/main/scala/com/fasterxml/jackson/module/scala/modifiers/ScalaTypeModifier.scala seems to be what does this

[pjfanning]

In 2.12, the reference type deserializer is created in this stack - this does not happen in master, despite the same ReferenceType being in scope.

java.lang.Exception: >>>>>> EitherDeserializerResolver findReferenceDeserializer class scala.util.Either
    at com.fasterxml.jackson.module.scala.deser.EitherDeserializerResolver$.findReferenceDeserializer(EitherDeserializer.scala:121)
    at com.fasterxml.jackson.databind.deser.BasicDeserializerFactory._findCustomReferenceDeserializer(BasicDeserializerFactory.java:1976)
    at com.fasterxml.jackson.databind.deser.BasicDeserializerFactory.createReferenceDeserializer(BasicDeserializerFactory.java:1553)
    at com.fasterxml.jackson.databind.deser.DeserializerCache._createDeserializer2(DeserializerCache.java:409)
    at com.fasterxml.jackson.databind.deser.DeserializerCache._createDeserializer(DeserializerCache.java:349)
    at com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCache2(DeserializerCache.java:264)
    at com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCacheValueDeserializer(DeserializerCache.java:244)
    at com.fasterxml.jackson.databind.deser.DeserializerCache.findValueDeserializer(DeserializerCache.java:142)
    at com.fasterxml.jackson.databind.DeserializationContext.findNonContextualValueDeserializer(DeserializationContext.java:581)
    at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.resolve(BeanDeserializerBase.java:524)
    at com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCache2(DeserializerCache.java:293)
    at com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCacheValueDeserializer(DeserializerCache.java:244)
    at com.fasterxml.jackson.databind.deser.DeserializerCache.findValueDeserializer(DeserializerCache.java:142)
    at com.fasterxml.jackson.databind.DeserializationContext.findRootValueDeserializer(DeserializationContext.java:591)
    at com.fasterxml.jackson.databind.ObjectMapper._findRootDeserializer(ObjectMapper.java:4684)
    at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4545)
    at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3497)
    at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3480)

[pjfanning]

I found what was breaking the scala Either tests on master but quite a lot more stuff to fix still.

@cowtowncoder could you look at https://github.com/FasterXML/jackson-module-jsonSchema/issues/140 ?

[cowtowncoder]

@pjfanning Ok good wrt Either tests. Added a note on json-schema module: at this point I have no plans to support it for 3.x, will be end-of-life'd (not much work on 2.x either).

[pjfanning]

@cowtowncoder another set of tests that have stopped working in master relate to the scala BigDecimal and BigInteger classes which are different from the Java equivalents. The deserializer is registered as it was for Jackson 2.x but Jackson no longer seems to try to use the deserializer.

https://github.com/FasterXML/jackson-module-scala/blob/master/src/main/scala/com/fasterxml/jackson/module/scala/deser/ScalaNumberDeserializersModule.scala#L59

I added an implementation of hasDeserializerFor - which wasn't needed for Jackson 2.x - but this method is not called. There are some unit tests and when I run them, I can't find any calls being made to the ScalaNumberDeserializersModule.

Any ideas?

[cowtowncoder]

Ok, first hasDeserializerFor() -- this is not used when locating deserializers but was more intended to figure out "what is POJO" by process of elimination, figuring out things that do not use BeanDeserializer. Initial use case was to be wrt security checks for polymorphic deserialization, where something that has explicit deserializer would usually be considered relatively safe compared to POJO handling which can open security holes. So even if that was not implemented, it should not affect finding of value deserializer.

[cowtowncoder]

@pjfanning Odd. Registration code looks perfectly fine... one thing to check might be to see if for some reason other findXxxDeserializer() method was called for some reason. That seems unlikely but maybe worth checking.

Does the test result in a BeanDeserializer being created, or exception of some kind?

I assume other custom deserializers work ok, as Scala module has a few, right? I wonder if registration code differs somehow.

[pjfanning]

A high fraction of the custom scala deserializers are broken. I'm just working through 1 at a time.

[pjfanning]

In 2.12, this code finds the right deserlializer (BasicDeserializerFactory)

    protected JsonDeserializer<Object> _findCustomBeanDeserializer(JavaType type,
            DeserializationConfig config, BeanDescription beanDesc)
        throws JsonMappingException
    {
        for (Deserializers d  : _factoryConfig.deserializers()) {

But in master, the factoryConfig.deserializers() appears to be empty - same test run in 2.12 and the factoryConfig.deserializers() has loads of deserializers, including all the custom scala ones.

[pjfanning]

It appears to be something in the ScalaObjectMapper class - a standard JsonMapper works ok in the scala tests but the ScalaObjectMapper mixin doesn't work properly

[pjfanning]

I've fixed the issue in ScalaObjectMapper - that fixes 50 tests but still about 30 broken or yet to be rewritten.

[cowtowncoder]

Well that is good progress!

[pjfanning]

@cowtowncoder one of the issues that is happening in master branch is that deserialization is running invalid JSON in one case - {"map":{"foo":"bar"},"seq"} - the result should have "seq":[].

jackson-module-scala does not actually create the json result and is just trying the help the core jackson code by providing class metadata. There are other issues affecting this test case on the Scala module side but could you check to see why the core code produces invalid JSON?

[cowtowncoder]

@pjfanning If I can figure out how to run Scala module tests on Eclipse, can try to help. But which tests in particular has this issue?

[pjfanning]

This issue appears in com.fasterxml.jackson.module.scala.deser.ListMapTest

[cowtowncoder]

Ok at least I can run sbt test from command-line now (I got a new mac mini, reinstalling things). And I get 27 failed tests, for scala 2.13 I think; many with generator complaining but bad output state. I'll see if I can isolate ListMapTest

[pjfanning]

If you just run sbt - you go into interactive mode

You can run testOnly com.fasterxml.jackson.module.scala.deser.ListMapTest

[cowtowncoder]

I got tests to run in Idea so that's ok.

If I read output correctly, it looks like value is missing, which could probably be due to one of 2 things:

1. Serializer is simply omitting value write. This is not something serializer should do (at the point it is asked, it basically must output a json value; filtering needs to be applied before call)
2. Write fails due to bad output state, but something swallows exception?

Another note... failure for IterableSerializer might be due to

  override def serialize(value: collection.Iterable[Any], gen: JsonGenerator, provider: SerializerProvider): Unit = {
    collectionSerializer.serializeContents(value.asJavaCollection, gen, provider)
  }

  override def serializeContents(value: collection.Iterable[Any], gen: JsonGenerator, provider: SerializerProvider): Unit = {
    serialize(value, gen, provider)
  }

wherein first "serialize()" can not just delegate to "serializeContents()", I think, since latter does NOT write START_ARRAY / END_ARRAY surrounding contents. I am not sure what the right way to fix this here would be -- IterableSerializer.serialize(...) in jackson-databind mostly deals with possible unwrapping for single element case (and SerializationFeature.WRITE_SINGLE_ELEM_ARRAYS_UNWRAPPED). But I wonder why it passes for 2.x.

[pjfanning]

@cowtowncoder thanks, I now understand the problem

Ages ago when I started to try to get the code to work with master jackson-databind, I was getting compile errors due to this code being commented out - it is implemented in 2.x.

The code I put in was wrong because it doesn't have the array start and end. I can fix that.

    // 16-Apr-2018, tatu: Sample code, but sub-classes need to implement (for more
    //    efficient "is-single-unwrapped" check)

    // at least if they can provide access to actual size of value and use `writeStartArray()`
    // variant that passes size of array to output, which is helpful with some data formats
    /*
    @Override
    public void serialize(T value, JsonGenerator gen, SerializerProvider ctxt) throws IOException
    {
        if (provider.isEnabled(SerializationFeature.WRITE_SINGLE_ELEM_ARRAYS_UNWRAPPED)
                && hasSingleElement(value)) {
            serializeContents(value, gen, ctxt);
            return;
        }
        gen.writeStartArray(value);
        serializeContents(value, gen, ctxt);
        gen.writeEndArray();
    }
    */

[pjfanning]

Down to 7 test failures now but there are also a few disabled tests too.

[pjfanning]

@cowtowncoder One thing that is not well tested in jackson-module-scala is the behaviour when optional features are set in the ObjectMapper. I noticed SerializationFeature.WRITE_SINGLE_ELEM_ARRAYS_UNWRAPPED when fixing some of the test failures in master branch.

I tried to add tests in the 2.12 branch but hit the following issue:

    val mapper = newMapper.enable(SerializationFeature.WRITE_SINGLE_ELEM_ARRAYS_UNWRAPPED)
    mapper.getSerializerProvider
      .isEnabled(SerializationFeature.WRITE_SINGLE_ELEM_ARRAYS_UNWRAPPED) shouldEqual true
    mapper.writeValueAsString(Seq("abc")) shouldBe """"abc""""

[info] - should serialize correctly with SerializationFeature.WRITE_SINGLE_ELEM_ARRAYS_UNWRAPPED *** FAILED ***
[info]   java.lang.NullPointerException:
[info]   at com.fasterxml.jackson.databind.SerializerProvider.isEnabled(SerializerProvider.java:434)
[info]   at com.fasterxml.jackson.module.scala.ser.IterableSerializerTest.$anonfun$new$12(IterableSerializerTest.scala:93)
[info]   at org.scalatest.OutcomeOf.outcomeOf(OutcomeOf.scala:85)
[info]   at org.scalatest.OutcomeOf.outcomeOf$(OutcomeOf.scala:83)

Is there a more correct way to set this feature on an ObjectMapper?

  def newMapper: ObjectMapper = {
    val result = new ObjectMapper
    result.registerModule(module)
    result
  }

[cowtowncoder]

Ah. getSerializerProvider() should not really be called (it shouldn't be exposed to outside) as it is per-call context object. Or more accurately, there is "proto-type" (or factory) instance configured with mapper, but actual instance is created on mapper.writeValue(). To check for state, instead just use mapper.isEnabled() which delegates as necessary. Calling getSerializationConfig() would be fine too, it is stateless.

On setting the feature, 2.10+ added builder style so for example

def newMapper : .... JsonMapper.builder()
    .enable(feature)
    .build();

but also note that SerializationFeature can be (re)configured on ObjectWriter instances using methods with(feature) and without(feature).

Hope this helps!

[pjfanning]

@cowtowncoder I changed my test to set the feature using the JsonMapper but I still get the NPE

[info]   java.lang.NullPointerException:
[info]   at com.fasterxml.jackson.databind.SerializerProvider.isEnabled(SerializerProvider.java:434)

The code in AsArraySerializerBase uses SerializerProvider.isEnabled and that's why I'm trying to test if it works ok

[cowtowncoder]

@pjfanning Calling SerializerProvider.isEnabled() is perfectly fine from serializer when it is passed: problem is only mapper.getSerializerProvider() which should not be used, not even from tests. If you want to check whether mapper has defaults set properly, call mapper.isEnabled(); that should give canonical answer.

So the problem is not with setting the feature but the access part: SerializerProvider instance mapper has is sort-of half-configured base, and it does not have BaseSettings object which contains feature flags. Hence NPE.

[pjfanning]

@cowtowncoder the thing is that I have logging in the serializer and the SerializerProvider isEnabled returns false even though I've enabled the setting in the mapper.

Update: I think I have this working now

[cowtowncoder]

@pjfanning Ok if you still see the issue wrt configuration setting, let me know and I can help. But as per update hoping it is ok now. :)

[pjfanning]

@cowtowncoder some of the code that won't compile in the jackson-module-scala since jackson master branch changes is this.

//  def serialize(value: Product, jgen: JsonGenerator, provider: SerializerProvider): Unit = {
//    val stringWriter = new java.io.StringWriter()
//    val valueJgen = jgen.getCodec.getFactory.createJsonGenerator(stringWriter)

I had a quick look at jackson-databind master branch but can't find createJsonGenerator

[pjfanning]

Still a large number of tests disabled - marked with //TODO fix comments

[cowtowncoder]

@pjfanning createJsonGenerator and createJsonParser were deprecated a while ago (in 2.2), replacements are createGenerator and createParser (for 2.x).

But I think there is the additional difference in 3.0, addition of context object:

public abstract JsonParser createParser(ObjectReadContext readCtxt, InputStream in)
public JsonGenerator createGenerator(ObjectWriteContext writeCtxt, OutputStream out)

in which matching databind contexts (DeserializationContext for createParser(), SerializerProvider for createGenerator()) are passed. So it should be mechanical change. Contexts are needed to retain all settings throughout read/write cycle.

[pjfanning]

@cowtowncoder In the code sample, the getCodec method has been removed too. Any suggestions about how I can create a new generator?

It looks like I can create one from the SerializerProvider but the test fails if I do it this way.

// def serialize(value: Product, jgen: JsonGenerator, provider: SerializerProvider): Unit = { // val stringWriter = new java.io.StringWriter() // val valueJgen = jgen.getCodec.getFactory.createJsonGenerator(stringWriter)

[cowtowncoder]

Right, getCodec() was removed as access to context ought to be enough.

What is the use for secondary generator here? For buffering purposes TokenBuffer should usually be used (just as an example). But if absolutely required, provider does have method "getGeneratorFactory()" which can construct generators and parsers (passing this as ObjectWriteContext) so that'd probably be the direct replacement.

[pjfanning]

@cowtowncoder the test works in 2.12.0 but if I modify the val valueJgen = jgen.getCodec.getFactory.createJsonGenerator(stringWriter) it fails. There is something about creating the JsonGenerator from the existing one that makes it work while trying to use the SerializationProvider breaks it. I'm half tempted to give up and just use an ObjectMapper with the Scala module registered.

[cowtowncoder]

@pjfanning can you link to the test -- maybe some more context might help. And maybe exception you get?

[pjfanning]

The test is https://github.com/FasterXML/jackson-module-scala/blob/master/src/test/scala/com/fasterxml/jackson/module/scala/ser/MapSerializerTest.scala#L97

It uses TupleKeySerializer (declared in same scala file). I have temporarily switched to using an Object Mapper instead of using a Json Generator - but you can look at the file commit history to see the previous code. When the Json Generator code is used (built from the SerializerProvider), the key value is blank while with the 2.12.0 branch code and the current Object Mapper hack in master, the key value is as expected in the unit test (line 97 test highlighted above).

[cowtowncoder]

Ok. I guess that since this is test code, this makes some sense for testing perspective. Difference in 3.x handling is to fix the main issue in 2.x / getCodec(), which is that in most regular use cases delegating from JsonGenerator/JsonParser should not "forget" all the settings (as new context is created & write/read isn't related to existing context at all). This test just happened to rely on ability to use mapper similar to if a static instance existed.

So test writes content in "json-in-json" style, which is something that would actually be nice to allow in some way (there is a databind issue for it, I think), but not currently supported.

[pjfanning]

@cowtowncoder another thing that I have disabled in master version of jackson-module-scala is the support for canSerialize/canDeserialize. ObjectMapper has these methods in jackson-databind but they are removed in master. jackson-module-scala supports helper functions that delegate to jackson-databind canSerialize/canDeserialize. Should I just deprecate these jackson-module-scala functions and leave them out of jackson-module-scala master?

[cowtowncoder]

Removal of can[De]Serialize() is intentional as it seems that these cannot really be implemented in very meaningful manner, the way API works.

I would probably remove anything that relies (or builds) on them from Scala module master?

[pjfanning]

builds are passing (although a few tests that work in jackson 2 have been disabled for a while) - the long-standing issue where some extra tests failed when Scala 2.11 is used has been fixed today

[pjfanning]

@cowtowncoder some of the scala-module tests are still disabled in master branch - ones that pass in Jackson 2 but fail with Jackson 3.

One example is https://github.com/FasterXML/jackson-module-scala/blob/master/src/test/scala/com/fasterxml/jackson/module/scala/deser/CaseClassDeserializerTest.scala#L71

The classes are defined earlier in the source file. The issue seems to relate to the Metric class having a field of MetricPath type.

The test when enabled fails with

[info] - should deserialize a case class with multiple constructors *** FAILED ***
[info]   com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Invalid definition for property 'path' (of type `com.fasterxml.jackson.module.scala.deser.CaseClassDeserializerTest$Metric`): Could not find creator property with name 'path' (known Creator properties: [])
[info]  at [Source: (String)"{"path":{"path":"/path","level":1,"isRoot":false},"value":0.5,"time":"2017-05-10T00:00:00.000+02:00","tags":[]}"; line: 1, column: 1]
[info]   at com.fasterxml.jackson.databind.exc.InvalidDefinitionException.from(InvalidDefinitionException.java:60)
[info]   at com.fasterxml.jackson.databind.DeserializationContext.reportBadPropertyDefinition(DeserializationContext.java:1782)
[info]   at com.fasterxml.jackson.databind.deser.BeanDeserializerFactory.addBeanProps(BeanDeserializerFactory.java:620)
[info]   at com.fasterxml.jackson.databind.deser.BeanDeserializerFactory.buildBeanDeserializer(BeanDeserializerFactory.java:269)
[info]   at com.fasterxml.jackson.databind.deser.BeanDeserializerFactory.createBeanDeserializer(BeanDeserializerFactory.java:150)

I added some debug logging and in Jackson 2, the Scala introspection code is called for MetricPath but it is not called in Jackson 3. Any ideas what would have led to this behaviour change?

[cowtowncoder]

@pjfanning Where does Scala introspection code get called? Is that via AnnotationIntrospector or something else?

[pjfanning]

@cowtowncoder https://github.com/FasterXML/jackson-module-scala/blob/master/src/main/scala/com/fasterxml/jackson/module/scala/introspect/ScalaAnnotationIntrospectorModule.scala#L213

This code registers the scala code that analyses scala classes - it is not just annotations that are handled, the code that is plugged into jackson when this module is enabled checks for Scala classes generally.

In the test case I highlighted 2 days ago, the Metric class is analysed by ScalaAnnotationIntrospectorModule (because jackson-databind asks it to) but jackson-databind does not ask ScalaAnnotationIntrospectorModule to analyse MetricPath class (the type of one of the members of Metric class). When Jackson v2 is used, both classes are analysed because jackson-databind asks for both to be analysed.

[cowtowncoder]

@pjfanning Ok so ScalaAnnotationsIntrospector is added after JavaAnnotationIntrospector and both probably after default JacksonAnnotationIntrospector. When chaining, calls for later introspectors are usually made although it depends on specific method in question (for some, explicit non-null/non-empty answer from higher precedence one is enough), but handling depends on method in question.

So do you know if calls to specific method are not being made, or more generally? There have been some changes to signatures of methods too (to always pass MapperConfig); but I would expect that override match is checked by compiler. If it's for specific AnnotationIntrospector call that is not being made, I could have a look. I think introspector pair handling is also tested to some degree, although not extensively (to ensure that logic on chaining should work).