Event Handlers: does not support promises
tripflex opened this issue · 2 comments
atdatabases/packages/pg/src/Driver.ts
Lines 409 to 415 in 97be50f
I'm trying to implement a sort of "Activity Log" middleware utilizing the onQueryStart
and onQueryResults
event handlers, my idea is as follows:
onQueryStart
parse and if query isUPDATE
make a query to get current value and store in classonQueryResults
parse and if query is sameUPDATE
query, get resulting value. Do insert intoactivity_log
table withbefore
andafter
entries
I have everything implemented for this, parsing the query using node-sql-parser
everything is great no problems there.
The problem i'm running into though is that all of the event handlers require the returned result to be undefined
Would you be okay with me submitting a PR to update to add support event handlers to be promises that return undefined? Was there some other kind of functionality you were planning for event handlers that maybe I can help work on? Or do you have any other suggestions of an easier way to handle this?
note: I was trying to stay away from using triggers as there is some logic I need to do, like parsing the query for a specific "user" ID and only doing the insert for specific tables, when that ID has a value, etc
Another issue I expect you will run into is that the onQueryStart
and onQueryResults
are not passed the transaction/connection (or any way to run a query within the current transaction/connection). This means that even if you have the option to await
, you will not be able to safely run any other db queries as they would run outside the current transaction context.
If you intend to asynchronously log to some external service, i.e. not Postgres, you wouldn't have this issue, but it would still not be a great idea for performance. At the point that onQueryStart
is called, an exclusive lock on the database connection has been acquired to run that query. If we supported promises, this lock would not be released until after your async logging operation completes.
In my projects, I normally just log to stdout/stderr, via console.log
/console.warn
, and then parse that log in a separate process as needed (we use JSON in our logs for structured data). You could alternatively just log without waiting for the result, e.g.
function onQueryStart(_query, formattedQuery) {
writeToRemoteLog(formattedQuery).catch(ex => {
console.error(`Failed to write to log: ${ex.message}`)
})
}
The reason for enforcing undefined
was to ensure that if we wanted to add support for additional functionality in the future, we could do so without breaking changes. The features I can currently see that would make sense here are:
- Returning a modified query from
onQueryStart
to run instead of the provided query. A good use case for this would be adding comments so that we can more easily trace the source of slow/expensive queries (e.g. https://google.github.io/sqlcommenter/spec/#format). - Aborting queries in
onQueryStart
before running them if we encounter certain errors. This can be done by throwing an error, but this may not always be what you want to do. - Returning results from a cache in
onQueryStart
instead of actually running the query. This would be a pretty advanced use case, and normally I'm inclined to do caching at a higher level, rather than within the database driver, but you certainly could implement this if we added a way to skip running the query and return fake results instead.
We could support the query modification use case by just relaxing the types. You could have something like:
function getCurrentController(): string {
// TODO: Use AsyncLocalStorage to get the "controller" for the current request
return 'unknown'
}
function onQueryStart(_query, formattedQuery) {
// @ts-expect-error formattedQuery.text is marked as readonly
formattedQuery.text = formattedQuery.text + `/* controller='${getCurrentController()}' */`
}
I'm not confident enough about the caching use case to know what the API should be yet. It's also possible we should add a hook that runs much earlier for this. Ideally, if you call .query(...)
on the ConnectionPool
, and there's a handler that can respond to your query from the cache, we shouldn't need to get a connection from the connection pool at all. This becomes especially important if your cache is an external service such as Redis/Memcached.
@ForbesLindesay thanks for the reply! Turns out this situation I was just trying to force being able to do it through @databases instead of just using triggers.. I was against it but with a little bit of tweaks I was able to get triggers to work as needed.
Still happen to work on a PR for this if it's something you're interested in having (so I can give back), let me know what your thoughts are though, don't want to create additional headache if not needed