Moshi: Support Kotlin inline classes when they become stable

[Coneys]

Moshi with Kotlin reflection wasn't working at all, so I tried to switch to Codegen. Everything is working good except Inline classes:
inline class Seconds(val value: Long)
with my adapter:
class SecondsAdapter() { @FromJson fun fromJson(jsonReader: JsonReader, delegate: JsonAdapter<Seconds>): Seconds { val long = jsonReader.nextLong() return Seconds(long) } }
Error:
java.lang.ClassCastException: java.lang.Long cannot be cast to pl.amistad.stratapp.time.Seconds
Am I doing something wrong, or Moshi isn't ready yet for inline classes?

[ZacSweers]

We haven't looked into inline classes as they're not stable yet and the version of metadata we use doesn't support it yet

Want to file a separate issue for what you were seeing go wrong with reflection? They should have feature parity

[Coneys]

Yeah, I will create separate issue for reflection. I was suprised, because without reflection and without codegen, parsing inline classes was working out of the box.
I am really confused with Moshi, every way of using it has some problems. Anyway, thanks for response

[swankjesse]

Could you please name the other issues you encountered? I'm not too surprised inline classes don't work; that's an unstable Kotlin feature we haven't spent any time on.

[Coneys]

Sure, inline classes wasn't most important thing here (but as I mentioned, without reflection or Codegen, on clear Moshi inline classes are working out of the box).
My api return null when some field is missing, and Codegen doesn't use default values from Kotlin class.
I have read these issues:

554

552

but I couldn't find anything usefull. With Codegen I have to create new adapter for every class that can be null and peek if token is null:

 if (reader.peek() == JsonReader.Token.NULL) {
            reader.nextNull<Unit>()
        }

With reflection I had two problems.

1. IllegalArgumentException from constructor:
   Callable expects 11 arguments, but 10 were provided (here was description of my constrcutor)
   My constructor had 10 arguments, so I have no idea what causes that.
   What is more, when I tried to reproduce it, I can't. Now it just works, and I am more than sure that I didn't change anything else (I had that code on another branch)
2. When some field is missing from json, moshi throws
   Expected at least one @ToJson or @FromJson method on com.squareup.moshi.kotlin.reflect.KotlinJsonAdapterFactory
3. Still nulls aren't handled with default values from constructor (I tried to add nullSafe to adapter)

Class:

data class UserInfo(
    val username: String,
    @Json(name = "first_name") val firstName: String?,
    @Json(name = "last_name") val lastName: String?,
    val score: Int,
    val phone: String?,
    val gender: Gender = Gender.UNKNOWN,
    val time: Seconds = Seconds(0),
    val totalScore: Int = 0,
    val totalTime: Seconds = Seconds(0),
    val ticketBoothResult: TicketBoothResult?
)

I provided adapters for Seconds and Gender.

Only solution for me now is to stick with Codegen and create many adapters just to handle null. Is there another way?

[swankjesse]

1. Glad this started working.
2. That's a very weird exception for that cause. Could you send a standalone test case for this? Seems like two independent problems might be making things difficult for you!
3. We deliberately treat null as distinct from default, and also as distinct from absent. If you need another behavior you're going to need to handle it manually. I stand behind this policy; I think converting null to default will make it difficult for you to change your model over time.

[Coneys]

1. Me too ;)
2. I found the issue. I was adding KotlinJsonAdapterFactory() twice. I had something like "default" Moshi builder which already had that factory, and in my function I added it again.
3. I understant that it should be treated differently, but is there a way to change that behaviour? Something like default adapter for anything: "if you find null, treat it like absent". Maybe some clever way to remove null values from json?

[volo-droid]

I'm facing a similar issue:

IllegalArgumentException from constructor: Callable expects 4 arguments, but 3 were provided

It's indeed related to the inline class. Whenever your class contains a property of inline class type, synthetic constructor is generated with dummy parameter of DefaultConstructorMarker type

For example, for these Kotlin classes:

inline class UnitIdentifier(val value: String)

data class Entity(
    @Json(name = "unitId") val unitIdentifier: UnitIdentifier,
    @Json(name = "distance") val distance: Double,
    @Json(name = "coordinates") val location: LatLng
)

the following constructors are generated (decompiled Kotlin bytecode into Java):

private Entity(String unitIdentifier, double distance, LatLng location) {
    this.unitIdentifier = unitIdentifier;
    this.distance = distance;
    this.location = location;
}

// $FF: synthetic method
public Entity(
    @Json(name = "unitId") String unitIdentifier, 
    @Json(name = "distance") double distance, 
    @Json(name = "coordinates") LatLng location, 
    DefaultConstructorMarker $constructor_marker
) {
    this(unitIdentifier, distance, location);
} 

That behaviour is not affected by the presence or absence of the @Json annotation, and IMHO, is a Kotlin compiler issue, as the synthetic constructor there looks totally redundant.

[Wasabi375]

I have recently experimented with moshi and the only feature I was missing is code-gen for inline classes. So I looked at the moshi code and just added it myself. Are you interested in a PR?

We haven't looked into inline classes as they're not stable yet and the version of metadata we use doesn't support it yet

Since inline classes are not yet stable you might want to decide not to support them. If you are interested I will create a PR and also look in how you test the code-gen to write proper tests for it.

[ZacSweers]

Right, we're not looking to add anything around it formally until it's
stable. Happy to have a writeup here of any pitfalls you encountered that
we should be aware of, or link to your fork. I don't want to recommend
making a PR yet since we will likely be changing master around underneath
it and cause you to have to fix rebase conflicts. Happy to get a PR once
it's stable though!

On Fri, Aug 23, 2019 at 9:25 PM Burkhard Mittelbach <
[email protected]> wrote:

I have recently experimented with moshi and the only feature I was missing
is code-gen for inline classes. So I looked at the moshi code and just
added it myself. Are you interested in a PR?

We haven't looked into inline classes as they're not stable yet and the
version of metadata we use doesn't support it yet

Since inline classes are not yet stable you might want to decide not to
support them. If you are interested I will create a PR and also look in how
you test the code-gen to write proper tests for it.

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
https://github.com/square/moshi/issues/776?email_source=notifications&email_token=AAKMJPXLJLARK3EAVZJNXDTQGCEXHA5CNFSM4GMKKA42YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5BU7TA#issuecomment-524505036,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAKMJPXD4COG3QXBXVUMB7TQGCEXHANCNFSM4GMKKA4Q
.

[d4rken]

I'd also be interested in your changes, maybe a small gist? A snippet ☺️

[Wasabi375]

My fork is here: https://github.com/Wasabi375/moshi and the relevant commit is https://github.com/Wasabi375/moshi/commit/57fea525ee39f6d71342aac1de5ce47afb51533e

Happy to get a PR once it's stable though!

Just let me know when. If I have the time I'm happy to make this into a full commit :wink:

[volo-droid]

@ZacSweers that's a little bit annoying for those of us who already use inline classes in data/domain layer and Moshi for their serialization. On the other hand they're still experimental, so your point is fair enough.

KotlinJsonAdapter also fails on deserializing inline classes: https://github.com/Kotlin/KEEP/issues/104#issuecomment-526573074.
Should I create a separate issue for the problem to keep it visible, or it's fine to just leave a reference here (since we were discussing KotlinJsonAdapter here anyway)?

[ZacSweers]

Yes let’s just track them both here, I’ll edit the title of the issue

[Wasabi375]

It seem's like #903 has fixed the issue with kotlin inline classes at least for the code-gen part of moshi.

[ZacSweers]

Taking a closer look at this, I don't think #923 quite enough because it treats inline classes as if they're any other object with just a single property. While it _technically_ works, this is counter-intuitive to how inline classes are intended to be used.

Consider the example in the added test in #923:

inline class InlineClass(val f: Int)

in the current form, the equivalent json is treated as

{
  "f": 6
}

when in Kotlin and at runtime, this should really semantically just mean 6. As a result, I think it's more appropriate for the JSON of this to just be 6.

Same with it being used as a property:

data class Foo(val value: InlineClass)

The JSON should be

{
  "value": 6
}

My proposal would be to create an adapter that just delegates the value read/write to an adapter for the value type. This would be consistent with how our existing typealias support works, and also how other languages with similar constructs work with serialization formats that have notions of type aliases such as thrift typedefs. Here's also an example of how Go has the same behavior in JSON with its aliases - https://play.golang.org/p/Y8R2XmNBegO

In generated code, it would look like this

class InlineClassJsonAdapter(moshi: Moshi) : JsonAdapter<InlineClass>() {

  private val valueDelegate: JsonAdapter<Int> = moshi.adapter(Int::class.java)

  override fun fromJson(reader: JsonReader): InlineClass {
    val f = valueDelegate.fromJson(reader)
        // TODO we'd format this message like the other Util.unexpectedNull() methods to reduce string pool churn
        ?: throw JsonDataException("Non-null inline class value for type 'InlineClass' was null at ${reader.path}")
    return InlineClass(f)
  }

  override fun toJson(writer: JsonWriter, value: InlineClass?) {
    if (value == null) {
      throw NullPointerException("value was null! Wrap in .nullSafe() to write nullable values.")
    }
    valueDelegate.toJson(writer, value.f)
  }

  override fun toString() = "GeneratedJsonAdapter(InlineClass)"
}

For reflect - we'd need to tweak it to do the same behavior. One good piece of news is that as of Kotlin 1.3.20, we _can_ differentiate these types from reflection in both Java and Kotlin, so Moshi's core type resolution can differentiate them.

Thoughts?

[ZacSweers]

@JakeWharton made a good point offline that inline classes could be expanded in the future to have multiple vals. They can also have computed properties now. In light of that, I think we can run with #923 as a regression test for now. Moshi still doesn't _formally_ support them since they're not stable yet, but I think the PR at least shows that the original issue here has been mitigated (likely by the aforementioned reflection support added in 1.3.20).

I'll probably write a separate little library that offers functionality like the above for those that want it using the generator property in @JsonClass.