Disallow more non-deterministic queries in writes

sqlitecluster/SQLite.cpp

@@ -988,14 +988,21 @@ int SQLite::_authorize(int actionCode, const char* detail1, const char* detail2,
             !strcmp(detail2, "strftime") ||
             !strcmp(detail2, "changes") ||
             !strcmp(detail2, "last_insert_rowid") ||
-            !strcmp(detail2, "sqlite3_version")
+            !strcmp(detail2, "sqlite_version")
         ) {
             _isDeterministicQuery = false;
         }
 
-        if (!strcmp(detail2, "current_timestamp")) {
+        // Prevent using certain non-deterministic functions in writes which could cause synchronization with followers to
+        // result in inconsistent data. Some are not included here because they can be used in a deterministic way that is valid.
+        // i.e. you can do UPDATE x = DATE('2024-01-01') and its deterministic whereas UPDATE x = DATE('now') is not. It's up to
+        // callers to prevent using these functions inappropriately.
+        if (!strcmp(detail2, "current_timestamp") ||
+            !strcmp(detail2, "random") ||
+            !strcmp(detail2, "last_insert_rowid") ||
+            !strcmp(detail2, "changes") ||
+            !strcmp(detail2, "sqlite_version")) {
             if (_currentlyWriting) {
-                // Prevent using `current_timestamp` in writes which could cause synchronization with followers to result in inconsistent data.
                 return SQLITE_DENY;
             }
         }

sqlitecluster/SQLite.h

@@ -468,6 +468,15 @@ class SQLite {
     void _checkInterruptErrors(const string& error) const;
 
     // Called internally by _sqliteAuthorizerCallback to authorize columns for a query.
+    //
+    // PRO-TIP: you can play with the authorizer using the `sqlite3` CLI tool, by running `.auth ON` then running
+    // your query. The columns displayed are the same as what is passed to this function.
+    //
+    // The information passed to this function is different based on the first parameter, actionCode.
+    // You can see what information is passed for each action code here https://www.sqlite.org/c3ref/c_alter_table.html.
+    // Note that as of writing this comment, the page seems slightly out of date and the parameter numbers are all off
+    // by one. That is, the first paramter passed to the callback funciton is actually the integer action code, not the
+    // second.
     int _authorize(int actionCode, const char* detail1, const char* detail2, const char* detail3, const char* detail4);
 
     // It's possible for certain transactions (namely, timing out a write operation, see here:

test/tests/WriteTest.cpp

@@ -1,4 +1,5 @@
 #include <libstuff/SData.h>
+#include <libstuff/SRandom.h>
 #include <test/lib/BedrockTester.h>
 
 struct WriteTest : tpunit::TestFixture {
@@ -18,7 +19,7 @@ struct WriteTest : tpunit::TestFixture {
                               TEST(WriteTest::updateAndInsertWithHttp),
                               TEST(WriteTest::shortHandSyntax),
                               TEST(WriteTest::keywordsAsValue),
-                              TEST(WriteTest::blockTimeFunctions),
+                              TEST(WriteTest::blockNonDeterministicFunctions),
                               AFTER_CLASS(WriteTest::tearDown)) { }
 
     BedrockTester* tester;
@@ -38,7 +39,8 @@ struct WriteTest : tpunit::TestFixture {
         for (int i = 0; i < 50; i++) {
             SData query("Query");
             query["writeConsistency"] = "ASYNC";
-            query["query"] = "INSERT INTO foo VALUES ( RANDOM() );";
+            uint64_t rand = SRandom::rand64();
+            query["query"] = "INSERT INTO foo VALUES (" + to_string(rand) + ");";
             tester->executeWaitVerifyContent(query);
         }
 
@@ -181,7 +183,7 @@ struct WriteTest : tpunit::TestFixture {
         tester->executeWaitVerifyContent(query3);
     }
 
-    void blockTimeFunctions() {
+    void blockNonDeterministicFunctions() {
         // Verify writing the string 'CURRENT_TIMESTAMP' is fine.
         SData query("query: INSERT INTO stuff VALUES ( NULL, 11, 'CURRENT_TIMESTAMP' );");
         tester->executeWaitVerifyContent(query);
@@ -193,6 +195,18 @@ struct WriteTest : tpunit::TestFixture {
         // But allow the function to run in reads.
         query.methodLine = "query: SELECT CURRENT_TIMESTAMP;";
         tester->executeWaitVerifyContent(query);
+
+        // Verify writing the string 'RANDOM' is fine.
+        query.methodLine = "query: INSERT INTO stuff VALUES ( NULL, 11, 'RANDOM' );";
+        tester->executeWaitVerifyContent(query);
+
+        // But verify calling the function RANDOM is blocked when writing.
+        query.methodLine = "query: INSERT INTO stuff VALUES ( NULL, 11, RANDOM() );";
+        tester->executeWaitVerifyContent(query, "502 Query failed");
+
+        // But allow the function to run in reads.
+        query.methodLine = "query: SELECT random();";
+        tester->executeWaitVerifyContent(query);
     }
 
 } __WriteTest;