piccolo icon indicating copy to clipboard operation
piccolo copied to clipboard

Choices enum not reflected in Swagger

Open smythp opened this issue 2 years ago • 4 comments

I'm using Piccolo with FastAPI, which generates documentation in Swagger. In my tables file, I've created a table (relationship) that has a field based on an enum

    class RelationshipType(str, enum.Enum):
        colleague = "colleague"
        friend = "friend"
        ...

    type= Varchar(choices=RelationshipType)

When I use create_pydantic_model to generate a pydantic model based on the table, I had expected the "choices" field to get passed along to Swagger, but that doesn't seem to be the case. Swagger treats it as a vanilla string.

I dumped the schemas for both a Pydantic model created directly that uses an enum and a Pydantic model created using the create_pydantic_model method, and they're laid out pretty differently. I'll add them at the end.

Do you have any guidance on how to get the enum created in piccolo tables to get reflected in Swagger? Do I have to drop down and create Pydantic models directly if I'd like this functionality?

# Direct Pydantic model 
model.construct().schema()
{'title': 'RelationshipTest', 'type': 'object', 'properties': {'class_1': {'title': 'Class 1', 'type': 'integer'}, 'class2': {'title': 'Class2', 'type': 'integer'}, 'type': {'$ref': '#/definitions/RelationshipType'}}, 'required': ['class_1', 'class2', 'type'], 'definitions': {'RelationshipType': {'title': 'RelationshipType', 'description': 'An enumeration.', 'enum': ['colleague', 'friend', 'relative'], 'type': 'string'}}}
# Creating a model from Piccolo table
model.construct().schema()
{'title': 'RelationshipIn', 'type': 'object', 'properties': {'first_contact': {'title': 'First Contact', 'extra': {'foreign_key': True, 'to': 'contact', 'target_column': 'id', 'help_text': None, 'choices': None}, 'nullable': True, 'type': 'integer'}, 'second_contact': {'title': 'Second Contact', 'extra': {'foreign_key': True, 'to': 'contact', 'target_column': 'id', 'help_text': None, 'choices': None}, 'nullable': True, 'type': 'integer'}, 'type': {'title': 'Type', 'extra': {'help_text': None, 'choices': {'colleague': {'display_name': 'Colleague', 'value': 'colleague'}, 'friend': {'display_name': 'Friend', 'value': 'friend'}, 'relative': {'display_name': 'Relative', 'value': 'relative'}, 'partner': {'display_name': 'Partner', 'value': 'partner'}, 'ex': {'display_name': 'Ex', 'value': 'ex'}, 'child': {'display_name': 'Child', 'value': 'child'}, 'teacher': {'display_name': 'Teacher', 'value': 'teacher'}, 'student': {'display_name': 'Student', 'value': 'student'}}}, 'nullable': False, 'maxLength': 255, 'type': 'string'}}, 'help_text': None}

smythp avatar Mar 24 '22 03:03 smythp

@smythp Thanks for reporting this issue. You're right, the column choices don't currently reflect properly in the Pydantic schema.

If we have the following table:

class RelationshipType(str, enum.Enum):
    colleague = "colleague"
    friend = "friend"

class Person(Table):
    type = Varchar(choices=RelationshipType)    

It should becomes this Pydantic model:

class PersonModel(BaseModel):
    type: RelationshipType

The code responsible is here:

https://github.com/piccolo-orm/piccolo/blob/52f01aebbea0e1e1d22d4ba8decae6749ccd7dee/piccolo/utils/pydantic.py#L210-L229

If the column has defaults provided, then the value_type should be the choices enum.

You'll notice that we do add the choices under the extra field:

https://github.com/piccolo-orm/piccolo/blob/52f01aebbea0e1e1d22d4ba8decae6749ccd7dee/piccolo/utils/pydantic.py#L241-L244

This is for Piccolo Admin. In retrospect it would have made more sense to implement choices in the official Pydantic way - it was probably just ignorance at the time about this particular Pydantic feature.

I think it would be good to make this change (i.e. changing the value_type to an Enum, but also keeping the choices under extra for backwards compatibility). We just need to do some testing to make sure Piccolo Admin doesn't break for some reason.

If it's not possible to do it in a backwards compatible way, we have been considering an alternative API to create_pydantic_model (see https://github.com/piccolo-orm/piccolo/issues/331), where we can tidy up these issues.

In the immediate term, defining a Pydantic model by hand is the best workaround until we get this issue fixed.

dantownsend avatar Mar 24 '22 09:03 dantownsend

One other workaround for now, instead of defining the entire Pydantic model from scratch, you can override some of the fields:

PersonModel = create_pydantic_model(table=Person)

class PersonModelExtended(PersonModel):
    type: RelationshipType # you can just override certain fields

dantownsend avatar Mar 24 '22 11:03 dantownsend

@dantownsend That workaround isn't too onerous. Glad I wasn't missing anything , and I'll keep an eye on the issue. :)

smythp avatar Mar 24 '22 16:03 smythp

@smythp Cool, thanks

dantownsend avatar Mar 25 '22 05:03 dantownsend

Not sure it's the right place, but another minor annoyance I've come across when generating pydantic models from Table is that there's no way to specify that extra fields are forbidden (ie extra = 'forbid' in pydantic's Config subclass). This could perhaps be an extra option to the create_pydantic_model function (apologies if it's already there; I wasn't able to find it)

The current solution is to manually (re)define the pydantic model adding the Config subclass, but I think there's room for improvement.

waldner avatar Dec 02 '22 18:12 waldner

Not sure it's the right place, but another minor annoyance I've come across when generating pydantic models from Table is that there's no way to specify that extra fields are forbidden (ie extra: 'forbid' in pydantic's Config subclass). This could perhaps be an extra option to the create_pydantic_model function (apologies if it's already there; I wasn't able to find it)

I haven't used that feature of Pydantic before, but I can see it being useful.

It seems like extra: allow is the default behaviour.

We can definitely add this feature - just needs an extra argument to create_pydantic_model.

dantownsend avatar Dec 02 '22 19:12 dantownsend