Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature Request: A more flexible PolymorphicJsonAdapter #1273

Closed
KirkBushman opened this issue Nov 23, 2020 · 5 comments
Closed

Feature Request: A more flexible PolymorphicJsonAdapter #1273

KirkBushman opened this issue Nov 23, 2020 · 5 comments

Comments

@KirkBushman
Copy link

KirkBushman commented Nov 23, 2020

Hi,

I wanted to start a conversation about how to make Polymorphic Deserialization better in the future.
Sometimes you find yourself dealing with a very bad API that cannot be changed, cause you don't have access to the backend, and in those cases, I found the PolymorphicJsonAdapter approach to be challenging to use.

Let's look at this example:

interface Account { ... val type: String ... }
data class User( ... override val type: String ... ) : Account
data class SuspendedUser( ... override val type: String ... ): Account

representing:

// normal user 
{ ... "type":"logged_account" ... }
// suspended user 
{ ... "type":"suspended_account" ... }

Dealing with this would be very simple:

val moshi = Moshi.Builder()
    .add(
        PolymorphicJsonAdapterFactory.of(Account::class.java, "type")
            .withSubtype(User::class.java, "logged_account")
            .withSubtype(SuspendedUser::class.java, "suspended_account")
    )
    .build()

But now let's think for a moment about this situation:

interface Account
data class User( ... ) : Account
data class SuspendedUser( ... val isSuspended: Boolean ... ): Account
data class DeletedUser( ... val isDeleted: Boolean ... ): Account

representing:

// normal user 
{ ... }
// suspended user 
{ ... "is_suspended": true ... }
// deleted user 
{ ... "is_deleted": true ... }

This situation is much more problematic to handle, for two reasons:

  • PJA is assuming a unique label field for differentiating the classes.
  • PJA seems to work with string values only.

Following #1094 there is a way to control unknown labels, with fallbackJsonAdapter, but if you have a lot of these cases your code might end up messy. Also if you generate your adapters with code-gen, the classes take in a Moshi instance, so you would need one to build another? Very cumbersome.

Since I was dealing with such cases very often, I developed an alternative method:

class PolyJsonAdapterFactory<T>(

    private val baseType: Class<T>,
    private val possibleTypes: Array<Type>,
    private inline val selectType: (label: String, value: Any?) -> Type?
) : JsonAdapter.Factory {

    override fun create(type: Type, annotations: Set<Annotation?>, moshi: Moshi): JsonAdapter<Any>? {

        if (Types.getRawType(type) != baseType || annotations.isNotEmpty()) {
            return null
        }

        val map = HashMap<Type, JsonAdapter<Any?>>()
        possibleTypes.forEach {

            map[it] = moshi.adapter(it)
        }

        return PolyJsonAdapter(map, selectType).nullSafe()
    }

    internal class PolyJsonAdapter(

        private val adaptersMap: Map<Type, JsonAdapter<Any?>>,
        private inline val selectType: (label: String, value: Any?) -> Type?
    ) : JsonAdapter<Any>() {

        override fun fromJson(reader: JsonReader): Any? {
            val peeked = reader.peekJson()
            peeked.setFailOnUnknown(false)

            var type: Type?
            peeked.use {
                type = matchType(it)
            }

            if (type == null) {
                throw JsonDataException("No match found in JSON object!")
            }

            val adapter = adaptersMap[type]
                ?: throw IllegalStateException("Adapter not found!")

            return adapter.fromJson(reader)
        }

        override fun toJson(writer: JsonWriter, value: Any?) {
            val type = value!!.javaClass
            val adapter = adaptersMap[type]
                ?: throw IllegalStateException("Adapter not found!")

            writer.beginObject()
            val flattenToken = writer.beginFlatten()
            adapter.toJson(writer, value)
            writer.endFlatten(flattenToken)
            writer.endObject()
        }

        private fun matchType(reader: JsonReader): Type? {
            reader.beginObject()
            while (reader.hasNext()) {

                val label = reader.nextName()
                val value = reader.readJsonValue()

                val type = selectType(label, value)
                if (type != null) {
                    return type
                }
            }

            return null
        }
    }
}

That can be used like this:

val moshi = Moshi.Builder()
    .add(
        PolyJsonAdapterFactory(
            baseType = Account::class.java,
            possibleTypes = arrayOf(User::class.java, SuspendedUser::class.java, DeletedUser::class.java),
            selectType = { label, value ->

                when {
                    label == "has_verified_email" -> User::class.java // or any unique param for User::class
                    label == "is_suspended" && value == true -> SuspendedUser::class.java
                    label == "is_deleted" && value == true -> DeletedUser::class.java
                    else -> null
                }
             }
        )
    )
    .build()

This is not the best code ever written, not the prettiest, but it's working in almost any situation.
Can we build a more futurable adapter that maybe uses a Matcher<> class comparing labels and values, so we can build logic to identify what subtype we are dealing with, in a simpler way?

What do you guys think?
Do you have a better solution?

Thanks in advance

@JakeWharton
Copy link
Collaborator

I think a better API would be to read the entire object as a json value and then pass it to a lambda for each subtype.

PolymorphicJsonAdapter.builder(Account::class.java)
  .add(User::class.java, { it["label"] == "has_verified_email" })
  .add(SuspendedUser::class.java, { it["is_suspended"] == true })
  .add(DeletedUser::class.java, { it["is_deleted"] == true })
  .build()

That being said, is this a good use of polymorphism? What if you want to differentiate suspended users who have verified emails? The polymorphic support is for when the fields of a type are unrelated to each other and discerned by a discriminator. It is not for mapping state of an object which fundamental represents the same thing into some kind of hierarchy.

You're probably going to be on your own for this one...

@KirkBushman
Copy link
Author

Hi Jake,

While creating the class I wanted to stay close to the original, but if I had to read the whole object we could do it like this:

PolymorphicJsonAdapter.builder(Account::class.java)
  .add(SuspendedUser::class.java, { it["is_suspended"] == true })
  .add(DeletedUser::class.java, { it["is_deleted"] == true })
  .default(User::class.java)
  .build()

Much better.

I thought of this example on the spot since I cannot share company code, maybe I could have been clearer. The issue at the center are those situations where you have to discern between subclasses but:

  • There is no discriminator.
  • There is no envelope with a type.
  • The discriminator is not a single field with a single label but made up of a variety of fields. (like the booleans above)

If I had a say in the matter I would add a field with the type and use PJA as it is, but oftentimes you just have to deal with a bad API and you need a more complex logic to identify the subclass.

I think covering more situations could be beneficial, but then again if you say I'm on my own...

@MurtadhaS
Copy link

I don't think there is a need to map these states as different concrete models.

@KirkBushman
Copy link
Author

There is no concrete need, but there is a consideration to do about nullability.
You can have one model for all states, using nullable fields everywhere. In the above case you'll have to check for nullability in your code, even thought you know they are present the vast majority of times. (whenever you are not dealing with suspended or deleted accounts)

In my mind it's easier to have submodels to bind around. Others might have a different opinion.

@ZacSweers
Copy link
Collaborator

Nothing more to do here, if you need this kind of customization it's better to write your own.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants