sqlite: Exceptions on conflict handling with database.applyChangeset()

[Renegade334]

Refs: #54181

The SQLite session extension provides for conflict resolution when applying changesets. The way this works within sqlite3changeset_apply() is:

• on conflict, the user-provided xConflict callback is called with a conflict code eConflict
• the xConflict callback returns one of the resolution codes SQLITE_CHANGESET_OMIT, SQLITE_CHANGESET_ABORT or SQLITE_CHANGESET_REPLACE
• sqlite3changeset_apply() applies the requested resolution

To create an xConflict callback, the onConflict option passed to database.applyChangeset() is transformed into a simple lambda which ignores eConflict entirely and always returns the given resolution code:

node/src/node_sqlite.cc

Line 482 in 552a182

conflictCallback = [conflictInt]() -> int { return conflictInt; };

This is mostly fine, except where SQLITE_CHANGESET_REPLACE is concerned.

As per the documentation, this code may only be returned where the eConflict passed to the callback is SQLITE_CHANGESET_DATA or SQLITE_CHANGESET_CONFLICT. The current implementation does not honour this, and so will result in the function aborting, rolling back and returning SQLITE_MISUSE if it is asked to deal with a different type of conflict. Minimal repro:

const { DatabaseSync, SQLITE_CHANGESET_REPLACE } = require('node:sqlite');
const [ source, target ] = Array.from({ length: 2 }, () => {
  const db = new DatabaseSync(':memory:');
  db.exec('CREATE TABLE data(key INTEGER PRIMARY KEY)');
  return db;
});

source.prepare('INSERT INTO data (key) VALUES (?)').run(1);
const session = source.createSession();
source.prepare('DELETE FROM data WHERE key = ?').run(1);

target.applyChangeset(session.changeset(), { onConflict: SQLITE_CHANGESET_REPLACE });
// Uncaught [Error: not an error] {
//   code: 'ERR_SQLITE_ERROR',
//   errcode: 0,
//   errstr: 'not an error'
// }

In this instance, the conflict code passed to xConflict is SQLITE_CHANGESET_NOTFOUND, and replying with SQLITE_CHANGESET_REPLACE is illegal. The function aborts and returns SQLITE_MISUSE, and this results in an error being thrown by DatabaseSync::ApplyChangeset().

In addition, there is an issue with using CHECK_ERROR_OR_THROW here, because aborted returns from sqlite3changeset_apply() that arise from within the changeset functions' internal logic (rather than as a result of an unsuccessful database operation) don't set an error code on the database handle.
This is the cause of the nonsensical error message in the example, as CHECK_ERROR_OR_THROW ends up setting the properties of the thrown error by interrogating the database handle, not by using the error code passed to the macro.
This is slightly small-print, but not all errors returned by the changeset functions are associated with database operations (eg. SQLITE_NOMEM). It's not clear to me how important it would be to handle these cases, or indeed whether similar cases arise in other areas of node_sqlite.

In terms of things to address:

• Conflict handling needs to handle gracefully the case where options.onConflict == SQLITE_CHANGESET_REPLACE but the conflict code isn't one that accepts this response.
  • Should the callback just return SQLITE_CHANGESET_OMIT instead in these cases?
  • Whatever the resolution is, the documentation will need to be clear on the new behaviour.
• May need to look into whether error handling via CHECK_ERROR_OR_THROW needs to account for errors which aren't associated with the database handle.

[cjihrig]

cc: @louwers

[louwers]

@Renegade334 @cjihrig Thanks I'll pick this up in the weekend!

I think we should make the onConflict handler a function.