sqlite: make sourceSQL and expandedSQL string-valued properties

Change sourceSQL and expandedSQL from being methods to being string-valued properties. These fields - are conceptually properties (and not actions), - are derived deterministically from the current state of the object, - require no parameters, and - are inexpensive to compute. Also, following the naming conventions of ECMAScript for new features, most function names should usually contain a verb, whereas names of (dynamically computed) properties generally should not, so the current names also seem more appropriate for properties than for functions. PR-URL: #54721 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
nodejs · Oct 6, 2024 · 7af434f · 7af434f
1 parent a49abec
commit 7af434f
Show file tree

Hide file tree

Showing 4 changed files with 47 additions and 19 deletions.
diff --git a/doc/api/sqlite.md b/doc/api/sqlite.md
@@ -194,17 +194,17 @@ objects. If the prepared statement does not return any results, this method
 returns an empty array. The prepared statement [parameters are bound][] using
 the values in `namedParameters` and `anonymousParameters`.
 
-### `statement.expandedSQL()`
+### `statement.expandedSQL`
 
 <!-- YAML
 added: v22.5.0
 -->
 
-* Returns: {string} The source SQL expanded to include parameter values.
+* {string} The source SQL expanded to include parameter values.
 
-This method returns the source SQL of the prepared statement with parameter
+The source SQL text of the prepared statement with parameter
 placeholders replaced by the values that were used during the most recent
-execution of this prepared statement. This method is a wrapper around
+execution of this prepared statement. This property is a wrapper around
 [`sqlite3_expanded_sql()`][].
 
 ### `statement.get([namedParameters][, ...anonymousParameters])`
@@ -293,15 +293,15 @@ be used to read `INTEGER` data using JavaScript `BigInt`s. This method has no
 impact on database write operations where numbers and `BigInt`s are both
 supported at all times.
 
-### `statement.sourceSQL()`
+### `statement.sourceSQL`
 
 <!-- YAML
 added: v22.5.0
 -->
 
-* Returns: {string} The source SQL used to create this prepared statement.
+* {string} The source SQL used to create this prepared statement.
 
-This method returns the source SQL of the prepared statement. This method is a
+The source SQL text of the prepared statement. This property is a
 wrapper around [`sqlite3_sql()`][].
 
 ### Type conversion between JavaScript and SQLite

diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc
@@ -18,8 +18,11 @@ using v8::Array;
 using v8::ArrayBuffer;
 using v8::BigInt;
 using v8::Boolean;
+using v8::ConstructorBehavior;
 using v8::Context;
+using v8::DontDelete;
 using v8::Exception;
+using v8::FunctionCallback;
 using v8::FunctionCallbackInfo;
 using v8::FunctionTemplate;
 using v8::Integer;
@@ -31,6 +34,7 @@ using v8::Name;
 using v8::Null;
 using v8::Number;
 using v8::Object;
+using v8::SideEffectType;
 using v8::String;
 using v8::Uint8Array;
 using v8::Value;
@@ -643,7 +647,7 @@ void StatementSync::Run(const FunctionCallbackInfo<Value>& args) {
   args.GetReturnValue().Set(result);
 }
 
-void StatementSync::SourceSQL(const FunctionCallbackInfo<Value>& args) {
+void StatementSync::SourceSQLGetter(const FunctionCallbackInfo<Value>& args) {
   StatementSync* stmt;
   ASSIGN_OR_RETURN_UNWRAP(&stmt, args.This());
   Environment* env = Environment::GetCurrent(args);
@@ -657,7 +661,7 @@ void StatementSync::SourceSQL(const FunctionCallbackInfo<Value>& args) {
   args.GetReturnValue().Set(sql);
 }
 
-void StatementSync::ExpandedSQL(const FunctionCallbackInfo<Value>& args) {
+void StatementSync::ExpandedSQLGetter(const FunctionCallbackInfo<Value>& args) {
   StatementSync* stmt;
   ASSIGN_OR_RETURN_UNWRAP(&stmt, args.This());
   Environment* env = Environment::GetCurrent(args);
@@ -717,6 +721,23 @@ void IllegalConstructor(const FunctionCallbackInfo<Value>& args) {
   node::THROW_ERR_ILLEGAL_CONSTRUCTOR(Environment::GetCurrent(args));
 }
 
+static inline void SetSideEffectFreeGetter(
+    Isolate* isolate,
+    Local<FunctionTemplate> class_template,
+    Local<String> name,
+    FunctionCallback fn) {
+  Local<FunctionTemplate> getter =
+      FunctionTemplate::New(isolate,
+                            fn,
+                            Local<Value>(),
+                            v8::Signature::New(isolate, class_template),
+                            /* length */ 0,
+                            ConstructorBehavior::kThrow,
+                            SideEffectType::kHasNoSideEffect);
+  class_template->InstanceTemplate()->SetAccessorProperty(
+      name, getter, Local<FunctionTemplate>(), DontDelete);
+}
+
 Local<FunctionTemplate> StatementSync::GetConstructorTemplate(
     Environment* env) {
   Local<FunctionTemplate> tmpl =
@@ -730,8 +751,14 @@ Local<FunctionTemplate> StatementSync::GetConstructorTemplate(
     SetProtoMethod(isolate, tmpl, "all", StatementSync::All);
     SetProtoMethod(isolate, tmpl, "get", StatementSync::Get);
     SetProtoMethod(isolate, tmpl, "run", StatementSync::Run);
-    SetProtoMethod(isolate, tmpl, "sourceSQL", StatementSync::SourceSQL);
-    SetProtoMethod(isolate, tmpl, "expandedSQL", StatementSync::ExpandedSQL);
+    SetSideEffectFreeGetter(isolate,
+                            tmpl,
+                            FIXED_ONE_BYTE_STRING(isolate, "sourceSQL"),
+                            StatementSync::SourceSQLGetter);
+    SetSideEffectFreeGetter(isolate,
+                            tmpl,
+                            FIXED_ONE_BYTE_STRING(isolate, "expandedSQL"),
+                            StatementSync::ExpandedSQLGetter);
     SetProtoMethod(isolate,
                    tmpl,
                    "setAllowBareNamedParameters",

diff --git a/src/node_sqlite.h b/src/node_sqlite.h
@@ -62,8 +62,9 @@ class StatementSync : public BaseObject {
   static void All(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void Get(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void Run(const v8::FunctionCallbackInfo<v8::Value>& args);
-  static void SourceSQL(const v8::FunctionCallbackInfo<v8::Value>& args);
-  static void ExpandedSQL(const v8::FunctionCallbackInfo<v8::Value>& args);
+  static void SourceSQLGetter(const v8::FunctionCallbackInfo<v8::Value>& args);
+  static void ExpandedSQLGetter(
+      const v8::FunctionCallbackInfo<v8::Value>& args);
   static void SetAllowBareNamedParameters(
       const v8::FunctionCallbackInfo<v8::Value>& args);
   static void SetReadBigInts(const v8::FunctionCallbackInfo<v8::Value>& args);

diff --git a/test/parallel/test-sqlite-statement-sync.js b/test/parallel/test-sqlite-statement-sync.js
@@ -135,8 +135,8 @@ suite('StatementSync.prototype.run()', () => {
   });
 });
 
-suite('StatementSync.prototype.sourceSQL()', () => {
-  test('returns input SQL', (t) => {
+suite('StatementSync.prototype.sourceSQL', () => {
+  test('equals input SQL', (t) => {
     const db = new DatabaseSync(nextDb());
     t.after(() => { db.close(); });
     const setup = db.exec(
@@ -145,12 +145,12 @@ suite('StatementSync.prototype.sourceSQL()', () => {
     t.assert.strictEqual(setup, undefined);
     const sql = 'INSERT INTO types (key, val) VALUES ($k, $v)';
     const stmt = db.prepare(sql);
-    t.assert.strictEqual(stmt.sourceSQL(), sql);
+    t.assert.strictEqual(stmt.sourceSQL, sql);
   });
 });
 
-suite('StatementSync.prototype.expandedSQL()', () => {
-  test('returns expanded SQL', (t) => {
+suite('StatementSync.prototype.expandedSQL', () => {
+  test('equals expanded SQL', (t) => {
     const db = new DatabaseSync(nextDb());
     t.after(() => { db.close(); });
     const setup = db.exec(
@@ -164,7 +164,7 @@ suite('StatementSync.prototype.expandedSQL()', () => {
       stmt.run({ $k: '33' }, '42'),
       { changes: 1, lastInsertRowid: 33 },
     );
-    t.assert.strictEqual(stmt.expandedSQL(), expanded);
+    t.assert.strictEqual(stmt.expandedSQL, expanded);
   });
 });