Skip to content

sqlite: add getter to detect transactions #57925

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 20, 2025
Merged

sqlite: add getter to detect transactions #57925

merged 1 commit into from
Apr 20, 2025

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Apr 18, 2025

This commit adds an isTransaction getter to the DatabaseSync class for determining if the database is currently within a transaction.

Fixes: #57922

@cjihrig
sqlite: add getter to detect transactions
cf604db 
This commit adds an isTransaction getter to the DatabaseSync
class for determining if the database is currently within a
transaction.

Fixes: nodejs#57922
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/sqlite

@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 Apr 18, 2025
@geeksilva97 geeksilva97 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 18, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 18, 2025
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/66350/

@codecov Codecov
Copy link

codecov bot commented Apr 18, 2025 •
edited
Loading

Codecov Report

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

Project coverage is 90.27%. Comparing base (52d95f5) to head (cf604db).
Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 88.88% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #57925   +/-   ##
=======================================
  Coverage   90.26%   90.27%           
=======================================
  Files         630      630           
  Lines      185933   185942    +9     
  Branches    36450    36449    -1     
=======================================
+ Hits       167829   167851   +22     
- Misses      10972    10975    +3     
+ Partials     7132     7116   -16
Files with missing lines Coverage Δ
src/node_sqlite.h 68.00% <ø> (ø)
src/node_sqlite.cc 80.53% <88.88%> (+0.04%) ⬆️

... and 25 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 18, 2025 •
edited by jasnell
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/66353/ 💛

@cjihrig cjihrig added commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Apr 18, 2025
Renegade334
Renegade334 reviewed Apr 19, 2025
View reviewed changes
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.

👍

doc/api/sqlite.md
@@ -291,6 +291,15 @@ added: v23.11.0

* {boolean} Whether the database is currently open or not.

### `database.isTransaction`
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor semantics: is or in?

To my eyes, db.isTransaction expands to "the database is a transaction?", which works semantically for eg. db.isOpen, but doesn't seem quite right here. inTransaction or activeTransaction would fit the bill slightly better, imo.

@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 20, 2025
@nodejs-github-bot nodejs-github-bot merged commit 2e0ec72 into nodejs:main Apr 20, 2025
80 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 2e0ec72

@cjihrig cjihrig deleted the is-transaction branch April 20, 2025 16:02
RafaelGSS pushed a commit that referenced this pull request May 1, 2025
@cjihrig @RafaelGSS
sqlite: add getter to detect transactions
885c61f 
This commit adds an isTransaction getter to the DatabaseSync
class for determining if the database is currently within a
transaction.

Fixes: #57922
PR-URL: #57925
Reviewed-By: Edy Silva <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request May 1, 2025
Merged
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
@cjihrig @RafaelGSS
sqlite: add getter to detect transactions
da05add 
This commit adds an isTransaction getter to the DatabaseSync
class for determining if the database is currently within a
transaction.

Fixes: #57922
PR-URL: #57925
Reviewed-By: Edy Silva <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
aduh95 pushed a commit that referenced this pull request May 6, 2025
@cjihrig @aduh95
sqlite: add getter to detect transactions
7ba6958 
This commit adds an isTransaction getter to the DatabaseSync
class for determining if the database is currently within a
transaction.

Fixes: #57922
PR-URL: #57925
Reviewed-By: Edy Silva <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
aduh95 pushed a commit that referenced this pull request May 6, 2025
@cjihrig @aduh95
sqlite: add getter to detect transactions
4b4d379 
This commit adds an isTransaction getter to the DatabaseSync
class for determining if the database is currently within a
transaction.

Fixes: #57922
PR-URL: #57925
Reviewed-By: Edy Silva <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
RafaelGSS pushed a commit that referenced this pull request May 14, 2025
@cjihrig @RafaelGSS
sqlite: add getter to detect transactions
d54a0a2 
This commit adds an isTransaction getter to the DatabaseSync
class for determining if the database is currently within a
transaction.

Fixes: #57922
PR-URL: #57925
Reviewed-By: Edy Silva <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
aduh95 pushed a commit that referenced this pull request May 16, 2025
@cjihrig @aduh95
sqlite: add getter to detect transactions
45cb52a 
This commit adds an isTransaction getter to the DatabaseSync
class for determining if the database is currently within a
transaction.

Fixes: #57922
PR-URL: #57925
Reviewed-By: Edy Silva <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
aduh95 pushed a commit that referenced this pull request May 17, 2025
@cjihrig @aduh95
sqlite: add getter to detect transactions
72b900f 
This commit adds an isTransaction getter to the DatabaseSync
class for determining if the database is currently within a
transaction.

Fixes: #57922
PR-URL: #57925
Reviewed-By: Edy Silva <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
aduh95 pushed a commit that referenced this pull request May 17, 2025
@cjihrig @aduh95
sqlite: add getter to detect transactions
59d842d 
This commit adds an isTransaction getter to the DatabaseSync
class for determining if the database is currently within a
transaction.

Fixes: #57922
PR-URL: #57925
Reviewed-By: Edy Silva <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
aduh95 pushed a commit that referenced this pull request May 17, 2025
@cjihrig @aduh95
sqlite: add getter to detect transactions
48c897d 
This commit adds an isTransaction getter to the DatabaseSync
class for determining if the database is currently within a
transaction.

Fixes: #57922
PR-URL: #57925
Reviewed-By: Edy Silva <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
aduh95 pushed a commit that referenced this pull request May 18, 2025
@cjihrig @aduh95
sqlite: add getter to detect transactions
ecaca07 
This commit adds an isTransaction getter to the DatabaseSync
class for determining if the database is currently within a
transaction.

Fixes: #57922
PR-URL: #57925
Reviewed-By: Edy Silva <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
@github-actions github-actions bot mentioned this pull request May 19, 2025
Merged
aduh95 pushed a commit that referenced this pull request May 19, 2025
@cjihrig @aduh95
sqlite: add getter to detect transactions
0e999eb 
This commit adds an isTransaction getter to the DatabaseSync
class for determining if the database is currently within a
transaction.

Fixes: #57922
PR-URL: #57925
Reviewed-By: Edy Silva <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
@AuntyEms AuntyEms mentioned this pull request Jun 8, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Renegade334 Renegade334 Renegade334 left review comments

@jasnell jasnell jasnell approved these changes

@targos targos targos approved these changes

@himself65 himself65 himself65 approved these changes

@geeksilva97 geeksilva97 geeksilva97 approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. sqlite Issues and PRs related to the SQLite subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[sqlite] Add inTransaction property
7 participants
@cjihrig @nodejs-github-bot @jasnell @targos @Renegade334 @himself65 @geeksilva97