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

[BUG] [Dart] additional properties of type: object doesn't generate with mapCastOfType #12165

Closed
5 of 6 tasks
0xNF opened this issue Apr 19, 2022 · 12 comments · Fixed by #12426
Closed
5 of 6 tasks

[BUG] [Dart] additional properties of type: object doesn't generate with mapCastOfType #12165

0xNF opened this issue Apr 19, 2022 · 12 comments · Fixed by #12426

Comments

@0xNF
Copy link
Contributor

0xNF commented Apr 19, 2022

Bug Report Checklist

  • Have you provided a full/minimal spec to reproduce the issue?
  • Have you validated the input using an OpenAPI validator (example)?
  • Have you tested with the latest master to confirm the issue still exists?
  • Have you searched for related issues/PRs?
  • What's the actual output vs expected output?
  • [Optional] Sponsorship to speed up the bug fix or feature request (example)
Description

The additionalProperties field, which specified a Map<T1, T2> in Dart, doesn't generate properly by fields defined by

schema:
   type: string
   additionalProperties:
      type: object
openapi-generator version

6.0.0

OpenAPI declaration file content or url
openapi: 3.0.3
info:
  version: "1.1"
  title: Dart MappedValue Fail
servers:
  - url: 'localhost'
    variables:
      host:
        default: localhost
components:
  schemas:
    ItemGood:
      type: object
      properties:
        mappedProps:
          type: string
          additionalProperties:
            type: string
    ItemBad:
      type: object
      properties:
        mappedProps:
          type: string
          additionalProperties:
            type: object
paths:
  /items:
    get:
      operationId: GetItems
      responses:
        "200":
          description: A list of Items
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/ItemGood'
Generation Details
  1. java -jar ..\openapi-generator\modules\openapi-generator-cli\target\openapi-generator-cli.jar generate -i .\shadowspec.yaml -g dart -o mapped
Steps to reproduce

After generation, see the ./mapped/lib/model/item_good.dart and ./mapped/lib/model/item_bad.dart files.

ItemGood, defined as

    ItemGood:
      type: object
      properties:
        mappedProps:
          type: string
          additionalProperties:
            type: string

generates correctly with

      return ItemGood(
        mappedProps: mapCastOfType<String, String>(json, r'mappedProps') ?? const {},
      );

However, ItemBad, defined as

    ItemBad:
      type: object
      properties:
        mappedProps:
          type: string
          additionalProperties:
            type: object

generates incorrectly, using mapValueOfType instead of mapCastOfType:

      return ItemBad(
        mappedProps: mapValueOfType<Map<String, Object>>(json, r'mappedProps') ?? const {},
      );
Related issues/PRs
Suggest a fix

Maps should try to generate with mapValueOfType in as many situations as possible

@0xNF
Copy link
Contributor Author

0xNF commented May 8, 2022

Slightly modifying the schema to look like this instead:

 ItemBad:
      type: object
      properties:
        mappedProps:
          type: string
          additionalProperties: true

and then inserting a check for additionalPropertiesIsAnyType into modules\openapi-generator\src\main\resources\dart2\serialization\native\native_class.mustache:173

                {{^items.isMap}}
                  {{#additionalPropertiesIsAnyType}}
        {{{name}}}: mapCastOfType<String, Object>(json, r'{{{baseName}}}'){{#required}}{{^isNullable}}!{{/isNullable}}{{/required}}{{^required}}{{#defaultValue}} ?? {{{.}}}{{/defaultValue}}{{/required}},
                  {{/additionalPropertiesIsAnyType}}
                  {{^additionalPropertiesIsAnyType}}
        {{{name}}}: mapValueOfType<{{{datatypeWithEnum}}}>(json, r'{{{baseName}}}'){{#required}}{{^isNullable}}!{{/isNullable}}{{/required}}{{^required}}{{#defaultValue}} ?? {{{.}}}{{/defaultValue}}{{/required}},
                  {{/additionalPropertiesIsAnyType}}
                {{/items.isMap}}

I can get the dart generators to correctly deserialize the value using mapCastOfType instead of mapValueOfType

But I don't know what the correct way to do this really is. Some issues I have include:

  1. The existing examples for mapCastOfType use <String, dynamic> but because the instance member is generated as Map<String, Object> and not a Map<String, dynamic> I have to deviate from the pre-existing pattern.
  2. I have to modify my schema to work around the generator. I don't see what the semantic difference is between additionalProperties: true and additionalProperties: type: object is, these should be equivalent (I think?), and so the generator shouldn't be distinguishing between the two in the way it does, but I don't know where to fix this.

@ahmednfwela
Copy link
Contributor

ahmednfwela commented May 22, 2022

I am wondering if the spec itself is valid in your case
shouldn't additional properties be added to only type: object ?
a type string won't have properties in the first place, so how can it have additional properties?

docs: https://swagger.io/docs/specification/data-models/dictionaries/

@0xNF
Copy link
Contributor Author

0xNF commented May 22, 2022

It's possibly not valid. I admit that I don't fully understand the OpenAPI spec format -- I've included it here in a simplified form because it's something I encountered in the wild at my current company.

Even if we take the type: object part away, I'm still having trouble generating a Map<String, Object> correctly when the more appropriate additionalProperties: true field is used.

@ahmednfwela
Copy link
Contributor

does this schema not work for you?

type: object
additionalProperties: true

@0xNF
Copy link
Contributor Author

0xNF commented May 22, 2022

I'll double check, but previously that was producing what looked like a valid Dart file, but would fail at runtime when actual access occurred on the generated map, since the generated object was some kind of _linkedInternalHashMap that doesn't permit indexing access.

@ahmednfwela
Copy link
Contributor

if that happens please include a repro code of what causes the error

@0xNF
Copy link
Contributor Author

0xNF commented May 22, 2022

I tried again with the latest master branch -- this time I don't actually get the error described above, but I do get a different and equally bad error.

When the check in castValueOfType<Map<String, Object>> occurs, the Map is successfully placed into a proper map, but the type assertion fails at api_helper.dart:73.

It's trying to assert that value is of type Map<String, Object> but it fails, because it's actually _InternalLinkedHashMap<String, dynamic>, which are not equal to each other, so it will return null instead.

You can try it for yourself here:
https://github.com/0xNF/openapi_dart_map

spec file:

openapi: 3.0.3
info:
  version: "1.1"
  title: Dart Bad Map
servers:
  - url: 'localhost'
    variables:
      host:
        default: localhost
components:
  schemas:
    ItemsWithMapStringAnyWithPropsTrue:
      type: object
      description: "map-string-any defined with `additionalProperties: true`"
      properties:
        objectMap:
          type: object
          additionalProperties: true
paths:
  /items:
    get:
      operationId: GetItemWithMapStringObjects
      responses:
        "200":
          description: A list of ItemWithMapStringObject
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/ItemsWithMapStringAnyWithPropsTrue'

the test file at test\item_with_map_string_any_with_props_object_test.dart will show the error:

  final String s = '{"objectMap": {"item_int": 1, "item_bool": true, "item_string": "str", "item_double": 4.3, "item_map": {"subProps": 1}, "item_array": [-1]}}';
    test('to test the property `objectMap`', () async {
      final m = JsonDecoder().convert(s);
      final i = ItemsWithMapStringAnyWithPropsTrue.fromJson(m)!;

      /* this fails because of the incorrect type assertion in mapValueOfType */
      assert(i.objectMap.isNotEmpty);

      assert(i.objectMap["item_int"] == 1);
      assert(i.objectMap["item_bool"] == true);
      assert(i.objectMap["item_string"] == "str");
      assert(i.objectMap["item_double"] == 4.3);
      assert((i.objectMap["item_map"] as Map)["subProps"] == 1);
      assert((i.objectMap["item_array"] as List)[0] == -1);
    });

@ahmednfwela
Copy link
Contributor

that actually makes sense
it's because JsonDecoder().convert(s) decodes objectMap as just Map not Map<String, Object> so it fails
the fix is to replace

return ItemsWithMapStringAnyWithPropsTrue(
  objectMap: mapValueOfType<Map<String, Object>>(json, r'objectMap') ?? {},
);

with

return ItemsWithMapStringAnyWithPropsTrue(
  objectMap: mapCastOfType<String, Object>(json, r'objectMap') ?? {},
);

@0xNF
Copy link
Contributor Author

0xNF commented May 22, 2022

Yes, that's what I was trying to solve with the PR -- the fix I included adds that mapCastOfType call to the mustache file for complex maps.

edit: my bad, I didn't realize this wasn't the PR thread. I was talking about this PR #12426

@ahmednfwela
Copy link
Contributor

ahmednfwela commented May 22, 2022

I am wondering tho, why would isMap be false in the first place

@ahmednfwela
Copy link
Contributor

sounds like a problem with DefaultCodegen.java @wing328 can you confirm ?

@0xNF
Copy link
Contributor Author

0xNF commented May 22, 2022

I also wondered about that, I figured it was some arcane thing that existed for a good reason -- it's from the core generator, so its beyond the scope of just the dart portion of the project.

I spent a long, long time debugging the issue to find out that isMap was false when I don't think it should have been.

I recall stepping through it that by that portion of the generator, it's looking at the inner map items instead of the map item itself, which is sees as just a generic object

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants