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

Moshi failOnUnknown() isn't working as expected #1

Open
ColtonIdle opened this issue Sep 6, 2018 · 11 comments
Open

Moshi failOnUnknown() isn't working as expected #1

ColtonIdle opened this issue Sep 6, 2018 · 11 comments

Comments

@ColtonIdle
Copy link
Owner

I'm new to using Moshi and Kotlin. I was excited to see that you have the ability to fail when you hit unexpected json.

The thing is, it didn't seem to work for me in my real project, so I created this sandbox project.

I want to fail in two scenarios (especially during development). Arguably, I may want to disable this or be more lenient in production.

The two scenarios I want to fail are as follows:

  1. If I'm expecting something to be there in my model (that came from the network), but it isn't there.

  2. When the key is in my JSON but absent in your model.

In this project I'm using a freely available API call: https://jsonplaceholder.typicode.com/todos/1

It returns

{
  "userId": 1,
  "id": 1,
  "title": "delectus aut autem",
  "completed": false
}

If my model is setup as

data class Todos(
        val userId: Int = 0,
        val id: Int = 0,
        val title: String = "",
        val completed: Boolean = false
)

then everything works fine. If I remove the 'd' from userId then I fail with an error of "Cannot skip unexpected name..." This satisfies scenario 1 that I want to fail in. The message to me doesn't make sense though. Something like "Cannot skip expected name" makes more sense for this scenario.

To satisfy scenario 2, I try inserted a value that I assume to be necessary.

data class Todos(
        val userId: Int = 0,
        val id: Int = 0,
        val title: String = "",
        val completed: Boolean = false,
        val assumeINeedThis: Boolean = false
)

This doesn't cause any issues. Additionally if I remove the default value, then this still doesn't cause any issues.

data class Todos(
        val userId: Int = 0,
        val id: Int = 0,
        val title: String = "",
        val completed: Boolean = false,
        val assumeINeedThis: Boolean
)

Does anyone have any thoughts on how to trigger scenario 2?
Follow up: Is there any clever way to always have default values and turn off failOnUnknown for production build variants?

@NightlyNexus
Copy link

Scenario 1 is failOnUnknown working. "Cannot skip unexpected name..." means that the adapter/client was not expecting that name and will fail instead of skipping it.

Scenario 2 requires adding Kotlin support. See https://github.com/square/moshi#kotlin to get it added. After you add the reflective or codegen support for Kotlin, the non-optional parameter will fail as expected.

  1. using failOnUnknown only on production variants. I use Dagger 2. I would provide a Converter.Factory in a module and switch out the module in my built flavor (with Gradle). My production one would provide a regular MoshiConverterFactory, and my other flavors would provide a MoshiConverterFactory with the failOnUnknown flag set.

@ColtonIdle
Copy link
Owner Author

@NightlyNexus thank you for your time.

Scenario 1 is failOnUnknown working. "Cannot skip unexpected name..." means that the adapter/client was not expecting that name and will fail instead of skipping it.

Right. It's working here. It's not working in my other project, so it must be something in the other project. Proof that it works as intended here though, so that's good.

Scenario 2 requires adding Kotlin support. See https://github.com/square/moshi#kotlin to get it added. After you add the reflective or codegen support for Kotlin, the non-optional parameter will fail as expected.

Hm. This project has Kotlin support with codegen. It's not failing. Let me check the code to make sure I'm not going crazy, but I'm pretty sure I did this.

Re: 3
Thanks. Makes sense to me.

@ColtonIdle
Copy link
Owner Author

@NightlyNexus

As I thought, https://github.com/ColtonIdle/MoshiSandbox/blob/master/app/build.gradle contains moshi and the moshi kotlin codegen dependency. Am I missing something? Scenario 2 should fail, but it still parses the json response successfully in this repo.

@NightlyNexus
Copy link

To use the Kotlin codegen, your Todos model has to have the annotation @JsonClass(generateAdapter = true).

@ColtonIdle
Copy link
Owner Author

Wait what? How was it working before if I wasn't using the reflection or codegen dependencies?

@NightlyNexus
Copy link

It was using the Java reflection adapter. The backing fields can still be set from there, but it's not officially supported for Kotlin usage since it won't know anything about nullability, default values, etc.

@ColtonIdle
Copy link
Owner Author

ColtonIdle commented Sep 9, 2018

It was using the Java reflection adapter

Excuse my lack of knowledge. What's that? Is that something I set somewhere? Or is that just something Moshi does as a "fallback" of sorts?

The backing fields can still be set from there, but it's not officially supported for Kotlin usage since it won't know anything about nullability, default values, etc.

In order to test whether I had moshi working and I was reaping the benefits of Kotlin + moshi (mostly default values was the biggest thing missing from gson), I could have sworn I set a few things to default values to see if they would work... and they did unlike gson. I'm going to try a few things out again and see if anything works unexpectedly.

Additionally, I'd like to note that I read the docs for Moshi and see that it said "Enable it by annotating each class that you want to encode as JSON:"

I didn't/still don't think it really states that you need @JsonClass(generateAdapter = true) for "decoding" json to a kotlin class. I thought the adapter was only if you wanted to take a kotlin class and encode to JSON. Can you let me know if I'm misunderstanding that section of the docs?

@NightlyNexus
Copy link

By default, Moshi uses Java reflection for type adapters.

"Enable it by annotating each class that you want to encode as JSON:"
it means encoding and decoding both.

@ColtonIdle
Copy link
Owner Author

@NightlyNexus forgot to say thanks for working with me through this. All of this really helped. Thank you.

@ColtonIdle
Copy link
Owner Author

@NightlyNexus one last question, a bit off topic, but somewhat related.

My backend serves me a lot of similar responses but none of them are truly identical. (This is the reason for my team to be migrating to Kotlin + Moshi. So we can catch things that are off, and so that we can create 1:1 mappings between Response objects and calls in Retrofit. This should give us a bit more determinism).

To combat this, I map every call from retrofit with it's own unique XYZResponse kotlin data class.

There are times where the models inside the responses are similar and I want to make sure I don't misuse one of them or import a class that I shouldn't. Do you recommend doing anything if I have:

XResponse.kt

data class XResponse(
        val id: Long = 0,
        val typeName: String = "",
        val image: Image = Image()
) {
    data class Image(
            val id: Int = 0,
            val filename: String = ""
    )
}

YResponse.kt

data class YResponse(
        val name: Long = 0,
        val thing: String = "",
        val imageThing: Image = Image()
) {
    data class Image(
            val url: String = ""
    )
}

If I go into one of my other classes and start typing Image, it will give me the option to import one of the two. In reality, I don't every want to use any of those Images. I don't know if I'm nitpicking, or if there's a way to close up the use all of the nested elements in a Response as I'm going to see a ton of duplication of class names, even though they are all slightly different.

@NightlyNexus
Copy link

seems fine.

or, you could perhaps make an type like

data class XResponse(
        val id: Long = 0,
        val typeName: String = "",
        val imageId: Int,
        val imageFileName: String
)

and have a custom JsonAdapter to map the JSON to this type.

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

2 participants