From 3cf706675287b410e16487e478c527ea7d249369 Mon Sep 17 00:00:00 2001 From: Rafe Colton Date: Fri, 10 Nov 2023 21:08:03 +0000 Subject: [PATCH 01/10] Pass DB handle to _constructNextRunDATETIME --- plugins/Jobs.cpp | 14 +++++++------- plugins/Jobs.h | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/plugins/Jobs.cpp b/plugins/Jobs.cpp index e700ff47f..87a8665bc 100644 --- a/plugins/Jobs.cpp +++ b/plugins/Jobs.cpp @@ -515,7 +515,7 @@ void BedrockJobsCommand::process(SQLite& db) { SWARN("Repeat is set in CreateJob, but is set to the empty string. Job Name: " << job["name"] << ", removing attribute."); job.erase("repeat"); - } else if (!_validateRepeat(job["repeat"])) { + } else if (!_validateRepeat(db, job["repeat"])) { STHROW("402 Malformed repeat"); } } @@ -840,7 +840,7 @@ void BedrockJobsCommand::process(SQLite& db) { } string currentTime = SUNQUOTED_CURRENT_TIMESTAMP(); string retryAfterDateTime = "DATETIME(" + SQ(currentTime) + ", " + SQ(job["retryAfter"]) + ")"; - string repeatDateTime = _constructNextRunDATETIME(job["nextRun"], currentTime, job["repeat"]); + string repeatDateTime = _constructNextRunDATETIME(db, job["nextRun"], currentTime, job["repeat"]); string nextRunDateTime = repeatDateTime != "" ? "MIN(" + retryAfterDateTime + ", " + repeatDateTime + ")" : retryAfterDateTime; bool isRepeatBasedOnScheduledTime = SToUpper(job["repeat"]).find("SCHEDULED") != string::npos; string dataUpdateQuery = " "; @@ -908,7 +908,7 @@ void BedrockJobsCommand::process(SQLite& db) { if (request["repeat"].empty()) { SWARN("Repeat is set in UpdateJob, but is set to the empty string. jobID: " << request["jobID"] << "."); - } else if (!_validateRepeat(request["repeat"])) { + } else if (!_validateRepeat(db, request["repeat"])) { STHROW("402 Malformed repeat"); } } @@ -946,7 +946,7 @@ void BedrockJobsCommand::process(SQLite& db) { // Passed next run takes priority over the one computed via the repeat feature string newNextRun; if (request["nextRun"].empty()) { - newNextRun = request["repeat"].size() ? _constructNextRunDATETIME(nextRun, lastRun, request["repeat"]) : ""; + newNextRun = request["repeat"].size() ? _constructNextRunDATETIME(db, nextRun, lastRun, request["repeat"]) : ""; } else { newNextRun = SQ(request["nextRun"]); } @@ -1134,7 +1134,7 @@ void BedrockJobsCommand::process(SQLite& db) { if (!retryAfter.empty() && SToUpper(repeat).find("SCHEDULED") != string::npos) { lastScheduled = originalDataNextRun; } - safeNewNextRun = _constructNextRunDATETIME(lastScheduled, lastRun, repeat); + safeNewNextRun = _constructNextRunDATETIME(db, lastScheduled, lastRun, repeat); } else if (SIEquals(requestVerb, "RetryJob")) { const string& newNextRun = request["nextRun"]; @@ -1145,7 +1145,7 @@ void BedrockJobsCommand::process(SQLite& db) { STHROW("402 Must specify a non-negative delay when retrying"); } repeat = "FINISHED, +" + SToStr(delay) + " SECONDS"; - safeNewNextRun = _constructNextRunDATETIME(nextRun, lastRun, repeat); + safeNewNextRun = _constructNextRunDATETIME(db, nextRun, lastRun, repeat); if (safeNewNextRun.empty()) { STHROW("402 Malformed delay"); } @@ -1356,7 +1356,7 @@ void BedrockJobsCommand::process(SQLite& db) { } } -string BedrockJobsCommand::_constructNextRunDATETIME(const string& lastScheduled, const string& lastRun, const string& repeat) { +string BedrockJobsCommand::_constructNextRunDATETIME(SQLite& db, const string& lastScheduled, const string& lastRun, const string& repeat) { if (repeat.empty()) { return ""; } diff --git a/plugins/Jobs.h b/plugins/Jobs.h index 54a19c359..e056249ea 100644 --- a/plugins/Jobs.h +++ b/plugins/Jobs.h @@ -33,8 +33,8 @@ class BedrockJobsCommand : public BedrockCommand { private: // Helper functions - string _constructNextRunDATETIME(const string& lastScheduled, const string& lastRun, const string& repeat); - bool _validateRepeat(const string& repeat) { return !_constructNextRunDATETIME("", "", repeat).empty(); } + string _constructNextRunDATETIME(SQLite& db, const string& lastScheduled, const string& lastRun, const string& repeat); + bool _validateRepeat(SQLite& db, const string& repeat) { return !_constructNextRunDATETIME(db, "", "", repeat).empty(); } bool _hasPendingChildJobs(SQLite& db, int64_t jobID); void _validatePriority(const int64_t priority); From 56f7a5dd718c41bf2ec6b81898533bfa7995a71a Mon Sep 17 00:00:00 2001 From: Rafe Colton Date: Fri, 10 Nov 2023 21:32:21 +0000 Subject: [PATCH 02/10] Refactor to apply parts during loop --- plugins/Jobs.cpp | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/plugins/Jobs.cpp b/plugins/Jobs.cpp index 87a8665bc..de5dd2e76 100644 --- a/plugins/Jobs.cpp +++ b/plugins/Jobs.cpp @@ -1377,32 +1377,39 @@ string BedrockJobsCommand::_constructNextRunDATETIME(SQLite& db, const string& l } // Make sure the first part indicates the base (eg, what we are modifying) - list safeParts; string base = parts.front(); parts.pop_front(); if (base == "SCHEDULED") { - safeParts.push_back(SQ(lastScheduled)); + base = SQ(lastScheduled); } else if (base == "STARTED") { - safeParts.push_back(SQ(lastRun)); + base = SQ(lastRun); } else if (base == "FINISHED") { - safeParts.push_back(SCURRENT_TIMESTAMP()); + base = SCURRENT_TIMESTAMP(); } else { SWARN("Syntax error, failed parsing repeat '" << repeat << "': missing base (" << base << ")"); return ""; } for (const string& part : parts) { - // Validate the sqlite date modifiers - if (!SIsValidSQLiteDateModifier(part)){ + if (SToUpper(part) == "START OF HOUR") { + // Transform to strftime. + + } else if (!SIsValidSQLiteDateModifier(part)){ + // Validate the sqlite date modifiers SWARN("Syntax error, failed parsing repeat "+part); return ""; - } + } else { + SQResult result; + if (!db.read("SELECT DATETIME(" + base + ", " + SQ(part) + ");", result) || result.empty()) { + SWARN("Syntax error, failed parsing repeat "+part); + return ""; + } - safeParts.push_back(SQ(part)); + base = SQ(result[0][0]); + } } - // Combine the parts together and return the full DATETIME statement - return "DATETIME( " + SComposeList(safeParts) + " )"; + return base; } // ========================================================================== From e1e2d4d8d5e3e73e1786181b8b1295489cd097e9 Mon Sep 17 00:00:00 2001 From: Rafe Colton Date: Fri, 10 Nov 2023 21:38:33 +0000 Subject: [PATCH 03/10] Add support for START OF HOUR --- plugins/Jobs.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/plugins/Jobs.cpp b/plugins/Jobs.cpp index de5dd2e76..f5a732e4e 100644 --- a/plugins/Jobs.cpp +++ b/plugins/Jobs.cpp @@ -1391,12 +1391,18 @@ string BedrockJobsCommand::_constructNextRunDATETIME(SQLite& db, const string& l } for (const string& part : parts) { + // This isn't supported natively by SQLite, so do it manually here instead. if (SToUpper(part) == "START OF HOUR") { - // Transform to strftime. + SQResult result; + if (!db.read("SELECT STRFTIME('%Y-%m-%d %H:00:00', " + base + ");", result) || result.empty()) { + SWARN("Syntax error, failed parsing repeat "+part); + return ""; + } + base = SQ(result[0][0]); } else if (!SIsValidSQLiteDateModifier(part)){ // Validate the sqlite date modifiers - SWARN("Syntax error, failed parsing repeat "+part); + SWARN("Syntax error, failed parsing repeat " + part); return ""; } else { SQResult result; From 8f50a8503bb8b38ff375c88a1cce26500138533a Mon Sep 17 00:00:00 2001 From: Rafe Colton Date: Sat, 11 Nov 2023 01:12:13 +0000 Subject: [PATCH 04/10] Add test for START OF HOUR --- test/tests/jobs/RetryJobTest.cpp | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/test/tests/jobs/RetryJobTest.cpp b/test/tests/jobs/RetryJobTest.cpp index 6544edc89..44d79eb0c 100644 --- a/test/tests/jobs/RetryJobTest.cpp +++ b/test/tests/jobs/RetryJobTest.cpp @@ -16,6 +16,7 @@ struct RetryJobTest : tpunit::TestFixture { TEST(RetryJobTest::positiveDelay), TEST(RetryJobTest::delayError), TEST(RetryJobTest::hasRepeat), + TEST(RetryJobTest::hasRepeatStartOfHour), TEST(RetryJobTest::inRunqueuedState), TEST(RetryJobTest::simplyRetryWithNextRun), TEST(RetryJobTest::changeNameAndPriority), @@ -334,6 +335,37 @@ struct RetryJobTest : tpunit::TestFixture { ASSERT_EQUAL(difftime(nextRunTime, createdTime), 3600); } + // Retry a job with a repeat that uses the custom "START OF HOUR" modifier + void hasRepeatStartOfHour() { + uint64_t now = STimeNow(); + + // Create the job + SData command("CreateJob"); + command["name"] = "job"; + command["repeat"] = "STARTED, START OF DAY, +5 MINUTES, START OF HOUR"; + command["nextRun"] = now; + STable response = tester->executeWaitVerifyContentTable(command); + string jobID = response["jobID"]; + + // Get the job + command.clear(); + command.methodLine = "GetJob"; + command["name"] = "job"; + tester->executeWaitVerifyContent(command); + + // Retry it + command.clear(); + command.methodLine = "RetryJob"; + command["jobID"] = jobID; + tester->executeWaitVerifyContent(command); + + // Confirm nextRun is at the beginning of the day and not 5 minutes after + SQResult result; + tester->readDB("SELECT nextRun FROM jobs WHERE jobID = " + jobID + ";", result); + ASSERT_EQUAL(result.size(), 1); + ASSERT_EQUAL(result[0][0], SComposeTime("%Y-%m-%d 00:00:00", now)); + } + // Retry job in RUNQUEUED state void inRunqueuedState() { // Create a job From 1d007f605f06b832e75ae73b5918cdca2a1264bc Mon Sep 17 00:00:00 2001 From: Rafe Colton Date: Sat, 11 Nov 2023 01:35:36 +0000 Subject: [PATCH 05/10] Fix bug Make sure we still surround the next run in `DATETIME()` so we catch invalid dates --- plugins/Jobs.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/Jobs.cpp b/plugins/Jobs.cpp index f5a732e4e..11de799f9 100644 --- a/plugins/Jobs.cpp +++ b/plugins/Jobs.cpp @@ -1415,7 +1415,7 @@ string BedrockJobsCommand::_constructNextRunDATETIME(SQLite& db, const string& l } } - return base; + return "DATETIME(" + base + ")"; } // ========================================================================== From 8ab925c98d7d9533497b26a4201881d09660dbc1 Mon Sep 17 00:00:00 2001 From: Rafe Colton Date: Sat, 11 Nov 2023 01:38:33 +0000 Subject: [PATCH 06/10] Use more appropriate variable name --- plugins/Jobs.cpp | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/plugins/Jobs.cpp b/plugins/Jobs.cpp index 11de799f9..7288f3834 100644 --- a/plugins/Jobs.cpp +++ b/plugins/Jobs.cpp @@ -1377,16 +1377,16 @@ string BedrockJobsCommand::_constructNextRunDATETIME(SQLite& db, const string& l } // Make sure the first part indicates the base (eg, what we are modifying) - string base = parts.front(); + string nextRun = parts.front(); parts.pop_front(); - if (base == "SCHEDULED") { - base = SQ(lastScheduled); - } else if (base == "STARTED") { - base = SQ(lastRun); - } else if (base == "FINISHED") { - base = SCURRENT_TIMESTAMP(); + if (nextRun == "SCHEDULED") { + nextRun = SQ(lastScheduled); + } else if (nextRun == "STARTED") { + nextRun = SQ(lastRun); + } else if (nextRun == "FINISHED") { + nextRun = SCURRENT_TIMESTAMP(); } else { - SWARN("Syntax error, failed parsing repeat '" << repeat << "': missing base (" << base << ")"); + SWARN("Syntax error, failed parsing repeat '" << repeat << "': missing base (" << nextRun << ")"); return ""; } @@ -1394,28 +1394,28 @@ string BedrockJobsCommand::_constructNextRunDATETIME(SQLite& db, const string& l // This isn't supported natively by SQLite, so do it manually here instead. if (SToUpper(part) == "START OF HOUR") { SQResult result; - if (!db.read("SELECT STRFTIME('%Y-%m-%d %H:00:00', " + base + ");", result) || result.empty()) { + if (!db.read("SELECT STRFTIME('%Y-%m-%d %H:00:00', " + nextRun + ");", result) || result.empty()) { SWARN("Syntax error, failed parsing repeat "+part); return ""; } - base = SQ(result[0][0]); + nextRun = SQ(result[0][0]); } else if (!SIsValidSQLiteDateModifier(part)){ // Validate the sqlite date modifiers SWARN("Syntax error, failed parsing repeat " + part); return ""; } else { SQResult result; - if (!db.read("SELECT DATETIME(" + base + ", " + SQ(part) + ");", result) || result.empty()) { + if (!db.read("SELECT DATETIME(" + nextRun + ", " + SQ(part) + ");", result) || result.empty()) { SWARN("Syntax error, failed parsing repeat "+part); return ""; } - base = SQ(result[0][0]); + nextRun = SQ(result[0][0]); } } - return "DATETIME(" + base + ")"; + return "DATETIME(" + nextRun + ")"; } // ========================================================================== From 64040b651219e64862ecd8a3fdd1eca02442e74d Mon Sep 17 00:00:00 2001 From: Rafe Colton Date: Mon, 13 Nov 2023 19:16:18 +0000 Subject: [PATCH 07/10] Style and add another test, per feedback --- plugins/Jobs.cpp | 4 ++-- test/tests/jobs/RetryJobTest.cpp | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/plugins/Jobs.cpp b/plugins/Jobs.cpp index 7288f3834..7ef28fede 100644 --- a/plugins/Jobs.cpp +++ b/plugins/Jobs.cpp @@ -1395,7 +1395,7 @@ string BedrockJobsCommand::_constructNextRunDATETIME(SQLite& db, const string& l if (SToUpper(part) == "START OF HOUR") { SQResult result; if (!db.read("SELECT STRFTIME('%Y-%m-%d %H:00:00', " + nextRun + ");", result) || result.empty()) { - SWARN("Syntax error, failed parsing repeat "+part); + SWARN("Syntax error, failed parsing repeat " + part); return ""; } @@ -1407,7 +1407,7 @@ string BedrockJobsCommand::_constructNextRunDATETIME(SQLite& db, const string& l } else { SQResult result; if (!db.read("SELECT DATETIME(" + nextRun + ", " + SQ(part) + ");", result) || result.empty()) { - SWARN("Syntax error, failed parsing repeat "+part); + SWARN("Syntax error, failed parsing repeat " + part); return ""; } diff --git a/test/tests/jobs/RetryJobTest.cpp b/test/tests/jobs/RetryJobTest.cpp index 44d79eb0c..eb204f9a2 100644 --- a/test/tests/jobs/RetryJobTest.cpp +++ b/test/tests/jobs/RetryJobTest.cpp @@ -17,6 +17,7 @@ struct RetryJobTest : tpunit::TestFixture { TEST(RetryJobTest::delayError), TEST(RetryJobTest::hasRepeat), TEST(RetryJobTest::hasRepeatStartOfHour), + TEST(RetryJobTest::hasRepeatStartOfHourNotLast), TEST(RetryJobTest::inRunqueuedState), TEST(RetryJobTest::simplyRetryWithNextRun), TEST(RetryJobTest::changeNameAndPriority), @@ -366,6 +367,37 @@ struct RetryJobTest : tpunit::TestFixture { ASSERT_EQUAL(result[0][0], SComposeTime("%Y-%m-%d 00:00:00", now)); } + // Same as hasRepeatStartOfHour but "START OF HOUR" is not last, to confirm it works when not last. + void hasRepeatStartOfHourNotLast() { + uint64_t now = STimeNow(); + + // Create the job + SData command("CreateJob"); + command["name"] = "job"; + command["repeat"] = "STARTED, START OF DAY, +30 MINUTES, START OF HOUR, +5 MINUTES"; + command["nextRun"] = now; + STable response = tester->executeWaitVerifyContentTable(command); + string jobID = response["jobID"]; + + // Get the job + command.clear(); + command.methodLine = "GetJob"; + command["name"] = "job"; + tester->executeWaitVerifyContent(command); + + // Retry it + command.clear(); + command.methodLine = "RetryJob"; + command["jobID"] = jobID; + tester->executeWaitVerifyContent(command); + + // Confirm nextRun is at the beginning of the day and not 5 minutes after + SQResult result; + tester->readDB("SELECT nextRun FROM jobs WHERE jobID = " + jobID + ";", result); + ASSERT_EQUAL(result.size(), 1); + ASSERT_EQUAL(result[0][0], SComposeTime("%Y-%m-%d 00:05:00", now)); + } + // Retry job in RUNQUEUED state void inRunqueuedState() { // Create a job From 87757e20c1a6bcfa6c1e9f5968f963a31590cfcb Mon Sep 17 00:00:00 2001 From: Ionatan Wiznia Date: Mon, 13 Nov 2023 16:06:04 -0600 Subject: [PATCH 08/10] Support view in verifyTable --- sqlitecluster/SQLite.cpp | 4 ++-- sqlitecluster/SQLite.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/sqlitecluster/SQLite.cpp b/sqlitecluster/SQLite.cpp index 8dc02077c..6289541fe 100644 --- a/sqlitecluster/SQLite.cpp +++ b/sqlitecluster/SQLite.cpp @@ -385,13 +385,13 @@ bool SQLite::beginTransaction(TRANSACTION_TYPE type) { return _insideTransaction; } -bool SQLite::verifyTable(const string& tableName, const string& sql, bool& created) { +bool SQLite::verifyTable(const string& tableName, const string& sql, bool& created, const string& type) { // sqlite trims semicolon, so let's not supply it else we get confused later SASSERT(!SEndsWith(sql, ";")); // First, see if it's there SQResult result; - SASSERT(read("SELECT sql FROM sqlite_master WHERE type='table' AND tbl_name=" + SQ(tableName) + ";", result)); + SASSERT(read("SELECT sql FROM sqlite_master WHERE type=" + SQ(type) + " AND tbl_name=" + SQ(tableName) + ";", result)); const string& collapsedSQL = SCollapse(sql); if (result.empty()) { // Table doesn't already exist, create it diff --git a/sqlitecluster/SQLite.h b/sqlitecluster/SQLite.h index 6321ac6da..8a9683d09 100644 --- a/sqlitecluster/SQLite.h +++ b/sqlitecluster/SQLite.h @@ -93,7 +93,7 @@ class SQLite { // Verifies a table exists and has a particular definition. If the database is left with the right schema, it // returns true. If it had to create a new table (ie, the table was missing), it also sets created to true. If the // table is already there with the wrong schema, it returns false. - bool verifyTable(const string& name, const string& sql, bool& created); + bool verifyTable(const string& name, const string& sql, bool& created, const string& type = "table"); // Verifies an index exists on the given table with the given definition. Optionally create it if it doesn't exist. // Be careful, creating an index can be expensive on large tables! From b934a2ec5d63b777be537501077d7d0be286a524 Mon Sep 17 00:00:00 2001 From: Rafe Colton Date: Mon, 13 Nov 2023 22:23:38 +0000 Subject: [PATCH 09/10] Use `SCHEDULED` > `STARTED` to avoid flaky tests --- test/tests/jobs/RetryJobTest.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/tests/jobs/RetryJobTest.cpp b/test/tests/jobs/RetryJobTest.cpp index eb204f9a2..cf21c9ade 100644 --- a/test/tests/jobs/RetryJobTest.cpp +++ b/test/tests/jobs/RetryJobTest.cpp @@ -343,7 +343,7 @@ struct RetryJobTest : tpunit::TestFixture { // Create the job SData command("CreateJob"); command["name"] = "job"; - command["repeat"] = "STARTED, START OF DAY, +5 MINUTES, START OF HOUR"; + command["repeat"] = "SCHEDULED, START OF DAY, +5 MINUTES, START OF HOUR"; command["nextRun"] = now; STable response = tester->executeWaitVerifyContentTable(command); string jobID = response["jobID"]; @@ -374,7 +374,7 @@ struct RetryJobTest : tpunit::TestFixture { // Create the job SData command("CreateJob"); command["name"] = "job"; - command["repeat"] = "STARTED, START OF DAY, +30 MINUTES, START OF HOUR, +5 MINUTES"; + command["repeat"] = "SCHEDULED, START OF DAY, +30 MINUTES, START OF HOUR, +5 MINUTES"; command["nextRun"] = now; STable response = tester->executeWaitVerifyContentTable(command); string jobID = response["jobID"]; From 5b0e7356e3197ce257fb8e8ccbbf48c52e57e029 Mon Sep 17 00:00:00 2001 From: Rafe Colton Date: Tue, 14 Nov 2023 20:14:37 +0000 Subject: [PATCH 10/10] Fix possible flaky test --- test/tests/jobs/RetryJobTest.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/tests/jobs/RetryJobTest.cpp b/test/tests/jobs/RetryJobTest.cpp index cf21c9ade..fb350130c 100644 --- a/test/tests/jobs/RetryJobTest.cpp +++ b/test/tests/jobs/RetryJobTest.cpp @@ -344,7 +344,7 @@ struct RetryJobTest : tpunit::TestFixture { SData command("CreateJob"); command["name"] = "job"; command["repeat"] = "SCHEDULED, START OF DAY, +5 MINUTES, START OF HOUR"; - command["nextRun"] = now; + command["firstRun"] = SComposeTime("%Y-%m-%d %H:%M:%S", now); STable response = tester->executeWaitVerifyContentTable(command); string jobID = response["jobID"]; @@ -375,7 +375,7 @@ struct RetryJobTest : tpunit::TestFixture { SData command("CreateJob"); command["name"] = "job"; command["repeat"] = "SCHEDULED, START OF DAY, +30 MINUTES, START OF HOUR, +5 MINUTES"; - command["nextRun"] = now; + command["firstRun"] = SComposeTime("%Y-%m-%d %H:%M:%S", now); STable response = tester->executeWaitVerifyContentTable(command); string jobID = response["jobID"];