Database error text not being shown in admin UI #167

Activity

1. dantownsend commented on Jun 13, 2022

   

   dantownsend

   on Jun 13, 2022

   Last edited by dantownsend

   Member

   More actions

   Unique constraints aren't handled well currently - it should return a proper error message, as you say.

   Most of the Piccolo Admin backend is built upon a class called PiccoloCRUD which maps HTTP methods to SQL queries. We rely heavily on Pydantic for input validation. But Pydantic can't validate unique constraints - we need to rely on the database. This is the relevant method on PiccoloCRUD.

   We need to catch the database errors, and returns something more useful.
2. sinisaos commented on Jun 14, 2022

   

   sinisaos

   on Jun 14, 2022

   Last edited by sinisaos

   Member

   More actions

   We can use generic exception to catch database errors on post_single, put_single and patch_single in Piccolo API crud endpoints like this:

   except Exception as e:
       return Response(
           f"Unable to save the resource: {e}", status_code=400
       )

   Result is
   

   or in Postgres with biggest alert box

   
3. dantownsend commented on Jun 14, 2022

   

   dantownsend

   on Jun 14, 2022

   Member

   More actions

   @sinisaos Thanks, that could work.

   Ideally we would be able to just catch certain exceptions, for example unique errors. There's a PR open in Piccolo, but I haven't finished it yet. The idea is it will map the asyncpg or aiosqlite exceptions into generic Piccolo exceptions (for example UniqueException). For now though, this might be our best option.
4. sinisaos commented on Jun 14, 2022

   

   sinisaos

   on Jun 14, 2022

   Member

   More actions

   @dantownsend Like you said that would be ideal because they would have the same error message for different db drivers. Once you finish with Piccolo PR you can mention me in the comments and I can do PR in Piccolo API and Piccolo Admin to implement this feature. Something like this

   from piccolo.engine.exceptions import UniqueConstraintError # or different name of exception
   ...
   except UniqueConstraintError as exception:
       return Response(
           f"Unable to save the resource: {exception}", status_code=400
       )
   ...
5. dantownsend commented on Jun 14, 2022

   

   dantownsend

   on Jun 14, 2022

   Last edited by dantownsend

   Member

   More actions

   @sinisaos Cool, thanks - that looks good.

   If I don't manage to get that PR closed soon, we'll go with your original solution.

   👍React with 👍1Reacted by sinisaos
6. ethagnawl commented on Jun 14, 2022

   

   ethagnawl

   on Jun 14, 2022

   ContributorAuthor

   More actions

   Thanks for the prompt response, @sinisaos and @dantownsend! Any of the above would make for a much better UX.

   👍React with 👍2Reacted by sinisaos and Daniel Townsend
7. dantownsend commented on Sep 1, 2022

   

   dantownsend

   on Sep 1, 2022

   Last edited by dantownsend

   Member

   More actions

   Sorry, progress on this stalled. Going with something similar to @sinisaos solution (piccolo-orm/piccolo_api#187). It will now show a proper error message if a unique constraint fails rather than just a 500.
8. dantownsend

   added

   enhancementNew feature or requestNew feature or request

   on Sep 2, 2022
9. dantownsend commented on Sep 2, 2022

   

   dantownsend

   on Sep 2, 2022

   Last edited by dantownsend

   Member

   More actions

   In piccolo_api>=0.47.0 this should be fixed.

   I'll create some follow up tasks, as there's still more we can do with error messages in Piccolo Admin.

   🎉React with 🎉1Reacted by Pete Doherty
10. dantownsend

    closed this as completedon Sep 2, 2022