Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add optional overwrite parameter to CreateJob and CreateJobs #916

Merged
merged 6 commits into from Oct 27, 2020
Merged

Add optional overwrite parameter to CreateJob and CreateJobs #916

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7956e80
Add param to prevent overwriting existing, unique jobs
Oct 21, 2020
d1517bf
Merge branch 'master' into amechler-overwrite-create-job
Oct 22, 2020
b865b8a
Fix tests, remove SINFO
Oct 26, 2020
7f04ed1
Update commit
Oct 27, 2020
13e1c4e
Wrong param name
Oct 27, 2020
69d5240
Check for empty string as well
Oct 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 14 additions & 8 deletions plugins/Jobs.cpp
View file
Open in desktop
Expand Up @@ -402,6 +402,8 @@ void BedrockJobsCommand::process(SQLite& db) {
// - jobPriority - High priorities go first (optional, default 500)
// - unique - if true, it will check that no other job with this name already exists, if it does it will
// return that jobID
// - overwrite - if true, and unique is true, it will overwrite the existing job with the new jobs data
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// - overwrite - if true, and unique is true, it will overwrite the existing job with the new jobs data
// - overwrite - Only applicable when submit is is true. When set to true it will overwrite the existing job with the new jobs data

// (optional, default: true)
// - parentJobID - The ID of the parent job (optional)
// - retryAfter - Amount of auto-retries before marking job as failed (optional)
//
Expand All @@ -421,6 +423,8 @@ void BedrockJobsCommand::process(SQLite& db) {
// - jobPriority - High priorities go first (optional, default 500)
// - unique - if true, it will check that no other job with this name already exists, if it does it will
// return that jobID
// - overwrite - if true, and unique is true, it will overwrite the existing job with the new jobs data
// (optional, default: true)
// - parentJobID - The ID of the parent job (optional)
// - retryAfter - Amount of auto-retries before marking job as failed (optional)
//
Expand Down Expand Up @@ -566,14 +570,16 @@ void BedrockJobsCommand::process(SQLite& db) {

// Are we creating a new job, or updating an existing job?
if (updateJobID) {
// Update the existing job.
if(!db.writeIdempotent("UPDATE jobs SET "
"repeat = " + SQ(SToUpper(job["repeat"])) + ", " +
"data = JSON_PATCH(data, " + safeData + "), " +
"priority = " + SQ(priority) + " " +
"WHERE jobID = " + SQ(updateJobID) + ";"))
{
STHROW("502 update query failed");
if (!SContains(job, "overwrite") || job["overwrite"] == "true") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you also want to check if overwrite is == and treat it as true too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite following. What would we check if overwrite is == to?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damn sorry, I mean is equal to ''

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good catch, will add that

// Update the existing job.
if(!db.writeIdempotent("UPDATE jobs SET "
"repeat = " + SQ(SToUpper(job["repeat"])) + ", " +
"data = JSON_PATCH(data, " + safeData + "), " +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm you sure about PATCH? Don't we want to replace the whole data?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same query as before, just indented. I agree it probably should not be a PATCH, but I don't want to change existing behavior (in case any relies on this)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh... 🤦 you are right

"priority = " + SQ(priority) + " " +
"WHERE jobID = " + SQ(updateJobID) + ";"))
{
STHROW("502 update query failed");
}
}

// If we are calling CreateJob, return early, there are no more jobs to create.
Expand Down
22 changes: 22 additions & 0 deletions test/tests/jobs/CreateJobTest.cpp
View file
Open in desktop
Expand Up @@ -199,6 +199,28 @@ struct CreateJobTest : tpunit::TestFixture {
ASSERT_EQUAL(updatedJob[0][7], data);
ASSERT_EQUAL(updatedJob[0][8], originalJob[0][8]);
ASSERT_EQUAL(updatedJob[0][9], originalJob[0][9]);

// Try to recreate the job with new data, without overwriting the existing data
string data2 = "{\"blabla2\":\"test2\"}";
command["data"] = data2;
command["overwrite"] = "false";
response = tester->executeWaitVerifyContentTable(command);
ASSERT_EQUAL(stol(response["jobID"]), jobID);

SQResult nonoverwritenJob;
tester->readDB("SELECT created, jobID, state, name, nextRun, lastRun, repeat, data, priority, parentJobID FROM jobs WHERE jobID = " + response["jobID"] + ";", nonoverwritenJob);
ASSERT_EQUAL(updatedJob.size(), 1);
// Assert that we have not overwritten the job data
ASSERT_EQUAL(nonoverwritenJob[0][0], updatedJob[0][0]);
ASSERT_EQUAL(nonoverwritenJob[0][1], updatedJob[0][1]);
ASSERT_EQUAL(nonoverwritenJob[0][2], updatedJob[0][2]);
ASSERT_EQUAL(nonoverwritenJob[0][3], updatedJob[0][3]);
ASSERT_EQUAL(nonoverwritenJob[0][4], updatedJob[0][4]);
ASSERT_EQUAL(nonoverwritenJob[0][5], updatedJob[0][5]);
ASSERT_EQUAL(nonoverwritenJob[0][6], updatedJob[0][6]);
ASSERT_EQUAL(nonoverwritenJob[0][7], updatedJob[0][7]);
ASSERT_EQUAL(nonoverwritenJob[0][8], updatedJob[0][8]);
ASSERT_EQUAL(nonoverwritenJob[0][9], updatedJob[0][9]);
}

void createWithBadData() {
Expand Down