Generate proper API description for Map types using "additionalProperties" by clementdenis · Pull Request #155 · cloudendpoints/endpoints-java

clementdenis · 2018-08-01T09:35:09Z

This PR changes the way Cloud Endpoints handles Map types when generating API specs:

Previously, maps were always described as JsonMap, making the API Explorer buggy, and the Java clients barely usable.
The new behavior is to use "additionalProperties" in the JsonSchema, that work really well for String-keyed maps, and generate correctly typed Java clients.
A flag system based both on env variables and system properties (see MapSchemaFlag) can be used to fine tune the new behavior.

This introduces backward incompatible changes to the generated Java clients, and the stricter validation of acceptable Map types can make some APIs fail to generate the spec.
Legacy behavior can be reintroduced for both of these using flags in MapSchemaFlag (see FORCE_JSON_MAP_SCHEMA and IGNORE_UNSUPPORTED_KEY_TYPES).

A note about Map types with array-like values:

This use case is properly supported by both the Discovery and Swagger formats, and works fine in the API explorer.
However, it is not supported by the cloud-based client library generator (which is presumably base on this project), it will generate invalid Java code.
So the default behavior is to keep using JsonMap schema for the array-valued maps, but it can be forced to use additionalProperties with the MapSchemaFlag.SUPPORT_ARRAYS_VALUES flag.

I previously opened an issue about this: #142

codecov-io · 2018-08-01T09:39:57Z

Codecov Report

Merging #155 into master will increase coverage by 0.13%.
The diff coverage is 91.8%.

@@             Coverage Diff              @@
##             master     #155      +/-   ##
============================================
+ Coverage     80.12%   80.26%   +0.13%     
- Complexity     1689     1711      +22     
============================================
  Files           157      158       +1     
  Lines          5629     5684      +55     
  Branches        735      748      +13     
============================================
+ Hits           4510     4562      +52     
- Misses          838      840       +2     
- Partials        281      282       +1

Impacted Files	Coverage Δ	Complexity Δ
...com/google/api/server/spi/config/model/Schema.java	`100% <ø> (ø)`	`2 <0> (ø)`	⬇️
...e/api/server/spi/discovery/DiscoveryGenerator.java	`88.93% <100%> (+0.09%)`	`64 <0> (+1)`	⬆️
...oogle/api/server/spi/swagger/SwaggerGenerator.java	`77.91% <100%> (+0.67%)`	`59 <0> (+3)`	⬆️
.../com/google/api/server/spi/config/model/Types.java	`90% <77.77%> (-2.69%)`	`32 <3> (+3)`
...gle/api/server/spi/config/model/MapSchemaFlag.java	`90.9% <90.9%> (ø)`	`4 <4> (?)`
.../api/server/spi/config/model/SchemaRepository.java	`92.42% <94.28%> (+0.26%)`	`38 <8> (+10)`	⬆️
.../server/spi/discovery/CommonPathPrefixBuilder.java	`100% <0%> (+5.55%)`	`9% <0%> (+1%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 413e434...7cb5035. Read the comment docs.

tangiel · 2018-08-20T17:40:38Z

Looks like there is a conflict now (may have merged out of order)

- Two unit tests disabled (to reenable) - Missing flag to enable new map handling - Changed description handling in SwaggerGenerator to follow spec

- 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

- 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

- 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

- Handle subclasses of Map with concrete key and value (exclude others) - Exclude raw Map type - Better support nested Maps

clementdenis · 2018-08-27T18:18:15Z

I rebased from latest master (conflict was only in imports).

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

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

tangiel · 2018-09-04T21:14:42Z

...ints-framework/src/test/java/com/google/api/server/spi/discovery/DiscoveryGeneratorTest.java

+  }
+
+  @Test
+  public void testWriteDiscovery_MapEndpoint_InvalidKeyTypeKO() throws Exception {


Is "KO" intentional?

It is, but I will make the name more explicit.

tangiel · 2018-09-04T21:16:32Z

test-utils/src/main/java/com/google/api/server/spi/testing/MapEndpoint.java

@@ -0,0 +1,121 @@
+/*
+ * Copyright 2016 Google Inc. All Rights Reserved.


Let's keep the year up to date on new files.

clementdenis · 2018-09-11T20:48:32Z

I fixed formatting and copyright issues.

clementdenis force-pushed the map_support_in_generators branch from 14c1d58 to ee0b460 Compare August 10, 2018 14:09

Clément Denis added 6 commits August 27, 2018 20:13

Proper Map support for Discovery and Swagger with additionalProperties

eb01f22

- Two unit tests disabled (to reenable) - Missing flag to enable new map handling - Changed description handling in SwaggerGenerator to follow spec

Improve readability of MapSchemaFlag

3ae1b77

Support more types of Map to generate "additionalProperties" schema:

1e573c8

- Handle subclasses of Map with concrete key and value (exclude others) - Exclude raw Map type - Better support nested Maps

clementdenis force-pushed the map_support_in_generators branch from ee0b460 to 1e573c8 Compare August 27, 2018 18:15

tangiel and others added 2 commits August 28, 2018 13:02

Merge branch 'master' into map_support_in_generators

60d1615

Fix Swagger tests for maps after merging cloudendpoints#155

09762b2

clementdenis mentioned this pull request Aug 29, 2018

Map support in generators AODocs/endpoints-java#11

Closed

tangiel reviewed Sep 4, 2018

View reviewed changes

Fixed formatting and copyright headers

7cb5035

tangiel approved these changes Sep 19, 2018

View reviewed changes

tangiel merged commit 6bb25ed into cloudendpoints:master Sep 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Generate proper API description for Map types using "additionalProperties"#155
tangiel merged 9 commits intocloudendpoints:masterfrom
AODocs:map_support_in_generators

clementdenis commented Aug 1, 2018

Uh oh!

codecov-io commented Aug 1, 2018 •

edited

Loading

Uh oh!

tangiel commented Aug 20, 2018

Uh oh!

clementdenis commented Aug 27, 2018

Uh oh!

Uh oh!

Uh oh!

tangiel Sep 4, 2018

Uh oh!

clementdenis Sep 11, 2018

Uh oh!

tangiel Sep 4, 2018

Uh oh!

clementdenis commented Sep 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -0,0 +1,121 @@
		/*
		* Copyright 2016 Google Inc. All Rights Reserved.

Conversation

clementdenis commented Aug 1, 2018

Uh oh!

codecov-io commented Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tangiel commented Aug 20, 2018

Uh oh!

clementdenis commented Aug 27, 2018

Uh oh!

Uh oh!

Uh oh!

tangiel Sep 4, 2018

Choose a reason for hiding this comment

Uh oh!

clementdenis Sep 11, 2018

Choose a reason for hiding this comment

Uh oh!

tangiel Sep 4, 2018

Choose a reason for hiding this comment

Uh oh!

clementdenis commented Sep 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-io commented Aug 1, 2018 •

edited

Loading