Merge pull request #1685 from Expensify/main

Update expensify_prod branch
Expensify · Mar 26, 2024 · 4d81213 · 4d81213
2 parents ea9b075 + e1672ed
commit 4d81213
Show file tree

Hide file tree

Showing 2 changed files with 70 additions and 8 deletions.
diff --git a/plugins/Jobs.cpp b/plugins/Jobs.cpp
@@ -281,12 +281,12 @@ bool BedrockJobsCommand::peek(SQLite& db) {
             }
 
             // Verify unique, but only do so when creating a single job using CreateJob
-            if (SIEquals(requestVerb, "CreateJob") && SContains(job, "unique") && job["unique"] == "true") {
+            if (SContains(job, "unique") && job["unique"] == "true") {
                 SQResult result;
                 SINFO("Unique flag was passed, checking existing job with name " << job["name"] << ", mocked? "
                       << (mockRequest ? "true" : "false"));
                 string operation = mockRequest ? "IS NOT" : "IS";
-                if (!db.read("SELECT jobID, data "
+                if (!db.read("SELECT jobID, data, parentJobID "
                              "FROM jobs "
                              "WHERE name=" + SQ(job["name"]) +
                              "  AND JSON_EXTRACT(data, '$.mockRequest') " + operation + " NULL;",
@@ -295,12 +295,19 @@ bool BedrockJobsCommand::peek(SQLite& db) {
                 }
 
                 // If there's no job or the existing job doesn't match the data we've been passed, escalate to leader.
-                if (!result.empty() && ((job["data"].empty() && result[0][1] == "{}") || (!job["data"].empty() && result[0][1] == job["data"]))) {
-                    // Return early, no need to pass to leader, there are no more jobs to create.
-                    SINFO("Job already existed and unique flag was passed, reusing existing job " << result[0][0] << ", mocked? "
-                      << (mockRequest ? "true" : "false"));
-                    jsonContent["jobID"] = result[0][0];
-                    return true;
+                if (!result.empty()) {
+                    // If the parent passed does not match the parent the job already had, then it must mean we did something
+                    // wrong or made a bad CQ, so we throw so we can investigate. Updating the parent here would be
+                    // confusing, as it could leave the original parent in a bad state (like for example paused forever)
+                    if (result[0][2] != "0" && result[0][2] != job["parentJobID"]) {
+                        STHROW("404 Trying to create a child that already exists, but it is tied to a different parent");
+                    }
+                    if (SIEquals(requestVerb, "CreateJob") && ((job["data"].empty() && result[0][1] == "{}") || (!job["data"].empty() && result[0][1] == job["data"]))) {
+                        // Return early, no need to pass to leader, there are no more jobs to create.
+                        SINFO("Job already existed and unique flag was passed, reusing existing job " << result[0][0] << ", mocked? " << (mockRequest ? "true" : "false"));
+                        jsonContent["jobID"] = result[0][0];
+                        return true;
+                    }
                 }
             }
         }

diff --git a/test/tests/jobs/CreateJobsTest.cpp b/test/tests/jobs/CreateJobsTest.cpp
@@ -11,6 +11,7 @@ struct CreateJobsTest : tpunit::TestFixture {
                               TEST(CreateJobsTest::createWithInvalidJson),
                               TEST(CreateJobsTest::createWithParentIDNotRunning),
                               TEST(CreateJobsTest::createWithParentMocked),
+                              TEST(CreateJobsTest::createUniqueChildWithWrongParent),
                               AFTER(CreateJobsTest::tearDown),
                               AFTER_CLASS(CreateJobsTest::tearDownClass)) { }
 
@@ -195,4 +196,58 @@ struct CreateJobsTest : tpunit::TestFixture {
 
         ASSERT_EQUAL(result.rows.size(), 0);
     }
+
+    void createUniqueChildWithWrongParent()
+    {
+        // Create 2 parents
+        SData command("CreateJob");
+        command["name"] = "createWithParent1";
+        string response = tester->executeWaitVerifyContent(command);
+        STable responseJSON = SParseJSONObject(response);
+        string parentID1 = responseJSON["jobID"];
+        command.clear();
+        command.methodLine = "CreateJob";
+        command["name"] = "createWithParent2";
+        response = tester->executeWaitVerifyContent(command);
+        responseJSON = SParseJSONObject(response);
+        string parentID2 = responseJSON["jobID"];
+
+        // Get the parents (to set it running)
+        command.clear();
+        command.methodLine = "GetJob";
+        command["name"] = "createWithParent1";
+        tester->executeWaitVerifyContent(command);
+        command.clear();
+        command.methodLine = "GetJob";
+        command["name"] = "createWithParent2";
+        tester->executeWaitVerifyContent(command);
+
+        // Send the command to create the child in parent1
+        command.clear();
+        command.methodLine = "CreateJobs";
+        STable job1Content;
+        STable data1;
+        data1["blabla"] = "blabla";
+        job1Content["name"] = "createWithParent_child";
+        job1Content["data"] = SComposeJSONObject(data1);
+        job1Content["parentJobID"] = parentID1;
+        vector<string> jobs;
+        jobs.push_back(SComposeJSONObject(job1Content));
+        command["jobs"] = SComposeJSONArray(jobs);
+        tester->executeWaitVerifyContent(command);
+
+        // Try to create the same child with unique, but pass parent2 instead
+        command.clear();
+        command.methodLine = "CreateJobs";
+        data1["blabla"] = "blabla";
+        job1Content["name"] = "createWithParent_child";
+        job1Content["unique"] = "true";
+        job1Content["data"] = SComposeJSONObject(data1);
+        job1Content["parentJobID"] = parentID2;
+        jobs.clear();
+        jobs.push_back(SComposeJSONObject(job1Content));
+        command["jobs"] = SComposeJSONArray(jobs);
+        tester->executeWaitVerifyContent(command, "404 Trying to create a child that already exists, but it is tied to a different parent");
+    }
+
 } __CreateJobsTest;