From c1e6ed69eff3e94408819ce4a37dd69dc34b71a1 Mon Sep 17 00:00:00 2001 From: Monil Bhavsar Date: Mon, 2 Dec 2024 18:03:40 +0530 Subject: [PATCH 1/5] Overload functions to result for writes --- sqlitecluster/SQLite.cpp | 17 ++++++++++++----- sqlitecluster/SQLite.h | 4 +++- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/sqlitecluster/SQLite.cpp b/sqlitecluster/SQLite.cpp index 5ea59ced0..76c70ff00 100644 --- a/sqlitecluster/SQLite.cpp +++ b/sqlitecluster/SQLite.cpp @@ -566,18 +566,25 @@ bool SQLite::write(const string& query) { } // This is literally identical to the idempotent version except for the check for _noopUpdateMode. - return _writeIdempotent(query); + SQResult ignore; + return _writeIdempotent(query, ignore); } bool SQLite::writeIdempotent(const string& query) { - return _writeIdempotent(query); + SQResult ignore; + return _writeIdempotent(query, ignore); +} + +bool SQLite::writeIdempotent(const string& query, SQResult& result) { + return _writeIdempotent(query, result); } bool SQLite::writeUnmodified(const string& query) { - return _writeIdempotent(query, true); + SQResult ignore; + return _writeIdempotent(query, ignore, true); } -bool SQLite::_writeIdempotent(const string& query, bool alwaysKeepQueries) { +bool SQLite::_writeIdempotent(const string& query, SQResult& result, bool alwaysKeepQueries) { SASSERT(_insideTransaction); _queryCache.clear(); _queryCount++; @@ -600,7 +607,7 @@ bool SQLite::_writeIdempotent(const string& query, bool alwaysKeepQueries) { { shared_lock lock(_sharedData.writeLock); if (_enableRewrite) { - resultCode = SQuery(_db, "read/write transaction", query, 2'000'000, true); + resultCode = SQuery(_db, "read/write transaction", query, result, 2'000'000, true); if (resultCode == SQLITE_AUTH) { // Run re-written query. _currentlyRunningRewritten = true; diff --git a/sqlitecluster/SQLite.h b/sqlitecluster/SQLite.h index 842b51430..d7392670b 100644 --- a/sqlitecluster/SQLite.h +++ b/sqlitecluster/SQLite.h @@ -1,5 +1,6 @@ #pragma once #include +#include #include class SQLite { @@ -108,6 +109,7 @@ class SQLite { // It's intended to be used for `mockRequest` enabled commands, such that we only run a version of them that's // known to be repeatable. What counts as repeatable is up to the individual command. bool writeIdempotent(const string& query); + bool writeIdempotent(const string& query, SQResult& result); // This runs a query completely unchanged, always adding it to the uncommitted query, such that it will be recorded // in the journal even if it had no effect on the database. This lets replicated or synchronized queries be added @@ -400,7 +402,7 @@ class SQLite { static thread_local int64_t _conflictPage; static thread_local string _conflictTable; - bool _writeIdempotent(const string& query, bool alwaysKeepQueries = false); + bool _writeIdempotent(const string& query, SQResult& result, bool alwaysKeepQueries = false); // Constructs a UNION query from a list of 'query parts' over each of our journal tables. // Fore each table, queryParts will be joined with that table's name as a separator. I.e., if you have a tables From f0163c1235f3c732f3ac963d40d8b731a78bc80c Mon Sep 17 00:00:00 2001 From: Monil Bhavsar Date: Mon, 2 Dec 2024 18:04:15 +0530 Subject: [PATCH 2/5] Pass missing param for non rewrite mode --- sqlitecluster/SQLite.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sqlitecluster/SQLite.cpp b/sqlitecluster/SQLite.cpp index 76c70ff00..da14f4590 100644 --- a/sqlitecluster/SQLite.cpp +++ b/sqlitecluster/SQLite.cpp @@ -617,7 +617,7 @@ bool SQLite::_writeIdempotent(const string& query, SQResult& result, bool always _currentlyRunningRewritten = false; } } else { - resultCode = SQuery(_db, "read/write transaction", query); + resultCode = SQuery(_db, "read/write transaction", query, result); } } From 15701ec984f3ae9c43c3f88753268df4b27a800c Mon Sep 17 00:00:00 2001 From: Monil Bhavsar Date: Mon, 2 Dec 2024 18:09:35 +0530 Subject: [PATCH 3/5] Add comment --- sqlitecluster/SQLite.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sqlitecluster/SQLite.h b/sqlitecluster/SQLite.h index d7392670b..e8c68e3e9 100644 --- a/sqlitecluster/SQLite.h +++ b/sqlitecluster/SQLite.h @@ -109,6 +109,9 @@ class SQLite { // It's intended to be used for `mockRequest` enabled commands, such that we only run a version of them that's // known to be repeatable. What counts as repeatable is up to the individual command. bool writeIdempotent(const string& query); + + // Executes a write query and retrieves the result. + // Designed for use with queries that include a RETURNING clause bool writeIdempotent(const string& query, SQResult& result); // This runs a query completely unchanged, always adding it to the uncommitted query, such that it will be recorded From b496479b05a1459b4d220978b5d1cc81e2e26e42 Mon Sep 17 00:00:00 2001 From: Monil Bhavsar Date: Tue, 3 Dec 2024 13:23:56 +0530 Subject: [PATCH 4/5] Add automated test --- test/tests/LibStuffTest.cpp | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/test/tests/LibStuffTest.cpp b/test/tests/LibStuffTest.cpp index 80a6c92f8..cfd5d5193 100644 --- a/test/tests/LibStuffTest.cpp +++ b/test/tests/LibStuffTest.cpp @@ -35,6 +35,7 @@ struct LibStuff : tpunit::TestFixture { TEST(LibStuff::SREMatchTest), TEST(LibStuff::SREReplaceTest), TEST(LibStuff::SQResultTest), + TEST(LibStuff::testReturningClause), TEST(LibStuff::SRedactSensitiveValuesTest) ) { } @@ -746,6 +747,37 @@ struct LibStuff : tpunit::TestFixture { ASSERT_EQUAL(result[0]["coco"], "name1"); } + void testReturningClause() { + // Given a sqlite DB with a table and pre inserted values + SQLite db(":memory:", 1000, 1000, 1); + db.beginTransaction(SQLite::TRANSACTION_TYPE::EXCLUSIVE); + db.write("CREATE TABLE testReturning(id INTEGER PRIMARY KEY, name STRING, value STRING, created DATE);"); + db.write("INSERT INTO testReturning VALUES(11, 'name1', 'value1', '2024-12-02');"); + db.write("INSERT INTO testReturning VALUES(21, 'name2', 'value2', '2024-12-03');"); + db.prepare(); + db.commit(); + + // When trying to delete a row by returning the deleted items + db.beginTransaction(SQLite::TRANSACTION_TYPE::SHARED); + SQResult result; + db.writeIdempotent("DELETE FROM testReturning WHERE id = 21 AND name = 'name2' RETURNING id, name;", result); + db.prepare(); + db.commit(); + + // Verify that deleted items were returned as expected + ASSERT_EQUAL("21", result[0][0]); + ASSERT_EQUAL("name2", result[0][1]); + + // Verify that the row was successfully deleted and now the table has only one row + db.beginTransaction(SQLite::TRANSACTION_TYPE::SHARED); + db.read("SELECT name, value FROM testReturning ORDER BY id;", result); + db.rollback(); + + ASSERT_EQUAL(1, result.size()); + ASSERT_EQUAL(result[0]["name"], "name1"); + ASSERT_EQUAL(result[0]["value"], "value1"); + } + void SRedactSensitiveValuesTest() { string logValue = R"({"edits":["test1", "test2", "test3"]})"; SRedactSensitiveValues(logValue); From 72bb681807990909932613676a8a6924a6d06e36 Mon Sep 17 00:00:00 2001 From: Monil Bhavsar Date: Mon, 9 Dec 2024 23:15:20 +0530 Subject: [PATCH 5/5] Update write method to accept result argument --- sqlitecluster/SQLite.cpp | 10 ++++++++++ sqlitecluster/SQLite.h | 6 +++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/sqlitecluster/SQLite.cpp b/sqlitecluster/SQLite.cpp index da14f4590..12e74c634 100644 --- a/sqlitecluster/SQLite.cpp +++ b/sqlitecluster/SQLite.cpp @@ -570,6 +570,16 @@ bool SQLite::write(const string& query) { return _writeIdempotent(query, ignore); } +bool SQLite::write(const string& query, SQResult& result) { + if (_noopUpdateMode) { + SALERT("Non-idempotent write in _noopUpdateMode. Query: " << query); + return true; + } + + // This is literally identical to the idempotent version except for the check for _noopUpdateMode. + return _writeIdempotent(query, result); +} + bool SQLite::writeIdempotent(const string& query) { SQResult ignore; return _writeIdempotent(query, ignore); diff --git a/sqlitecluster/SQLite.h b/sqlitecluster/SQLite.h index e8c68e3e9..ac3e786fd 100644 --- a/sqlitecluster/SQLite.h +++ b/sqlitecluster/SQLite.h @@ -101,10 +101,14 @@ class SQLite { bool addColumn(const string& tableName, const string& column, const string& columnType); // Performs a read/write query (eg, INSERT, UPDATE, DELETE). This is added to the current transaction's query list. - // Returns true on success. + // Returns true on success. // If we're in noop-update mode, this call alerts and performs no write, but returns as if it had completed. bool write(const string& query); + // Performs a read/write query + // Designed for use with queries that include a RETURNING clause + bool write(const string& query, SQResult& result); + // This is the same as `write` except it runs successfully without any warnings or errors in noop-update mode. // It's intended to be used for `mockRequest` enabled commands, such that we only run a version of them that's // known to be repeatable. What counts as repeatable is up to the individual command.