IBX-12033: Fixed content name reverting when translations are published in parallel#779
IBX-12033: Fixed content name reverting when translations are published in parallel#779Sztig wants to merge 2 commits into
Conversation
ibexa-workflow-automation-1
Bot
requested review from
Steveb-p,
ViniTou,
alongosz,
barw4,
bnowak,
ciastektk,
konradoboza,
mikadamczyk,
tbialcz and
wiewiurdp
and removed request for
a team
|
| $versionInfo = $currentVersionContent->getVersionInfo(); | ||
| $contentType = $currentVersionContent->getContentType(); | ||
|
|
||
| $currentVersionContent = $this->internalLoadContentById( |
There was a problem hiding this comment.
I don't think that reloading content version 2nd time solves the issue completely - it's just narrowing down the time when the issue can happen. Ofc, there's much less chances for that now, but we're no 100% sure it wil work every time.
Instead, I'd rather extend db transaction in publishVersion method to include also loading content in 1st line, and use pessimistic locking to block concurrent processes to load the same content (which is currently publishing). That way, next process should wait until 1st will be finished and will load its new (already published) version.
You can look at https://github.com/ibexa/taxonomy/pull/423 as an example 😉
Also, that would need to be covered by an integration test that checks this lock/block for concurent connection. You can check testTreeRootLockBlocksConcurrentConnection test from mentioned PR
There was a problem hiding this comment.
I don't think the locking approach would fix the problem that we have here as this scenario is completely sequential, each step only starts after the previous one is fully commited. The lock would be acquired instantly and changes nothing.
There was a problem hiding this comment.
@bnowak Taxonomy is a specific case because it uses ORM. Overall in the product and especially in core we use DBAL (that's why I don't know ORM that well btw ;-) ).
That being said, @Sztig maybe it's worth exploring, though I'm not exactly sure how much work that's gonna be. This whole batch of operations is already wrapped by transaction. So I wonder what would happen if we tried to do pessimistic write lock for queries selecting proper rows here too. On DBAL layer \Doctrine\DBAL\Platforms\AbstractPlatform::getWriteLockSQL method returns proper platform-specific portion of SQL, which usually is FOR UPDATE. It's appended as SELECT ... FOR UPDATE to a query which should lock row it applies to (is it content_name relation or version_attribute relation we're talking about here?).
As for tests, we already had some parallel integration test in core, done differently than what I had to do for taxo. Sadly it's legacy integration layer - \Ibexa\Tests\Integration\Core\Repository\Parallel\BaseParallelTestCase. We'd need sth similar extending RepositoryTestCase.
| ); | ||
| } | ||
|
|
||
| public function testCopyTranslationsFromPublishedToDraftWithNonTranslatableField(): void |
There was a problem hiding this comment.
Are we sure that the test is covering the issue? Will it fail if you comment out the change in ContentService::copyNonTranslatableFieldsFromPublishedVersion?
If not, it means that it doesn't fulfil its role and could be removed or rewritten.
There was a problem hiding this comment.
The test fails without the fix, at least locally.
5 participants
Description:
Prerequisite for the bug to happen - content type must contain non translatable field.
When 2 translations of the same content were edited and published in parallel (order of operations described in detail in the jira ticket), publishing the the second draft reverted the content name to its pre published value.
During the publishing the second translation, we already query the stale content through
internalLoadContentById(), thencopyTranslationsFromPublishedVersion()manages to "fix" the stale content .However, we still pass the same initially queried content with potentially outdated name to
copyNonTranslatableFieldsFromPublishedVersion(). My fix here is just reloading the content again in this method to not rely on the stale content.Case described in jira ticket has been covered in an integration test.