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: support ArrayBuffer and TypedArray in StatementSync #56385

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

Conversation

himself65
Copy link
Member

Fixes: #56384

@himself65 himself65 requested a review from cjihrig December 28, 2024 20:04
@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 28, 2024
@himself65 himself65 force-pushed the himself65/20241228/fix branch from 79ca6ef to aa1218f Compare December 28, 2024 20:39
Copy link

codecov bot commented Dec 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.53%. Comparing base (91c6b8a) to head (430743a).
Report is 10 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #56385   +/-   ##
=======================================
  Coverage   88.53%   88.53%           
=======================================
  Files         657      657           
  Lines      190689   190720   +31     
  Branches    36597    36603    +6     
=======================================
+ Hits       168826   168862   +36     
+ Misses      15030    15029    -1     
+ Partials     6833     6829    -4     
Files with missing lines Coverage Δ
src/node_sqlite.cc 82.27% <100.00%> (+0.47%) ⬆️

... and 35 files with indirect coverage changes

@@ -930,7 +930,8 @@ bool StatementSync::BindParams(const FunctionCallbackInfo<Value>& args) {
int anon_idx = 1;
int anon_start = 0;

if (args[0]->IsObject() && !args[0]->IsUint8Array()) {
if (args[0]->IsObject() && !args[0]->IsUint8Array() &&
!args[0]->IsTypedArray() && !args[0]->IsArrayBuffer()) {
Copy link
Member

Choose a reason for hiding this comment

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

Using IsArrayBufferView() here is likely a lot better than separate IsUint8Array() and IsTypedArray() checks.

}

const arrayBuffer = new Uint8Array([1, 2, 3, 4, 5, 6, 7, 8]).buffer;
const TypedArrays = [
Copy link
Member

Choose a reason for hiding this comment

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

Should DataView also be supported?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I think so

src/node_sqlite.cc Outdated Show resolved Hide resolved
Co-authored-by: Antoine du Hamel <[email protected]>
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.

node:sqlite support statement.run with TypedArray
6 participants