search

vapor / postgres-nio

🐘 Non-blocking, event-driven Swift client for PostgreSQL.
https://api.vapor.codes/postgresnio/documentation/postgresnio/
MIT License
317 stars 72 forks source link

Add good PSQLError debugDescription #356

Closed fabianfett closed 1 year ago

fabianfett commented 1 year ago

This shall be the start of our discussion.

Will eventually fix #354.

codecov-commenter commented 1 year ago

Codecov Report

Merging #356 (91bbd07) into main (1516e0c) will decrease coverage by 0.53%. The diff coverage is 0.00%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #356 +/- ## ========================================== - Coverage 41.20% 40.67% -0.53% ========================================== Files 117 117 Lines 9657 9760 +103 ========================================== - Hits 3979 3970 -9 - Misses 5678 5790 +112 ``` | [Impacted Files](https://codecov.io/gh/vapor/postgres-nio/pull/356?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vapor) | Coverage Ξ” | | |---|---|---| | [...ces/PostgresNIO/Deprecated/PostgresData+UInt.swift](https://codecov.io/gh/vapor/postgres-nio/pull/356?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vapor#diff-U291cmNlcy9Qb3N0Z3Jlc05JTy9EZXByZWNhdGVkL1Bvc3RncmVzRGF0YStVSW50LnN3aWZ0) | `0.00% <ΓΈ> (ΓΈ)` | | | [Sources/PostgresNIO/New/PSQLError.swift](https://codecov.io/gh/vapor/postgres-nio/pull/356?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vapor#diff-U291cmNlcy9Qb3N0Z3Jlc05JTy9OZXcvUFNRTEVycm9yLnN3aWZ0) | `31.22% <0.00%> (-16.98%)` | :arrow_down: | | [...urces/PostgresNIO/New/PostgresChannelHandler.swift](https://codecov.io/gh/vapor/postgres-nio/pull/356?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vapor#diff-U291cmNlcy9Qb3N0Z3Jlc05JTy9OZXcvUG9zdGdyZXNDaGFubmVsSGFuZGxlci5zd2lmdA==) | `61.93% <0.00%> (-0.14%)` | :arrow_down: | ... and [10 files with indirect coverage changes](https://codecov.io/gh/vapor/postgres-nio/pull/356/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vapor)
trasch commented 1 year ago

My thoughts about the serverInfo part:

Apart from the nitpicking: Thanks @fabianfett , this is already a big improvement!

fabianfett commented 1 year ago

@trasch What do you think about going through debugDescription to prevent users from accidentally leaking database details?

trasch commented 1 year ago

@fabianfett The question is, for what should it be used? I think you rightly made the description as generic as possible because that would be the string that would be displayed to users, whereas debugDescription is for developers who need to know what's going on.

But what is actually the realistic scenario? Someone using this library for web backends or something similar will probably (hopefully) just send a message like "Something has gone wrong" to clients and not the debugDescription, but they still want a nice debug message for themselves. Developers who use this library for some internal data crunching or whatever will very much want to have a detailed error message. And developers who write something like "PGAdmin" want to send the debugDescription to the client because that's what the user expects.

So - in my opinion - information leaking isn't a problem here, and it should be more about giving useful error messages to developers... (and at least I never found it interesting that some error occured in postgres.c line 3312 πŸ˜ƒ )

But maybe I'm overlooking something here?

trasch commented 1 year ago

@fabianfett Thinking about it a bit longer, I'm not so convinced anymore about the security problem. I mean, there are of course developers who have no clue about security, and oversights happen to the best of us, but is it really the job of a library to prevent this?

I think that description should be a short error description, like in the Postgres logs: ERROR: canceling statement due to statement timeout and debugDescription should be the full monty, as it is now.

But maybe there are other users of this library that have yet another opinion? πŸ˜ƒ