sqlite: handle exceptions in filter callback database.applyChangeset()

[louwers]

Resolves#56890

Also adds additional test coverage for filter callback and clarifies behavior in documentation.

[cjihrig]

I don't think you need the TryCatch here. The problem is the use of ToLocalChecked(). Take a look at this code. You can tell if V8 has an exception pending if the ToLocal() call does not succeed.

[louwers]

@jasnell@cjihrig Actually I don't think we should propagate the exception. Because if different exceptions are thrown only the last is propagated.

You can't really abort the whole application of the changeset from the filter callback. So I just interpret an exception as "do not include changes from this table".

[cjihrig]

Because if different exceptions are thrown only the last is propagated.

Can't we stop attempting more work once the first exception is detected?

[jasnell]

Actually I don't think we should propagate the exception. Because if different exceptions are thrown only the last is propagated...

Unfortunately we need to. For instance, the exception thrown could be something like a fatal out of memory or some other condition where we really shoiuld not just ignore and proceed. Also, not throwing the error can lead to subtle bugs in user code where their filter may be throwing an error that gets swallowed. Any error thrown by the filter callback needs to be propagated.

[louwers]

Can't we stop attempting more work once the first exception is detected?

Not from the filter callback. SQLite is calling us at that point.

Any error thrown by the filter callback needs to be propagated.

@jasnell What should we do when multiple errors are thrown?

[cjihrig]

It looks like there is already a transaction in place, we just need to verify:

All changes made by these functions are enclosed in a savepoint transaction. If any other error (aside from a constraint failure when attempting to write to the target database) occurs, then the savepoint transaction is rolled back, restoring the target database to its original state, and an SQLite error code returned.

What should we do when multiple errors are thrown?

Once the first error is thrown we should avoid trying to do any more work that calls into JavaScript so that no more exceptions occur.

[louwers]

@cjihrig In that case we should change the API. Instead of a function we can think about passing a Set of table names or a regular expression.

[cjihrig]

Once the first exception is detected, wouldn't it make sense to return SQLITE_CHANGESET_ABORT? I haven't used this API before, but based on the documentation, it seems like that may work?

[louwers]

@cjihrig The filter callback cannot abort, that is the conflict handler callback (only called when a conflict is detected, not for every change).

[louwers]

Maybe the filter callback is called prior to applying any changes. If that is the case it would be possible.

I doubt it though, because that would probably require more memory.

[cjihrig]

I think you'll need some state in the C++ code that can be shared between the filter and conflict functions. If the filter function creates a JS error, let it be thrown, but also roll back in the conflict function.

[louwers]

@cjihrig That won't work, because the conflict handler is not called if there are no conflicts. Sorry, edited my comment too late.

https://sqlite.org/forum/forumpost/c2135257d2

[cjihrig]

OK, I'll have to look into the code in more detail when I'm not at work 😄. In the worst case, once the first exception is encountered, we should probably return false from all remaining filter functions to avoid calling into JS any more.

[codecov]

Codecov Report

Attention: Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 89.18%. Comparing base (69502d8) to head (4adb1bf).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_sqlite.cc	80.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #56903 +/- ##
=======================================
Coverage 89.17% 89.18% =======================================
Files 665 665 Lines 192602 192609 +7 Branches 37057 37057 =======================================
+ Hits 171755 171771 +16 
Misses 13657 13657 + Partials 7190 7181 -9 

Files with missing lines	Coverage Δ
src/node_sqlite.cc	79.60% <80.00%> (-0.05%)	⬇️

... and 24 files with indirect coverage changes

[cjihrig]

I looked into this a bit more, and it appears that the tables in the changeset are looped over when we call applyChangeset(). For each table, the filter() function is called. If filter() returns true, then the onConflict() function is called. Then, SQLite3 moves on to the next table. If onConflict() returns SQLITE_CHANGESET_ABORT at any point, then everything is rolled back and no more progress is attempted.

What I believe we need to do is:

• Run the filter() function. If an exception occurs, we need to create state that there is a pending exception.
• When onConflict() runs and there is not a pending exception, the JS callback is executed. If that causes a pending exception, that invocation of onConflict() should return SQLITE_CHANGESET_ABORT.
• If at any point there is a pending exception, all invocations of filter() should immediately return true and all invocations of onConflict() should immediately return SQLITE_CHANGESET_ABORT. In practice only one failing onConflict() invocation will happen.

Does that make sense? If my understanding of the control flow is correct, that should prevent us from ever getting in a situation where there are multiple exceptions.

[louwers]

As I said in my previous comment, the conflict handler is not called when there are no conflicts... So that approach will not work.

[cjihrig]

Then I think the only options are:

• Wrapping applyChangeset() in our own transaction that we can roll back.
• Not supporting the filter function at all.
• Not rolling back after an error. We should not do this though.

We will still need some logic like what I laid out in my previous comment though because we need to stop executing JS when there is an exception. The only difference is maybe filter() immediately returns false when an exception is pending and onConflict() doesn't get to execute at all.

[louwers]

Wrapping applyChangeset() in our own transaction that we can roll back.

OK this is doable. We could use SQLITE_CHANGESETAPPLY_NOSAVEPOINT to avoid the internal savepoint from being set once sqlite3changeset_apply_v2 is stable.

[addaleax]

Looks good!

I think it would be an improvement to apply the same steps to the conflict callback, i.e. remove the TryCatch there and instead handle an empty MaybeLocal value, just like you're doing here.

If you want to improve on it even more, I think ideally we'd want to keep track of whether one of the callbacks has seen an exception (i.e. empty MaybeLocal result) on the context object, and not try to call any JS function if a previous call had already thrown.

[addaleax]

Not technically wrong, but you don't need this because you already handle the empty case in the next conditional

Suggested change

	if (maybe_result.IsEmpty()) {
	returnfalse;
	}

[louwers]

I will still add some logic here, I think this branch is taken when an exception is thrown, correct?

[targos]

Yes, it should be taken when filterFunc throws an exception. But the point of the comment here is that in the next condition maybe_result.ToLocal(&result) returns false in the same case.

[cjihrig]

@louwers are you still working on this (it's marked as a draft)?

[louwers]

@cjihrig Yes, I still intend to finish this. I got stuck trying to write a RAII wrapper to create and roll back a savepoint. Do we have something like that already? Otherwise I think I will handle it manually.

[cjihrig]

OK, thanks. No, nothing like that exists yet as far as I know.

[Renegade334]

@louwers: I completely missed that you'd been working on this, and drafted up d017d7735de233a0dd9550d8061325a9a841c7c8. If it's worthless to you then no worries, but figured I'd share just in case!

[louwers]

@Renegade334 OK thx I can use that.

[louwers]

Part of this PR was merged with #59848.

Will make another PR.