Event Handlers: does not support promises

[tripflex]

atdatabases/packages/pg/src/Driver.ts

Lines 409 to 415 in 97be50f

	function enforceUndefined(value: void) {
	if (value !== undefined) {
	throw new Error(
	`Your event handlers must return "undefined". This is to allow for the possibility of event handlers being used as hooks with more advanced functionality in the future.`,
	);
	}
	}

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 is UPDATE make a query to get current value and store in class
• onQueryResults parse and if query is same UPDATE query, get resulting value. Do insert into activity_log table with before and after 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

[ForbesLindesay]

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:

1. 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).
2. 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.
3. 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.

[tripflex]

@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