Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sqlite: allow passing conflict resolution handler function #56352

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

louwers
Copy link
Contributor

@louwers louwers commented Dec 24, 2024

Resolves #56210 by allowing to pass a conflict resolution handler that returns a conflict resolution type instead of just a conflict resolution type.

As @Renegade334 points out in that issue, it does not make sense to pass SQLITE_CHANGESET_REPLACE because this conflict resolution type is only valid for certain conflict types. Therefore an API where the conflict-resolution handler receives the conflict type makes sense, so the user can return a valid conflict resolution type for a particular conflict type.

An API where an individual (conflicting) change can be inspected (as is possible with the SQLite C conflict handler) would be a good follow-up. This would then be added as a second argument to the conflict handler.

Note that this is a breaking change, but since the SQLite module is experimental I don't think that is an issue.

Note: some of the documentation I added comes pretty much verbatim from the SQLite Documentation, but this is not an issue because the SQLite documentation is dedicated to the public domain. Also I linked to the SQLite documentation.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem. labels Dec 24, 2024
@louwers louwers force-pushed the sqlite-conflict-handler branch 4 times, most recently from 552a1e8 to 933ce21 Compare December 24, 2024 16:11
@louwers louwers force-pushed the sqlite-conflict-handler branch from 933ce21 to 3635cd9 Compare December 24, 2024 16:11
@louwers louwers changed the title sqlite: pass conflict type to conflict-resulution handler sqlite: pass conflict type to conflict resolution handler Dec 24, 2024
Copy link
Contributor

@Renegade334 Renegade334 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of quick observations about callback handling.

src/node_sqlite.cc Outdated Show resolved Hide resolved
src/node_sqlite.cc Outdated Show resolved Hide resolved
Copy link

codecov bot commented Dec 24, 2024

Codecov Report

Attention: Patch coverage is 97.05882% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.55%. Comparing base (4fd5db4) to head (a06a068).
Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 97.05% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #56352    +/-   ##
========================================
  Coverage   88.54%   88.55%            
========================================
  Files         657      657            
  Lines      190395   190682   +287     
  Branches    36552    36581    +29     
========================================
+ Hits       168586   168849   +263     
- Misses      14992    15004    +12     
- Partials     6817     6829    +12     
Files with missing lines Coverage Δ
src/node_sqlite.cc 82.22% <97.05%> (+0.42%) ⬆️

... and 45 files with indirect coverage changes

@louwers louwers force-pushed the sqlite-conflict-handler branch from f63c3b4 to a06a068 Compare December 24, 2024 20:58

if (CreateSQLiteError(isolate, errstr).ToLocal(&e)) {
e->Set(isolate->GetCurrentContext(),
OneByteString(isolate, "errcode"),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding an errcode similar to CreateSQLiteError(Isolate* isolate, sqlite3* db). However, this seems to be an undocumented feature.

@louwers
Copy link
Contributor Author

louwers commented Dec 24, 2024

@cjihrig Could you run CI?

@cjihrig cjihrig added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 24, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 24, 2024
@nodejs-github-bot
Copy link
Collaborator

@louwers louwers changed the title sqlite: pass conflict type to conflict resolution handler sqlite: allow passing conflict resolution handler function Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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