Expose pydantic config class to user

[waldner]

create_pydantic_model accepts a new argument specifying the base class
to use for the generated model config.

Should partially fix #467.

[codecov-commenter]

Codecov Report

Merging #705 (4959544) into master (eb6ba32) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #705   +/-   ##
=======================================
  Coverage   91.29%   91.30%           
=======================================
  Files          99       99           
  Lines        7238     7244    +6     
=======================================
+ Hits         6608     6614    +6     
  Misses        630      630           

Impacted Files	Coverage Δ
piccolo/utils/pydantic.py	98.07% <100.00%> (+0.11%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[dantownsend]

@waldner Thanks a lot for this - it looks good.

I need to check if passing the choices to the Pydantic model has any effect on Piccolo Admin. If so, we might just have to add an option - like include_choices, so we can turn it off for Piccolo Admin.

@sinisaos I don't know if this is something we've tested before - do you remember?

[sinisaos]

@dantownsend I only remember this PR and the integer validation didn't work. I hope this PR fixes that. We probably need to make changes to Piccolo Admin as well.
UPDATE:
I tried these new changes in pydantic.py with Piccolo Admin. My comment from #469 is still valid. With comment changes strings works in Admin, but the integer doesn't due to a validation error.

[waldner]

My understanding is that piccolo admin was using the choices field of the extra attribute of the schema (as per #467 (comment)), so I did not touch that in this patch. But I don't know the internals of piccolo admin.

[dantownsend]

@waldner You're right - Piccolo Admin just uses the extra attribute to decide what to the render in the UI.

I'd better check quickly though, as having stronger validation on the Pydantic models might cause Piccolo Admin to fail in some way when posting back data.

@sinisaos Thanks for looking into it.

[waldner]

So FWIW, it does indeed seem that piccolo admin has trouble with the new changes.

Using the following table schema:

class MyStrChoices(str, Enum):
    choice1 = 'choice1'
    choice2 = 'choice2'

class MyIntChoices(int, Enum):
    choice1 = 1
    choice2 = 6

class Task(Table):
    """
    An example table.
    """

    name = Varchar()
    completed = Boolean(default=False)
    stringcol = Varchar(choices=MyStrChoices)
    integercol = Integer(choices=MyIntChoices)

When I try to add a new row to the table using piccolo admin, no fields are shown and the following error is produced in the python interpreter (only the relevant part):

...
  File "pydantic/main.py", line 342, in pydantic.main.BaseModel.__init__
pydantic.error_wrappers.ValidationError: 2 validation errors for TaskOptional
stringcol
  value is not a valid enumeration member; permitted: 'choice1', 'choice2' (type=type_error.enum; enum_values=[<MyStrChoices.choice1: 'choice1'>, <MyStrChoices.choice2: 'choice2'>])
integercol
  value is not a valid enumeration member; permitted: 1, 6 (type=type_error.enum; enum_values=[<MyIntChoices.choice1: 1>, <MyIntChoices.choice2: 6>])
Server error

presumably because piccolo admin would set those fiels as empty initially, or with a placeholder value not among the allowed choices. What about setting it to the first value of the enumeration?

EDIT: in fact the error appears to come from piccolo-api:

  File "/home/cz/venv/lib/python3.10/site-packages/piccolo_api/fastapi/endpoints.py", line 181, in new
    return await piccolo_crud.get_new(request=request)
  File "/home/cz/venv/lib/python3.10/site-packages/piccolo_api/crud/validators.py", line 129, in inner_coroutine_function
    return await function(*args, **kwargs)
  File "/home/cz/venv/lib/python3.10/site-packages/piccolo_api/crud/endpoints.py", line 870, in get_new
    self.pydantic_model_optional(**row_dict).json()
  File "pydantic/main.py", line 342, in pydantic.main.BaseModel.__init__
pydantic.error_wrappers.ValidationError: 2 validation errors for TaskOptional
...[same as before]

[waldner]

If the propagation of choices to pydantic proves to be too complicated, for now I can rollback that part of the PR and only keep the extra policy part, which should not be problematic at all.

[sinisaos]

@waldner As @dantownsend already said, Piccolo admin uses an extra attribute for rendering in UI and in it uses

piccolo/piccolo/columns/base.py

Line 217 in 94c55f8

def get_choices_dict(self) -> t.Optional[t.Dict[str, t.Any]]:

whose output is the display_name to nice display in the select dropdown and the value it uses as data.

"choices":{
   "male":{
      "display_name":"Male", <-nice display in the select dropdown
      "value":"m" <- data
   },
   "female":{
      "display_name":"Female",
      "value":"f"
   },
   "non_binary":{
      "display_name":"Non Binary",
      "value":"n"
   }
},

I think with your changes we would have to use schema definitions for choices, but we need to see how or ignore it for Piccolo Admin.

[dantownsend]

It would be great if Piccolo Admin worked properly with Pydantic choices, but I'm not sure how much work it will be currently.

So maybe lets move it out of this PR, and into another one, so we can get this merged in.

[waldner]

Done.

[dantownsend]

Sorry - one minor thing. Can we make this:

pydantic_extra_fields: t.Literal["ignore", "allow", "forbid"] = "ignore",

[waldner]

Originally I actually wanted to do this:

pydantic_extra_fields: pydantic.Extra = pydantic.Extra.ignore

Is there a stylistic or other reason for your suggested change? (I'm really curious, not trying to be confrontational, I'll implement whatever you decide)

[dantownsend]

Yeah, that works too:

pydantic_extra_fields: pydantic.Extra = pydantic.Extra.ignore

I don't think there's clearly a best. The reason I suggested Literal is it means a linter can detect typos in the string.

I think the Enum is probably better though. It's slightly more verbose to use, but means that if Pydantic adds another option in the future, we don't have to update Literal to include the new options.

Ideally it would be good to expose all of Pydantic's config options. We could take a different approach entirely, and do this:

def create_pydantic_model(pydantic_config: t.Type[BaseConfig] = BaseConfig, ...):
    ...
    class CustomConfig(pydantic_config):
        schema_extra = {
            "help_text": table._meta.help_text,
            **schema_extra_kwargs,
        }
        extra = pydantic.Extra(pydantic_extra_fields)
        json_encoders: t.Dict[t.Any, t.Callable] = JSON_ENCODERS
        arbitrary_types_allowed = True

Which means that every Pydantic option is exposed to the user. What do you think?

[waldner]

Looks good to me . I'll work on this.

[waldner]

I see there are quite a few places in piccolo-api that import Config from utils/pydantic, IIUC that class would now go away since we'd now be instantiating just CustomConfig based on pydantic_config, shall I (try to) adjust those other places too?

[dantownsend]

Ah, yeah - good point. I wonder if we keep Config, then this is possible?

def create_pydantic_model(pydantic_config: t.Type[BaseConfig] = BaseConfig, ...):
    ...
    class CustomConfig(Config, pydantic_config):
        schema_extra = {
            "help_text": table._meta.help_text,
            **schema_extra_kwargs,
        }
        extra = pydantic.Extra(pydantic_extra_fields)

I think the mro still works, even though both classes inherit from BaseConfig.

[waldner]

Sorry for being a PITA, but now I wonder what we should do with the original pydantic_extra_fields parameter, as now the user could just do

class MyConfig(pydantic.BaseConfig):
    extra = 'forbid'

model = create_pydantic_model(pydantic_config=MyConfig, ...)

and obtain the same result as passing pydantic_extra_fields='forbid'. I don't have a strong opinion on this, although using pydantic_extra_fields seems marginally easier. Comments?

[dantownsend]

We probably don't need them both. I agree that having a pydantic_extra_fields argument is more convenient. But over time, as we support more Pydantic options, create_pydantic_model could end up with loads of arguments.

We could accept a dict, and anything passed into it automatically applied to the config (rather than having to pass in a config object):

create_pydantic_model(pydantic_config={'extra': Extra.ignore})

It's more convenient, but there's no type checking. We could use TypedDict, but would constantly be updating it as Pydantic adds more arguments.

Sorry, I'm kind of thinking out loud here ... what do you think? I think passing in the Config is probably best.

[dantownsend]

@waldner This is great, thanks! Sorry about all of the back and forth. I think we have a nice design.