-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Make EnumTypeAdapter friendly with obfuscation #1495
Conversation
Test case? |
I tried to integrate proguard obfuscation on testcase like this. And intent of this mvn rule are this order.
But order of plugin execution does not work as intended.
Can I get any idea? |
You don't need to integrate ProGuard. Just specify a different name and
ensure it's honored in the JSON output.
…On Fri, Mar 22, 2019 at 3:37 PM YOUNG HO CHA (aka ganachoco) < ***@***.***> wrote:
I tried to integrate proguard obfuscation on testcase like this.
***@***.***
<ganadist@251ec47>
And intent of this mvn rule are this order.
- copy-resources (pre-obfuscate-class)
- proguard (default)
- copy-resources (post-obfuscate-class)
But order of plugin execution does not work as intended.
- copy-resources (pre-obfuscate-class)
- copy-resources (post-obfuscate-class)
- proguard (default)
Can I get any idea?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1495 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAEEEabvLIMEqbJOJAtqlKXYhGQvUzj4ks5vZTEVgaJpZM4cDGDK>
.
|
Gson has already com.google.gson.functional.testEnumCaseMapping() testcase. |
I updated first conversation comment of this PR. because it could be misleading. |
I ran into this problem as well. This is a good fix. Let's add a testcase. |
To verify this change, enum class in testcase must be obfuscated by proguard. But it still has a problem to integrate proguard. :( |
Proguard integration on Maven testcase is pretty dirty, but.
|
throw new AssertionError(e); | ||
} catch (NullPointerException e) { | ||
throw new AssertionError(e); | ||
} catch (ExceptionInInitializerError e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why catch this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but only because the method can throw the error does not mean that it should be caught here. I assume if that error actually occurred it could also happen in the non-Proguard specific code and there it is not caught either. So in my opinion it should not be caught here.
(Though note that I am not a maintainer of this project)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed at eba1415
} catch (NoSuchFieldException e) { | ||
} catch (IllegalAccessException e) { | ||
throw new AssertionError(e); | ||
} catch (NullPointerException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why catch this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NullPointerException
- if the specified object is null and the field is an instance field.
But the enum constants are static
fields so this should not be possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed at eba1415
if (!field.isEnumConstant()) { | ||
continue; | ||
} | ||
field.setAccessible(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why make the field accessible? Enum element fields are public so making them accessible should not be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this line, unittests failed with following error.
Failed tests: testCollectionOfEnumsDeserialization(com.google.gson.functional.EnumTest): AssertionError (GSON 2.8.6-SNAPSHOT): java.lang.IllegalAccessException: class com.google.gson.internal.bind.TypeAdapters$EnumTypeAdapter cannot access a member of class com.google.gson.functional.EnumTest$MyEnum with modifiers "public static final"
testTopLevelEnumDeserialization(com.google.gson.functional.EnumTest): AssertionError (GSON 2.8.6-SNAPSHOT): java.lang.IllegalAccessException: class com.google.gson.internal.bind.TypeAdapters$EnumTypeAdapter cannot access a member of class com.google.gson.functional.EnumTest$MyEnum with modifiers "public static final"
testClassWithEnumFieldSerialization(com.google.gson.functional.EnumTest): java.lang.IllegalAccessException: class com.google.gson.internal.bind.TypeAdapters$EnumTypeAdapter cannot access a member of class com.google.gson.functional.EnumTest$MyEnum with modifiers "public static final"
testClassWithEnumFieldDeserialization(com.google.gson.functional.EnumTest): AssertionError (GSON 2.8.6-SNAPSHOT): java.lang.IllegalAccessException: class com.google.gson.internal.bind.TypeAdapters$EnumTypeAdapter cannot access a member of class com.google.gson.functional.EnumTest$MyEnum with modifiers "public static final"
testCollectionOfEnumsSerialization(com.google.gson.functional.EnumTest): AssertionError (GSON 2.8.6-SNAPSHOT): java.lang.IllegalAccessException: class com.google.gson.internal.bind.TypeAdapters$EnumTypeAdapter cannot access a member of class com.google.gson.functional.EnumTest$MyEnum with modifiers "public static final"
testTopLevelEnumSerialization(com.google.gson.functional.EnumTest): java.lang.IllegalAccessException: class com.google.gson.internal.bind.TypeAdapters$EnumTypeAdapter cannot access a member of class com.google.gson.functional.EnumTest$MyEnum with modifiers "public static final"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enum constants are public, but the enum class itself isn't necessarily. If it isn't, then you'll get IllegalAccessException
without the field.setAccessible(true)
.
My concern here is that if there is a SecurityManager, the field.setAccessible(true)
call might fail, whereas the code using classOfT.getEnumConstants()
would have worked. So we'll have fixed one problem (getting enum constants in ProGuarded code) but introduced another (getting enum constants when there is a SecurityManager).
I think the code here should read something like this:
for (final Field field : classOfT.getDeclaredFields()) {
if (!field.isEnumConstant()) {
continue;
}
AccessController.doPrivileged(
new PrivilegedAction<Void>() {
@Override public Void run() {
field.setAccessible(true);
return null;
}
});
@SuppressWarnings("unchecked")
T constant = (T) field.get(null);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed at 59a8aed
continue; | ||
} | ||
field.setAccessible(true); | ||
T constant = (T)(field.get(null)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a @SuppressWarnings("unchecked")
for this variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed at ddee26e
|
||
package com.google.gson.functional; | ||
|
||
import java.lang.reflect.Field; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This import is unused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed at 7dbfce9
gson/testcases-proguard.conf
Outdated
@@ -0,0 +1,20 @@ | |||
# Options from Android Gradle Plugins |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to move this to gson/src/test/resources
? I think that might be better than having it directly in the gson
folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed at a25f96b
|
||
public void testEnumClassWithObfuscated() { | ||
for (Gender enumConstant: Gender.class.getEnumConstants()) { | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind fixing the indentation on lines 48–50 so it uses the same two-space indentation as the rest of the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed at d8c5fcf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting this change together, especially the tests! Sorry it has taken so long for us to get to it.
There are just a couple of changes that I think would make sense.
I'm going to test this change against Google's many internal uses of Gson and if it comes through clean we should be good to go.
When enum value was obfuscated by proguard, EnumTypeAdapter raise NoSuchFieldException even if apply SerializedName annotation. Because EnumTypeAdapter cannot find obfuscated enum constant field with its name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick update!
When enum value was obfuscated by proguard, EnumTypeAdapter raise NoSuchFieldException, even if apply SerializedName annotation.
Because EnumTypeAdapter cannot find obfuscated enum constant field with its name.
But without this workaround, there is no way to obfuscate enum name.