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

Java 14/15 records can not set final field #1794

Closed
IsmailMarmoush opened this issue Oct 8, 2020 · 25 comments · Fixed by #2201
Closed

Java 14/15 records can not set final field #1794

IsmailMarmoush opened this issue Oct 8, 2020 · 25 comments · Fixed by #2201

Comments

@IsmailMarmoush
Copy link

Hi, as the following error describes, java records (preview feature) deserialization isn't working in gson, Jackson is adding support in its very soon release 2.12, is there going to be same support/fix in gson ?

java.lang.AssertionError: AssertionError (GSON 2.8.6): java.lang.IllegalAccessException: Can not set final java.lang.String field JsonGsonRecordTest$Emp.name to java.lang.String

Here's a sample test

  record Emp(String name) {}

  @Test
  void deserializeEngineer() {
    Gson j = new GsonBuilder().setPrettyPrinting().create();
    var empJson = """
            {
              "name": "bob"
            }""";
    var empObj = j.fromJson(empJson, Emp.class);
  }
@Baizley
Copy link

Baizley commented Oct 10, 2020

The problem seems to exist only in Java 15 with preview enabled. Running your test under Java 14 with preview enabled and printing the empObj gives Emp[name=bob].

JDKs used,

  1. openjdk-14.0.2_linux-x64_bin
  2. openjdk-15_linux-x64_bin

This is due to changes in Java 15 that makes final fields in records notmodifiable via reflection. More information can be found here:

  1. (15) RFR: JDK-8247444: Trust final fields in records
  2. JDK-8247444 - Trust final fields in records

The relevant part on handling them going forward:

This change impacts 3rd-party frameworks including 3rd-party
serialization framework that rely on core reflection setAccessible or
sun.misc.Unsafe::allocateInstance and objectFieldOffset etc to
construct records but not using the canonical constructor.
These frameworks would need to be updated to construct records via its
canonical constructor as done by the Java serialization.

I see this change gives a good opportunity to engage the maintainers of
the serialization frameworks and work together to support new features
including records, inline classes and the new serialization mechanism
and which I think it is worth the investment.

A current workaround is to write a Deserializers that uses the records constructor instead of reflection.

@GandalfTheWhite80
Copy link

The following Deserializer works fine with JDK16 (workaround)

package de.sph.gsonrecordtest;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotSame;
import static org.junit.Assert.assertTrue;

import java.lang.reflect.Type;

import com.google.gson.Gson;
import com.google.gson.GsonBuilder;
import com.google.gson.JsonDeserializationContext;
import com.google.gson.JsonDeserializer;
import com.google.gson.JsonElement;
import com.google.gson.JsonObject;
import com.google.gson.JsonParseException;

import org.junit.jupiter.api.Test;


class RecordTest {
	
	public RecordTest()
	{
		//
	}

	@Test
	void testSerAndDeser() {
		// creating a new record
		final TestMe testMeRecord = new TestMe(42L, "Question");

		// Use a GsonBuilder
		final GsonBuilder gb = new GsonBuilder();
		// register our deserializer
		gb.registerTypeAdapter(TestMe.class, new TestMeDeserializer());

		// creating a gson object.
		final Gson gson = gb.create();

		// Test serialize
		final String str = gson.toJson(testMeRecord);
		assertEquals("{\"longValue\":42,\"stringValue\":\"Question\"}", str);

		// Test deserialize
		final TestMe dTestMeRecord = gson.fromJson(str, TestMe.class);
		assertNotSame(testMeRecord, dTestMeRecord);

		// test values
		assertEquals(testMeRecord.longValue(), dTestMeRecord.longValue());
		assertEquals(testMeRecord.stringValue(), dTestMeRecord.stringValue());

		// test generated record information
		assertEquals(testMeRecord.toString(), dTestMeRecord.toString());
		assertEquals(testMeRecord.hashCode(), dTestMeRecord.hashCode());
		assertTrue(testMeRecord.equals(dTestMeRecord));
	}

	private static record TestMe(long longValue, String stringValue) {
	};

	protected class TestMeDeserializer implements JsonDeserializer<TestMe> {
		/** Constructor */
		public TestMeDeserializer() {
			// nothing
		}

		@Override
		public TestMe deserialize(final JsonElement json, final Type typeOfT, final JsonDeserializationContext context)
				throws JsonParseException {
			final JsonObject jObject = json.getAsJsonObject();
			final long longValue = jObject.get("longValue").getAsLong();
			final String stringValue = jObject.get("stringValue").getAsString();
			return new TestMe(longValue, stringValue);
		}
	}
}

PS Start the test with "--add-opens YourTestModul/de.sph.gsonrecordtest=com.google.gson"

@thinkbigthings
Copy link

@GandalfTheWhite80 The code you supplied works, but I find it does NOT work when putting the record and deserializer in the method itself (co-located inside the method with the assertions). It does work if I move the deserializer inside the method as an anonymous class, but can't move the record inside the method as well. Logically it should be the same thing. Did you encounter that as well, and do you have ideas as to why that is?

@Marcono1234
Copy link
Collaborator

Marcono1234 commented Mar 21, 2021

but can't move the record inside the method as well

Likely because Gson does not support local classes, see also #1510. The rationale was probably that local classes could capture variables from the enclosing context, which could prevent correct serialization and deserialization. However, for Records this restriction by Gson should not be needed because they are always static (and therefore cannot capture anything), even as local classes, see JLS 16 §8.10.

Edit: Have created #1969 proposing to support static local classes.

@SR-Lut3t1um
Copy link

I'm also running in this issue. Can provide a reproducer if wanted.

@sceutre
Copy link

sceutre commented Apr 4, 2021

Another workaround would be to use a factory so you don't have to write deserializers for each record.

package com.smeethes.server;

import java.io.IOException;
import java.lang.reflect.Constructor;
import java.lang.reflect.InvocationTargetException;
import java.util.HashMap;

import com.google.gson.Gson;
import com.google.gson.GsonBuilder;
import com.google.gson.TypeAdapter;
import com.google.gson.TypeAdapterFactory;
import com.google.gson.reflect.TypeToken;
import com.google.gson.stream.JsonReader;
import com.google.gson.stream.JsonToken;
import com.google.gson.stream.JsonWriter;

public class RecordsWithGson {

   public static class RecordTypeAdapterFactory implements TypeAdapterFactory {

      @Override
      public <T> TypeAdapter<T> create(Gson gson, TypeToken<T> type) {
         @SuppressWarnings("unchecked")
         Class<T> clazz = (Class<T>) type.getRawType();
         if (!clazz.isRecord()) {
            return null;
         }
         TypeAdapter<T> delegate = gson.getDelegateAdapter(this, type);

         return new TypeAdapter<T>() {
            @Override
            public void write(JsonWriter out, T value) throws IOException {
               delegate.write(out, value);
            }

            @Override
            public T read(JsonReader reader) throws IOException {
               if (reader.peek() == JsonToken.NULL) {
                  reader.nextNull();
                  return null;
               } else {
                  var recordComponents = clazz.getRecordComponents();
                  var typeMap = new HashMap<String,TypeToken<?>>();
                  for (int i = 0; i < recordComponents.length; i++) {
                     typeMap.put(recordComponents[i].getName(), TypeToken.get(recordComponents[i].getGenericType()));
                  }
                  var argsMap = new HashMap<String,Object>();
                  reader.beginObject();
                  while (reader.hasNext()) {
                     String name = reader.nextName();
                     argsMap.put(name, gson.getAdapter(typeMap.get(name)).read(reader));
                  }
                  reader.endObject();

                  var argTypes = new Class<?>[recordComponents.length];
                  var args = new Object[recordComponents.length];
                  for (int i = 0; i < recordComponents.length; i++) {
                     argTypes[i] = recordComponents[i].getType();
                     args[i] = argsMap.get(recordComponents[i].getName());
                  }
                  Constructor<T> constructor;
                  try {
                     constructor = clazz.getDeclaredConstructor(argTypes);
                     constructor.setAccessible(true);
                     return constructor.newInstance(args);
                  } catch (NoSuchMethodException | InstantiationException | SecurityException | IllegalAccessException | IllegalArgumentException | InvocationTargetException e) {
                     throw new RuntimeException(e);
                  }
               }
            }
         };
      }
   }
   
   private record FooA(int a) {}
   private record FooB(int b) {}
   private record Bar(FooA fooA, FooB fooB, int bar) {}
   private class AClass {
      String data;
      Bar bar;
      public String toString() { return "AClass [data=" + data + ", bar=" + bar + "]"; }
   }
   
   public static void main(String[] args) {
      var gson = new GsonBuilder()
            .registerTypeAdapterFactory(new RecordTypeAdapterFactory())
            .create();
      var text = """
      { 
         "data": "some data",
         "bar": {
            "fooA": { "a": 1 },
            "fooB": { "b": 2 },
            "bar": 3
         }
      }
      """;
      
      AClass a = gson.fromJson(text, AClass.class);
      System.out.println(a);
   }
}

@gharia
Copy link

gharia commented Apr 14, 2021

Tried with jdk16 and the issue still exists. Is this planned to be supported in Gson in near future?

aroelke added a commit to aroelke/mtg-workstation that referenced this issue May 1, 2021
GSON doesn't like it, but there is a workaround at
google/gson#1794
@simon04
Copy link
Contributor

simon04 commented May 4, 2021

@sceutre, thank you for sharing the RecordTypeAdapterFactory. To correctly deal with List<Xxx>, I've adapted your code to com.google.gson.reflect.TypeToken:

diff --git a/RecordsWithGson.java b/RecordsWithGson.java
--- a/RecordsWithGson.java
+++ b/RecordsWithGson.java
@@ -40,9 +40,9 @@ public T read(JsonReader reader) throws IOException {
                         return null;
                     } else {
                         var recordComponents = clazz.getRecordComponents();
-                        var typeMap = new HashMap<String,Class<?>>();
+                        var typeMap = new HashMap<String,TypeToken<?>>();
                         for (int i = 0; i < recordComponents.length; i++) {
-                            typeMap.put(recordComponents[i].getName(), recordComponents[i].getType());
+                            typeMap.put(recordComponents[i].getName(), TypeToken.get(recordComponents[i].getGenericType()));
                         }
                         var argsMap = new HashMap<String,Object>();
                         reader.beginObject();

@sceutre
Copy link

sceutre commented May 9, 2021

@sceutre, thank you for sharing the RecordTypeAdapterFactory. To correctly deal with List<Xxx>, I've adapted your code to com.google.gson.reflect.TypeToken:

Very cool, thanks. I updated my comment with those changes.

@alexx-dvorkin
Copy link

alexx-dvorkin commented Jul 8, 2021

Sometimes the record we are deserializing into is does not exactly represent the serialized stream (some fields might be missing).
In this case, I suggest to replace in the RecordTypeAdapterFactory above, line:

 argsMap.put(name, gson.getAdapter(typeMap.get(name)).read(reader));

with:

if (typeMap.containsKey(name)) {
    argsMap.put(name, gson.getAdapter(typeMap.get(name)).read(reader));
} else {
    reader.skipValue();
}

@Sollace
Copy link

Sollace commented Jul 11, 2021

@sceutre Another suggestion to the above: In order to properly handle primitive types where the field is omitted from the input json I suggest changing this line:
args[i] = argsMap.get(recordComponents[i].getName());

To something more like this:

String name = recordComponents[i].getName();
Object value = argsMap.get(name);
TypeToken<?> type = typeMap.get(name);
if (value == null && (type != null && type.getRawType().isPrimitive())) {
    value = PRIMITIVE_DEFAULTS.get(type.getRawType());
}
args[i] = value;

And also with this map (sorry, I couldn't find any way to infer defaults):

    private static final Map<Class<?>, Object> PRIMITIVE_DEFAULTS = new HashMap<>();
    static {
        PRIMITIVE_DEFAULTS.put(byte.class, (byte)0);
        PRIMITIVE_DEFAULTS.put(int.class, 0);
        PRIMITIVE_DEFAULTS.put(long.class, 0L);
        PRIMITIVE_DEFAULTS.put(short.class, (short)0);
        PRIMITIVE_DEFAULTS.put(double.class, 0D);
        PRIMITIVE_DEFAULTS.put(float.class, 0F);
        PRIMITIVE_DEFAULTS.put(char.class, '\0');
        PRIMITIVE_DEFAULTS.put(boolean.class, false);
    }

@Marcono1234
Copy link
Collaborator

@Sollace, your map is missing short.

@Sollace
Copy link

Sollace commented Jul 13, 2021

@Marcono1234 Fixed! Thank you

@bristermitten
Copy link

bristermitten commented Jul 24, 2021

One issue I have noticed is that this workaround doesn't seem to work with field naming policies or the @SerializedName annotation. Making it work with naming policies doesn't seem a possibility at the moment without somehow transforming a RecordComponent into a Field.

I made some tweaks to the code to make sure it works with the SerializedName annotation, although as I mentioned, I don't think field naming policies will work at the moment.

It would be great to see some proper support for records in the future. If it's not a priority at the moment as the technology is new, I would be willing to PR support, and I'm sure others would too.

https://gist.github.com/knightzmc/cf26d9931d32c78c5d777cc719658639

(edit: converted to a gist)

@oujesky
Copy link

oujesky commented Sep 13, 2021

if (typeMap.containsKey(name)) {
    argsMap.put(name, gson.getAdapter(typeMap.get(name)).read(reader));
} else {
    gson.getAdapter(Object.class).read(reader);
}

@alexx-dvorkin I'm wondering, whether just calling reader.skipValue() wouldn't be enough when the field isn't mapped at all?

@alexx-dvorkin
Copy link

alexx-dvorkin commented Sep 14, 2021

@oujesky I replaced gson.getAdapter(Object.class).read(reader); with reader.skipValue() and it worked as well for the case of non declared fields. Indeed, just skipping is a more efficient solution. I replaced it inside the snippet I wrote above.

@oujesky
Copy link

oujesky commented Sep 15, 2021

One more thing, that the solution presented in this thread is missing, is the support for field-level @JsonAdapter annotation (to specify a type adapter responsible for that specific field of the record).

This one is a bit trickier, since the Gson code responsible for handling those is "hidden" inside ReflectiveTypeAdapterFactory and the components required for handling the type adapter instantiation based on the annotation (JsonAdapterAnnotationTypeAdapterFactory) is private inside Gson instance. Without core change, we're probably limited to either duplicating some of the code or a accessing it reflectively (which brings certain implications related to performance etc.).

The reflective access "workaround" (can we maybe call it even a "hack"?) can look somehow like this and in case there is the @JsonAdapter annotation present on a given record field, specified type adapter should be used instead the one coming from gson.getAdapter(typeToken):

    private <T> TypeAdapter<T> getTypeAdapterFromAnnotation(Gson context, TypeToken<T> fieldType, JsonAdapter annotation) {
        try {
            var constructorConstructorField = Gson.class.getDeclaredField("constructorConstructor");
            constructorConstructorField.setAccessible(true);
            var constructorConstructor = (ConstructorConstructor) constructorConstructorField.get(context);

            var jsonAdapterFactoryField = Gson.class.getDeclaredField("jsonAdapterFactory");
            jsonAdapterFactoryField.setAccessible(true);
            var jsonAdapterFactory = (JsonAdapterAnnotationTypeAdapterFactory) jsonAdapterFactoryField.get(context);

            var getTypeAdapterMethod = JsonAdapterAnnotationTypeAdapterFactory.class.getDeclaredMethod("getTypeAdapter", ConstructorConstructor.class, Gson.class, TypeToken.class, JsonAdapter.class);
            getTypeAdapterMethod.setAccessible(true);

            return (TypeAdapter<T>) getTypeAdapterMethod.invoke(jsonAdapterFactory, constructorConstructor, context, fieldType, annotation);
        } catch (NoSuchFieldException | IllegalAccessException | NoSuchMethodException | InvocationTargetException e) {
            throw new IllegalStateException("Unable to process @JsonAdapter annotation", e);
        }
    }

    private JsonAdapter getJsonAdapterAnnotation(RecordComponent recordComponent) {
        try {
            return recordComponent.getDeclaringRecord().getDeclaredField(recordComponent.getName()).getAnnotation(JsonAdapter.class);
        } catch (NoSuchFieldException e) {
            throw new IllegalStateException(String.format("Unable to find record field '%s'", recordComponent.getName()), e);
        }
    }

@Marcono1234
Copy link
Collaborator

Marcono1234 commented Sep 27, 2021

In case someone is interested, I have created the project https://github.com/Marcono1234/gson-record-type-adapter-factory which implements a Record class type adapter factory as standalone library. It includes the features which have been mentioned here in the comments: @SerializedName and @JsonAdapter support, and support for unknown JSON properties and missing values for Record components of primitive types (opt-in features).
Feedback is appreciated in case someone wants to try it out 🙂

(Note that I am not a maintainer of Gson, but I would also like to see built-in Gson Record support in the future. However, Gson is currently still built for Java 6, so the best choice for now is probably a separate library adding Record support.)

@eamonnmcmanus
Copy link
Member

Thanks @Marcono1234! I have been meaning to look into this for a while but haven't been able to find time. We might be able to add your code directly to mainline Gson by using reflection to access methods that were added after Java 6 (specifically the methods for reflecting on records). It's kind of tedious, but I've done this sort of thing before.

@Marcono1234
Copy link
Collaborator

We might be able to add your code directly to mainline Gson by using reflection to access methods that were added after Java 6 (specifically the methods for reflecting on records).

Maybe it would be safer to implement this in the form of a Multi-Release JAR; however Gson would then have to verify that the version specific classes are loaded correctly. Often when users build an "uber JAR" / JAR with dependencies they (or the build tools they are using) do not set Multi-Release: true in the MANIFEST.MF, causing the version specific classes to not be loaded.

Gson could implement Record class support also with less intrusive changes than in my project. I had to "reinvent" multiple aspects (naming strategy, type resolution) because Gson does not expose them (and also because Gson's behavior is not ideal in some cases).
Gson's ReflectiveTypeAdapterFactory already handles serialization by reading the component fields, the main issue is deserialization because it requires calling the canonical constructor. Similarly FieldNamingStrategy works already by being passed the component fields as well. @JsonAdapter and @SerializedName work too because the annotations on the Record components are propagated to the component fields (see JLS). The only issue might be user-defined annotations with only RECORD_COMPONENT as target because FieldNamingStrategy would not be able to access those (though maybe such situations would be rare because most likely a user would also specify FIELD as target to allow using the annotation on fields of non-Record classes).

The question is also whether Gson should prefer the public accessor method instead of getting the component value directly from the backing component field. The advantage of using the accessor method is that users using the module system for their project do not have to open their module to Gson (assuming the Record class is public). Though that might not be a very strong argument.

rybak added a commit to rybak/resoday that referenced this issue Apr 13, 2022
Class GuiSettings cannot be converted into a record at this time due to
lack of support for records in Gson library, which is used for
serialization and deserialization to/from JSON.
google/gson#1794
So mark it with @SuppressWarnings annotation to make the code green in
IntelliJ IDEA.
@Likqez
Copy link

Likqez commented Apr 22, 2022

Although gson is in maintenance mode, is this still being considered?
Missing the record feature is a hard no-go for some.

@yeikel
Copy link

yeikel commented Apr 29, 2022

If it helps anyone, this issue does not happen with Jackson

@crummy
Copy link

crummy commented Apr 30, 2022

For what it's worth, we added in the type adapter mentioned above (https://github.com/Marcono1234/gson-record-type-adapter-factory) and it works OK. That said we are thinking about moving off Gson (probably to Jackson?) long term.

eamonnmcmanus pushed a commit that referenced this issue Oct 11, 2022
* Support Java Records when present in JVM.

Fixes #1794

Added support in the ReflectionHelper to detect if a class is a record
on the JVM (via reflection), and if so, we will create a special
RecordAdapter to deserialize records, using the canoncial constructor.

The ReflectionTypeAdapterFactory had to be refactored a bit to support
this. The Adapter class inside the factory is now abstract, with
concrete implementations for normal field reflection and for Records.
The common code is in the Adapter, with each implementation
deserializing values into an intermediary object.

For the FieldReflectionAdapter, the intermediary is actually the final
result, and field access is used to write to fields as before. For the
RecordAdapter the intermediary is the Object[] to pass to the Record
constructor.

* Fixed comments from @Marcono1234

Also updated so that we now use the record accessor method to read out
values from a record, so that direct field access is not necessary.

Also added some tests, that should only execute on Java versions with
record support, and be ignored for other JVMs

* Fixed additional comments from @Marcono1234

* Made Adapter in ReflectiveTypeAdapterFactory public

Fix comment from @eamonnmcmanus
@simondemartini
Copy link

Happy to see this was merged, thank you! Do you all have any details when this is scheduled to be released in the next version?

@eamonnmcmanus
Copy link
Member

The just-released 2.10 includes these changes.

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.