Skip to content

Dart codegen bug fixes: honor prefix/suffix, handle operation return types, replaces Object with Map #12574

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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
+224 −160
Open

Dart codegen bug fixes: honor prefix/suffix, handle operation return types, replaces Object with Map #12574

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e8d7107
added returnInnerType for container types, made Dart generator recogn…
jeffmikels Jun 11, 2022
ad6a3fc
Merge branch 'OpenAPITools:master' into master
jeffmikels Jun 11, 2022
f1babda
added returnInnerType for container types, made Dart generator recogn…
jeffmikels Jun 11, 2022
7fd619d
added returnInnerType for container types, made Dart generator recogn…
jeffmikels Jun 11, 2022
57a5a27
added returnInnerType for container types, made Dart generator recogn…
jeffmikels Jun 11, 2022
3afd82a
making sure modelNamePrefix is honored throughout the library
jeffmikels Jun 13, 2022
fc9037e
fixed the default option for API_CLIENT_NAME when testing the DartDio…
jeffmikels Jun 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/generators/dart-dio.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ These options may be applied as additional-properties (cli) or configOptions (pl
| Option | Description | Values | Default |
| ------ | ----------- | ------ | ------- |
|allowUnicodeIdentifiers|boolean, toggles whether unicode identifiers are allowed in names or not, default is false| |false|
|apiClientName|Specify the api client class name, defaults to 'ApiClient'| |ApiClient|
|dateLibrary|Specify Date library|<dl><dt>**core**</dt><dd>[DEFAULT] Dart core library (DateTime)</dd><dt>**timemachine**</dt><dd>Time Machine is date and time library for Flutter, Web, and Server with support for timezones, calendars, cultures, formatting and parsing.</dd></dl>|core|
|disallowAdditionalPropertiesIfNotPresent|If false, the 'additionalProperties' implementation (set to true by default) is compliant with the OAS and JSON schema specifications. If true (default), keep the old (incorrect) behaviour that 'additionalProperties' is set to false by default.|<dl><dt>**false**</dt><dd>The 'additionalProperties' implementation is compliant with the OAS and JSON schema specifications.</dd><dt>**true**</dt><dd>Keep the old (incorrect) behaviour that 'additionalProperties' is set to false by default.</dd></dl>|true|
|ensureUniqueParams|Whether to ensure parameter names are unique in an operation (rename parameters that are not).| |true|
Expand Down
1 change: 1 addition & 0 deletions docs/generators/dart.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ These options may be applied as additional-properties (cli) or configOptions (pl
| Option | Description | Values | Default |
| ------ | ----------- | ------ | ------- |
|allowUnicodeIdentifiers|boolean, toggles whether unicode identifiers are allowed in names or not, default is false| |false|
|apiClientName|Specify the api client class name, defaults to 'ApiClient'| |ApiClient|
|disallowAdditionalPropertiesIfNotPresent|If false, the 'additionalProperties' implementation (set to true by default) is compliant with the OAS and JSON schema specifications. If true (default), keep the old (incorrect) behaviour that 'additionalProperties' is set to false by default.|<dl><dt>**false**</dt><dd>The 'additionalProperties' implementation is compliant with the OAS and JSON schema specifications.</dd><dt>**true**</dt><dd>Keep the old (incorrect) behaviour that 'additionalProperties' is set to false by default.</dd></dl>|true|
|ensureUniqueParams|Whether to ensure parameter names are unique in an operation (rename parameters that are not).| |true|
|enumUnknownDefaultCase|If the server adds new enum cases, that are unknown by an old spec/client, the client will fail to parse the network response.With this option enabled, each enum will have a new case, 'unknown_default_open_api', so that when the server sends an enum case that is not known by the client/spec, they can safely fallback to this case.|<dl><dt>**false**</dt><dd>No changes to the enum's are made, this is the default option.</dd><dt>**true**</dt><dd>With this option enabled, each enum will have a new case, 'unknown_default_open_api', so that when the enum case sent by the server is not known by the client/spec, can safely be decoded to this case.</dd></dl>|false|
Expand Down
6 changes: 4 additions & 2 deletions modules/openapi-generator/src/main/java/org/openapitools/codegen/CodegenOperation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public class CodegenOperation {
isRestfulIndex, isRestfulShow, isRestfulCreate, isRestfulUpdate, isRestfulDestroy,
isRestful, isDeprecated, isCallbackRequest, uniqueItems, hasDefaultResponse = false,
hasErrorResponseObject; // if 4xx, 5xx responses have at least one error object defined
public String path, operationId, returnType, returnFormat, httpMethod, returnBaseType,
public String path, operationId, returnType, returnFormat, httpMethod, returnBaseType, returnInnerType,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what extra information does returnInnerType add ?

Copy link
Author

@jeffmikels jeffmikels Jun 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an example, when the returnType is List<Map<String, String>> then returnBaseType will be List but the dart code, to be type safe, will want to call .cast<Map<String, String>>(). Therefore, we need to expose Map<String, String> to the template somehow.

In this case, returnInnerType will contain the string 'Map<String, String>'

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why use this instead of just items property?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The generated code in the earlier version was simply wrong. Dart would fail to compile it under certain cases.

To see where it matters, look at the template in dart2 named api.mustache on line 180. In this PR, you'll see this:

    {{#isArray}}
      final responseBody = await _decodeBodyBytes(response);
      return (await apiClient.deserializeAsync(responseBody, '{{{returnType}}}') as List)
        .cast<{{{returnInnerType}}}>()
        .{{#uniqueItems}}toSet(){{/uniqueItems}}{{^uniqueItems}}toList(){{/uniqueItems}};
    {{/isArray}}

Previously, it said this:

    {{#isArray}}
      final responseBody = await _decodeBodyBytes(response);
      return (await apiClient.deserializeAsync(responseBody, '{{{returnType}}}') as List)
        .cast<{{{returnBaseType}}}>()
        .{{#uniqueItems}}toSet(){{/uniqueItems}}{{^uniqueItems}}toList(){{/uniqueItems}};
    {{/isArray}}

In most cases, that is fine, but consider the following example from the openApi document from the Mattermost project. There is this api endpoint:

    "/teams/{team_id}/groups_by_channels": {
      "get": {
        "tags": [
          "groups"
        ],
        "summary": "Get team groups by channels",
        "description": "Retrieve the set of groups associated with the channels in the given team grouped by channel.\n\n##### Permissions\nMust have `manage_system` permission or can access only for current user\n\n__Minimum server version__: 5.11\n",
        "operationId": "GetGroupsAssociatedToChannelsByTeam",
        "parameters": [
          {
            "name": "team_id",
            "in": "path",
            "description": "Team GUID",
            "required": true,
            "schema": {
              "type": "string"
            }
          },
          {
            "name": "page",
            "in": "query",
            "description": "The page to select.",
            "schema": {
              "type": "integer",
              "default": 0
            }
          },
          {
            "name": "per_page",
            "in": "query",
            "description": "The number of groups per page.",
            "schema": {
              "type": "integer",
              "default": 60
            }
          },
          {
            "name": "filter_allow_reference",
            "in": "query",
            "description": "Boolean which filters in the group entries with the `allow_reference` attribute set.",
            "schema": {
              "type": "boolean",
              "default": false
            }
          },
          {
            "name": "paginate",
            "in": "query",
            "description": "Boolean to determine whether the pagination should be applied or not",
            "schema": {
              "type": "boolean",
              "default": false
            }
          }
        ],
        "responses": {
          "200": {
            "description": "Group list retrieval successful",
            "content": {
              "application/json": {
                "schema": {
                  "type": "array",
                  "items": {
                    "$ref": "#/components/schemas/GroupsAssociatedToChannels"
                  }
                }
              }
            }
          },
          "400": {
            "$ref": "#/components/responses/BadRequest"
          },
          "401": {
            "$ref": "#/components/responses/Unauthorized"
          },
          "403": {
            "$ref": "#/components/responses/Forbidden"
          },
          "500": {
            "$ref": "#/components/responses/InternalServerError"
          },
          "501": {
            "$ref": "#/components/responses/NotImplemented"
          }
        }
      }
    },

In this case, the returnType will be List<Map<String, List<GroupWithSchemeAdmin>>> but the returnBaseType will be Map which will result in the following Dart code:

      return (await apiClient.deserializeAsync(responseBody, 'List<Map<String, List<GroupWithSchemeAdmin>>>') as List)
          .cast<Map>()
          .toList();

However, this code will not compile because the .cast operation coerces all list objects to Map which is the same as Map<dynamic, dynamic> and is LESS SPECIFIC than Map<String, List<GroupWithSchemeAdmin>>. The compiler can't guarantee that this cast will be valid and so the code won't compile.

By adding the returnInnerType variable to the java class and by changing the mustache template, the new code will look like this:

      return (await apiClient.deserializeAsync(responseBody, 'List<Map<String, List<GroupWithSchemeAdmin>>>') as List)
          .cast<Map<String, List<GroupWithSchemeAdmin>>>()
          .toList();

This code will compile.

Why not use the items property?

It's true that items.dataType contains the exact same data, but that value is not available to api.mustache in the context where it is needed.

This value must be exposed in the same context as returnType.

returnContainer, summary, unescapedNotes, notes, baseName, defaultResponse;
public CodegenDiscriminator discriminator;
public List<Map<String, String>> consumes, produces, prioritizedContentTypes;
Expand Down Expand Up @@ -320,6 +320,7 @@ public String toString() {
sb.append(", returnType='").append(returnType).append('\'');
sb.append(", httpMethod='").append(httpMethod).append('\'');
sb.append(", returnBaseType='").append(returnBaseType).append('\'');
sb.append(", returnInnerType='").append(returnInnerType).append('\'');
sb.append(", returnContainer='").append(returnContainer).append('\'');
sb.append(", summary='").append(summary).append('\'');
sb.append(", unescapedNotes='").append(unescapedNotes).append('\'');
Expand Down Expand Up @@ -397,6 +398,7 @@ public boolean equals(Object o) {
Objects.equals(returnType, that.returnType) &&
Objects.equals(httpMethod, that.httpMethod) &&
Objects.equals(returnBaseType, that.returnBaseType) &&
Objects.equals(returnInnerType, that.returnInnerType) &&
Objects.equals(returnContainer, that.returnContainer) &&
Objects.equals(summary, that.summary) &&
Objects.equals(unescapedNotes, that.unescapedNotes) &&
Expand Down Expand Up @@ -442,7 +444,7 @@ public int hashCode() {
isArray, isMultipart, isResponseBinary, isResponseFile, isResponseOptional, hasReference,
hasDefaultResponse, isRestfulIndex, isRestfulShow, isRestfulCreate, isRestfulUpdate, isRestfulDestroy,
isRestful, isDeprecated, isCallbackRequest, uniqueItems, path, operationId, returnType, httpMethod,
returnBaseType, returnContainer, summary, unescapedNotes, notes, baseName, defaultResponse,
returnBaseType, returnInnerType, returnContainer, summary, unescapedNotes, notes, baseName, defaultResponse,
discriminator, consumes, produces, prioritizedContentTypes, servers, bodyParam, allParams, bodyParams,
pathParams, queryParams, headerParams, formParams, cookieParams, requiredParams, optionalParams,
authMethods, tags, responses, callbacks, imports, examples, requestBodyExamples, externalDocs,
Expand Down
2 changes: 2 additions & 0 deletions modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3907,9 +3907,11 @@ protected void handleMethodResponse(Operation operation,
ArraySchema as = (ArraySchema) responseSchema;
CodegenProperty innerProperty = fromProperty("response", getSchemaItems(as));
op.returnBaseType = innerProperty.baseType;
op.returnInnerType = innerProperty.dataType;
} else if (ModelUtils.isMapSchema(responseSchema)) {
CodegenProperty innerProperty = fromProperty("response", getAdditionalProperties(responseSchema));
op.returnBaseType = innerProperty.baseType;
op.returnInnerType = innerProperty.dataType;
} else {
if (cm.complexType != null) {
op.returnBaseType = cm.complexType;
Expand Down
Loading