diff --git a/plugins/Jobs.cpp b/plugins/Jobs.cpp index e700ff47f..7ef28fede 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 ""; } @@ -1377,32 +1377,45 @@ string BedrockJobsCommand::_constructNextRunDATETIME(const string& lastScheduled } // Make sure the first part indicates the base (eg, what we are modifying) - list safeParts; - string base = parts.front(); + string nextRun = parts.front(); parts.pop_front(); - if (base == "SCHEDULED") { - safeParts.push_back(SQ(lastScheduled)); - } else if (base == "STARTED") { - safeParts.push_back(SQ(lastRun)); - } else if (base == "FINISHED") { - safeParts.push_back(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 ""; } for (const string& part : parts) { - // Validate the sqlite date modifiers - if (!SIsValidSQLiteDateModifier(part)){ - SWARN("Syntax error, failed parsing repeat "+part); + // 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', " + nextRun + ");", result) || result.empty()) { + SWARN("Syntax error, failed parsing repeat " + part); + return ""; + } + + 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(" + nextRun + ", " + SQ(part) + ");", result) || result.empty()) { + SWARN("Syntax error, failed parsing repeat " + part); + return ""; + } - safeParts.push_back(SQ(part)); + nextRun = SQ(result[0][0]); + } } - // Combine the parts together and return the full DATETIME statement - return "DATETIME( " + SComposeList(safeParts) + " )"; + return "DATETIME(" + nextRun + ")"; } // ========================================================================== 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); diff --git a/sqlitecluster/SQLite.cpp b/sqlitecluster/SQLite.cpp index 2ca08af57..181988ed9 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 ac5dd30e0..509ff47fb 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! diff --git a/test/tests/jobs/RetryJobTest.cpp b/test/tests/jobs/RetryJobTest.cpp index 6544edc89..fb350130c 100644 --- a/test/tests/jobs/RetryJobTest.cpp +++ b/test/tests/jobs/RetryJobTest.cpp @@ -16,6 +16,8 @@ struct RetryJobTest : tpunit::TestFixture { TEST(RetryJobTest::positiveDelay), TEST(RetryJobTest::delayError), TEST(RetryJobTest::hasRepeat), + TEST(RetryJobTest::hasRepeatStartOfHour), + TEST(RetryJobTest::hasRepeatStartOfHourNotLast), TEST(RetryJobTest::inRunqueuedState), TEST(RetryJobTest::simplyRetryWithNextRun), TEST(RetryJobTest::changeNameAndPriority), @@ -334,6 +336,68 @@ 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"] = "SCHEDULED, START OF DAY, +5 MINUTES, START OF HOUR"; + command["firstRun"] = SComposeTime("%Y-%m-%d %H:%M:%S", 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)); + } + + // 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"] = "SCHEDULED, START OF DAY, +30 MINUTES, START OF HOUR, +5 MINUTES"; + command["firstRun"] = SComposeTime("%Y-%m-%d %H:%M:%S", 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