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

TypeAdapters don't apply to map keys #1722

Closed
Nycto opened this issue Jun 13, 2020 · 6 comments · May be fixed by #1961
Closed

TypeAdapters don't apply to map keys #1722

Nycto opened this issue Jun 13, 2020 · 6 comments · May be fixed by #1961

Comments

@Nycto
Copy link

Nycto commented Jun 13, 2020

When converting a Map to Json, TypeAdapters are not being used to generate the keys. Here is a minimal repro:

import com.google.gson.Gson;
import com.google.gson.GsonBuilder;
import com.google.gson.TypeAdapter;
import com.google.gson.stream.JsonReader;
import com.google.gson.stream.JsonWriter;

import java.io.IOException;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;

class Main {

    /** An example class that just wraps a String. */
    static class Thinger {
        public final String key;

        Thinger(String key) {
            this.key = key;
        }

        @Override
        public String toString() {
            return "Thinger(" + key + ")";
        }

        @Override
        public boolean equals(Object o) {
            if (this == o) return true;
            if (o == null || getClass() != o.getClass()) return false;
            Thinger that = (Thinger) o;
            return Objects.equals(key, that.key);
        }

        @Override
        public int hashCode() {
            return Objects.hash(key);
        }
    }

    /** A TypeAdapter that can encode and decode a Thinger */
    static class ThingerAdapter extends TypeAdapter<Thinger> {

        @Override
        public void write(JsonWriter out, Thinger value) throws IOException {
            out.value(value.key);
        }

        @Override
        public Thinger read(JsonReader in) throws IOException {
            return new Thinger(in.nextString());
        }
    }

    public static void main(String[] args) {

        // Sample data with custom keys and values
        final Map<Thinger, Thinger> map = new HashMap<>();
        map.put(new Thinger("Foo"), new Thinger("Bar"));
        map.put(new Thinger("Baz"), new Thinger( "Qux"));

        // Register the adapter we defined above
        final Gson gson = new GsonBuilder().registerTypeAdapter(Thinger.class, new ThingerAdapter()).create();

        // Print the encoded JSON
        System.out.println(gson.toJson(map));
    }
}

I expect this to print:

{"Foo":"Bar","Baz":"Qux"}

But it actually calls the toString method and prints:

{"Thinger(Foo)":"Bar","Thinger(Baz)":"Qux"}

I'm not familiar with your code base, but it looks like the problem is related to this line of code

@lyubomyr-shaydariv
Copy link
Contributor

I think they don't have to. If you'd change the type adapter so that it writes/reads other than just a single string (including nulls) how would this type adapter recognize the context and distinguish between map keys that must be strings and map values of arbitrary JSON structure?

@Nycto
Copy link
Author

Nycto commented Jun 13, 2020

Yup, I definitely agree that it’s problematic. But I think the spirit of this request is that some mechanism should exist for encoding keys. For example, a new function could be added to TypeAdapter that is just responsible for encoding a key name.

@Nycto
Copy link
Author

Nycto commented Jun 13, 2020

Actually, it looks like there is already a built-in mechanism for handling complex JsonElements as keys in that file:

private String keyToString(JsonElement keyElement) {
if (keyElement.isJsonPrimitive()) {
JsonPrimitive primitive = keyElement.getAsJsonPrimitive();
if (primitive.isNumber()) {
return String.valueOf(primitive.getAsNumber());
} else if (primitive.isBoolean()) {
return Boolean.toString(primitive.getAsBoolean());
} else if (primitive.isString()) {
return primitive.getAsString();
} else {
throw new AssertionError();
}
} else if (keyElement.isJsonNull()) {
return "null";
} else {
throw new AssertionError();
}
}

That appears to be for when you're encoding JsonElements, not encoding into a stream, though. However, I think it definitely defines a precedent.

@Nycto
Copy link
Author

Nycto commented Jun 13, 2020

Looks like this is my miss. Picking through the code more, this functionality is hidden behind a feature flag, enableComplexMapKeySerialization:

final Gson gson = new GsonBuilder()
    .enableComplexMapKeySerialization()
    .registerTypeAdapter(Thinger.class, new ThingerAdapter())
    .create();

Based on the implementation, I understand why this is hidden behind a flag -- it has to instantiate JsonElement instances for every key, which is slow. Would you be amenable to adding functionality to TypeAdapter? I'm thinking a function like this would solve the problem without changing the performance much:

public abstract class TypeAdapter<T> {
  // ... SNIP ...
  public String toKey(T value) throws IOException {
    return String.valueOf(value);
  }
  // ... SNIP ...
}

This would allow the default behaviour to continue, shouldn't break any existing consumers, would allow custom key serialization, maintains type safety, and should have reasonable performance.

Thoughts? Again, I'm new to your code base so I assume I'm missing some nuances here.

@lyubomyr-shaydariv
Copy link
Contributor

lyubomyr-shaydariv commented Jun 13, 2020

a new function could be added to TypeAdapter that is just responsible for encoding a key name.

You could just implement your custom map type adapter factory that might do the job for you or provide a transformed Map<String, ...> before its gets to the toJson method. The reason why the default map type adapter deals with strings only is that JSON object keys must be strings, that also keeps the type adapter API more simple requiring no provided context or adding special methods, whereas type adapters are designed for JSON values of arbitratry structure only. I'm afraid it's not a good idea to add a new JSON objects-only method to type adapters (I can see at least two pitfalls: sacrificing the type adapter class design for JSON objects; potentially complex reuse because of inheritance). By the way, type JsonReader and JsonWriter use nextName() and name() for JSON object keys, not nextString()/value(String).

Actually, it looks like there is already a built-in mechanism for handling complex JsonElements as keys in that file:

It does not produce/consume a JSON object: it represents a map using a two-dimensional JSON array (with n x 2 elements, literally [[k1, v1], [k2, v2], ...]), since arrays can hold arbitrary JSON elements, including null values. This is where keyToString comes into play.

I do believe that if you find that the Map-dedicated type adapter does not work for you, you might reimplement it not diving deep into the core design.

final class TransformedKeyMapTypeAdapterFactory
		implements TypeAdapterFactory {

	private static final Type[] rawKeyValueTypes = { Object.class, Object.class };

	private final IKeyConverter keyConverter;
	private final Function<? super Type, ? extends Map<?, ?>> createMap;

	private TransformedKeyMapTypeAdapterFactory(final IKeyConverter keyConverter, final Function<? super Type, ? extends Map<?, ?>> createMap) {
		this.keyConverter = keyConverter;
		this.createMap = createMap;
	}

	static TypeAdapterFactory create(final IKeyConverter keyConverter) {
		return new TransformedKeyMapTypeAdapterFactory(keyConverter, MapFactory::create);
	}

	static TypeAdapterFactory create(final IKeyConverter keyConverter, final Function<? super Type, ? extends Map<?, ?>> createMap) {
		return new TransformedKeyMapTypeAdapterFactory(keyConverter, createMap);
	}

	@Override
	@Nullable
	public <T> TypeAdapter<T> create(final Gson gson, final TypeToken<T> typeToken) {
		if ( !Map.class.isAssignableFrom(typeToken.getRawType()) ) {
			return null;
		}
		final Type mapType = typeToken.getType();
		// insufficient type check for simplicity
		final Type[] actualTypeArguments = mapType instanceof ParameterizedType
				? ((ParameterizedType) mapType).getActualTypeArguments()
				: rawKeyValueTypes;
		final Type keyType = actualTypeArguments[0];
		final Type valueType = actualTypeArguments[1];
		@SuppressWarnings("unchecked")
		final TypeAdapter<Object> valueTypeAdapter = gson.getAdapter((TypeToken<Object>) TypeToken.get(valueType));
		final TypeAdapter<? extends Map<?, ?>> mapTypeAdapter = TransformedKeyMapTypeAdapter.create(
				key -> keyConverter.toName(keyType, key),
				name -> keyConverter.fromName(keyType, name),
				() -> {
					@SuppressWarnings("unchecked")
					final Map<Object, Object> castMap = (Map<Object, Object>) createMap.apply(mapType);
					return castMap;
				},
				valueTypeAdapter
		);
		@SuppressWarnings("unchecked")
		final TypeAdapter<T> castMapTypeAdapter = (TypeAdapter<T>) mapTypeAdapter;
		return castMapTypeAdapter;
	}

	private static final class TransformedKeyMapTypeAdapter<K, V>
			extends TypeAdapter<Map<K, V>> {

		private final Function<? super K, String> toName;
		private final Function<? super String, ? extends K> fromName;
		private final Supplier<? extends Map<K, V>> createMap;
		private final TypeAdapter<V> valueTypeAdapter;

		private TransformedKeyMapTypeAdapter(final Function<? super K, String> toName, final Function<? super String, ? extends K> fromName,
				final Supplier<? extends Map<K, V>> createMap, final TypeAdapter<V> valueTypeAdapter) {
			this.toName = toName;
			this.fromName = fromName;
			this.createMap = createMap;
			this.valueTypeAdapter = valueTypeAdapter;
		}

		private static <K, V> TypeAdapter<Map<K, V>> create(final Function<? super K, String> toName, final Function<? super String, ? extends K> fromName,
				final Supplier<? extends Map<K, V>> createMap, final TypeAdapter<V> valueTypeAdapter) {
			return new TransformedKeyMapTypeAdapter<>(toName, fromName, createMap, valueTypeAdapter)
					.nullSafe();
		}

		@Override
		@SuppressWarnings("resource")
		public void write(final JsonWriter out, final Map<K, V> map)
				throws IOException {
			out.beginObject();
			for ( final Map.Entry<K, V> e : map.entrySet() ) {
				out.name(toName.apply(e.getKey()));
				valueTypeAdapter.write(out, e.getValue());
			}
			out.endObject();
		}

		@Override
		public Map<K, V> read(final JsonReader in)
				throws IOException {
			in.beginObject();
			final Map<K, V> map = createMap.get();
			while ( in.hasNext() ) {
				final String name = in.nextName();
				final K key = fromName.apply(name);
				final V value = valueTypeAdapter.read(in);
				final V replaced = map.put(key, value);
				if ( replaced != null ) {
					throw new JsonSyntaxException("duplicate key: " + key);
				}
			}
			in.endObject();
			return map;
		}

	}

}
interface IKeyConverter {

	@Nonnull
	<K> String toName(@Nonnull Type type, @Nullable K key);

	@Nullable
	<K> K fromName(@Nonnull Type type, @Nonnull String name);

}
final class KeyConverter
		implements IKeyConverter {

	private final BiFunction<? super Type, Object, String> toName;
	private final BiFunction<? super Type, ? super String, Object> fromName;

	private KeyConverter(final BiFunction<? super Type, Object, String> toName, final BiFunction<? super Type, ? super String, Object> fromName) {
		this.toName = toName;
		this.fromName = fromName;
	}

	public static IKeyConverter of(final BiFunction<? super Type, Object, String> toName,
			final BiFunction<? super Type, ? super String, Object> fromName) {
		return new KeyConverter(toName, fromName);
	}

	@Nonnull
	@Override
	public <K> String toName(@Nonnull final Type type, @Nullable final K key) {
		return toName.apply(type, key);
	}

	@Nullable
	@Override
	public <K> K fromName(@Nonnull final Type type, @Nonnull final String name) {
		@SuppressWarnings("unchecked")
		final K key = (K) fromName.apply(type, name);
		return key;
	}

}
final class KeyConverterBuilder {

	@AllArgsConstructor(access = AccessLevel.PRIVATE)
	private static final class Mapper<K> {

		private final Function<? super K, String> toName;
		private final Function<? super String, ? extends K> fromName;

	}

	private final Map<Class<?>, Mapper<?>> classMappers = new HashMap<>();
	private final Map<Class<?>, Mapper<?>> subClassMappers = new HashMap<>();

	private KeyConverterBuilder() {
	}

	static KeyConverterBuilder create() {
		return new KeyConverterBuilder();
	}

	<K> KeyConverterBuilder forClass(final Class<K> clazz, final Function<? super K, String> toName, final Function<? super String, ? extends K> fromName) {
		classMappers.put(clazz, new Mapper<>(toName, fromName));
		return this;
	}

	<K> KeyConverterBuilder forSubClass(final Class<K> clazz, final Function<? super K, String> toName, final Function<? super String, ? extends K> fromName) {
		subClassMappers.put(clazz, new Mapper<>(toName, fromName));
		return this;
	}

	IKeyConverter build() {
		return KeyConverter.of(
				(type, key) -> toName(type, key, classMappers, subClassMappers),
				(type, name) -> fromName(type, name, classMappers, subClassMappers)
		);
	}

	private static <K> String toName(final Type type, final K key, final Map<Class<?>, Mapper<?>> classMappers,
			final Map<Class<?>, Mapper<?>> subClassMappers) {
		@Nullable
		final Mapper<K> classMapper = resolveMapper(type, classMappers, subClassMappers);
		if ( classMapper == null ) {
			return String.valueOf(key);
		}
		return classMapper.toName.apply(key);
	}

	private static <K> K fromName(@Nonnull final Type type, @Nonnull final String name, final Map<Class<?>, Mapper<?>> classMappers,
			final Map<Class<?>, Mapper<?>> subClassMappers) {
		@Nullable
		final Mapper<K> classMapper = resolveMapper(type, classMappers, subClassMappers);
		if ( classMapper == null ) {
			throw new IllegalArgumentException("Cannot convert " + name + " to " + type);
		}
		return classMapper.fromName.apply(name);
	}

	@Nullable
	private static <K> Mapper<K> resolveMapper(final Type type, final Map<Class<?>, Mapper<?>> classMappers,
			final Map<Class<?>, Mapper<?>> subClassMappers) {
		// TODO cache
		if ( type instanceof Class ) {
			final Class<?> clazz = (Class<?>) type;
			@Nullable
			@SuppressWarnings("unchecked")
			final Mapper<K> classMapper = (Mapper<K>) classMappers.get(type);
			if ( classMapper != null ) {
				return classMapper;
			}
			for ( final Map.Entry<Class<?>, Mapper<?>> e : subClassMappers.entrySet() ) {
				if ( e.getKey().isAssignableFrom(clazz) ) {
					@SuppressWarnings("unchecked")
					final Mapper<K> subClassMapper = (Mapper<K>) e.getValue();
					return subClassMapper;
				}
			}
		}
		return null;
	}

}
final class MapFactory {

	private MapFactory() {
	}

	static <K, V> Map<K, V> create(final Type mapType) {
		if ( mapType instanceof Class ) {
			return create((Class<?>) mapType);
		}
		if ( mapType instanceof ParameterizedType ) {
			final ParameterizedType mapParameterizedType = (ParameterizedType) mapType;
			return create(mapParameterizedType.getRawType());
		}
		throw new IllegalArgumentException("Cannot resolve a default map instance for " + mapType);
	}

	private static <K, V> Map<K, V> create(final Class<?> mapClass) {
		if ( LinkedHashMap.class.isAssignableFrom(mapClass) ) {
			return new LinkedHashMap<>();
		}
		if ( HashMap.class.isAssignableFrom(mapClass) ) {
			return new HashMap<>();
		}
		if ( TreeMap.class.isAssignableFrom(mapClass) ) {
			return new TreeMap<>();
		}
		if ( Map.class.isAssignableFrom(mapClass) ) {
			return new LinkedHashMap<>();
		}
		throw new IllegalArgumentException("Cannot create a default map instance for " + mapClass);
	}

}

@Data
private static final class Name {

	private final String value;

}

private static final Type nameToNameMapType = new TypeToken<Map<Name, Name>>() {}.getType();

private static final Gson gson = new GsonBuilder()
		.disableHtmlEscaping()
		.registerTypeAdapterFactory(TransformedKeyMapTypeAdapterFactory.create(
				KeyConverterBuilder.create()
						.forClass(Name.class, name -> name.value, Name::new)
						.build()
		))
		.create();

public static void main(final String... args) {
	final Map<Name, Name> before = ImmutableMap.of(new Name("foo"), new Name("bar"));
	System.out.println(before);
	final String json = gson.toJson(before, nameToNameMapType);
	System.out.println(json);
	final Map<Name, Name> after = gson.fromJson(json, nameToNameMapType);
	System.out.println(after);
	System.out.println(before.equals(after));
}
{I1722.Name(value=foo)=I1722.Name(value=bar)}
{"foo":{"value":"bar"}}
{I1722.Name(value=foo)=I1722.Name(value=bar)}
true

@Nycto
Copy link
Author

Nycto commented Jun 23, 2020

Thanks for your help with this! I'm going to go ahead and resolve this issue. Seems like, at a philosophical level, it's believed this should be the responsibility of the library consumers and not the library itself.

Cheers!

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

Successfully merging a pull request may close this issue.

2 participants