Admin/CRUD: Unfriendly 500 Error if field is empty but required

[shattl]

Hi! We get this red message in Admin panel while adding model entity:

"Error Internal server error"

I wish there would be message in front of the field saying this field is required instead of some abstract server error

In the other case if saving with all empty fields, message is like this: Traceback (most recent call last): File "/usr/local/lib/python3.10/site-packages/anyio/streams/memory.py", line 94, in receive return self.receive_nowait() File "/usr/local/lib/python3.10/site-packages/anyio/streams/memory.py", line 89, in receive_nowait raise WouldBlock anyio.WouldBlock During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/usr/local/lib/python3.10/site-packages/starlette/middleware/base.py", line 77, in call_next message = await recv_stream.receive() File "/usr/local/lib/python3.10/site-packages/anyio/streams/memory.py", line 114, in receive raise EndOfStream anyio.EndOfStream During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/usr/local/lib/python3.10/site-packages/starlette/middleware/errors.py", line 162, in __call__ await self.app(scope, receive, _send) File "/usr/local/lib/python3.10/site-packages/starlette/middleware/exceptions.py", line 79, in __call__ raise exc File "/usr/local/lib/python3.10/site-packages/starlette/middleware/exceptions.py", line 68, in __call__ await self.app(scope, receive, sender) File "/usr/local/lib/python3.10/site-packages/fastapi/middleware/asyncexitstack.py", line 21, in __call__ raise e File "/usr/local/lib/python3.10/site-packages/fastapi/middleware/asyncexitstack.py", line 18, in __call__ await self.app(scope, receive, send) File "/usr/local/lib/python3.10/site-packages/starlette/routing.py", line 706, in __call__ await route.handle(scope, receive, send) File "/usr/local/lib/python3.10/site-packages/starlette/routing.py", line 443, in handle await self.app(scope, receive, send) File "/usr/local/lib/python3.10/site-packages/fastapi/applications.py", line 270, in __call__ await super().__call__(scope, receive, send) File "/usr/local/lib/python3.10/site-packages/starlette/applications.py", line 124, in __call__ await self.middleware_stack(scope, receive, send) File "/usr/local/lib/python3.10/site-packages/starlette/middleware/errors.py", line 174, in __call__ response = await self.handler(request, exc) File "/usr/local/lib/python3.10/site-packages/piccolo_admin/endpoints.py", line 331, in log_error raise exc File "/usr/local/lib/python3.10/site-packages/starlette/middleware/errors.py", line 162, in __call__ await self.app(scope, receive, _send) File "/usr/local/lib/python3.10/site-packages/starlette/middleware/base.py", line 106, in __call__ response = await self.dispatch_func(request, call_next) File "/usr/local/lib/python3.10/site-packages/piccolo_api/csrf/middleware.py", line 179, in dispatch return await call_next(request) File "/usr/local/lib/python3.10/site-packages/starlette/middleware/base.py", line 80, in call_next raise app_exc File "/usr/local/lib/python3.10/site-packages/starlette/middleware/base.py", line 69, in coro await self.app(scope, receive_or_disconnect, send_no_error) File "/usr/local/lib/python3.10/site-packages/starlette/middleware/exceptions.py", line 79, in __call__ raise exc File "/usr/local/lib/python3.10/site-packages/starlette/middleware/exceptions.py", line 68, in __call__ await self.app(scope, receive, sender) File "/usr/local/lib/python3.10/site-packages/fastapi/middleware/asyncexitstack.py", line 21, in __call__ raise e File "/usr/local/lib/python3.10/site-packages/fastapi/middleware/asyncexitstack.py", line 18, in __call__ await self.app(scope, receive, send) File "/usr/local/lib/python3.10/site-packages/starlette/routing.py", line 706, in __call__ await route.handle(scope, receive, send) File "/usr/local/lib/python3.10/site-packages/starlette/routing.py", line 443, in handle await self.app(scope, receive, send) File "/usr/local/lib/python3.10/site-packages/starlette/middleware/authentication.py", line 48, in __call__ await self.app(scope, receive, send) File "/usr/local/lib/python3.10/site-packages/fastapi/applications.py", line 270, in __call__ await super().__call__(scope, receive, send) File "/usr/local/lib/python3.10/site-packages/starlette/applications.py", line 124, in __call__ await self.middleware_stack(scope, receive, send) File "/usr/local/lib/python3.10/site-packages/starlette/middleware/errors.py", line 174, in __call__ response = await self.handler(request, exc) File "/usr/local/lib/python3.10/site-packages/piccolo_admin/endpoints.py", line 331, in log_error raise exc File "/usr/local/lib/python3.10/site-packages/starlette/middleware/errors.py", line 162, in __call__ await self.app(scope, receive, _send) File "/usr/local/lib/python3.10/site-packages/starlette/middleware/exceptions.py", line 79, in __call__ raise exc File "/usr/local/lib/python3.10/site-packages/starlette/middleware/exceptions.py", line 68, in __call__ await self.app(scope, receive, sender) File "/usr/local/lib/python3.10/site-packages/fastapi/middleware/asyncexitstack.py", line 21, in __call__ raise e File "/usr/local/lib/python3.10/site-packages/fastapi/middleware/asyncexitstack.py", line 18, in __call__ await self.app(scope, receive, send) File "/usr/local/lib/python3.10/site-packages/starlette/routing.py", line 706, in __call__ await route.handle(scope, receive, send) File "/usr/local/lib/python3.10/site-packages/starlette/routing.py", line 276, in handle await self.app(scope, receive, send) File "/usr/local/lib/python3.10/site-packages/starlette/routing.py", line 66, in app response = await func(request) File "/usr/local/lib/python3.10/site-packages/fastapi/routing.py", line 235, in app raw_response = await run_endpoint_function( File "/usr/local/lib/python3.10/site-packages/fastapi/routing.py", line 161, in run_endpoint_function return await dependant.call(**values) File "/usr/local/lib/python3.10/site-packages/piccolo_api/fastapi/endpoints.py", line 280, in post return await piccolo_crud.root(request=request) File "/usr/local/lib/python3.10/site-packages/piccolo_api/crud/endpoints.py", line 513, in root return await self.post_single(request, data) File "/app/apps/admin/patches.py", line 48, in inner_coroutine_function return await func(*args, **kwargs) File "/usr/local/lib/python3.10/site-packages/piccolo_api/crud/exceptions.py", line 52, in inner return await func(*args, **kwargs) File "/usr/local/lib/python3.10/site-packages/piccolo_api/crud/endpoints.py", line 819, in post_single response = await row.save().run() File "/usr/local/lib/python3.10/site-packages/piccolo/query/base.py", line 203, in run results = await engine.run_querystring( File "/usr/local/lib/python3.10/site-packages/piccolo/engine/postgres.py", line 458, in run_querystring return await self._run_in_pool(query, query_args) File "/usr/local/lib/python3.10/site-packages/piccolo/engine/postgres.py", line 423, in _run_in_pool response = await connection.fetch(query, *args) File "/usr/local/lib/python3.10/site-packages/asyncpg/connection.py", line 621, in fetch return await self._execute( File "/usr/local/lib/python3.10/site-packages/asyncpg/connection.py", line 1659, in _execute result, _ = await self.__execute( File "/usr/local/lib/python3.10/site-packages/asyncpg/connection.py", line 1684, in __execute return await self._do_execute( File "/usr/local/lib/python3.10/site-packages/asyncpg/connection.py", line 1731, in _do_execute result = await executor(stmt, None) File "asyncpg/protocol/protocol.pyx", line 201, in bind_execute asyncpg.exceptions.NotNullViolationError: null value in column "model" of relation "car" violates not-null constraint DETAIL: Failing row contains (6293232c-58db-4879-9b66-e9f591c8adf9, null, 0, , , 0.000000, 0.000000, null, , 0.00, , 0, null, 0.00, 0.00, 0.00, 5, 4, t, 0).

[dantownsend]

@shattl Yeah, I came across a similar issue recently.

If you upgrade to the latest version of piccolo_api (0.50.0), it should catch this exception and show something more useful in the admin UI.

We currently catch a few asyncpg errors, but will add more over time:

https://github.com/piccolo-orm/piccolo_api/blob/20252be6e7d8bc14097ae3da8d56d8d8ef743081/piccolo_api/crud/exceptions.py#L53-L79

[shattl]

@dantownsend There should be no request at all, I mean, not only the DB query, but even request "FRONT->BACK", and here are reasons for that:

here I am talking about not filling the fields by user -- and this fields we already know are required as we mark fields "null=False" in the model and this information is available to all admin backend and could be available to admin frontend

IF the admin front-end would be look after required fields, server never gets this erroneous request in the normal conditions

-- nevertheless, catching DB errors is also a good practice

[dantownsend]

It would be better if the front end caught it.

If you add required=True to your column:

class MyTable(Table):
    my_column = Varchar(required=True)

Then the Pydantic model should catch that it wasn't provided. That way it shouldn't reach the database.

We can look at adding some validation to the front end.

[sinisaos]

If you add required=True to your column:

class MyTable(Table):
    my_column = Varchar(required=True)

Then the Pydantic model should catch that it wasn't provided. That way it shouldn't reach the database.

@dantownsend Unfortunately (I hope I'm doing something wrong), Piccolo Admin currently doesn't handle required fields at all. Haven't we already solved this?

If you add a required field to the Ticket table in the demo app (booked_by = Varchar(length=255, required=True), you can save an empty field without a problem, which is wrong (probably Pydantic treats an empty string as a valid input). Can you confirm this?

We could fix this on frontend by adding an HTML required field to InputField.vue if the field is in the Pydantic schemas required list, or on backend if we convert empty string to None.

[dantownsend]

@sinisaos The problem could just be Varchar() - Pydantic sees the value as an empty string, which it accepts. Try it with something like an Integer and see what happens.

[sinisaos]

@dantownsend Yes. The problem is Varchar() because the default is an empty string, and Pydantic treats this as valid input. Boolean() or Integer() has a default value of False or 0 and there is no problem.

[dantownsend]

@sinisaos We might have to make it so when required=True an empty string isn't allowed.

[shattl]

There is a Django way to save a few man-years )

never consider empty string as valid value

(for Numeric if used "default=None" option, admin panel says "value is not a valid decimal", but should say "field required" instead and even not start validation process // no matter decimal or non-decimal, it's empty)

[sinisaos]

(for Numeric if used "default=None" option, admin panel says "value is not a valid decimal", but should say "field required" instead and even not start validation process // no matter decimal or non-decimal, it's empty)

@shattl Piccolo Admin uses Pydantic for validation and in this case it is the correct behavior (at least that's how I understand it, if I'm wrong, sorry) and for Pydantic None value is not a valid decimal.

@dantownsend This can be done by writing a custom validator for the empty string in pydantic.py, Something like this.

# piccolo orm pydantic.py
def pydantic_empty_string_validator(cls, value):
    if value == "":
        raise ValueError("Required field cannot be empty")
    return value

# and later use validator
elif isinstance(column, Varchar):
  if not column._meta.required:
      value_type = pydantic.constr(max_length=column.length)
  else:
      value_type = column.value_type
      validators[
          f"{column_name}_is_empty_string"
      ] = pydantic.validator(column_name, allow_reuse=True)(
          pydantic_empty_string_validator
      )

# in test_pydantic.py
def test_varchar_required(self):
    class Director(Table):
        name = Varchar(length=10, required=True)

    pydantic_model = create_pydantic_model(table=Director)

    with self.assertRaises(ValidationError):
        pydantic_model(name="")

    pydantic_model(name="non-empty field")

We can also do frontend validation if you want with the HTML required tag (although it can be bypassed, so I don't think it makes much sense). In Piccolo Admin it works and returns booked_by field - Required field cannot be empty if field is empty.