From 8245f6fd3ab999e76312ffb5c06bf76f1cc25938 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Sat, 1 Nov 2014 10:20:50 -0700 Subject: [PATCH] JCLOUDS-750 Revert 5b6f1e929ef4b6438facc06df0f081ddef8c9cf6 in favor of tighter contract on @SerializedNames. --- .../org/jclouds/json/SerializedNames.java | 27 ++++- .../org/jclouds/json/config/GsonModule.java | 15 +-- .../json/internal/NamingStrategies.java | 34 ++++-- .../java/org/jclouds/reflect/Reflection2.java | 2 +- .../test/java/org/jclouds/json/JsonTest.java | 101 +++++++++--------- ...orAndReflectiveTypeAdapterFactoryTest.java | 3 +- .../json/internal/NamingStrategiesTest.java | 11 +- 7 files changed, 111 insertions(+), 82 deletions(-) diff --git a/core/src/main/java/org/jclouds/json/SerializedNames.java b/core/src/main/java/org/jclouds/json/SerializedNames.java index 35a32ceee1..b6dbb8fbe5 100644 --- a/core/src/main/java/org/jclouds/json/SerializedNames.java +++ b/core/src/main/java/org/jclouds/json/SerializedNames.java @@ -16,7 +16,6 @@ */ package org.jclouds.json; -import static java.lang.annotation.ElementType.CONSTRUCTOR; import static java.lang.annotation.ElementType.METHOD; import static java.lang.annotation.RetentionPolicy.RUNTIME; @@ -25,9 +24,31 @@ import java.lang.annotation.Target; import com.google.common.annotations.Beta; -/** An ordered list of json field names that correspond to factory method or constructor parameters. */ -@Beta @Target({ CONSTRUCTOR, METHOD }) @Retention(RUNTIME) +/** + * This annotation identifies the canonical factory method on an {@code AutoValue} type used for json. + * It also dictates the serialized naming convention of the fields. This is required as there's currently + * no way to add annotations to the fields generated by {@code AutoValue}. + * + *

Example: + * 

{@code @AutoValue class Resource {
+ *   abstract String id();
+ *   @Nullable abstract Map metadata();
+ *
+ *   @AutoValueSerializedNames({ "Id", "Metadata" }) // Note case format is controlled here!
+ *   static Resource create(String id, Map metadata) {
+ *     return new AutoValue_Resource(id, metadata);
+ *   }
+ * }}
+ */ +@Beta @Target(METHOD) @Retention(RUNTIME) public @interface SerializedNames { + /** + * Ordered values that dictate the naming convention for serialization. + * + *

Note

+ * The order of these names must exactly match the factory method parameters and also match the order of the + * auto-value constructor parameters. + */ String[] value(); } diff --git a/core/src/main/java/org/jclouds/json/config/GsonModule.java b/core/src/main/java/org/jclouds/json/config/GsonModule.java index 4c62be7df6..934541b16c 100644 --- a/core/src/main/java/org/jclouds/json/config/GsonModule.java +++ b/core/src/main/java/org/jclouds/json/config/GsonModule.java @@ -65,7 +65,6 @@ import com.google.common.collect.Sets; import com.google.common.primitives.Bytes; import com.google.gson.ExclusionStrategy; import com.google.gson.FieldAttributes; -import com.google.gson.FieldNamingPolicy; import com.google.gson.FieldNamingStrategy; import com.google.gson.Gson; import com.google.gson.GsonBuilder; @@ -85,12 +84,6 @@ import com.google.inject.Provides; public class GsonModule extends AbstractModule { - /** Optionally override the fallback field naming strategy when name annotations aren't on fields. */ - private static class FallbackFieldNamingStrategy { - @Inject(optional = true) - FieldNamingStrategy fallback = FieldNamingPolicy.IDENTITY; - } - @SuppressWarnings("rawtypes") @Provides @Singleton @@ -101,11 +94,10 @@ public class GsonModule extends AbstractModule { MultimapTypeAdapterFactory multimap, IterableTypeAdapterFactory iterable, CollectionTypeAdapterFactory collection, ListTypeAdapterFactory list, ImmutableListTypeAdapterFactory immutableList, FluentIterableTypeAdapterFactory fluentIterable, - ImmutableMapTypeAdapterFactory immutableMap, DefaultExclusionStrategy exclusionStrategy, - FallbackFieldNamingStrategy fallbackFieldNamingStrategy) { + ImmutableMapTypeAdapterFactory immutableMap, DefaultExclusionStrategy exclusionStrategy) { FieldNamingStrategy serializationPolicy = new AnnotationOrNameFieldNamingStrategy(ImmutableSet.of( - new ExtractSerializedName(), new ExtractNamed()), fallbackFieldNamingStrategy.fallback); + new ExtractSerializedName(), new ExtractNamed())); GsonBuilder builder = new GsonBuilder().setFieldNamingStrategy(serializationPolicy) .setExclusionStrategies(exclusionStrategy); @@ -135,7 +127,8 @@ public class GsonModule extends AbstractModule { ImmutableSet.of(new ExtractNamed())); builder.registerTypeAdapterFactory(new DeserializationConstructorAndReflectiveTypeAdapterFactory( - new ConstructorConstructor(ImmutableMap.>of()), serializationPolicy, Excluder.DEFAULT, deserializationPolicy)); + new ConstructorConstructor(ImmutableMap.>of()), serializationPolicy, + Excluder.DEFAULT, deserializationPolicy)); // complicated (serializers/deserializers as they need context to operate) builder.registerTypeHierarchyAdapter(Enum.class, new EnumTypeAdapterThatReturnsFromValue()); diff --git a/core/src/main/java/org/jclouds/json/internal/NamingStrategies.java b/core/src/main/java/org/jclouds/json/internal/NamingStrategies.java index 5f4ed893fc..cf3c5aceea 100644 --- a/core/src/main/java/org/jclouds/json/internal/NamingStrategies.java +++ b/core/src/main/java/org/jclouds/json/internal/NamingStrategies.java @@ -18,6 +18,7 @@ package org.jclouds.json.internal; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.common.base.Preconditions.checkState; import static com.google.common.base.Predicates.in; import static com.google.common.collect.Iterables.transform; import static com.google.common.collect.Iterables.tryFind; @@ -26,6 +27,7 @@ import static org.jclouds.reflect.Reflection2.constructors; import java.beans.ConstructorProperties; import java.lang.annotation.Annotation; import java.lang.reflect.Field; +import java.lang.reflect.Modifier; import java.util.Arrays; import java.util.Collection; import java.util.Map; @@ -162,33 +164,47 @@ public class NamingStrategies { public static class AnnotationFieldNamingStrategy extends AnnotationBasedNamingStrategy implements FieldNamingStrategy { - private final FieldNamingStrategy fallback; - - public AnnotationFieldNamingStrategy(Iterable> extractors, - FieldNamingStrategy fallback) { + public AnnotationFieldNamingStrategy(Iterable> extractors) { super(extractors); checkArgument(extractors.iterator().hasNext(), "you must supply at least one name extractor, for example: " + ExtractSerializedName.class.getSimpleName()); - this.fallback = checkNotNull(fallback, "fallback fieldNamingPolicy"); } @Override public String translateName(Field f) { + // Determining if AutoValue is tough, since annotations are SOURCE retention. + if (Modifier.isAbstract(f.getDeclaringClass().getSuperclass().getModifiers())) { // AutoValue means abstract. + for (Invokable target : constructors(TypeToken.of(f.getDeclaringClass().getSuperclass()))) { + SerializedNames names = target.getAnnotation(SerializedNames.class); + if (names != null && target.isStatic()) { // == factory method + // Fields and constructor params are written by AutoValue in same order as methods are declared. + // By contract, SerializedNames factory methods must declare its names in the same order, + Field[] fields = f.getDeclaringClass().getDeclaredFields(); + checkState(fields.length == names.value().length, "Incorrect number of names on " + names); + for (int i = 0; i < fields.length; i++) { + if (fields[i].equals(f)) { + return names.value()[i]; + } + } + // The input field was not a declared field. Accidentally placed on something not AutoValue? + throw new IllegalStateException("Inconsistent state. Ensure type is AutoValue on " + target); + } + } + } for (Annotation annotation : f.getAnnotations()) { if (annotationToNameExtractor.containsKey(annotation.annotationType())) { return annotationToNameExtractor.get(annotation.annotationType()).apply(annotation); } } - return fallback.translateName(f); + return null; } } public static class AnnotationOrNameFieldNamingStrategy extends AnnotationFieldNamingStrategy implements FieldNamingStrategy { - public AnnotationOrNameFieldNamingStrategy(Iterable> extractors, - FieldNamingStrategy fallback) { - super(extractors, fallback); + public AnnotationOrNameFieldNamingStrategy(Iterable> extractors) { + super(extractors); } @Override diff --git a/core/src/main/java/org/jclouds/reflect/Reflection2.java b/core/src/main/java/org/jclouds/reflect/Reflection2.java index 4c08b08eee..f592466341 100644 --- a/core/src/main/java/org/jclouds/reflect/Reflection2.java +++ b/core/src/main/java/org/jclouds/reflect/Reflection2.java @@ -89,7 +89,7 @@ public class Reflection2 { } /** - * return all constructors present in the class as {@link Invokable}s. + * return all constructors or static factory methods present in the class as {@link Invokable}s. * * @param ownerType * corresponds to {@link Invokable#getOwnerType()} diff --git a/core/src/test/java/org/jclouds/json/JsonTest.java b/core/src/test/java/org/jclouds/json/JsonTest.java index cf477b482f..dc8e3ee76a 100644 --- a/core/src/test/java/org/jclouds/json/JsonTest.java +++ b/core/src/test/java/org/jclouds/json/JsonTest.java @@ -38,8 +38,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.gson.FieldAttributes; -import com.google.gson.FieldNamingPolicy; -import com.google.gson.FieldNamingStrategy; import com.google.gson.Gson; import com.google.gson.TypeAdapter; import com.google.gson.TypeAdapterFactory; @@ -226,105 +224,110 @@ public class JsonTest { } @AutoValue - abstract static class UpperCamelCasedType { + abstract static class SerializedNamesType { abstract String id(); @Nullable abstract Map volumes(); - // Currently, this only works for deserialization. Need to set a naming policy for serialization! @SerializedNames({ "Id", "Volumes" }) - private static UpperCamelCasedType create(String id, Map volumes) { - return new AutoValue_JsonTest_UpperCamelCasedType(id, volumes); + private static SerializedNamesType create(String id, Map volumes) { + return new AutoValue_JsonTest_SerializedNamesType(id, volumes); } } - public void upperCamelCaseWithSerializedNames() { - Json json = Guice.createInjector(new GsonModule(), new AbstractModule() { - @Override protected void configure() { - bind(FieldNamingStrategy.class).toInstance(FieldNamingPolicy.UPPER_CAMEL_CASE); - } - }).getInstance(Json.class); + public void autoValueSerializedNames() { + Json json = Guice.createInjector(new GsonModule()).getInstance(Json.class); - UpperCamelCasedType resource = UpperCamelCasedType.create("1234", Collections.emptyMap()); + SerializedNamesType resource = SerializedNamesType.create("1234", Collections.emptyMap()); String spinalJson = "{\"Id\":\"1234\",\"Volumes\":{}}"; assertEquals(json.toJson(resource), spinalJson); - assertEquals(json.fromJson(spinalJson, UpperCamelCasedType.class), resource); - } - - public void upperCamelCaseWithSerializedNamesNullJsonValue() { - Json json = Guice.createInjector(new GsonModule(), new AbstractModule() { - @Override protected void configure() { - bind(FieldNamingStrategy.class).toInstance(FieldNamingPolicy.UPPER_CAMEL_CASE); - } - }).getInstance(Json.class); - - assertEquals(json.fromJson("{\"Id\":\"1234\",\"Volumes\":null}", UpperCamelCasedType.class), - UpperCamelCasedType.create("1234", null)); + assertEquals(json.fromJson(spinalJson, SerializedNamesType.class), resource); } @AutoValue - abstract static class NestedCamelCasedType { - abstract UpperCamelCasedType item(); + abstract static class SerializedNamesTooFewType { + abstract String id(); - abstract List items(); + @Nullable abstract Map volumes(); - // Currently, this only works for deserialization. Need to set a naming policy for serialization! - @SerializedNames({ "Item", "Items" }) - private static NestedCamelCasedType create(UpperCamelCasedType item, List items) { - return new AutoValue_JsonTest_NestedCamelCasedType(item, items); + @SerializedNames("Id") // TODO: check things like this with error-prone, not runtime! + private static SerializedNamesTooFewType create(String id, Map volumes) { + return new AutoValue_JsonTest_SerializedNamesTooFewType(id, volumes); } } - private final NestedCamelCasedType nested = NestedCamelCasedType - .create(UpperCamelCasedType.create("1234", Collections.emptyMap()), - Arrays.asList(UpperCamelCasedType.create("5678", ImmutableMap.of("Foo", "Bar")))); + /** Common problem. Someone adds a parameter, but forgets to add it to the names list. */ + @Test(expectedExceptions = IllegalStateException.class, expectedExceptionsMessageRegExp = "Incorrect number .*") + public void autoValueSerializedNames_tooFew() { + Json json = Guice.createInjector(new GsonModule()).getInstance(Json.class); + json.toJson(SerializedNamesTooFewType.create("1234", null)); + } - public void nestedCamelCasedType() { - Json json = Guice.createInjector(new GsonModule(), new AbstractModule() { - @Override protected void configure() { - bind(FieldNamingStrategy.class).toInstance(FieldNamingPolicy.UPPER_CAMEL_CASE); - } - }).getInstance(Json.class); + public void autoValueSerializedNames_nullValueInJson() { + Json json = Guice.createInjector(new GsonModule()).getInstance(Json.class); + + assertEquals(json.fromJson("{\"Id\":\"1234\",\"Volumes\":null}", SerializedNamesType.class), + SerializedNamesType.create("1234", null)); + } + + @AutoValue + abstract static class NestedSerializedNamesType { + abstract SerializedNamesType item(); + + abstract List items(); + + @SerializedNames({ "Item", "Items" }) + private static NestedSerializedNamesType create(SerializedNamesType item, List items) { + return new AutoValue_JsonTest_NestedSerializedNamesType(item, items); + } + } + + private final NestedSerializedNamesType nested = NestedSerializedNamesType + .create(SerializedNamesType.create("1234", Collections.emptyMap()), + Arrays.asList(SerializedNamesType.create("5678", ImmutableMap.of("Foo", "Bar")))); + + public void autoValueSerializedNames_nestedType() { + Json json = Guice.createInjector(new GsonModule()).getInstance(Json.class); String spinalJson = "{\"Item\":{\"Id\":\"1234\",\"Volumes\":{}},\"Items\":[{\"Id\":\"5678\",\"Volumes\":{\"Foo\":\"Bar\"}}]}"; assertEquals(json.toJson(nested), spinalJson); - assertEquals(json.fromJson(spinalJson, NestedCamelCasedType.class), nested); + assertEquals(json.fromJson(spinalJson, NestedSerializedNamesType.class), nested); } - public void nestedCamelCasedTypeOverriddenTypeAdapterFactory() { + public void autoValueSerializedNames_overriddenTypeAdapterFactory() { Json json = Guice.createInjector(new GsonModule(), new AbstractModule() { @Override protected void configure() { } @Provides public Set typeAdapterFactories() { - return ImmutableSet.of(new NestedCamelCasedTypeAdapterFactory()); + return ImmutableSet.of(new NestedSerializedNamesTypeAdapterFactory()); } }).getInstance(Json.class); assertEquals(json.toJson(nested), "{\"id\":\"1234\",\"count\":1}"); - assertEquals(json.fromJson("{\"id\":\"1234\",\"count\":1}", NestedCamelCasedType.class), nested); + assertEquals(json.fromJson("{\"id\":\"1234\",\"count\":1}", NestedSerializedNamesType.class), nested); } - private class NestedCamelCasedTypeAdapterFactory extends TypeAdapter + private class NestedSerializedNamesTypeAdapterFactory extends TypeAdapter implements TypeAdapterFactory { @Override public TypeAdapter create(Gson gson, TypeToken typeToken) { - if (!(NestedCamelCasedType.class.isAssignableFrom(typeToken.getRawType()))) { + if (!(NestedSerializedNamesType.class.isAssignableFrom(typeToken.getRawType()))) { return null; } return (TypeAdapter) this; } - @Override public void write(JsonWriter out, NestedCamelCasedType value) throws IOException { + @Override public void write(JsonWriter out, NestedSerializedNamesType value) throws IOException { out.beginObject(); out.name("id").value(value.item().id()); out.name("count").value(value.items().size()); out.endObject(); } - @Override public NestedCamelCasedType read(JsonReader in) throws IOException { + @Override public NestedSerializedNamesType read(JsonReader in) throws IOException { in.beginObject(); in.nextName(); in.nextString(); diff --git a/core/src/test/java/org/jclouds/json/internal/DeserializationConstructorAndReflectiveTypeAdapterFactoryTest.java b/core/src/test/java/org/jclouds/json/internal/DeserializationConstructorAndReflectiveTypeAdapterFactoryTest.java index b4f02020d6..1d54d09c61 100644 --- a/core/src/test/java/org/jclouds/json/internal/DeserializationConstructorAndReflectiveTypeAdapterFactoryTest.java +++ b/core/src/test/java/org/jclouds/json/internal/DeserializationConstructorAndReflectiveTypeAdapterFactoryTest.java @@ -43,7 +43,6 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Lists; import com.google.common.collect.Maps; -import com.google.gson.FieldNamingPolicy; import com.google.gson.FieldNamingStrategy; import com.google.gson.Gson; import com.google.gson.GsonBuilder; @@ -63,7 +62,7 @@ public final class DeserializationConstructorAndReflectiveTypeAdapterFactoryTest static DeserializationConstructorAndReflectiveTypeAdapterFactory parameterizedCtorFactory() { FieldNamingStrategy serializationPolicy = new AnnotationOrNameFieldNamingStrategy(ImmutableSet.of( - new ExtractSerializedName(), new ExtractNamed()), FieldNamingPolicy.IDENTITY); + new ExtractSerializedName(), new ExtractNamed())); AnnotationConstructorNamingStrategy deserializationPolicy = new AnnotationConstructorNamingStrategy( ImmutableSet.of(ConstructorProperties.class, SerializedNames.class, Inject.class), ImmutableSet.of(new ExtractNamed())); diff --git a/core/src/test/java/org/jclouds/json/internal/NamingStrategiesTest.java b/core/src/test/java/org/jclouds/json/internal/NamingStrategiesTest.java index 20c6046128..917d0bfe76 100644 --- a/core/src/test/java/org/jclouds/json/internal/NamingStrategiesTest.java +++ b/core/src/test/java/org/jclouds/json/internal/NamingStrategiesTest.java @@ -36,7 +36,6 @@ import org.testng.annotations.Test; import com.google.common.collect.ImmutableSet; import com.google.common.reflect.Invokable; -import com.google.gson.FieldNamingPolicy; import com.google.gson.FieldNamingStrategy; import com.google.gson.annotations.SerializedName; @@ -101,18 +100,16 @@ public final class NamingStrategiesTest { } public void testAnnotationFieldNamingStrategy() throws Exception { - FieldNamingStrategy strategy = new AnnotationFieldNamingStrategy(ImmutableSet.of(new ExtractNamed()), - FieldNamingPolicy.UPPER_CAMEL_CASE); + FieldNamingStrategy strategy = new AnnotationFieldNamingStrategy(ImmutableSet.of(new ExtractNamed())); - assertEquals(strategy.translateName(SimpleTest.class.getDeclaredField("a")), "A"); // Per fallback! - assertEquals(strategy.translateName(SimpleTest.class.getDeclaredField("b")), "B"); // Per fallback! + assertEquals(strategy.translateName(SimpleTest.class.getDeclaredField("a")), null); + assertEquals(strategy.translateName(SimpleTest.class.getDeclaredField("b")), null); assertEquals(strategy.translateName(SimpleTest.class.getDeclaredField("c")), "cat"); assertEquals(strategy.translateName(SimpleTest.class.getDeclaredField("d")), "dog"); } public void testAnnotationOrNameFieldNamingStrategy() throws Exception { - FieldNamingStrategy strategy = new AnnotationOrNameFieldNamingStrategy(ImmutableSet.of(new ExtractNamed()), - FieldNamingPolicy.IDENTITY); + FieldNamingStrategy strategy = new AnnotationOrNameFieldNamingStrategy(ImmutableSet.of(new ExtractNamed())); assertEquals(strategy.translateName(SimpleTest.class.getDeclaredField("a")), "a"); assertEquals(strategy.translateName(SimpleTest.class.getDeclaredField("b")), "b");