Default values for primitive parameters

[michaelhixson]

I was surprised by the behavior of Jackson+Kotlin with respect to primitive parameters that have default values. It seems like those default values are ignored, and zero is used instead.

Here's an example:

class Main private constructor() {
  compainion object {
    @JvmStatic fun main(args: Array<String>) {
      val mapper = ObjectMapper().registerModule(KotlinModule())
      val string1 = "{\"i\":3}"
      val string2 = "{}"
      val value1 = mapper.readValue(string1, Foo::class.java)
      val value2 = mapper.readValue(string2, Foo::class.java)
      println("value1: $value1")
      println("value2: $value2")
    }
  }

  data class Foo
  @JsonCreator constructor(@JsonProperty val i: Int = 5)
}

That prints:

value1: Foo(i=3)
value2: Foo(i=0)

But I wanted it to print:

value1: Foo(i=3)
value2: Foo(i=5)

Is that beyond the scope of this module? Is it impossible for some reason?

[cowtowncoder]

@michaelhixson Value defaulting like this is not supported; there is no mechanism to do that at this point on jackson-databind side. Deserializers may can provide value defaults (and do that to provide for 0 and other Java defaults), so perhaps one could extend system for Kotlin module to further create alternate deserializer instances if different defaults are wanted.

Personally I think value defaulting is a bad idea with data formats, but in this case it looks like language itself is to provide defaulting, which is more sensible. Things just get tricky with respect to division of responsibilities; if there is a way for deserializer to provide something to indicate runtime to populate values maybe that could work. I don't know Kotlin runtime well enough to know if or how that could be done.

[michaelhixson]

@cowtowncoder Thanks. So I'm looking into how one might accomplish this. I'm not familiar with Kotlin either, but it appears that I can:

• At runtime, convert a Java Class object into a Kotlin Class object.
• With that Kotlin Class, use reflection to find its constructors, their parameter names, and which parameters are optional (which have default values).
• Build a map from parameter names to values, possibly omitting some of the optional parameters, and invoke one of the constructors.

Is there a place in Jackson, wherever it deals with beans, where it's juuuust about to use reflection to invoke a Java constructor with a certain set of parameters, and I can intercept that and do something else?

And if so, would I be able to tell apart parameters whose values came from the JSON source from parameters who didn't appear in the source?

[cowtowncoder]

@michaelhixson Abstraction you might want to use is ValueInstantiator; that gets called to create an instance of value, instead of using default no-args constructor. There is functionality to allow merging in other sources, like values passed using @JacksonInject, and default values (as per what JsonDeserializer says is the default for type).

[michaelhixson]

@cowtowncoder It doesn't seem possible to achieve what I want, but a small change to jackson-databind would make it possible. Mind giving me your thoughts on my proposal below?

I'd be willing to write the pull requests for jackson-databind and jackson-module-kotlin to make all this happen, assuming the changes sound reasonable to you.

My code that almost works (see the FIXME) looks like this:

class KotlinMod : SimpleModule() {
    override fun setupModule(context: SetupContext) {
        context.addValueInstantiators { conf, desc, instantiator ->
            if (instantiator is StdValueInstantiator) {
                KotlinValueInstantiator(instantiator)
            } else {
                instantiator
            }
        }
    }
}

class KotlinValueInstantiator(src: StdValueInstantiator): StdValueInstantiator(src) {
    override fun createFromObjectWith(ctxt: DeserializationContext,
                                      args: Array<out Any>)
            : Any? {
        val creator = withArgsCreator
        val kotlinFunction = when (creator) {
            is AnnotatedConstructor -> creator.annotated.kotlinFunction
            is AnnotatedMethod -> creator.annotated.kotlinFunction
            else -> null
        } ?: return super.createFromObjectWith(ctxt, args)
        val wasSeen: (KParameter) -> Boolean = {
            //
            // FIXME: This is wrong.
            //
            // This is not distinguishing between "the parameter was seen
            // and its value was zero" and "the parameter was not seen".
            //
            // The information I want lives in PropertyValueBuffer, in the
            // _paramsSeen and _paramsSeenBig fields.  The
            // PropertyBasedCreator had that information available (by way
            // of the buffer) when it called this method.
            //
            // Could it pass that information along somehow?
            //
            args[it.index] != 0
        }
        return kotlinFunction.callBy(kotlinFunction.parameters
                .filter { !it.isOptional || wasSeen(it) }
                .associateBy({ it }, { args[it.index] }))
    }
}

Proposal

1. Add a new interface to jackson-databind:

package com.fasterxml.jackson.databind.deser;

public interface ParametersSeen {
    boolean wasParameterSeen(int index);
}

2. Make PropertyValueBuffer implement that interface:

@Override
public boolean wasParameterSeen(int index) {
    if (index < 0 || index >= _creatorParameters.length) {
        return false;
    }
    if (_paramsSeenBig == null) {
        int mask = _paramsSeen >> index;
        return (mask & 1) == 1;
    } else {
        return _paramsSeenBig.get(index);
    }
}

3. Add a new method to ValueInstantiator:

public Object createFromObjectWith(DeserializationContext ctxt,
                                   Object[] args,
                                   ParametersSeen seen) throws IOException {
    return createFromObjectWith(ctxt, args);
}

4. Change PropertyBasedCreator to use that new method:

public Object build(DeserializationContext ctxt, PropertyValueBuffer buffer) throws IOException
{
    Object bean = _valueInstantiator.createFromObjectWith(ctxt,
            buffer.getParameters(_allProperties),
            buffer);

5. Then I could change my code to this:

class KotlinValueInstantiator(src: StdValueInstantiator): StdValueInstantiator(src) {
    override fun createFromObjectWith(ctxt: DeserializationContext,
                                      args: Array<out Any>,
                                      seen: ParametersSeen)
            : Any? {
        val creator = withArgsCreator
        val kotlinFunction = when (creator) {
            is AnnotatedConstructor -> creator.annotated.kotlinFunction
            is AnnotatedMethod -> creator.annotated.kotlinFunction
            else -> null
        } ?: return super.createFromObjectWith(ctxt, args, seen)
        return kotlinFunction.callBy(kotlinFunction.parameters
                .filter { !it.isOptional || seen.wasParameterSeen(it.index) }
                .associateBy({ it }, { args[it.index] }))
    }
}

[cowtowncoder]

@michaelhixson No immediate objections, although I'll need to have a closer look. The main concern as usual is backwards compatibility... and addition of a new argument to ValueInstantiators method seems like a potentially risky thing to do. It is a publicly available extension point, and probably used quite widely.

[cowtowncoder]

Extending on my earlier comment: there would really be a good upgrade story on how existing code would be kept working with new method, while allowing use of the new method. Since ValueInstantiator is an abstract class, it would be possible to make base implementation of the new method simply delegate to the new method, and deprecate old method. This would seem to keep things compatible on short term?

Another question concerns naming of ParametersSeen: that would make sense here, but I wonder if there are any other similar use cases where we could use this? While addition of one new class is not a huge deal (databind has 577 already) it would be nice to have fewer one-offs, if some reuse is possible.
Boolean checking by index seems potentially general enough to be used for other things.

Anyway, I think this approach could well work and be useful, so looking forward to a PR?

[michaelhixson]

Exploring alternatives...

In my use case, in my first proposal, Jackson is doing some unwanted work. It is eagerly finding default values for all missing parameters regardless of whether I need them. Ideally it would find those values lazily and individually upon request.

Proposal 2

1. In PropertyValueBuffer, change the visibility of getParameters(SettableBeanProperty[] props) from protected to public.

2. Add two more methods to PropertyValueBuffer:

// true if the parameter was seen in the source.
public boolean hasParameter(SettableBeanProperty prop)
{
    if (_paramsSeenBig == null) {
        return ((_paramsSeen >> prop.getCreatorIndex()) & 1) == 1;
    } else {
        return _paramsSeenBig.get(prop.getCreatorIndex());
    }
}

// The value of the parameter if seen, else fill in a default.
// Like a single-argument version of the existing getParameters method.
public Object getParameter(SettableBeanProperty prop)
    throws JsonMappingException
{
    Object value;
    if (hasParameter(prop)) {
        value = _creatorParameters[prop.getCreatorIndex()];
    } else {
        value = _creatorParameters[prop.getCreatorIndex()] = _findMissing(prop);
    }
    if (_context.isEnabled(DeserializationFeature.FAIL_ON_NULL_CREATOR_PROPERTIES) && value == null) {
        throw _context.mappingException(
            "Null value for creator property '%s'; DeserializationFeature.FAIL_ON_NULL_FOR_CREATOR_PARAMETERS enabled",
            prop.getName(), prop.getCreatorIndex());
    }
    return value;
}

3. Add this method to ValueInstantiator:

// Delegates to the existing createFromObjectWith method for backwards compatibility.
// Essentially does what the first line of PropertyBasedCreator.build(...) used to do.
public Object createFromObjectWith(DeserializationContext ctxt,
        SettableBeanProperty[] props, PropertyValueBuffer buffer)
    throws IOException
{
    return createFromObjectWith(ctxt, buffer.getParameters(props));
}

4. Change PropertyBasedCreator to use that new method:

public Object build(DeserializationContext ctxt, PropertyValueBuffer buffer) throws IOException
{
    // Instead of immediately calling buffer.getParameters(_allProperties), let the value
    // instantiator decide what to do.  That way, applications can override the behavior.
    // The default implementation is equal to the old behavior.
    Object bean = _valueInstantiator.createFromObjectWith(ctxt,
            _allProperties, buffer);

5. Then my KotlinValueInstantiator looks like this:

class KotlinValueInstantiator(src: StdValueInstantiator): StdValueInstantiator(src) {
    override fun createFromObjectWith(ctxt: DeserializationContext,
                                      props: Array<out SettableBeanProperty>,
                                      buffer: PropertyValueBuffer): Any? {
        val creator = withArgsCreator;
        val kotlinFunction = when (creator) {
            is AnnotatedConstructor -> creator.annotated.kotlinFunction
            is AnnotatedMethod -> creator.annotated.kotlinFunction
            else -> null
        } ?: return super.createFromObjectWith(ctxt, props, buffer)
        return kotlinFunction.callBy(kotlinFunction.parameters
                .filter { !it.isOptional or buffer.hasParameter(props[it.index]) }
                .associate { it to buffer.getParameter(props[it.index]) })
    }
}

[michaelhixson]

@cowtowncoder Let me know if you would you prefer that I phrase my idea as an incomplete pull request (without javadoc, tests, or meaningful commit messages) instead of writing it here as I have been.

[cowtowncoder]

@michaelhixson Yes I think that makes sense. From what I can see I think suggestion makes sense, but it is easier to see pieces fit together as part of PR.

Thank you for working on this and improving handling. As I mentioned earlier I think this will be useful for Scala as well.

[knes1]

@michaelhixson If you're working for a fix on this, could you also please look into #29 ? I believe it's almost the same issue, except for non-primitive values (e.g. Jackson deserializes missing property as null when it could have used the default value provided in @JsonCreator). It would be great if your work could resolve both issues.

[michaelhixson]

@knes1 It is the same issue. I worded this issue poorly; it's not only about primitive types. The changes discussed above should work for reference types as well.

[michaelhixson]

@cowtowncoder Thanks for merging that PR. I have questions about dependency versions...

Right now, jackson-module-kotlin depends on version 2.7.4 of Jackson core/annotations/databind/joda. Those changes that you merged aren't in that version of jackson-databind. How do I handle this? Do I bump these dependency versions to 2.8.0-SNAPSHOT, or do I wait to make this Kotlin PR until 2.8.0 is stable and released?

If I bump the dependency versions to 2.8.0-SNAPSHOT, some of the existing tests fail for me: TestCasesFromSlack1, TestCasesFromSlack2, TestGithub15. They seem to have problems deserializing enums. Is that a known issue?

[cowtowncoder]

@michaelhixson You should bump dependencies to 2.8.0-SNAPSHOT, so that we can work through all the issues there may be. I did not realize that Kotlin hadn't yet bumped master to 2.8.0-SNAPSHOT.

But first I'll make sure there is 2.7 branch, as that will be needed for 2.7 patches like 2.7.5.

As to enums, I haven't been aware, but maybe @apatrida might be?

[cowtowncoder]

Ok created 2.7 branch, updated version of master. Left main dependencies as is for now so as not to break build before someone has a chance to see what's up with test failures.

[michaelhixson]

I figured out a fix for the enums. If I change this line:

jackson-module-kotlin/src/main/kotlin/com/fasterxml/jackson/module/kotlin/KotlinModule.kt

Line 70 in a3985d0

if (member is AnnotatedConstructor) {

from this:

        if (member is AnnotatedConstructor) {

to this:

        if (member is AnnotatedConstructor && !member.declaringClass.isEnum) {

then the tests all pass again in 2.8.0-SNAPSHOT.

Edit: Made a PR for this #31

[apatrida]

I can review the PR...

[apatrida]

I'm working on this now, the default values including those from #29

[apatrida]

Fixed, committing soon.

[michaelhixson]

Awesome, thanks!

Good idea on callable.isAccessible = true. I was struggling with IllegalCallableAccessException and didn't realize the solution was so simple.

[apatrida]

@michaelhixson I think there is a Jackson feature switch I should be checking (force accessibility) but in Kotlin there are so many things that could be unaccessible it'd pretty much force people to turn it on anyway. From the prospective of the caller to Jackson it is accessible, from the perspective of Jackson it isn't. I wonder if there is anyway I can check accessible from viewpoint of another class (the caller) and then set it automatically. not sure is possible, but for now just forcing accessibility.

[cowtowncoder]

FWTW, the feature (enabled by default) is typically only meant to be disabled on security-limited platforms like Applets or (maybe) Google AppEngine. So for general usage it is most likely enabled.
Assuming this is wrt MapperFeature.CAN_OVERRIDE_ACCESS_MODIFIERS.

There is also related MapperFeature.OVERRIDE_PUBLIC_ACCESS_MODIFIERS, disabling of which would only try to force access on cases that may need it for access: the reason it is called on public things, by default, is that there was a significant performance benefit from doing so.

[apatrida]

I don't see those flags really being used by StdValueInstantiator or things it uses (AnnotatedConstructor for example which just calls ClassUtil.checkAndFixAccess if not accessible without checking the feature) .. But I added them to the Kotlin module, will commit in a moment.

[apatrida]

For the paths do not short circuit and just call the super class method for creating object:

            val accessible = callable.isAccessible
            if ((!accessible && ctxt.config.isEnabled(MapperFeature.CAN_OVERRIDE_ACCESS_MODIFIERS)) ||
                    (accessible && ctxt.config.isEnabled(MapperFeature.OVERRIDE_PUBLIC_ACCESS_MODIFIERS))) {
                callable.isAccessible = true
            }

            callable.callBy(callableParametersByName)