proteus
proteus copied to clipboard
jonbodner
Running UPDATE statements with a ContextQuerier silently fails
I recently made the mistake of specifying a ContextQuerier where I needed a ContextExecutor.
Strangely enough, my test failed because the number of rows returned was zero. However, no error was returned from the function either.
I'm not sure if this is reasonable to ask or if it'd require Proteus to do something more intrusive like introspecting the query, but it might be nice if this edge case was signaled in a more disruptive fashion.
Hrm, I'm not sure how handle this in the common case. The query isn't being parsed by Proteus. It could contain all sorts of things like CTEs or stored procedures, and I don't want to write a SQL parser for all of the supported databases...
IIRC, a ContextExecutor can have a return type of int64, error, or int64 and error. I could put in a rule that you can't have a ContextQuerier have an int64 for a return type, but that seems unnecessarily restrictive.
What behavior would you like to see?
The only thing I have been able to come up with is issuing some kind of warning inside the init() startup. Like you said, it seems like any query introspection would be thwarted by more complicated queries.
I'm left wondering whether making the runtime behavior dependent on which interface is used is necessary, especially since both interfaces can be satisfied by an instance of sql.DB at the call-site. But I don't have an alternative in mind.
I'd consider a warning. Let me think about it a bit.
The reason for ContextQuerier vs ContextExecutor is to know whether to run QueryContext or ExecContext. I had played with doing this via a struct tag originally, but I liked the parameter because it makes it more visible. I had ruled out using the name of the function field, because that's all sorts of terrible.
The most recent versions of Proteus let you use the Builder directly, so you can just make Ad-Hoc queries with Exec or Query directly, but that removes type safety: https://github.com/jonbodner/proteus/blob/master/README.md#ad-hoc-database-queries
Or if you want to contribute the warning, I'll consider accepting the PR :-)
I would be open to that. No promises when, so feel free to steal it back. I'll open a draft PR once I've started.
Also, the new change you submitted that allows a sql.Result only for (Context)?Executor can be used to ensure that you don't use a ContextQuerier with an UPDATE.
To answer your question mark, the change I submitted should work for ContextExecutor as well. I believe it's covered by a unit test.
That does make this particular error a little less likely, depending on usage.
