[wish] Please throw informative "attempt to write a readonly database" exception
(1) By greenoldman (macias) on 2024-01-14 13:36:35 [source]
This is continuation of the post: https://github.com/dotnet/efcore/issues/32811
I come from C# world and noticed a little friction when it comes to error message. Quote:
The current exception message is:
Microsoft.Data.Sqlite.SqliteException
SQLite Error 8: 'attempt to write a readonly database'.
Please, make it a proper error message with the reference to the connection/file itself. SQLite is file/memory based, there is no login/password there. Like so:
Microsoft.Data.Sqlite.SqliteException
SQLite Error 8: 'attempt to write a readonly database Date Source=\tmp\my_database.db'.
This is change at the root place, once made, everybody will benefit because figuring out what is going on will have one obstacle less.
(2) By Chris Locke (chrisjlocke1) on 2024-01-14 15:55:46 in reply to 1 [link] [source]
If you've made the connection, surely you know what the file is? sqlite isn't opening a random file somewhere.
(4) By greenoldman (macias) on 2024-01-15 14:06:57 in reply to 2 [link] [source]
If you've made the connection, surely you know what the file is? sqlite isn't opening a random file somewhere.
Scenario: program uses 2 databases, predetermined, not-random, A.db and B.db, then the mentioned error appars on the screen.
Question for you -- which database does the error refer to?
(8) By Chris Locke (chrisjlocke1) on 2024-01-15 17:41:49 in reply to 4 [link] [source]
While the library may only report 'Could not open database' and you don't know the name of the database, I assume the program wouldn't simply report 'Syntax error' if a part of the program failed, but 'Syntax error at line 56'. The program itself would report the stages of progression and the success and failure of such. For example, 'opening configuration .. configuration read successfully .. opening system database on e:network .. failed'. So while the library doesn't report all the details, together with the application log, it reports more than enough.
(9) By greenoldman (macias) on 2024-01-15 20:14:53 in reply to 8 [link] [source]
It is nice picture, and I wish life is that simple. To figure out which file was affected I need to pepper every call with try-catch, just to extract this info. Meaning the end dev has more and more work. Every end dev.
But I sense this cause is lost because somehow the generic error is more valuable than full one. I don't see the added value in this, but well, maybe I miss a thrill of hard debugging :-).
Just for the record syntax error is irrelevant here, in case of crash you can get stack trace, if you do, it is not up to exact line number. But even then you only could find out the file this way, if the file was given in advance.
(10) By Tim Streater (Clothears) on 2024-01-15 21:40:49 in reply to 9 [link] [source]
It's not the job of the SQLite library, or indeed any library, to know how errors are to be handled and/or logged. That's the application writer's job, and will vary with the application. So you get to decide how to handle that.
I put wrappers around the methods provided by the host language I use and which interfaces to the SQLite C library. And each of those wrappers does the try/catch business and writes an error message (including the file name, and the failing SQL) to my log file, as appropriate.
So no, you don't have to pepper the place with try/catch, you just need to have thought about the issue ahead of time.
(11) By greenoldman (macias) on 2024-01-16 14:06:54 in reply to 10 [link] [source]
And each of those wrappers does the try/catch business and writes an error message (including the file name, and the failing SQL) to my log file
Logging is not an issue here, I didn't say a word about it. This post is about message/content itself.
So you get to decide how to handle that.
This is the point, I do NOT want to handle it (alter message).
When you consider two families of errors ("Key not found", "Host unreachable", "You cannot delete directory" vs. "Key 'joe4' not found", "Host 192.168.1.20 unreachable", "You cannot delete directory Videos because it is not empty") as I understand you are happy with with the first group, because it is up to end dev to change those message and adding missing pieces is easy (you "just" write a wrapper...), while I prefer the second group because working with such libraries is easier because I don't have to tweak those error messages at all (they have all information needed).
Different people, different opinions/preferences of course.
(3) By Spindrift (spindrift) on 2024-01-14 17:26:13 in reply to 1 [link] [source]
As mentioned in the reply to your GitHub post:
"It should be easy for you to know the database being used at the point where the exception is thrown."
(5) By greenoldman (macias) on 2024-01-15 14:08:10 in reply to 3 [link] [source]
"It should be easy for you to know the database being used at the point where the exception is thrown."
If you turn the perspective, working with library which provides full error messages is easier than with one that does not.
By all means I respect somebody work, so for sure, adding this info is a work of non-zero cost, but it is a fixed cost. While on the other side while the cost is also non-zero, it is not fixed, it adds up -- and that's the big difference.
In other words, IMHO it is always better to fix root issue, than hunt down all the cases/usages down the line.
(6.1) By Stephan Beal (stephan) on 2024-01-15 15:08:39 edited from 6.0 in reply to 5 [link] [source]
working with library which provides full error messages is easier than with one that does not.
While not wrong, here are two points for consideration:
Collecting that info into an error message would require allocation, which might itself fail, leading to an awkward situation for the library. The current error message strings are static, so "cannot fail."
Path information might be considered security-relevant, in that it can give away information about the host environment which a user might not otherwise have. Consider a web app which gives exception messages as-is to users.
Granted, you're talking about the C# wrapper, rather than the C library, so the first point doesn't apply in the same way (it will presumably thrown an OOM exception on OOM), but the second point still does. The library-level code cannot know whether revealing such info is appropriate.
Adding such info to library-level error messages would retroactively affect countless already-deployed applications, some of them in potentially security-relevant ways. (Edit: at a very minimum it may break countless test suites which check for expected error results.)
(7) By greenoldman (macias) on 2024-01-15 15:57:12 in reply to 6.1 [link] [source]
Good points, thank you. By all means I won't negate them, just maybe add some thoughts:
allocation, true, from the very strict environment perspective, like embedded system of some kind even more. It could be done with try-add-info approach, meaning if it is not possible the generic one is used
security, yes, I am aware of this, but honestly, if you have a case raw errors are sent to the receiving party which should not see them, there is a bigger security hole than db path :-)
test cases and in general backward compatibility. It depends how you treat error -- is this "SQLite Error 8:" part of the API or entire message? Secondly it is "make" case on a bigger scale (if we agree such change is useful) "let's not change it because I have already XX users", then rewind 10-20 years ahead and that previous count looks pale in comparison to the current number of users.