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

"type" mismatch with "enum" contents #69

Closed
davcamer opened this issue Feb 22, 2018 · 9 comments
Closed

"type" mismatch with "enum" contents #69

davcamer opened this issue Feb 22, 2018 · 9 comments
Labels
enhancement Enhancement proposal question Open question

Comments

@davcamer
Copy link

I'm attempting to generate a go client based on an openapi spec for the open source netbox API using drf-yasg. Some fields are ending up with a mismatch between their type fields and enum fields in the generated spec.

The effected fields all use the choices keyword param to map non-string values to a list of labels. There are several examples, some with integer as the underlying type and some with boolean:
InterfaceTemplate.form_factor
PowerPort.connection_status
Rack.width

netbox makes use of serializers to have three different projections of each Django model, the standard model, a nested model when it appears in another object and a writable model for PUTs, POSTs and PATCHes. This means that each of the above fields could appears in multiple places in the generated swagger, normally at least the writable and standard models.

For example:
standard rack
writable rack

In practice, the standard model includes a small sub-object with both the integer and string values:

      "width": {
        "value": 19,
        "label": "19 inches"
      }

While the writable model expects only the value for the field. Width becomes a flat integer:

      "width": 19

Would support for this style of usage be possible?

@axnsan12
Copy link
Owner

Hello!

I'm not sure I really understand your situation (there's quite a lot of code there!), but:

However I wasn't really able to follow what is your desired output. Does the type/enum mismatch cause problems for you? Which part exactly would you like to be generated differently?

It would really help if you could provide the exact definitions of each of your problematic serializers (this can be done by printing the serializer itself: http://www.django-rest-framework.org/api-guide/serializers/#inspecting-a-modelserializer), along with what you would want to be generated differently in the swagger schema.

@davcamer
Copy link
Author

It is definitely a lot of code. I wanted to share as much as possible up front, in case it was easy to find the problem with enough code. 😆

I'll focus on a single model and accompanying serializers because I think solving the problem for one type will solve it for all the types. Digging in to Racks then. drf-yasg is getting all the different serializers, so that is good.

The type mismatch is causing two problems. One very practical and the other more theoretical:

  1. I'm trying to use the swagger.json output to generate a golang client using go-swagger. The type mismatch is choking this code generation. As seen in that issue, go-swagger might be patched already to handle this case though.
  2. The generated openapi spec doesn't match the way that the existing API works. I'll detail the specifics of this.

Thanks for pointing out the way to inspect serializers. Here they are for Racks. Type and width are the fields that cause problems for Racks.

The RackSerializer:

RackSerializer():
    id = IntegerField(label='ID', read_only=True)
    name = CharField(max_length=50, required=True)
    facility_id = CharField(allow_blank=True, allow_null=True, label='Facility ID', max_length=50, required=True)
    display_name = ReadOnlyField()
    site = NestedSiteSerializer():
        id = IntegerField(label='ID', read_only=True)
        url = HyperlinkedIdentityField(view_name='dcim-api:site-detail')
        name = CharField(max_length=50, validators=[<UniqueValidator(queryset=Site.objects.all())>])
        slug = SlugField(max_length=50, validators=[<UniqueValidator(queryset=Site.objects.all())>])
    group = NestedRackGroupSerializer():
        id = IntegerField(label='ID', read_only=True)
        url = HyperlinkedIdentityField(view_name='dcim-api:rackgroup-detail')
        name = CharField(max_length=50)
        slug = SlugField(max_length=50)
    tenant = NestedTenantSerializer():
        id = IntegerField(label='ID', read_only=True)
        url = HyperlinkedIdentityField(view_name='tenancy-api:tenant-detail')
        name = CharField(max_length=30, validators=[<UniqueValidator(queryset=Tenant.objects.all())>])
        slug = SlugField(max_length=50, validators=[<UniqueValidator(queryset=Tenant.objects.all())>])
    role = NestedRackRoleSerializer():
        id = IntegerField(label='ID', read_only=True)
        url = HyperlinkedIdentityField(view_name='dcim-api:rackrole-detail')
        name = CharField(max_length=50, validators=[<UniqueValidator(queryset=RackRole.objects.all())>])
        slug = SlugField(max_length=50, validators=[<UniqueValidator(queryset=RackRole.objects.all())>])
    serial = CharField(allow_blank=True, label='Serial number', max_length=50, required=False)
    type = ChoiceFieldSerializer(choices=((100, '2-post frame'), (200, '4-post frame'), (300, '4-post cabinet'), (1000, 'Wall-mounted frame'), (1100, 'Wall-mounted cabinet')))
    width = ChoiceFieldSerializer(choices=((19, '19 inches'), (23, '23 inches')))
    u_height = IntegerField(label='Height (U)', max_value=100, min_value=1, required=False)
    desc_units = BooleanField(help_text='Units are numbered top-to-bottom', label='Descending units', required=False)
    comments = CharField(allow_blank=True, required=False, style={'base_template': 'textarea.html'})
    custom_fields = CustomFieldsSerializer(required=False)
    class Meta:
        validators = [<UniqueTogetherValidator(queryset=Rack.objects.all(), fields=('site', 'name'))>, <UniqueTogetherValidator(queryset=Rack.objects.all(), fields=('site', 'facility_id'))>]

The WritableRackSerializer:

WritableRackSerializer():
    id = IntegerField(label='ID', read_only=True)
    name = CharField(max_length=50)
    facility_id = CharField(allow_blank=True, allow_null=True, label='Facility ID', max_length=50, required=False)
    site = PrimaryKeyRelatedField(queryset=Site.objects.all())
    group = PrimaryKeyRelatedField(allow_null=True, queryset=RackGroup.objects.all(), required=False)
    tenant = PrimaryKeyRelatedField(allow_null=True, queryset=Tenant.objects.all(), required=False)
    role = PrimaryKeyRelatedField(allow_null=True, queryset=RackRole.objects.all(), required=False)
    serial = CharField(allow_blank=True, label='Serial number', max_length=50, required=False)
    type = ChoiceField(allow_null=True, choices=((100, '2-post frame'), (200, '4-post frame'), (300, '4-post cabinet'), (1000, 'Wall-mounted frame'), (1100, 'Wall-mounted cabinet')), required=False, validators=[<django.core.validators.MinValueValidator object>, <django.core.validators.MaxValueValidator object>])
    width = ChoiceField(choices=((19, '19 inches'), (23, '23 inches')), help_text='Rail-to-rail width', required=False, validators=[<django.core.validators.MinValueValidator object>, <django.core.validators.MaxValueValidator object>])
    u_height = IntegerField(label='Height (U)', max_value=100, min_value=1, required=False)
    desc_units = BooleanField(help_text='Units are numbered top-to-bottom', label='Descending units', required=False)
    comments = CharField(allow_blank=True, required=False, style={'base_template': 'textarea.html'})
    custom_fields = CustomFieldsSerializer(required=False)
    class Meta:
        validators = [<UniqueTogetherValidator(queryset=<QuerySet [<Rack: Test Rack (0.0.1)>]>, fields=('site', 'name'))>]

This made me realize that the fields causing the problems are using a custom serializer class in the standard seralizers, derived from rest_framework.serializers.Field:

class ChoiceFieldSerializer(Field):
    """
    Represent a ChoiceField as {'value': <DB value>, 'label': <string>}.
    """
    def __init__(self, choices, **kwargs):
        self._choices = dict()
        for k, v in choices:
            # Unpack grouped choices
            if type(v) in [list, tuple]:
                for k2, v2 in v:
                    self._choices[k2] = v2
            else:
                self._choices[k] = v
        super(ChoiceFieldSerializer, self).__init__(**kwargs)

    def to_representation(self, obj):
        return {'value': obj, 'label': self._choices[obj]}

    def to_internal_value(self, data):
        return self._choices.get(data)

This custom field serializer breaks the fields, such as width, in to a small sub-object:

{
  "id": 1,
  "name": "Test Rack",
  "facility_id": "0.0.1",
  "display_name": "Test Rack (0.0.1)",
  "site": {
    "id": 1,
    "url": "http://localhost:8000/api/dcim/sites/1/",
    "name": "Area 51",
    "slug": "area-51"
  },
  "group": null,
  "tenant": null,
  "role": null,
  "serial": "",
  "type": null,
  "width": {
    "value": 19,
    "label": "19 inches"
  },
  "u_height": 42,
  "desc_units": false,
  "comments": "",
  "custom_fields": {}
}

The generated swagger for this field doesn't show the nested object for the width field, it just shows a flat string:

    "Rack": {
      "required": [
        "name",
        "facility_id",
        "site",
        "group",
        "tenant",
        "role",
        "type",
        "width"
      ],
      "type": "object",
      "properties": {
        "id": {
          "title": "ID",
          "type": "integer",
          "readOnly": true
        },
        "name": {
          "title": "Name",
          "type": "string",
          "maxLength": 50
        },
        "facility_id": {
          "title": "Facility ID",
          "type": "string",
          "maxLength": 50
        },
        "display_name": {
          "title": "Display name",
          "type": "string",
          "readOnly": true
        },
        "site": {
          "$ref": "#/definitions/NestedSite"
        },
        "group": {
          "$ref": "#/definitions/NestedRackGroup"
        },
        "tenant": {
          "$ref": "#/definitions/NestedTenant"
        },
        "role": {
          "$ref": "#/definitions/NestedRackRole"
        },
        "serial": {
          "title": "Serial number",
          "type": "string",
          "maxLength": 50
        },
        "type": {
          "title": "Type",
          "type": "string"
        },
        "width": {
          "title": "Width",
          "type": "string"
        },
        "u_height": {
          "title": "Height (U)",
          "type": "integer",
          "maximum": 100,
          "minimum": 1
        },
        "desc_units": {
          "title": "Descending units",
          "description": "Units are numbered top-to-bottom",
          "type": "boolean"
        },
        "comments": {
          "title": "Comments",
          "type": "string"
        },
        "custom_fields": {
          "title": "Custom fields",
          "type": "string"
        }
      }
    }

I guess this would ideally be a reference to a type that includes the value and label:

        "width": {
          "$ref": "#/definitions/IntegerChoiceField"
        },
...
    "IntegerChoiceField": {
      "required": [
        "label",
        "value"
      ],
      "type": "object",
      "properties": {
        "label": {
          "title": "Label",
          "type": "string",
          "readOnly": true
        },
        "value": {
          "title": "Value",
          "type": "integer",
          "readOnly": true
        }
      }
    },

For the WritableRack serializer, as you pointed out, strings are accepted for input. They are the string form of the value, rather than the label, which I wasn't expecting. On the response though, the fields use integers if the choice values are integers. For example a curl to PATCH the width field returns an integer for width:

$ curl -X PATCH --header 'Content-Type: application/json' --header 'Accept: application/json' --header 'X-CSRFToken: IX6FtawujHl73Qc6qBBsB6KED9HNGUEP1KdfzhzpkK2Fnq2GqKcYf9jpR1zHReFQ' -d '{"width": 19}' 'http://localhost:8000/api/dcim/racks/1/'
{
  "id": 1,
  "name": "Test Rack",
  "facility_id": "0.0.1",
  "site": 1,
  "group": null,
  "tenant": null,
  "role": null,
  "serial": "",
  "type": null,
  "width": 19,
  "u_height": 42,
  "desc_units": false,
  "comments": "",
  "custom_fields": {}
}

So the swagger definition that currently shows string for the field:

    "WritableRack": {
      "required": [
        "name",
        "site"
      ],
      "type": "object",
      "properties": {
        "id": {
          "title": "ID",
          "type": "integer",
          "readOnly": true
        },
        "name": {
          "title": "Name",
          "type": "string",
          "maxLength": 50
        },
        "facility_id": {
          "title": "Facility ID",
          "type": "string",
          "maxLength": 50
        },
        "site": {
          "title": "Site",
          "type": "integer"
        },
        "group": {
          "title": "Group",
          "type": "integer"
        },
        "tenant": {
          "title": "Tenant",
          "type": "integer"
        },
        "role": {
          "title": "Role",
          "type": "integer"
        },
        "serial": {
          "title": "Serial number",
          "type": "string",
          "maxLength": 50
        },
        "type": {
          "title": "Type",
          "type": "string",
          "enum": [
            100,
            200,
            300,
            1000,
            1100
          ]
        },
        "width": {
          "title": "Width",
          "description": "Rail-to-rail width",
          "type": "string",
          "enum": [
            19,
            23
          ]
        },
        "u_height": {
          "title": "Height (U)",
          "type": "integer",
          "maximum": 100,
          "minimum": 1
        },
        "desc_units": {
          "title": "Descending units",
          "description": "Units are numbered top-to-bottom",
          "type": "boolean"
        },
        "comments": {
          "title": "Comments",
          "type": "string"
        },
        "custom_fields": {
          "title": "Custom fields",
          "type": "string"
        }
      }
    }

Should show integer instead:

...
        "width": {
          "title": "Width",
          "description": "Rail-to-rail width",
          "type": "integer",
          "enum": [
            19,
            23
          ]
        },
...

There are also fields backed by booleans, which hopefully could be approached the same way.

I've already learned a lot about how this works by putting together this email. Please let me know if you have ideas about how to handle the custom field serializer, since that seems to be the root of the trouble.

@axnsan12
Copy link
Owner

axnsan12 commented Feb 23, 2018

In the read case, the fields are generated as flat strings because drf-yasg has no idea about how to handle your ChoiceFieldSerializer field class. The write case is caused by the hardcoding I talked about above. It could be solved inside drf-yasg by peeking inside the underlying model field to inspect its type, but it could prove a little too presumptuous.

For both cases I think you could solve it with a custom FieldInspector added to DEFAULT_FIELD_INSPECTORS. See also FieldInspector.

Probably something along the lines of (kinda-pseudo-code, meaning I didn't try to run it):

Read case:

class CustomChoiceFieldInspector(FieldInspector):
    def field_to_swagger_object(self, field, swagger_object_type, use_references, **kwargs):
        # this returns a callable which extracts title, description and other stuff for you
        # https://drf-yasg.readthedocs.io/en/stable/_modules/drf_yasg/inspectors/base.html#FieldInspector._get_partial_types
        SwaggerType, _ = self._get_partial_types(field, swagger_object_type, use_references, **kwargs)

        if isinstance(field, ChoiceFieldSerializer):  # <---- your custom choices field
             integer_choice_field = SwaggerType(type=openapi.TYPE_OBJECT, required=["label","value"], properties={"label": openapi.Schema(type=openapi.TYPE_STRING, read_only=True), "value": openapi.Schema(type=openapi.TYPE_INTEGER, read_only=True)})

             # if you want to make it a separate reference
             # if not, "return integer_choice_field" would do
             if use_references:
                 # self.components is a ReferenceResolver: https://drf-yasg.readthedocs.io/en/stable/drf_yasg.html#drf_yasg.openapi.ReferenceResolver
                 definitions = self.components.with_scope(openapi.SCHEMA_DEFINITIONS)
                 definitions.setdefault("IntegerChoiceField", lambda: integer_choice_field)
                 return openapi.SchemaRef(definitions, "IntegerChoiceField)
             else:
                 return integer_choice_field
        

        return NotHandled

The write case is more complicated, you would need a way to discern your ChoiceFields from regular ChoiceFields, perhaps by switching on field.parent and field.name?

   ... same class as before
   def process_result(self, result, method_name, field, **kwargs):
         if <somehow figure out which fields you want to change>:
               result.type = <whatever you want>

         return result

@davcamer
Copy link
Author

Thanks. I was able to get a full client generated using your suggestions for custom field inspectors. I'll close this for now, and link the inspectors if we end up continuing with it.

axnsan12 added a commit that referenced this issue Feb 27, 2018
@axnsan12
Copy link
Owner

Hello!

The second part of your issue should be fixed with 1.4.4.

@davcamer
Copy link
Author

That's great! Thanks for following up on that.

@davcamer
Copy link
Author

Slightly unexpected but not bad result for boolean fields:

        "connection_status": {
          "title": "Connection status",
          "type": "boolean",
          "enum": [
            false,
            true
          ]
        }

The enum indicating true and false are the valid values seems extraneous? But the tool I'm using hasn't had trouble with it.

@davcamer
Copy link
Author

The PR to netbox that result from this, if you're interested:
https://github.com/digitalocean/netbox/pull/1930/files

@axnsan12
Copy link
Owner

axnsan12 commented Mar 1, 2018

The true/false enum does seem a bit extraneous, but I would guess it comes from a model BooleanField with choices=(True, False)? It could be argued that those are extraneous 😄

The netbox PR is interesting indeed!

@axnsan12 axnsan12 added enhancement Enhancement proposal question Open question labels Aug 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement proposal question Open question
Projects
None yet
Development

No branches or pull requests

2 participants