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           

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

@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?

@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.

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.

@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.

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]

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.

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

"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.

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.

Done.

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