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.

Sign up for GitHub

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

Jump to bottom

Database errors #59

Closed
wants to merge 2 commits into from
Closed

Database errors #59

wants to merge 2 commits into from

Conversation

aguillon
Copy link
Collaborator

@aguillon aguillon commented Oct 30, 2022 •
edited
Loading

Hi!

I'd like to fix #57 but I'm not quite sure what we should display to the user instead. I am not yet familiar with Caqti errors, but I believe the actual error message

ERROR: invalid input syntax for type uuid: "03c17d2e-6e82-4ef3-9348-g714a129001"

could help them know what they did wrong. Should I display it?

To reproduce the error, try using an incorrect UID, e.g., http://localhost:4000/topic/show/03c17d2e-6e82-4ef3-9348-g714a1290019 (character g is invalid here).

Edit : also, just to be sure: I don't need to add any dependency to the .opam file, as Str is already in Stdlib, right? And CI is failing on this because I seem to have an issue with ocamlformat at the moment.

@aguillon aguillon requested a review from gr-im October 30, 2022 21:34
@gr-im
Copy link
Collaborator

gr-im commented Oct 31, 2022 •
edited
Loading

I think this is the right fix, but why make a replace?
As a side note, I'm not really a fan of extending the standard library in this way, I find it makes the documentation inconsistent across versions: see here. I think the function replace can be put into an helper module for example.

@aguillon
Copy link
Collaborator Author

aguillon commented Oct 31, 2022 •
edited
Loading

Thank you for your answer.

I think this is the right fix, but why make a replace?

I don't know what @xvw uses in production (probably nothing fancy) but Caqti errors come with \n characters and I personally dislike having newlines in my logs.

I think the function replace can be put into an helper module for example.

Ok, I'll change this.

After a good night sleep and reading the Caqti_error module, I think we can provide better error messages to the user. I'll update the PR with something more sophisticated, probably tomorrow.

@aguillon aguillon closed this Sep 20, 2023
@aguillon
Copy link
Collaborator Author

I don't remember why I stopped doing this, probably because it required too much boilerplate code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gr-im gr-im Awaiting requested review from gr-im

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Visible SQL request when displaying error message
2 participants
@aguillon @gr-im