Generate proper API description for Map types using "additionalProperties" (#155)

* Proper Map support for Discovery and Swagger with additionalProperties
- Two unit tests disabled (to reenable)
- Missing flag to enable new map handling
- Changed description handling in SwaggerGenerator to follow spec

* Proper Map support for Discovery and Swagger with additionalProperties
- Enable new behaviour with a property / env flag
- Still use JsonMap for unsupported keys (not String) or value types (arrays)
- Add a flag to warn about unsupported Map type parameters
- Reenable disabled tests (now passing)
- Test old and new behaviour in unit tests

* Proper Map support for Discovery and Swagger with additionalProperties
- Support additional map key types (confirmed to work at runtime)
- Add new flag to enable support for array-like value types
- ALlow to use warn flag without actually generating new schema for Maps
- Add Javadoc on flags for maps
- Additional test cases for Map types

* Proper Map support for Discovery and Swagger with additionalProperties
- Improve Map schema-related flags with dedicated MapSchemaFlag class
- Default to Map schema using "additionalProperties", with flag for legacy mode
- Fail by default when encountering non-String Map keys, with flag to ignore
- Improve unit tests

* Improve readability of MapSchemaFlag

* Support more types of Map to generate "additionalProperties" schema:
- Handle subclasses of Map with concrete key and value (exclude others)
- Exclude raw Map type
- Better support nested Maps

Author: Clément Denis

endpoints-framework-tools/src/test/java/com/google/api/server/spi/tools/AnnotationApiConfigGeneratorTest.java

@@ -1922,7 +1922,7 @@ public void foo(R param) {}
 
   @Test
   public void testRequestDoesContainMap() throws Exception {
-    checkRequestIsNotEmpty(new SimpleFoo<Map<ServletContext, User>>() {}.getClass());
+    checkRequestIsNotEmpty(new SimpleFoo<Map<String, User>>() {}.getClass());
   }
 
   @Test

endpoints-framework/src/main/java/com/google/api/server/spi/config/model/MapSchemaFlag.java

@@ -0,0 +1,85 @@
+/*
+ * Copyright 2018 Google Inc. All Rights Reserved.
+ *
+ * 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.api.server.spi.config.model;
+
+import com.google.common.annotations.VisibleForTesting;
+
+/**
+ * These flags control the behavior of the schema generators regarding Map types.<br>
+ * <br>
+ * By default, schema generation uses "additionalProperties" in JsonSchema to describe Map types
+ * (both for Discovery and OpenAPI), with a proper description of the value types.<br> This mode
+ * supports key types that can be serialized from / to String, and supports any value type except
+ * array-like ones (see {@link MapSchemaFlag#SUPPORT_ARRAYS_VALUES} for more details).<br> In
+ * previous versions of Cloud Endpoints, Maps were always represented using the untyped "JsonMap"
+ * object (see {@link com.google.api.server.spi.config.model.SchemaRepository#MAP_SCHEMA}).<br>
+ * <br>
+ * To enable one of these enum flags, you can either:
+ * <ul>
+ * <li>Set system property {@link MapSchemaFlag#systemPropertyName} (defined as
+ * "endpoints.mapSchema." + systemPropertySuffix) to any value except a falsy one</li>
+ * <li>Set env variable {@link MapSchemaFlag#envVarName} (defined as "ENDPOINTS_MAP_SCHEMA_"
+ * + name()) to any value except a falsy one</li>
+ * </ul>
+ * <br>
+ * Notes:
+ * <ul>
+ * <li>System properties are evaluated before env variables.</li>
+ * <li>falsy is defined as a case-insensitive equality with "false".</li>
+ * </ul>
+ */
+public enum MapSchemaFlag {
+
+  /**
+   * Reenabled the previous behavior of Cloud Endpoints, using untyped "JsonMap" for all Map types.
+   */
+  FORCE_JSON_MAP_SCHEMA("forceJsonMapSchema"),
+
+  /**
+   * When enabled, schema generation will not throw an error when handling Map types with keys that
+   * are not serializable from / to string (previous Cloud Endpoints behavior). It will still
+   * probably generate an error when serializing / deserializing these types at runtime.
+   */
+  IGNORE_UNSUPPORTED_KEY_TYPES("ignoreUnsupportedKeyTypes"),
+
+  /**
+   * Array values in "additionalProperties" are supported by the API Explorer, but not by the Java
+   * client generation. This flag can be enabled when deploying an API to the server, but should
+   * always be disabled when generating Java clients.
+   */
+  SUPPORT_ARRAYS_VALUES("supportArrayValues");
+
+  private static final String ENV_VARIABLE_PREFIX = "ENDPOINTS_MAP_SCHEMA_";
+  private static final String SYSTEM_PROPERTY_PREFIX = "endpoints.mapSchema.";
+
+  @VisibleForTesting
+  public String envVarName;
+  @VisibleForTesting
+  public String systemPropertyName;
+
+  MapSchemaFlag(String systemPropertySuffix) {
+    this.envVarName = ENV_VARIABLE_PREFIX + name();
+    this.systemPropertyName = SYSTEM_PROPERTY_PREFIX + systemPropertySuffix;
+  }
+
+  public boolean isEnabled() {
+    String envVar = System.getenv(envVarName);
+    String systemProperty = System.getProperty(systemPropertyName);
+    return systemProperty != null && !"false".equalsIgnoreCase(systemProperty)
+        || envVar != null && !"false".equalsIgnoreCase(envVar);
+  }
+
+}

endpoints-framework/src/main/java/com/google/api/server/spi/config/model/Schema.java

@@ -21,6 +21,9 @@ public abstract class Schema {
   /** A map from field names to fields for the schema. */
   public abstract ImmutableSortedMap<String, Field> fields();
 
+  /** If the schema is a map, a reference to the map value type. */
+  @Nullable public abstract Field mapValueSchema();
+
   /**
    * If the schema is an enum, a list of possible enum values in their string representation.
    */
@@ -45,8 +48,9 @@ public abstract static class Builder {
 
     public abstract Builder setName(String name);
     public abstract Builder setType(String type);
-    @Nullable public abstract Builder setDescription(String description);
+    public abstract Builder setDescription(String description);
     public abstract Builder setFields(ImmutableSortedMap<String, Field> fields);
+    public abstract Builder setMapValueSchema(Field mapValueSchema);
     public Builder addField(String name, Field field) {
       fieldsBuilder.put(name, field);
       return this;
@@ -101,7 +105,7 @@ public static Builder builder() {
     public abstract static class Builder {
       public abstract Builder setName(String name);
       public abstract Builder setType(FieldType type);
-      @Nullable public abstract Builder setDescription(String description);
+      public abstract Builder setDescription(String description);
       public abstract Builder setSchemaReference(SchemaReference ref);
       public abstract Builder setArrayItemSchema(Field schema);
       public abstract Field build();

endpoints-framework/src/main/java/com/google/api/server/spi/config/model/SchemaRepository.java

@@ -11,11 +11,13 @@
 import com.google.api.server.spi.config.model.Schema.Field;
 import com.google.api.server.spi.config.model.Schema.SchemaReference;
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Optional;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.LinkedHashMultimap;
 import com.google.common.collect.Multimap;
 import com.google.common.reflect.TypeToken;
 
+import java.util.EnumSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
@@ -41,8 +43,19 @@ public class SchemaRepository {
       .setType("object")
       .build();
 
+  private static final EnumSet<FieldType> SUPPORTED_MAP_KEY_TYPES = EnumSet.of(
+      FieldType.STRING,
+      FieldType.ENUM,
+      FieldType.BOOLEAN,
+      FieldType.INT8, FieldType.INT16, FieldType.INT32, FieldType.INT64,
+      FieldType.FLOAT, FieldType.DOUBLE,
+      FieldType.DATE, FieldType.DATE_TIME
+  );
+
   @VisibleForTesting
   static final String ARRAY_UNUSED_MSG = "unused for array items";
+  @VisibleForTesting
+  static final String MAP_UNUSED_MSG = "unused for map values";
 
   private final Multimap<ApiKey, Schema> schemaByApiKeys = LinkedHashMultimap.create();
   private final Map<ApiSerializationConfig, Map<TypeToken<?>, Schema>> types = Maps.newHashMap();
@@ -96,8 +109,8 @@ public List<Schema> getAllSchemaForApi(ApiKey apiKey) {
   /**
    * Gets all schema for an API config.
    *
-   * @return a {@link Map} from {@link TypeToken} to {@link Schema}. If there are no schema for
-   *     this config, an empty map is returned.
+   * @return a {@link Map} from {@link TypeToken} to {@link Schema}. If there are no schema for this
+   * config, an empty map is returned.
    */
   private Map<TypeToken<?>, Schema> getAllTypesForConfig(ApiConfig config) {
     Map<TypeToken<?>, Schema> typesForConfig = types.get(config.getSerializationConfig());
@@ -147,9 +160,16 @@ private Schema getOrCreateTypeForConfig(
       schemaByApiKeys.put(key, ANY_SCHEMA);
       return ANY_SCHEMA;
     } else if (Types.isMapType(type)) {
-      typesForConfig.put(type, MAP_SCHEMA);
-      schemaByApiKeys.put(key, MAP_SCHEMA);
-      return MAP_SCHEMA;
+      schema = MAP_SCHEMA;
+      final TypeToken<Map<?, ?>> mapSupertype = type.getSupertype(Map.class);
+      final boolean hasConcreteKeyValue = Types.isConcreteType(mapSupertype.getType());
+      boolean forceJsonMapSchema = MapSchemaFlag.FORCE_JSON_MAP_SCHEMA.isEnabled();
+      if (hasConcreteKeyValue && !forceJsonMapSchema) {
+        schema = createMapSchema(mapSupertype, typesForConfig, config).or(schema);
+      }
+      typesForConfig.put(type, schema);
+      schemaByApiKeys.put(key, schema);
+      return schema;
     } else if (Types.isEnumType(type)) {
       Schema.Builder builder = Schema.builder()
           .setName(Types.getSimpleName(type, config.getSerializationConfig()))
@@ -186,6 +206,42 @@ private void addSchemaToApi(ApiKey key, Schema schema) {
         addSchemaToApi(key, f.schemaReference().get());
       }
     }
+    Field mapValueSchema = schema.mapValueSchema();
+    if (mapValueSchema != null && mapValueSchema.schemaReference() != null) {
+      addSchemaToApi(key, mapValueSchema.schemaReference().get());
+    }
+  }
+
+  private Optional<Schema> createMapSchema(
+      TypeToken<Map<?, ?>> mapType, Map<TypeToken<?>, Schema> typesForConfig, ApiConfig config) {
+    FieldType keyFieldType = FieldType.fromType(Types.getTypeParameter(mapType, 0));
+    boolean supportedKeyType = SUPPORTED_MAP_KEY_TYPES.contains(keyFieldType);
+    if (!supportedKeyType) {
+      String message = "Map field type '" + mapType + "' has a key type not serializable to String";
+      if (MapSchemaFlag.IGNORE_UNSUPPORTED_KEY_TYPES.isEnabled()) {
+        System.err.println(message + ", its schema will be JsonMap");
+      } else {
+        throw new IllegalArgumentException(message);
+      }
+    }
+    TypeToken<?> valueTypeToken = Types.getTypeParameter(mapType, 1);
+    FieldType valueFieldType = FieldType.fromType(valueTypeToken);
+    boolean supportArrayValues = MapSchemaFlag.SUPPORT_ARRAYS_VALUES.isEnabled();
+    boolean supportedValueType = supportArrayValues || valueFieldType != FieldType.ARRAY;
+    if (!supportedValueType) {
+      System.err.println("Map field type '" + mapType + "' "
+          + "has an array-like value type, its schema will be JsonMap");
+    }
+    if (!supportedKeyType || !supportedValueType) {
+      return Optional.absent();
+    }
+    TypeToken<?> valueSchemaType = ApiAnnotationIntrospector.getSchemaType(valueTypeToken, config);
+    Schema.Builder builder = Schema.builder()
+        .setName(Types.getSimpleName(mapType, config.getSerializationConfig()))
+        .setType("object");
+    Field.Builder fieldBuilder = Field.builder().setName(MAP_UNUSED_MSG);
+    fillInFieldInformation(fieldBuilder, valueSchemaType, null, typesForConfig, config);
+    return Optional.of(builder.setMapValueSchema(fieldBuilder.build()).build());
   }
 
   private Schema createBeanSchema(
@@ -200,7 +256,8 @@ private Schema createBeanSchema(
       TypeToken<?> propertyType = propertySchema.getType();
       if (propertyType != null) {
         Field.Builder fieldBuilder = Field.builder().setName(propertyName);
-        fillInFieldInformation(fieldBuilder, propertyType, propertySchema.getDescription(), typesForConfig, config);
+        fillInFieldInformation(fieldBuilder, propertyType, propertySchema.getDescription(),
+            typesForConfig, config);
         builder.addField(propertyName, fieldBuilder.build());
       }
     }

endpoints-framework/src/main/java/com/google/api/server/spi/config/model/Types.java

@@ -21,16 +21,21 @@
 import com.google.api.server.spi.config.ResourceTransformer;
 import com.google.api.server.spi.config.Transformer;
 import com.google.api.server.spi.response.CollectionResponse;
+import com.google.common.base.Predicate;
 import com.google.common.collect.Iterables;
 import com.google.common.reflect.TypeToken;
 
+import java.lang.reflect.GenericArrayType;
 import java.lang.reflect.ParameterizedType;
 import java.lang.reflect.Type;
 import java.lang.reflect.TypeVariable;
 import java.lang.reflect.WildcardType;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Map;
 
+import javax.annotation.Nullable;
+
 /**
  * Utilities for dealing with type information.
  */
@@ -59,6 +64,30 @@ public static boolean isMapType(TypeToken<?> type) {
     return type.isSubtypeOf(Map.class) && !isJavaClientEntity(type);
   }
 
+  /**
+   * Returns true if this type is not parameterized, or has only concrete type variables (checked
+   * recursively on parameterized type variables).
+   */
+  public static boolean isConcreteType(Type type) {
+    if (type instanceof ParameterizedType) {
+      Type[] typeArguments = ((ParameterizedType) type).getActualTypeArguments();
+      return Iterables.all(Arrays.asList(typeArguments), new Predicate<Type>() {
+        @Override
+        public boolean apply(@Nullable Type input) {
+          return isConcreteType(input);
+        }
+      });
+    }
+    if (type instanceof GenericArrayType) {
+      return isConcreteType(((GenericArrayType) type).getGenericComponentType());
+    }
+    if (type instanceof Class) {
+      return true;
+    }
+    //matches instanceof TypeVariable and WildcardType
+    return false;
+  }
+
   /**
    * Returns whether or not this type is a Google Java client library entity.
    */

endpoints-framework/src/main/java/com/google/api/server/spi/discovery/DiscoveryGenerator.java

@@ -258,6 +258,9 @@ private JsonSchema convertToDiscoverySchema(Schema schema) {
       }
       docSchema.setProperties(fields);
     }
+    if (schema.mapValueSchema() != null) {
+      docSchema.setAdditionalProperties(convertToDiscoverySchema(schema.mapValueSchema()));
+    }
     docSchema.setDescription(schema.description());
     if (!schema.enumValues().isEmpty()) {
       docSchema.setEnum(new ArrayList<>(schema.enumValues()));

endpoints-framework/src/main/java/com/google/api/server/spi/swagger/SwaggerGenerator.java

@@ -414,6 +414,9 @@ private Model convertToSwaggerSchema(Schema schema) {
       }
       docSchema.setProperties(fields);
     }
+    if (schema.mapValueSchema() != null) {
+      docSchema.setAdditionalProperties(convertToSwaggerProperty(schema.mapValueSchema()));
+    }
     return docSchema;
   }
 
@@ -438,7 +441,10 @@ private Property convertToSwaggerProperty(Field f) {
     if (p == null) {
       throw new IllegalArgumentException("could not convert field " + f);
     }
-    p.description(f.description());
+    //the spec explicitly disallows description on $ref
+    if (!(p instanceof RefProperty)) {
+      p.description(f.description());
+    }
     return p;
   }