Skip to content

Commit

Permalink
Merge pull request #1682 from Expensify/ionatan_throwparentdifferent
Browse files Browse the repository at this point in the history
Throw if parent of unique job is different than the one saved
  • Loading branch information
pecanoro authored Mar 26, 2024
2 parents 7bb1389 + 01aad67 commit e1672ed
Show file tree
Hide file tree
Showing 2 changed files with 70 additions and 8 deletions.
23 changes: 15 additions & 8 deletions plugins/Jobs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;",
Expand All @@ -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;
}
}
}
}
Expand Down
55 changes: 55 additions & 0 deletions test/tests/jobs/CreateJobsTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)) { }

Expand Down Expand Up @@ -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;

0 comments on commit e1672ed

Please sign in to comment.