axnsan12 / drf-yasg

Automated generation of real Swagger/OpenAPI 2.0 schemas from Django REST Framework code.
https://drf-yasg.readthedocs.io/en/stable/
Other
3.41k stars 438 forks source link

ListField with DictField as child created invalid TypeScript code #671

Open Schwankenson opened 3 years ago

Schwankenson commented 3 years ago

ListField with DictField as child created invalid TypeScript code

I have this field in my django response serializer fields:

detailpage_elements = serializers.ListField(
    required=False, 
    child=serializers.DictField()
)

in swagger.json it is shown as

"detailpage_elements":{
    "type":"array",
    "items":{
        "type":"object",
        "additionalProperties":{
            "type":"string"
        }
    }
},

I using https://www.npmjs.com/package/swagger-angular-generator to generate an angular SDK from it. It creates a syntax error

detailpage_elements?: [key: string]: string;[];

which is marked as syntax error.

I changed the child to serializer.JSONField() as workaround, which generates

"detailpage_elements":{
    "type":"array",
    "items":{
        "type":"object"
    }
},

and works

Schwankenson commented 3 years ago

Not sure if this is a swagger.json or a swagger-angular-generator issue, to I created another issue at the creators of the drf-yasg creators: https://github.com/jnwltr/swagger-angular-generator/issues/129

aarighi commented 3 years ago

I guess it's a problem in the generation of the OpenAPI Schema (so it comes before the generation of the angular SDK).

I am having the same issue with the Java SDK, the DictField is translated to a Map<String, String>, while it should be a Map<String, Object>. Changing the type in the generated Java code fixes the issue for me.

The point here is that a field

myfield = serializers.DictField(required=False, allow_empty=True, allow_null=False)

is rendered in the OpenAPI file as

    myfield:
        title: Myfield
        type: object
        additionalProperties:
          type: string

while it should just be

    myfield:
        title: Myfield
        type: object
        additionalProperties:
          type: object

Not sure if the last 2 lines can be omitted, maybe.

terencehonles commented 3 years ago

The issue is:

https://github.com/axnsan12/drf-yasg/blob/b99306f71c6a5779b62189df7d9c1f5ea1c794ef/src/drf_yasg/inspectors/field.py#L711-L724

then uses a child of:

https://github.com/axnsan12/drf-yasg/blob/b99306f71c6a5779b62189df7d9c1f5ea1c794ef/src/drf_yasg/inspectors/field.py#L749-L755

However, I think this is probably correct. With my change https://github.com/axnsan12/drf-yasg/pull/566 you'd also see x-nullable: true added to the additionalProperties. But if you want it to be an object there is no way for DRF YASG to know it should be an object not a string. I don't know if there's a good suggestion other than using a JSONField or maybe using a type annotated SerializerMethodField https://drf-yasg.readthedocs.io/en/stable/custom_spec.html#support-for-serializermethodfield

You may also want to look at https://drf-yasg.readthedocs.io/en/stable/custom_spec.html#serializer-meta-nested-class

aarighi commented 3 years ago

@terencehonles

But if you want it to be an object there is no way for DRF YASG to know it should be an object not a string.

Objects (and dicts) are generally expected to accept any type of data including integers, booleans or nested objects. It seems to me a particular limitation to assume that the dict will contain string-to-string mappings by default, where not specified.

In this case, I believe the proper default behaviour would be to omit the child type indication, unless otherwise specified.

There is a rather simple way of doing this:

myfield:
    title: Myfield
    type: object
    additionalProperties: {}

See https://swagger.io/docs/specification/data-models/dictionaries/#free-form

The Meta nested class (or another annotation of some sort) may then be used to specify special constraints that diverge from the generic use case, such as string-to-string or string-to-dict mappings. For example:

class MyExampleSerializer(serializers.BaseSerializer):
    myfield = serializers.DictField(required=False, allow_empty=True, allow_null=False)

    class Meta:
        additional_properties = {
            'myfield': {'nested_type': "string"}
        }
terencehonles commented 3 years ago

I'm not the one who wrote the code, but you're serializing to JSON so I also find it reasonable that the default is a string. In general (this is not just used for this case) the string default is probably OK, but for list and dict I also can see there being specialized logic to handle one case or the other.

If you think this should change I would suggest you open a PR to change it. I did the work of showing you where the logic is failing, but I'll leave updating the code to you. I was just passing by and figured I could probably confirm the issue and show you where to continue and I have done that. I unfortunately don't have the time to look into this further, but I hope this conversation has been helpful.