Moshi: Kotlin - Initializing objects with nulls

[maxandron]

Hi,

Might not be an issue with Moshi specifically, but when deserializing objects in Kotlin, I get objects with null values even though in Kotlin I did not declare them as nulls.

When parsing an empty json, Moshi first initiates the class with nulls
Object[] args = null; return (T) constructor.newInstance(args);

Kotlin does not throw a NullPointerException as I would have expected. And then when Moshi does not find suitable fields in the json the fields are just left as nulls.

Resulting in a Kotlin object that should be null safe, filled with nulls.

Here is an example of a failing test:

data class Foo(val barList: List<String>, val barName: String = "bar")

@Test fun testMoshi_NullKotlinObject() {
  val moshi = Moshi.Builder().build()
  val foo = moshi.adapter(Foo::class.java).fromJson("{}")

  // foo should not have any null values!
  assertNotNull(foo.barList) // Fails here
  assertNotNull(foo.barName)
}

[JakeWharton]

Kotlin's nullability is a facade and it's very easy to put nulls in a backing field whose property was declared as non-nullable when using reflection, as this library does heavily.

This is actually a request for a @Json(required=true) annotation which will put presence check before returning a class instance to application code. Although does an explicit null in the JSON actually fulfill the requirement for presence? Or does required also imply non-null?

[maxandron]

Yeah, that is what I was thinking as well. A @Json(required=true) annotation would be a good solution.

Regarding an explicit null in the json, I think the a null should satisfy the required attribute, because null is sometimes a legitimate value to expect.
But maybe it can be combined with a @NonNull annotation or another @Json(required=true, notnull=true) attribute as well

[sargunv]

Would there be any way to set the default? I want my entire API to have required=true but setting it on each field of each model would become tedious.

[FrancoisBlavoet]

Is there a way to tell moshi to just return a default value like emptyList() in that case ?
That's how I would end up with the most readable kotlin code

[swankjesse]

Unsure what action to take here. Might need a helper type adapter for Kotlin.

[serj-lotutovici]

I've took a deeper look into this. It's possible to write a JsonAdapter (at least with Kotlin 1.0.4) that will determine that a class was generated by Kotlin (see kotlin.Metadata annotation). The good news is that all fields are annotated with Jetbrains's NotNull and Nullable annotations, so it not that hard to determine which field should be initialised. The bad news, that there retention is RetentionPolicy.CLASS and they can't be accessed in runtime.

So (as I see) two actions could be taken here:

• Ether rely on some annotation processing library that will generated adapters in compile time.
• Or kindly ask the Kotlin guys to annotated the fields with an annotation that will be visible in runtime. Say kotlin.NotNull and kotlin.Nullable.

What are your thoughts?

[swankjesse]

Let's ask Kotlin for runtime retention.

[serj-lotutovici]

Done: https://youtrack.jetbrains.com/issue/KT-14909

[NightlyNexus]

Maybe something like this should just be in the samples? Although, it is unfortunate for apps that there is no way to distinguish between an explicit null and a missing field.

final class RequiredJsonAdapter extends JsonAdapter<Object> {
  public static final JsonAdapter.Factory FACTORY = new JsonAdapter.Factory() {
    @Override public JsonAdapter<?> create(Type type, Set<? extends Annotation> annotations,
        com.squareup.moshi.Moshi moshi) {
      List<Field> requiredFields = null;
      Field[] fields = Types.getRawType(type).getDeclaredFields(); // Does not check superclasses.
      for (Field field : fields) {
        field.setAccessible(true);
        if (field.isAnnotationPresent(Required.class)) {
          if (field.getType().isPrimitive()) {
            throw new IllegalStateException("Primitives may not be annotated with @Required.");
          }
          if (requiredFields == null) requiredFields = new ArrayList<>();
          field.setAccessible(true);
          requiredFields.add(field);
        }
      }
      if (requiredFields == null) {
        return null;
      }
      JsonAdapter<Object> delegate = moshi.nextAdapter(this, type, annotations);
      return new RequiredJsonAdapter(delegate, requiredFields);
    }
  };

  @Retention(RUNTIME) public @interface Required {
  }

  private final JsonAdapter<Object> delegate;
  private final List<Field> requiredFields;

  RequiredJsonAdapter(JsonAdapter<Object> delegate, List<Field> requiredFields) {
    this.delegate = delegate;
    this.requiredFields = requiredFields;
  }

  @Override public Object fromJson(JsonReader reader) throws IOException {
    Object value = delegate.fromJson(reader);
    checkRequiredFields(value);
    return value;
  }

  @Override public void toJson(JsonWriter writer, Object value) throws IOException {
    checkRequiredFields(value);
    delegate.toJson(value);
  }

  private void checkRequiredFields(Object value) {
    for (Field field : requiredFields) {
      try {
        if (field.get(value) == null) {
          throw new IllegalStateException(
              String.format(Locale.US, "Required field %1$s in %2$s was null.", field.getName(),
                  field.getDeclaringClass()));
        }
      } catch (IllegalAccessException e) {
        throw new AssertionError(e);
      }
    }
  }
}

[serj-lotutovici]

I investigated doing this with kotlin-reflection, but the dependency is too big to even considering. Unfortunately kotlin-reflect-lite doesn't give all the necessary info about the class, and also depends on proto-bug which is also about 5k methods.

@NightlyNexus would you be interested in contributing your RequiredJsonAdapter to this little lib?

[NightlyNexus]

@serj-lotutovici will do later today, thanks!

[serj-lotutovici]

Doe to the fact that producing a kotlin specific adapter, would require duplicating the functionality already present in ClassJsonAdapter, I vote for a @Json(required=Boolean) functionality. The specification being: "Require the key to be present in the input json".

A requireNonNull can be solved via a delegating adapter, and should not be necessarily present in the library.

@swankjesse what would be your verdict?

[swankjesse]

I think we could throw a JsonDataException if a required field is absent. I’d want everything to stay optional by default.

[swankjesse]

...the other option is for users to create a no-args constructor that provides defaults for everything. Not sure what that looks like in Kotlin.

[maxandron]

I too agree that required=true is the solution here, together with a default= argument

[rongi]

It will be very convenient if non-null fields are interpreted as @Json(required=true). Otherwise each data object will require moshi class with all fields optional and app class with proper nullability fields.

[swankjesse]

Moshi considers a kotlin property to be required if it is a parameter to the primary constructor, non-null, and has no default value.

[piospy]

Maybe the default values is another topic, but in case the parameter is null or not present in json when it's required, it would make sense to get the kotlin's default value of this parameter if it is defined in Kotlin's class.
See https://discuss.kotlinlang.org/t/default-values-and-reflection/874/2 .

[codetalks-new]

I forked KotlinJsonAdapter to provider default value .
so below was the orignal version of KotlinJsonAdapter

// Confirm all parameters are present, optional, or nullable.
    for (i in 0 until constructorSize) {
      if (values[i] === ABSENT_VALUE && !constructor.parameters[i].isOptional) {
        if (!constructor.parameters[i].type.isMarkedNullable) {
          throw JsonDataException(
              "Required value ${constructor.parameters[i].name} missing at ${reader.path}")
        }
        values[i] = null // Replace absent with null.
      }
    }

below is my modified version:
try to provide default value for missing field.

        // Confirm all parameters are present, optional, or nullable.
        for (i in 0 until constructorSize) {
            if ((mergedValues[i] === ABSENT_VALUE || mergedValues[i] == null) && !constructor.parameters[i].isOptional) {
                val param = constructor.parameters[i]
                val paramType = param.type
                if (!paramType.isMarkedNullable) {
                    if(paramType.isSubtypeOf(Number::class.createType())) {
                        mergedValues[i] = 0
                    }else if(paramType == String::class.createType()) {
                        mergedValues[i] = ""
                    }else if(paramType == Boolean::class.createType()) {
                        mergedValues[i] = false
                    }else if (paramType.isSubtypeOf(List::class.starProjectedType)) {
                        mergedValues[i] = emptyList<Any>()
                    }else if (paramType.isSubtypeOf(Map::class.starProjectedType)){
                        mergedValues[i] = emptyMap<Any,Any>()
                    }else{
                        throw JsonDataException(
                            "Required value ${constructor.parameters[i].name} missing at ${reader.path}")
                    }
                }else {
                    mergedValues[i] = null // Replace absent with null.
                }
            }
        }

[swankjesse]

No further action to take on this. We have a Kotlin adapter; please use that.