Improved checks for concurrent post edits

This commit is contained in:
Marcin Kurczewski 2014-06-10 10:40:07 +02:00
parent 538165e3ff
commit d242eedb31
21 changed files with 105 additions and 43 deletions

View file

@ -35,6 +35,19 @@ class PostRetriever implements IEntityRetriever
throw new ApiJobUnsatisfiedException($this->job);
}
public function retrieveForEditing()
{
$post = $this->retrieve();
if ($this->job->getContext() === IJob::CONTEXT_BATCH_ADD)
return $post;
$expectedRevision = $this->job->getArgument(JobArgs::ARG_POST_REVISION);
if ($expectedRevision != $post->getRevision())
throw new SimpleException('This post was already edited by someone else in the meantime');
return $post;
}
public function getRequiredArguments()
{
return JobArgs::Alternative(
@ -42,4 +55,14 @@ class PostRetriever implements IEntityRetriever
JobArgs::ARG_POST_NAME,
JobArgs::ARG_POST_ENTITY);
}
public function getRequiredArgumentsForEditing()
{
if ($this->job->getContext() === IJob::CONTEXT_BATCH_ADD)
return $this->getRequiredArguments();
return JobArgs::Conjunction(
$this->getRequiredArguments(),
JobArgs::ARG_POST_REVISION);
}
}

View file

@ -15,6 +15,7 @@ class JobArgs
const ARG_POST_ENTITY = 'post';
const ARG_POST_ID = 'post-id';
const ARG_POST_NAME = 'post-name';
const ARG_POST_REVISION = 'post-revision';
const ARG_TAG_NAME = 'tag-name';
const ARG_TAG_NAMES = 'tag-names';

View file

@ -10,8 +10,7 @@ class EditPostContentJob extends AbstractJob
public function execute()
{
$post = $this->postRetriever->retrieve();
$post = $this->postRetriever->retrieveForEditing();
if ($this->hasArgument(JobArgs::ARG_NEW_POST_CONTENT_URL))
{
$url = $this->getArgument(JobArgs::ARG_NEW_POST_CONTENT_URL);
@ -36,7 +35,7 @@ class EditPostContentJob extends AbstractJob
public function getRequiredArguments()
{
return JobArgs::Conjunction(
$this->postRetriever->getRequiredArguments(),
$this->postRetriever->getRequiredArgumentsForEditing(),
JobArgs::Alternative(
JobArgs::ARG_NEW_POST_CONTENT,
JobArgs::ARG_NEW_POST_CONTENT_URL));

View file

@ -16,8 +16,7 @@ class EditPostJob extends AbstractJob
public function execute()
{
$post = $this->postRetriever->retrieve();
$post = $this->postRetriever->retrieveForEditing();
$arguments = $this->getArguments();
$arguments[JobArgs::ARG_POST_ENTITY] = $post;
@ -43,7 +42,7 @@ class EditPostJob extends AbstractJob
public function getRequiredArguments()
{
return $this->postRetriever->getRequiredArguments();
return $this->postRetriever->getRequiredArgumentsForEditing();
}
public function getRequiredMainPrivilege()

View file

@ -10,7 +10,7 @@ class EditPostRelationsJob extends AbstractJob
public function execute()
{
$post = $this->postRetriever->retrieve();
$post = $this->postRetriever->retrieveForEditing();
$relatedPostIds = $this->getArgument(JobArgs::ARG_NEW_RELATED_POST_IDS);
if (!is_array($relatedPostIds))
@ -47,7 +47,7 @@ class EditPostRelationsJob extends AbstractJob
public function getRequiredArguments()
{
return JobArgs::Conjunction(
$this->postRetriever->getRequiredArguments(),
$this->postRetriever->getRequiredArgumentsForEditing(),
JobArgs::ARG_NEW_RELATED_POST_IDS);
}

View file

@ -10,7 +10,7 @@ class EditPostSafetyJob extends AbstractJob
public function execute()
{
$post = $this->postRetriever->retrieve();
$post = $this->postRetriever->retrieveForEditing();
$newSafety = new PostSafety($this->getArgument(JobArgs::ARG_NEW_SAFETY));
$oldSafety = $post->getSafety();
@ -33,7 +33,7 @@ class EditPostSafetyJob extends AbstractJob
public function getRequiredArguments()
{
return JobArgs::Conjunction(
$this->postRetriever->getRequiredArguments(),
$this->postRetriever->getRequiredArgumentsForEditing(),
JobArgs::ARG_NEW_SAFETY);
}

View file

@ -10,7 +10,7 @@ class EditPostSourceJob extends AbstractJob
public function execute()
{
$post = $this->postRetriever->retrieve();
$post = $this->postRetriever->retrieveForEditing();
$newSource = $this->getArgument(JobArgs::ARG_NEW_SOURCE);
$oldSource = $post->getSource();
@ -33,7 +33,7 @@ class EditPostSourceJob extends AbstractJob
public function getRequiredArguments()
{
return JobArgs::Conjunction(
$this->postRetriever->getRequiredArguments(),
$this->postRetriever->getRequiredArgumentsForEditing(),
JobArgs::ARG_NEW_SOURCE);
}

View file

@ -10,7 +10,7 @@ class EditPostTagsJob extends AbstractJob
public function execute()
{
$post = $this->postRetriever->retrieve();
$post = $this->postRetriever->retrieveForEditing();
$tagNames = $this->getArgument(JobArgs::ARG_NEW_TAG_NAMES);
if (!is_array($tagNames))
@ -50,7 +50,7 @@ class EditPostTagsJob extends AbstractJob
public function getRequiredArguments()
{
return JobArgs::Conjunction(
$this->postRetriever->getRequiredArguments(),
$this->postRetriever->getRequiredArgumentsForEditing(),
JobArgs::ARG_NEW_TAG_NAMES);
}

View file

@ -10,7 +10,7 @@ class EditPostThumbnailJob extends AbstractJob
public function execute()
{
$post = $this->postRetriever->retrieve();
$post = $this->postRetriever->retrieveForEditing();
$file = $this->getArgument(JobArgs::ARG_NEW_THUMBNAIL_CONTENT);
$post->setCustomThumbnailFromPath($file->filePath);
@ -28,7 +28,7 @@ class EditPostThumbnailJob extends AbstractJob
public function getRequiredArguments()
{
return JobArgs::Conjunction(
$this->postRetriever->getRequiredArguments(),
$this->postRetriever->getRequiredArgumentsForEditing(),
JobArgs::ARG_NEW_THUMBNAIL_CONTENT);
}

View file

@ -157,16 +157,13 @@ class PostController extends AbstractController
{
$post = PostModel::getByIdOrName($identifier);
$revision = InputHelper::get('revision');
if ($revision != $post->getRevision())
throw new SimpleException('This post was already edited by someone else in the meantime');
$jobArgs =
[
JobArgs::ARG_NEW_SAFETY => InputHelper::get('safety'),
JobArgs::ARG_NEW_TAG_NAMES => $this->splitTags(InputHelper::get('tags')),
JobArgs::ARG_NEW_SOURCE => InputHelper::get('source'),
JobArgs::ARG_NEW_RELATED_POST_IDS => $this->splitPostIds(InputHelper::get('relations')),
JobArgs::ARG_POST_REVISION => InputHelper::get('revision'),
];
$jobArgs = $this->appendPostIdentifierArgument($jobArgs, $identifier);

View file

@ -47,7 +47,7 @@ final class PostEntity extends AbstractEntity implements IValidatable, ISerializ
$this->setCache('comment_count', $row['comment_count']);
$this->setCache('fav_count', $row['fav_count']);
$this->setCache('score', $row['score']);
$this->setCache('revision', $row['revision']);
$this->setCache('revision', TextHelper::toInteger($row['revision']));
$this->setType(new PostType($row['type']));
$this->setSafety(new PostSafety($row['safety']));
}
@ -152,6 +152,11 @@ final class PostEntity extends AbstractEntity implements IValidatable, ISerializ
return (int) $this->getColumnWithCache('revision');
}
public function incRevision()
{
$this->setCache('revision', $this->getRevision() + 1);
}
public function getScore()
{
return (int) $this->getColumnWithCache('score');

View file

@ -11,6 +11,7 @@ final class PostModel extends AbstractCrudModel
protected static function saveSingle($post)
{
$post->validate();
$post->incRevision();
Core::getDatabase()->transaction(function() use ($post)
{
@ -30,7 +31,7 @@ final class PostModel extends AbstractCrudModel
'image_height' => $post->getImageHeight(),
'uploader_id' => $post->getUploaderId(),
'source' => $post->getSource(),
'revision' => $post->getRevision() + 1,
'revision' => $post->getRevision(),
];
$stmt = Sql\Statements::update();

View file

@ -65,7 +65,7 @@ class ApiArgumentTest extends AbstractFullApiTest
{
$this->testArguments(new EditPostContentJob(),
JobArgs::Conjunction(
$this->getPostSelector(),
$this->getPostSelectorForEditing(),
JobArgs::Alternative(
JobArgs::ARG_NEW_POST_CONTENT,
JobArgs::ARG_NEW_POST_CONTENT_URL)));
@ -74,14 +74,14 @@ class ApiArgumentTest extends AbstractFullApiTest
public function testEditPostJob()
{
$this->testArguments(new EditPostJob(),
$this->getPostSelector());
$this->getPostSelectorForEditing());
}
public function testEditPostRelationsJob()
{
$this->testArguments(new EditPostRelationsJob(),
JobArgs::Conjunction(
$this->getPostSelector(),
$this->getPostSelectorForEditing(),
JobArgs::ARG_NEW_RELATED_POST_IDS));
}
@ -89,7 +89,7 @@ class ApiArgumentTest extends AbstractFullApiTest
{
$this->testArguments(new EditPostSafetyJob(),
JobArgs::Conjunction(
$this->getPostSelector(),
$this->getPostSelectorForEditing(),
JobArgs::ARG_NEW_SAFETY));
}
@ -97,7 +97,7 @@ class ApiArgumentTest extends AbstractFullApiTest
{
$this->testArguments(new EditPostSourceJob(),
JobArgs::Conjunction(
$this->getPostSelector(),
$this->getPostSelectorForEditing(),
JobArgs::ARG_NEW_SOURCE));
}
@ -105,7 +105,7 @@ class ApiArgumentTest extends AbstractFullApiTest
{
$this->testArguments(new EditPostTagsJob(),
JobArgs::Conjunction(
$this->getPostSelector(),
$this->getPostSelectorForEditing(),
JobArgs::ARG_NEW_TAG_NAMES));
}
@ -113,7 +113,7 @@ class ApiArgumentTest extends AbstractFullApiTest
{
$this->testArguments(new EditPostThumbnailJob(),
JobArgs::Conjunction(
$this->getPostSelector(),
$this->getPostSelectorForEditing(),
JobArgs::ARG_NEW_THUMBNAIL_CONTENT));
}
@ -369,6 +369,13 @@ class ApiArgumentTest extends AbstractFullApiTest
JobArgs::ARG_POST_NAME);
}
protected function getPostSelectorForEditing()
{
return JobArgs::Conjunction(
JobArgs::ARG_POST_REVISION,
$this->getPostSelector());
}
protected function getPostSafeSelector()
{
return JobArgs::Alternative(

View file

@ -235,6 +235,7 @@ class ApiPrivilegeTest extends AbstractFullApiTest
new EditPostTagsJob(),
[
JobArgs::ARG_POST_ID => $post->getId(),
JobArgs::ARG_POST_REVISION => $post->getRevision(),
JobArgs::ARG_NEW_TAG_NAMES => ['test1', 'test2'],
]);
}, 'Insufficient privileges');
@ -248,6 +249,7 @@ class ApiPrivilegeTest extends AbstractFullApiTest
new EditPostTagsJob(),
[
JobArgs::ARG_POST_ID => $post->getId(),
JobArgs::ARG_POST_REVISION => $post->getRevision(),
JobArgs::ARG_NEW_TAG_NAMES => ['test1', 'test2'],
]);
});

View file

@ -4,7 +4,6 @@ class EditPostContentJobTest extends AbstractTest
public function testFile()
{
$this->prepare();
$this->grantAccess('editPostContent');
$post = $this->uploadFromFile('image.jpg');
$this->assert->doesNotThrow(function() use ($post)
{
@ -19,7 +18,6 @@ class EditPostContentJobTest extends AbstractTest
public function testFileJpeg()
{
$this->prepare();
$this->grantAccess('editPostContent');
$post = $this->uploadFromFile('image.jpg');
$this->assert->areEqual('image/jpeg', $post->getMimeType());
$this->assert->areEqual(PostType::Image, $post->getType()->toInteger());
@ -34,7 +32,6 @@ class EditPostContentJobTest extends AbstractTest
public function testFilePng()
{
$this->prepare();
$this->grantAccess('editPostContent');
$post = $this->uploadFromFile('image.png');
$this->assert->areEqual('image/png', $post->getMimeType());
$this->assert->areEqual(PostType::Image, $post->getType()->toInteger());
@ -49,7 +46,6 @@ class EditPostContentJobTest extends AbstractTest
public function testFileGif()
{
$this->prepare();
$this->grantAccess('editPostContent');
$post = $this->uploadFromFile('image.gif');
$this->assert->areEqual('image/gif', $post->getMimeType());
$this->assert->areEqual(PostType::Image, $post->getType()->toInteger());
@ -64,7 +60,6 @@ class EditPostContentJobTest extends AbstractTest
public function testFileInvalid()
{
$this->prepare();
$this->grantAccess('editPostContent');
$this->assert->throws(function()
{
$this->uploadFromFile('text.txt');
@ -74,7 +69,6 @@ class EditPostContentJobTest extends AbstractTest
public function testUrl()
{
$this->prepare();
$this->grantAccess('editPostContent');
$post = $this->uploadFromUrl('image.jpg');
$this->assert->doesNotThrow(function() use ($post)
{
@ -89,13 +83,12 @@ class EditPostContentJobTest extends AbstractTest
public function testUrlYoutube()
{
$this->prepare();
$this->grantAccess('editPostContent');
$post = $this->postMocker->mockSingle();
$post = Api::run(
new EditPostContentJob(),
[
JobArgs::ARG_POST_ID => $post->getId(),
JobArgs::ARG_POST_REVISION => $post->getRevision(),
JobArgs::ARG_NEW_POST_CONTENT_URL => 'http://www.youtube.com/watch?v=qWq_jydCUw4',
]);
$this->assert->areEqual(PostType::Youtube, $post->getType()->toInteger());
@ -119,17 +112,32 @@ class EditPostContentJobTest extends AbstractTest
new EditPostContentJob(),
[
JobArgs::ARG_POST_ID => 100,
JobArgs::ARG_NEW_POST_CONTENT =>
new ApiFileInput($this->testSupport->getPath('image.jpg'), 'test.jpg'),
JobArgs::ARG_POST_REVISION => 1,
JobArgs::ARG_NEW_POST_CONTENT_URL => 'irrelevant',
]);
}, 'Invalid post ID');
}
public function testWrongRevision()
{
$this->prepare();
$post = $this->postMocker->mockSingle();
$this->assert->throws(function() use ($post)
{
Api::run(
new EditPostContentJob(),
[
JobArgs::ARG_POST_ID => $post->getId(),
JobArgs::ARG_POST_REVISION => 100,
JobArgs::ARG_NEW_POST_CONTENT_URL => 'irrelevant',
]);
}, 'This post was already edited by someone else in the meantime');
}
public function testDuplicateFile()
{
$this->prepare();
$this->grantAccess('editPostContent');
$post = $this->uploadFromFile('image.png');
$this->assert->areEqual('image/png', $post->getMimeType());
$this->assert->throws(function()
@ -141,7 +149,6 @@ class EditPostContentJobTest extends AbstractTest
public function testDuplicateUrl()
{
$this->prepare();
$this->grantAccess('editPostContent');
$post = $this->uploadFromUrl('image.png');
$this->assert->areEqual('image/png', $post->getMimeType());
$this->assert->throws(function()
@ -153,8 +160,6 @@ class EditPostContentJobTest extends AbstractTest
public function testDuplicateYoutube()
{
$this->prepare();
$this->grantAccess('editPostContent');
$url = 'http://www.youtube.com/watch?v=qWq_jydCUw4';
$post = $this->postMocker->mockSingle();
@ -162,6 +167,7 @@ class EditPostContentJobTest extends AbstractTest
new EditPostContentJob(),
[
JobArgs::ARG_POST_ID => $post->getId(),
JobArgs::ARG_POST_REVISION => $post->getRevision(),
JobArgs::ARG_NEW_POST_CONTENT_URL => $url,
]);
@ -172,6 +178,7 @@ class EditPostContentJobTest extends AbstractTest
new EditPostContentJob(),
[
JobArgs::ARG_POST_ID => $anotherPost->getId(),
JobArgs::ARG_POST_REVISION => $anotherPost->getRevision(),
JobArgs::ARG_NEW_POST_CONTENT_URL => $url,
]);
}, 'Duplicate upload: @' . $post->getId());
@ -179,6 +186,7 @@ class EditPostContentJobTest extends AbstractTest
protected function prepare()
{
$this->grantAccess('editPostContent');
$this->login($this->userMocker->mockSingle());
}
@ -194,6 +202,7 @@ class EditPostContentJobTest extends AbstractTest
new EditPostContentJob(),
[
JobArgs::ARG_POST_ID => $post->getId(),
JobArgs::ARG_POST_REVISION => $post->getRevision(),
JobArgs::ARG_NEW_POST_CONTENT_URL => $url,
]);
@ -215,6 +224,7 @@ class EditPostContentJobTest extends AbstractTest
new EditPostContentJob(),
[
JobArgs::ARG_POST_ID => $post->getId(),
JobArgs::ARG_POST_REVISION => $post->getRevision(),
JobArgs::ARG_NEW_POST_CONTENT =>
new ApiFileInput($this->testSupport->getPath($fileName), 'test.jpg'),
]);

View file

@ -15,6 +15,7 @@ class EditPostJobTest extends AbstractTest
$args =
[
JobArgs::ARG_POST_ID => $post->getId(),
JobArgs::ARG_POST_REVISION => $post->getRevision(),
JobArgs::ARG_NEW_SAFETY => PostSafety::Sketchy,
JobArgs::ARG_NEW_SOURCE => 'some source huh',
JobArgs::ARG_NEW_POST_CONTENT =>
@ -41,6 +42,7 @@ class EditPostJobTest extends AbstractTest
$args =
[
JobArgs::ARG_POST_ID => $post->getId(),
JobArgs::ARG_POST_REVISION => $post->getRevision(),
JobArgs::ARG_NEW_SAFETY => PostSafety::Safe,
JobArgs::ARG_NEW_SOURCE => 'this should make it fail',
JobArgs::ARG_NEW_POST_CONTENT =>

View file

@ -14,6 +14,7 @@ class EditPostRelationsJobTest extends AbstractTest
new EditPostRelationsJob(),
[
JobArgs::ARG_POST_ID => $basePost->getId(),
JobArgs::ARG_POST_REVISION => $basePost->getRevision(),
JobArgs::ARG_NEW_RELATED_POST_IDS =>
[
$post1->getId(),
@ -46,6 +47,7 @@ class EditPostRelationsJobTest extends AbstractTest
new EditPostRelationsJob(),
[
JobArgs::ARG_POST_ID => $basePost->getId(),
JobArgs::ARG_POST_REVISION => $basePost->getRevision(),
JobArgs::ARG_NEW_RELATED_POST_IDS =>
[
$post2->getId(),
@ -76,6 +78,7 @@ class EditPostRelationsJobTest extends AbstractTest
new EditPostRelationsJob(),
[
JobArgs::ARG_POST_ID => $basePost->getId(),
JobArgs::ARG_POST_REVISION => $basePost->getRevision(),
JobArgs::ARG_NEW_RELATED_POST_IDS =>
[
]

View file

@ -13,6 +13,7 @@ class EditPostSafetyJobTest extends AbstractTest
new EditPostSafetyJob(),
[
JobArgs::ARG_POST_ID => $post->getId(),
JobArgs::ARG_POST_REVISION => $post->getRevision(),
JobArgs::ARG_NEW_SAFETY => PostSafety::Sketchy,
]);
});
@ -34,6 +35,7 @@ class EditPostSafetyJobTest extends AbstractTest
new EditPostSafetyJob(),
[
JobArgs::ARG_POST_ID => 100,
JobArgs::ARG_POST_REVISION => 1000,
JobArgs::ARG_NEW_SAFETY => PostSafety::Sketchy,
]);
}, 'Invalid post ID');
@ -51,6 +53,7 @@ class EditPostSafetyJobTest extends AbstractTest
new EditPostSafetyJob(),
[
JobArgs::ARG_POST_ID => $post->getId(),
JobArgs::ARG_POST_REVISION => $post->getRevision(),
JobArgs::ARG_NEW_SAFETY => '',
]);
}, 'Invalid safety type');

View file

@ -57,6 +57,7 @@ class EditPostSourceJobTest extends AbstractTest
new EditPostSourceJob(),
[
JobArgs::ARG_POST_ID => 100,
JobArgs::ARG_POST_REVISION => 1000,
JobArgs::ARG_NEW_SOURCE => 'alohaa',
]);
}, 'Invalid post ID');
@ -70,6 +71,7 @@ class EditPostSourceJobTest extends AbstractTest
new EditPostSourceJob(),
[
JobArgs::ARG_POST_ID => $post->getId(),
JobArgs::ARG_POST_REVISION => $post->getRevision(),
JobArgs::ARG_NEW_SOURCE => $text
]);
}

View file

@ -13,6 +13,7 @@ class EditPostTagsJobTest extends AbstractTest
new EditPostTagsJob(),
[
JobArgs::ARG_POST_ID => $post->getId(),
JobArgs::ARG_POST_REVISION => $post->getRevision(),
JobArgs::ARG_NEW_TAG_NAMES => $newTagNames,
]);
});
@ -39,6 +40,7 @@ class EditPostTagsJobTest extends AbstractTest
new EditPostTagsJob(),
[
JobArgs::ARG_POST_ID => $post->getId(),
JobArgs::ARG_POST_REVISION => $post->getRevision(),
JobArgs::ARG_NEW_TAG_NAMES => [],
]);
}, 'No tags set');
@ -56,6 +58,7 @@ class EditPostTagsJobTest extends AbstractTest
new EditPostTagsJob(),
[
JobArgs::ARG_POST_ID => $post->getId(),
JobArgs::ARG_POST_REVISION => $post->getRevision(),
JobArgs::ARG_NEW_TAG_NAMES => $newTagNames,
]);
}, 'Tag must have at least');
@ -73,6 +76,7 @@ class EditPostTagsJobTest extends AbstractTest
new EditPostTagsJob(),
[
JobArgs::ARG_POST_ID => $post->getId(),
JobArgs::ARG_POST_REVISION => $post->getRevision(),
JobArgs::ARG_NEW_TAG_NAMES => $newTagNames,
]);
}, 'Tag must have at most');
@ -90,6 +94,7 @@ class EditPostTagsJobTest extends AbstractTest
new EditPostTagsJob(),
[
JobArgs::ARG_POST_ID => $post->getId(),
JobArgs::ARG_POST_REVISION => $post->getRevision(),
JobArgs::ARG_NEW_TAG_NAMES => $newTagNames,
]);
}, 'Invalid tag');
@ -106,6 +111,7 @@ class EditPostTagsJobTest extends AbstractTest
new EditPostTagsJob(),
[
JobArgs::ARG_POST_ID => 100,
JobArgs::ARG_POST_REVISION => 1000,
JobArgs::ARG_NEW_TAG_NAMES => $newTagNames,
]);
}, 'Invalid post ID');

View file

@ -13,6 +13,7 @@ class EditPostThumbnailJobTest extends AbstractTest
new EditPostThumbnailJob(),
[
JobArgs::ARG_POST_ID => $post->getId(),
JobArgs::ARG_POST_REVISION => $post->getRevision(),
JobArgs::ARG_NEW_THUMBNAIL_CONTENT =>
new ApiFileInput($this->testSupport->getPath('thumb.jpg'), 'test.jpg'),
]);
@ -37,6 +38,7 @@ class EditPostThumbnailJobTest extends AbstractTest
new EditPostThumbnailJob(),
[
JobArgs::ARG_POST_ID => $post->getId(),
JobArgs::ARG_POST_REVISION => $post->getRevision(),
JobArgs::ARG_NEW_THUMBNAIL_CONTENT =>
new ApiFileInput($this->testSupport->getPath('image.jpg'), 'test.jpg'),
]);