Merge pull request #2019 from Expensify/main

sqlitecluster/SQLite.cpp

@@ -1012,14 +1012,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

@@ -480,6 +480,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:

sqlitecluster/SQLiteNode.cpp

@@ -56,20 +56,6 @@
 // dbCountAtStart:   The highest committed transaction in the DB at the start of this transaction on leader, for
 //                   optimizing replication.
 
-// NOTE: This comment as well as NODE_LOGIN should be removed after https://github.com/Expensify/Bedrock/pull/1999 is deployed.
-// On LOGIN vs NODE_LOGIN.
-// _onConnect sends a LOGIN message.
-// _onConnect is called in exctly two places:
-// 1. In response to a NODE_LOGIN message received on a newly connected socket on the sync port. It's expected when
-//    establishing a connection, a node sends this NODE_LOGIN as its first message.
-// 2. Immediately following establishing a TCP connection to another node and sending a NODE_LOGIN message. In the case that
-//    we are the initiating node, we immediately queue three messages:
-//    1. NODE_LOGIN
-//    2. PING
-//    3. LOGIN
-//
-// When we receive a NODE_LOGIN, we immediately respond with a PING followed by a LOGIN (by calling _onConnect).
-
 #undef SLOGPREFIX
 #define SLOGPREFIX "{" << _name << "/" << SQLiteNode::stateName(_state) << "} "
 
@@ -1269,7 +1255,10 @@ void SQLiteNode::_onMESSAGE(SQLitePeer* peer, const SData& message) {
             SINFO("Received PONG from peer '" << peer->name << "' (" << peer->latency/1000 << "ms latency)");
             return;
         } else if (SIEquals(message.methodLine, "NODE_LOGIN")) {
-            // Do nothing, this keeps this code from warning until NODE_LOGIN is deprecated.
+            // We need to return early here to ignore this deprecated message and avoid throwing:
+            // STHROW("not logged in");
+            // Below. We can remove this check after one more deploy cycle.
+            // https://github.com/Expensify/Expensify/issues/450953
             return;
         }
 
@@ -2553,7 +2542,7 @@ void SQLiteNode::postPoll(fd_map& fdm, uint64_t& nextActivity) {
     // with peers, so we store any that we can remove in this list.
     list<Socket*> socketsToRemove;
 
-    // Check each new connection for a NODE_LOGIN message.
+    // Check each new connection for a LOGIN message.
     for (auto socket : _unauthenticatedIncomingSockets) {
         STCPManager::postPoll(fdm, *socket);
         try {
@@ -2565,7 +2554,9 @@ void SQLiteNode::postPoll(fd_map& fdm, uint64_t& nextActivity) {
             int messageSize = message.deserialize(socket->recvBuffer);
             if (messageSize) {
                 socket->recvBuffer.consumeFront(messageSize);
-                // Allow either LOGIN or NODE_LOGIN until we deprecate NODE_LOGIN.
+                // Old nodes, for one more upgrade cycle, will still send `NODE_LOGIN`. We can remove this check after this
+                // code is deployed.
+                // See: https://github.com/Expensify/Expensify/issues/450953
                 if (SIEquals(message.methodLine, "NODE_LOGIN") || SIEquals(message.methodLine, "LOGIN")) {
                     SQLitePeer* peer = getPeerByName(message["Name"]);
                     if (peer) {
@@ -2591,7 +2582,7 @@ void SQLiteNode::postPoll(fd_map& fdm, uint64_t& nextActivity) {
                         STHROW("Unauthenticated node '" + message["Name"] + "' attempted to connected, rejecting.");
                     }
                 } else {
-                    STHROW("expecting LOGIN or NODE_LOGIN");
+                    STHROW("expecting LOGIN");
                 }
             } else if (STimeNow() > socket->lastRecvTime + 5'000'000) {
                 STHROW("Incoming socket didn't send a message for over 5s, closing.");
@@ -2614,10 +2605,6 @@ void SQLiteNode::postPoll(fd_map& fdm, uint64_t& nextActivity) {
         switch (result) {
             case SQLitePeer::PeerPostPollStatus::JUST_CONNECTED:
             {
-                // When NODE_LOGIN is deprecated, we can remove the next 3 lines.
-                SData login("NODE_LOGIN");
-                login["Name"] = _name;
-                _sendToPeer(peer, login);
                 _onConnect(peer);
                 _sendPING(peer);
             }

sqlitecluster/SQLiteNode.h

@@ -181,7 +181,7 @@ class SQLiteNode : public STCPManager {
     // would be a good idea for the caller to read any new commands or traffic from the network.
     bool update();
 
-    // Look up the correct peer by the name it supplies in a NODE_LOGIN
+    // Look up the correct peer by the name it supplies in a LOGIN
     // message. Does not lock, but this method is const and all it does is
     // access _peerList and peer->name, both of which are const. So it is safe
     // to call from other public functions.
@@ -293,7 +293,7 @@ class SQLiteNode : public STCPManager {
     const string _version;
 
     // These are sockets that have been accepted on the node port but have not yet been associated with a peer (because
-    // they need to send a NODE_LOGIN message with their name first).
+    // they need to send a LOGIN message with their name first).
     set<Socket*> _unauthenticatedIncomingSockets;
 
     // The write consistency requested for the current in-progress commit.

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;