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

enable the use of Python enums in custom forms #391

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

sinisaos
Copy link
Member

@sinisaos sinisaos commented May 3, 2024

Related to #386

@codecov-commenter
Copy link

codecov-commenter commented May 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.66%. Comparing base (1157c12) to head (8199ca3).
Report is 5 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #391      +/-   ##
==========================================
+ Coverage   93.42%   93.66%   +0.24%     
==========================================
  Files           5        6       +1     
  Lines         365      379      +14     
==========================================
+ Hits          341      355      +14     
  Misses         24       24              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines 441 to 457
def custom_form_choices() -> t.Dict[str, t.Any]:
choices = {}
for item in Permission:
choices[item.name] = {
"display_name": item.name,
"value": item.value,
}
return choices


class NewStaff(BaseModel):
name: str
email: EmailStr
superuser: bool
permissions: t.Any = Field(
json_schema_extra={"extra": {"choices": custom_form_choices()}}
)
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a clever workaround.

My concern though is it exposes a lot of internals.

How about modifying FormConfig instead? We could add an argument called choices.

FormConfig("My Form", MyModel, choices={"some_field": SomeEnum})

We could do something similar with help_text etc.

Or we could give the user the option of passing in a Pydantic model, or a custom class like this:

class MyModel(PiccoloAdminForm):
    name = Field(str, choices=SomeEnum, help_text="...")

FormConfig("My Form", MyModel, choices={"some_field": SomeEnum})

Which we then turn into a Pydantic model.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was just trying to mimic the Piccolo column choices (just converting the enum to a dict with a display_name and a value that the admin understands how to render) because the Piccolo Admin doesn't understand references through "$ref" which is usually used to represent an enum or other complex type inside a Pydantic model (which was asked in the issue). I'm sorry, but I can't say that I fully understand and know how to implement your suggested changes. Feel free to implement the changes you think are best.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking we would use your logic, but rather than making the user do it, instead it would be done automatically by FormConfig in the __init__ method.

Something like:

class FormConfig:
    def __init__(model, choices: dict, ...):
        for field_name, enum in choices.items():
            model.fields['field_name'].json_schema_extra["extra"]["choices"] = convert_enum_to_choices(enum)

The code above is pseudo code - I haven't tested it.

It would be used like this:

FormConfig(MyModel, choices={"my_column": MyEnum})

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried your suggestion. I was able to dynamically add a choices field to a Pydantic model like this

def convert_enum_to_choices(enum_data: t.Any) -> t.Dict[str, t.Any]:
    choices = {}
    for item in enum_data:
        choices[item.name] = {
            "display_name": item.name,
            "value": item.value,
        }
    return choices

class FormConfig:
    def __init__(pydantic_model, choices: t.Optional[t.Dict[str, t.Any]] = None, ...):
        self.pydantic_model = pydantic_model
        self.choices = choices
        if choices is not None:
            for field_name, field_value in choices.items():
                # update annotations
                pydantic_model.__annotations__.update({field_name: t.Any})
                # update model_fields
                pydantic_model.model_fields[field_name]: t.Any = Field(
                    json_schema_extra={
                        "extra": {
                            "choices": convert_enum_to_choices(field_value)
                        }
                    },
                )
                # update model_field annotation
                pydantic_model.model_fields[field_name].annotation = t.Any
                # if we print the fields, they are the same as when we define the permissions field in the NewStaff model as in example.py
                print(pydantic_model.fields) 

but the choices select is not displayed in the frontend form.

Copy link
Member

Choose a reason for hiding this comment

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

Does choices not appear in the schema in the endpoint?

Copy link
Member Author

@sinisaos sinisaos May 6, 2024

Choose a reason for hiding this comment

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

I had to use model_rebuild for the changes (dynamically adding choices in the Pydantic model) to take effect. Everything works now. The code is like this

class FormConfig:
    def __init__(pydantic_model, choices: t.Optional[t.Dict[str, t.Any]] = None, ...):
        self.pydantic_model = pydantic_model
        self.choices = choices
        if choices is not None:
           for field_name, field_value in choices.items():
              # update model_fields
              pydantic_model.model_fields[field_name] = Field(
                  json_schema_extra={
                      "extra": {
                          "choices": convert_enum_to_choices(field_value)
                      }
                  },
              )
              # update model_field annotation
              pydantic_model.model_fields[field_name].annotation = str
              # rebuild the model for the changes to take effect
              pydantic_model.model_rebuild(force=True)

Copy link
Member

Choose a reason for hiding this comment

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

Good catch. Is it necessary to set the annotation? Or can we just keep what the user defined?

pydantic_model.model_fields[field_name].annotation = str

Copy link
Member Author

Choose a reason for hiding this comment

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

We have to do this because otherwise the field annotation is None and validation fails when submitting the form. I made a new commit with the changes.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I see. Can we use the annotation that the user originally set instead of str, because the Enum might be something else, like an int.

Do you think there's any change we'll be able to handle enums natively as they appear in OpenAPI? I haven't looked into it much, but this is the only caution I have around this now.

I definitely do want to use this approach for help_text though - I think that will be super useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I see. Can we use the annotation that the user originally set instead of str, because the Enum might be something else, like an int.

I'm sorry, but I don't understand. The user does not set any annotation because the model field is dynamically added to the Pydantic model and we need to set the annotation otherwise the annotation is NoneType. If you mean the enum type we can use any type like this

pydantic_model.model_fields[field_name].annotation = t.Any # type: ignore

but the result is the same as we use str.

Do you think there's any change we'll be able to handle enums natively as they appear in OpenAPI? I haven't looked into it much, but this is the only caution I have around this now.

If you want to go with the native OpenAPI approach, that would be best, but Piccolo Admin or Piccolo API (create_pydantic_model) doesn't use complex references and for now uses choices which I try to mimic..

If you want to go with different approach I'll close this PR. I think this has become too complex. My first intention was to somehow allow users to use enums in custom forms (this is made possible by adding a choices to the InputField), but write custom code to use that enum. I gave an example of how to do it. After that, the choices argument is added to the FormConfig, with all the logic behind it, so the user doesn't have to do anything but just add the enum to the choices in the FormConfig constructor and it works. Sorry for the long comment.

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

Successfully merging this pull request may close these issues.

None yet

3 participants