Moshi: Provide a way to disable ClassJsonAdapter

[matejdro]

With Kotlin codegen, ClassJsonAdapter is not used nor desired since codegen creates better adapters.

However this adapter is still there and can become hidden source of bugs if developer accidentally forgets to add @JsonClass(generateAdapter = true) annotaion to the data class. Code will still seem to work at first glance, but some features like nullability checks will not work.

It would be nice if there was a way to disable that adapter, so all classes would require explicitly generated codegen adapters.

[ZacSweers]

If 100% of your classes should have generated adapters, you could write an lint rule. If you want something that fails harder, you could make an adapter that rejects any classes that aren't built-in (string, enums, primitives, collections, etc) and don't have the jsonclass annotation. You may be able to get some more fine grained information via limited kotlin class reflection, but not sure how much is available without pulling in kotlin-reflect.

Neither is ideal, but two ideas off the top of my head anyway :).

[matejdro]

100% of your classes should have generated adapters

Actually it is a mix of generated adapters (for data classes) and hand-written explicitly created adapters (for common object such as dates). That would make Lint rule kind of hard to implement, since it would also have to make sure that there is no explicit adapter installed into Moshi for that object. Same for extra validation adapter, not to mention that this adapter would cause additional overhead at runtime to check for object validity.

From checking Moshi code, this would not be very hard to do:

val moshi = Moshi.Builder()
    .setAutomaticAdapterGenerationEnabled(false)
    .build()

I can submit a pull request for that, but I just wanted to see if it is OK first so I don't do it for nothing.

[swankjesse]

I don’t think I want a PR for this. For this scenario I think you want a TypeAdapter.Factory that just confirms all of your types have the @JsonClass annotation on ’em, and throws if they don’t?

[matejdro]

No, I want to confirm that all types have either @JsonClass, they have manually added adapter via Builder.add() method or that they are supported natively (base types). I also do not want to create yet another adapter for that since it would perform unnecessary runtime checks.

My proposal does not add any runtime checks, it even removes more checks as ClassJsonAdapter would not be added to Moshi and then Moshi just errors out with no available adapter error if invalid class tries to get serialized.

[mopsalarm]

It is actually not easy to write an adapter factory that does the above job without replicating all the class type detection logic from the other inbuilt adapters.
For the time being I am going with this dirty hack: https://github.com/mopsalarm/Pr0/blob/master/api-kotlin/src/main/java/com/squareup/moshi/moshi-fixup.kt ¯_(ツ)_/¯

[matejdro]

This seems to be even better way to do it, without involving reflection. You need to call remove method before you create your first Moshi instance:

package com.squareup.moshi

import com.squareup.moshi.Moshi.BUILT_IN_FACTORIES

internal fun removeClassJsonAdapter() {
    BUILT_IN_FACTORIES.remove(ClassJsonAdapter.FACTORY)
}

it does remove it for all Moshi instances though, but you can just manually add it in the builder if you need it anywhere.

[swankjesse]

@matejdro be warned: code that calls package-private APIs is inherently fragile, and may break in any Moshi release. We do not guarantee any sort of API-compatibility for package-private APIs.

[matejdro]

Yeah I realize that, but since you refuse to let me provide good official solution, this is the best I can do to fix this issue.

It is not that dangreous though since it is not reflection-based. If something breaks in the future, my code will not compile, warning me that something is wrong. And since my app does not rely on this code to work (it is just safety net), I can remove it at any time without repercussions.

[swankjesse]

@matejdro sure. Just remember to recompile each time you get a new Moshi version. If your code is packaged as a library you may need to do this manually.

[matejdro]

Why are you so opposed to adding this officially to the Moshi though?

[swankjesse]

It locks in assumptions about how Moshi works internally and those assumptions are fragile. For example it assumes that class adapters are different from Map adapters in some permanent way.

I'm not forever opposed to this, but I don't think it's complexity I want to subject our users to at this time.

[matejdro]

I don't think my suggestion from above comment locks in any assumptions. Proposed syntax does not mention any class adapters, it only allows user to toggle (de)serialization of unknown classes. Maybe something like strict mode could be better name for this?