Repository: nodejs/node
Pull request: 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.

<!--
Before submitting a pull request, please read:

- the CONTRIBUTING guide at https://github.com/nodejs/node/blob/HEAD/CONTRIBUTING.md
- the commit message formatting guidelines at
  https://github.com/nodejs/node/blob/HEAD/doc/contributing/pull-requests.md#commit-message-guidelines

For code changes:
1. Include tests for any bug fixes or new features.
2. Update documentation if relevant.
3. Ensure that `make -j4 test` (UNIX), or `vcbuild test` (Windows) passes.

If you believe this PR should be highlighted in the Node.js CHANGELOG
please add the `notable-change` label.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.
-->


Changed files:
filename: doc/api/sqlite.md
patch: @@ -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

filename: src/node_sqlite.cc
patch: @@ -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",

filename: src/node_sqlite.h
patch: @@ -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);

filename: test/parallel/test-sqlite-statement-sync.js
patch: @@ -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);
   });
 });