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-kotlin does not work with proguard #345

Closed
grandstaish opened this issue Aug 26, 2017 · 15 comments · Fixed by #1191
Closed

Moshi-kotlin does not work with proguard #345

grandstaish opened this issue Aug 26, 2017 · 15 comments · Fixed by #1191

Comments

@grandstaish
Copy link

When ProGuard is enabled the call rawType.kotlin.primaryConstructor in KotlinJsonAdapterFactory returns null so it exits early.

I created a sample repository here which demonstrates the issue: https://github.com/grandstaish/moshi-kotlin-reflection

The only way I can get Moshi to work currently is to uncomment this line in proguard (-keep class kotlin.** { *; }). However, keeping literally everything in kotlin is going way overboard! Is there a better solution to this?

I have tried to figure this out for too long now and I'm going to have to pass this issue onto the experts, sorry! 😞

@swankjesse
Copy link
Member

Oooh, yes that’s tough. Not sure exactly what we’ll need to do.

@JakeWharton
Copy link
Member

JakeWharton commented Aug 28, 2017 via email

@grandstaish
Copy link
Author

I tried keeping the Metadata annotation, but it didn't help here unfortunately. My sample currently has the rules listed in the Moshi README.md, including keeping the methods of the Metadata annotation.

@grandstaish
Copy link
Author

grandstaish commented Aug 29, 2017

@JakeWharton was right all along 😥:

The required rules for kotlin are:

-dontwarn org.jetbrains.annotations.**
-keep class kotlin.Metadata { *; }

Also it might be worth mentioning that you need to keep the constructors and fields of your model classes too:

-keepclassmembers class my.models.package.** {
  <init>(...);
  <fields>;
}

Do you want me to create a PR to update the README.md?

@ktchernov
Copy link

Keeping Kotlin meta data is not enough. And keeping all constructors and fields of model classes is really annoying to do. Unless you move all your model classes into one package.

I'm trialling a workaround to add the Android Support annotation @Keep on model classes. @Keep has a rule of -keep @android.support.annotation.Keep class * {*;} in the defaultproguard-android.txt. May be enough to do the trick.

@grandstaish
Copy link
Author

grandstaish commented Sep 8, 2017

Since my last comment, I have had to add a rule to keep all of my model class names too. This is because the fully qualified classname is kept as a String in the @Metadata annotation, and sometimes kotlin-reflect will use it. For anyone interested, the rule looked like so:

-keepnames @kotlin.Metadata class my.models.package.**

I also changed another rule in the README to be more specific:

-keepclassmembers class * {
    @com.squareup.moshi.FromJson <methods>;
    @com.squareup.moshi.ToJson <methods>;
}

So far this has been working, but I'll comment in here if we find anything else.

Edit: @ktchernov you could also define your own annotation, and then define specific proguard rules to apply for classes with that annotation. That way you'll still get some small amount of obfuscation + you'll strip all the methods that aren't being used.

@LouisCAD
Copy link
Contributor

LouisCAD commented Oct 16, 2017

Hi,
The issue I often encounter with Square libraries is that the advertised proguard config doesn't account for users having proguard optimization turned on.

To be more specific, Retrofit and Moshi don't work OOB with the config below which has optimize.

proguardFiles getDefaultProguardFile('proguard-android-optimize.txt'), 'proguard-rules.pro'

Please Square developers, make sure your proguard config works correctly which optimizations, and update the advertised proguard configs accordingly.

@LouisCAD
Copy link
Contributor

LouisCAD commented Oct 16, 2017

On my side, I added the whole proguard config below, but I'm getting an NPE on release build on a when expression on an enum that should be converted from JSON.

#### OkHttp, Retrofit and Moshi
-dontwarn okhttp3.**
-dontwarn retrofit2.Platform$Java8
-dontwarn okio.**
-dontwarn javax.annotation.**
-keepclasseswithmembers class * {
    @retrofit2.http.* <methods>;
}
-keepclasseswithmembers class * {
    @com.squareup.moshi.* <methods>;
}
-keep @com.squareup.moshi.JsonQualifier interface *
-keepclassmembers class kotlin.Metadata {
    public <methods>;
}

-keepclassmembers class * {
    @com.squareup.moshi.FromJson <methods>;
    @com.squareup.moshi.ToJson <methods>;
}

-keepnames @kotlin.Metadata class io.myapp.mypackage.model.** #I used the real package name

Not sure what I should do to fix the issue.

FYI, here's what my adapters look like:

object MyApiJsonAdapter {
    @ToJson fun statusToJson(status: Status) = status.value
    @FromJson fun statusFromJson(statusValue: String) = Status.values().find {
        it.value == statusValue
    } ?: Status.UNKNOWN

    @ToJson fun modeToJson(mode: ThatEntity.Mode) = mode.value
    @FromJson fun modeFromJson(modeValue: String) = ThatEntity.Mode.values().find {
        it.value == modeValue
    } ?: ThatEntity.Mode.UNKNOWN
}

And the NPE is here:

when (theEntity.status) { // <- NPE thrown on status ordinal
    Status.IN_PROGRESS -> Unit
    ...

The Status enum looks like this:

enum class Status(val value: String) {
    IN_PROGRESS("inProgress"),
    COMPLETED("completed"),
    ENDING("ending"),
    CANCELED("cancel"),
    UNKNOWN("")
}

From what I understood, proguard inlined the enum to int, which is fine, but after proguarding, Moshi fails to provide a value to the status property, so returns null, leading the NPE:

java.lang.NullPointerException: Attempt to invoke virtual method 'int java.lang.Enum.ordinal()' on a null object reference

@LouisCAD
Copy link
Contributor

Update to my previous comments: This issue doesn't seem to be related to the enum, since it persisted after I got back to raw Strings.
Only keeping the models solved the issue. Here are my proguard rules:

#### OkHttp, Retrofit and Moshi
-dontwarn okhttp3.**
-dontwarn retrofit2.Platform$Java8
-dontwarn okio.**
-dontwarn javax.annotation.**
-keepclasseswithmembers class * {
    @retrofit2.http.* <methods>;
}
-keepclasseswithmembers class * {
    @com.squareup.moshi.* <methods>;
}
-keep @com.squareup.moshi.JsonQualifier interface *
-dontwarn org.jetbrains.annotations.**
-keep class kotlin.Metadata { *; }
-keepclassmembers class kotlin.Metadata {
    public <methods>;
}

-keepclassmembers class * {
    @com.squareup.moshi.FromJson <methods>;
    @com.squareup.moshi.ToJson <methods>;
}

-keepnames @kotlin.Metadata class com.myapp.packagename.model.**
-keep class com.myapp.packagnename.model.** { *; }
-keepclassmembers class com.myapp.packagename.model.** { *; }

These may be overly conservative, but at least, my app works properly with these. I'm waiting for better proguard rules, if any are possible. Hope it helps anyone encountering the same issue!

@LouisCAD
Copy link
Contributor

Update for my last comment: I still use Moshi, but replaced moshi-kotlin with kotshi, which plays fine with Proguard without special rules, dropping hundreds of kilobytes in the final apk.

@marcosalis
Copy link

Are there any workarounds for this? Using moshi-kotlin and ProGuard is a pretty common occurrence...
I keep getting a dreaded IllegalArgumentException: Unable to create converter for class (...) exception when ProGuard is enabled.

@LouisCAD
Copy link
Contributor

@marcosalis Just above your message, there's one, yes. Now, you can also use moshi kotlin codegen, see the README.

@marcosalis
Copy link

Thank you @LouisCAD. I actually still get the same exception when using moshi-kotlin with the latest versions and the above ProGuard rules. I'll try with kotshi to (hopefully) sort it out.
I wonder if it's an issue of the converter-moshi for Retrofit rather than the Kotlin code generator.

@marcosalis
Copy link

Update: I had to leave moshi-kotlin and reflection aside entirely, and use the new Kotlin codegen processor to get deserialization working with ProGuard.

@swankjesse swankjesse added this to the 1.7 milestone Sep 5, 2018
@swankjesse swankjesse modified the milestones: 1.7, 1.8 Oct 8, 2018
@swankjesse swankjesse modified the milestones: 1.8, Backlog Nov 6, 2018
toastkidjp added a commit to toastkidjp/Yobidashi_kt that referenced this issue Nov 9, 2019
julioromano added a commit to julioromano/push-notifications-android that referenced this issue Jan 27, 2020
pusher#101 enables moshi-kotlin reflection adapter which crashes if the kotlin.Metadata annotation is removed by proguard.

More background about this at: square/moshi#345


Stacktrace of the crash:
```
Caused by: java.lang.IllegalArgumentException: Cannot serialize abstract class com.pusher.pushnotifications.internal.ServerSyncJob
        at k.f.a.e0.a.b.a(:33)
        at k.f.a.a0.a(:5)
        at k.f.a.a0.a()
        at k.f.a.a0.a()
        at com.pusher.pushnotifications.internal.ServerSyncHandler$Companion.obtain$pushnotifications_release(:5)
        at com.pusher.pushnotifications.PushNotificationsInstance$serverSyncHandler$1.invoke()
        at com.pusher.pushnotifications.PushNotificationsInstance$serverSyncHandler$1.invoke()
        at com.pusher.pushnotifications.PushNotificationsInstance.<init>()
        at com.pusher.pushnotifications.PushNotifications.start()
        at e.a.a.b.w.c(:1)
        at ai.memory.dewo.mainactivity.MainActivity.onCreate(:9)
        at android.app.Activity.performCreate(Activity.java:6251)
        at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1107)
        at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2369)
        at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2476) 
        at android.app.ActivityThread.-wrap11(ActivityThread.java) 
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1344) 
        at android.os.Handler.dispatchMessage(Handler.java:102) 
        at android.os.Looper.loop(Looper.java:148) 
        at android.app.ActivityThread.main(ActivityThread.java:5417) 
        at java.lang.reflect.Method.invoke(Native Method) 
        at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726) 
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616) 
```
julioromano added a commit to julioromano/push-notifications-android that referenced this issue Jan 27, 2020
pusher#101 enables moshi-kotlin reflection adapter which crashes if the kotlin.Metadata annotation is removed by proguard.

More background about this at: square/moshi#345


Stacktrace of the crash:
```
Caused by: java.lang.IllegalArgumentException: Cannot serialize abstract class com.pusher.pushnotifications.internal.ServerSyncJob
        at k.f.a.e0.a.b.a(:33)
        at k.f.a.a0.a(:5)
        at k.f.a.a0.a()
        at k.f.a.a0.a()
        at com.pusher.pushnotifications.internal.ServerSyncHandler$Companion.obtain$pushnotifications_release(:5)
        at com.pusher.pushnotifications.PushNotificationsInstance$serverSyncHandler$1.invoke()
        at com.pusher.pushnotifications.PushNotificationsInstance$serverSyncHandler$1.invoke()
        at com.pusher.pushnotifications.PushNotificationsInstance.<init>()
        at com.pusher.pushnotifications.PushNotifications.start()
        at e.a.a.b.w.c(:1)
        at ai.memory.dewo.mainactivity.MainActivity.onCreate(:9)
        at android.app.Activity.performCreate(Activity.java:6251)
        at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1107)
        at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2369)
        at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2476) 
        at android.app.ActivityThread.-wrap11(ActivityThread.java) 
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1344) 
        at android.os.Handler.dispatchMessage(Handler.java:102) 
        at android.os.Looper.loop(Looper.java:148) 
        at android.app.ActivityThread.main(ActivityThread.java:5417) 
        at java.lang.reflect.Method.invoke(Native Method) 
        at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726) 
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616) 
```
Iljo added a commit to Iljo/moshi that referenced this issue Mar 27, 2020
In order to get moshi-kotlin working with reflection, while using R8, this line needs to be added. 
Maybe this line seams obvious, but it could save some hours to someone.

Solution is actually based on this answer:
square#345 (comment)

And it may solve this issue:
square#1095
@ZacSweers
Copy link
Collaborator

There are two factors for proguard rules.

1 - rules for kotlin-reflect. These have been historically finicky, but kotlin 1.4 will finally ship with minimal rules embedded directly in the kotlin-reflect artifact. These ensure that kotlin-reflect's machinery works, and should be the same for all users (hence them being bundled in the kotlin-reflect jar now). But this alone is not enough!

2 - rules for reflective serialization. This is the same problem that has always existed with reflective serialization and you have to keep any classes or fields/properties that you want to reflectively serialize. There are unique per application because they're your models. Common patterns for this include keeping a common models package with wildcards or denoting them with a marker annotation and keeping anything annotated with it.

For those that have consistent reproduction cases, try with Kotlin 1.4 (specifically with kotlin-reflect 1.4 or copy the rules from here) and ensure your proguard rules separately have rules for your reflectively serialized models.

If you still have issues with both, report back with a minimally reproducible sample project link. My hope though is that this is resolved with the combination of first-party rules and ensuring your own project's rules are correct.

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

Successfully merging a pull request may close this issue.

8 participants