-
Notifications
You must be signed in to change notification settings - Fork 4.4k
Introduce dedicated error handling for final-field mutation during deserialization (JEP 500) #2993
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
base: main
Are you sure you want to change the base?
Changes from all commits
1b928c4
9969bf5
63d8bc3
e337dd0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -206,6 +206,43 @@ public static RuntimeException createExceptionForUnexpectedIllegalAccess( | |||||||||
| exception); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Creates a {@link JsonIOException} indicating that Gson was unable to set a {@code final} | ||||||||||
| * instance field via reflection during deserialization. | ||||||||||
| * | ||||||||||
| * <p>This helper is used when {@link Field#set(Object, Object)} throws an {@link | ||||||||||
| * IllegalAccessException} for a {@code final} field. The returned exception message aims to be | ||||||||||
| * actionable by explaining that Gson cannot mutate final instance fields and suggesting | ||||||||||
| * alternatives such as registering a custom {@code TypeAdapter} or {@code InstanceCreator}, or | ||||||||||
| * making the field non-final. | ||||||||||
|
Comment on lines
+216
to
+217
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
| * | ||||||||||
| * <p>On newer Java runtimes, reflective mutation of {@code final} fields may be restricted (see | ||||||||||
| * JEP 500). Depending on JVM configuration, attempts to mutate final fields reflectively can | ||||||||||
| * cause {@link IllegalAccessException}s. | ||||||||||
| * | ||||||||||
| * @param field The final field which Gson attempted to assign | ||||||||||
| * @param exception The {@link IllegalAccessException} thrown by {@link Field#set(Object, Object)} | ||||||||||
| * @return A {@link JsonIOException} wrapping the original {@code exception} | ||||||||||
| */ | ||||||||||
| public static JsonIOException createExceptionForFinalFieldMutation( | ||||||||||
| Field field, IllegalAccessException exception) { | ||||||||||
| String fieldDescription = getAccessibleObjectDescription(field, false); | ||||||||||
| return new JsonIOException( | ||||||||||
| "Cannot set value of final " | ||||||||||
| + fieldDescription | ||||||||||
| + ".\n" | ||||||||||
| + "Gson cannot modify final instance fields during deserialization. Register a" | ||||||||||
| + " TypeAdapter or InstanceCreator for the declaring type, or change the field to be" | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above, using |
||||||||||
| + " non-final.\n" | ||||||||||
| + "Recent Java runtimes increasingly restrict reflective final-field mutation (JEP 500," | ||||||||||
| + " \"Prepare to Make Final Mean Final\"). If you are running on JDK 26+ and need to" | ||||||||||
| + " allow it, configure the JVM accordingly (for example" | ||||||||||
| + " --enable-final-field-mutation=ALL-UNNAMED or your module, and" | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing word / char for "or your module"? Though maybe this should recommend |
||||||||||
| + " --illegal-final-field-mutation=allow/warn/debug/deny). See" | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this really recommend |
||||||||||
| + " https://openjdk.org/jeps/500", | ||||||||||
| exception); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| private static RuntimeException createExceptionForRecordReflectionException( | ||||||||||
| ReflectiveOperationException exception) { | ||||||||||
| throw new RuntimeException( | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| /* | ||
| * Copyright (C) 2026 The Gson Authors | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package com.google.gson.internal.bind; | ||
|
|
||
| import static com.google.common.truth.Truth.assertThat; | ||
| import static org.junit.Assert.assertSame; | ||
|
|
||
| import com.google.gson.JsonIOException; | ||
| import com.google.gson.internal.reflect.ReflectionHelper; | ||
| import java.lang.reflect.Field; | ||
| import org.junit.Test; | ||
|
|
||
| /** Tests for the helper used by {@link ReflectiveTypeAdapterFactory} for final-field mutation. */ | ||
| public final class ReflectiveTypeAdapterFactoryFinalFieldTest { | ||
|
|
||
| private static final class ClassWithFinalField { | ||
| @SuppressWarnings("unused") | ||
| final String finalField; | ||
|
|
||
| ClassWithFinalField(String finalField) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Error Prone seems to complain about this constructor being unused as well (though it is needed to initialize the final field). So maybe add Side note: Error Prone requires JDK >= 21 and is skipped for older JDKs. In case you are using JDK < 21 that would explain why you did not see this warning locally. |
||
| this.finalField = finalField; | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void createExceptionForFinalFieldMutation_includesHelpfulMessageAndJepLink() | ||
| throws NoSuchFieldException { | ||
| Field finalField = ClassWithFinalField.class.getDeclaredField("finalField"); | ||
| IllegalAccessException cause = new IllegalAccessException("test"); | ||
|
|
||
| JsonIOException exception = | ||
| ReflectionHelper.createExceptionForFinalFieldMutation(finalField, cause); | ||
|
|
||
| assertThat(exception).hasMessageThat().contains("Cannot set value of final"); | ||
| assertThat(exception).hasMessageThat().contains("https://openjdk.org/jeps/500"); | ||
| assertSame(cause, exception.getCause()); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
Since this catches the
IllegalAccessExceptionnow and because that exception being a checked exception, it can probably be removed from thethrowsof the enclosing method and the callers, simplifying their code.