IBX-12033: Fixed content name reverting when translations are based on the content version and published sequentially. - #779
Conversation
|
| $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.
There was a problem hiding this comment.
After a brief investigation I still don't think we should pursue the fix through a locking mechanism.
There is no second transaction to exclude - the flow is sequential.
The name gets stale inside the same request: copyTranslationsFromPublishedVersion() writes the correct name however at this point the initially queried $content that is being used by copyNonTranslatableFieldsFromPublishedVersion() is using stale name.
Any locking fix would have to put its locking read exactly where the reload is now .
It's the re-read that fixes it, and the lock is just along for the ride.
There was a problem hiding this comment.
I may not have understood the scenario/steps clearly then.
Is publishing both translations done in parallel (concurrent separate publish requests triggered at the same time) or synchronously (first we're publishing English version and next we're publishing French, so both publishes are done step-by-step)?
I was suggested by the PR's title that they're concurrent requests;
There was a problem hiding this comment.
This is my bad, I should have worded it better, I will update the PR title.
So the order of the actions is:
Two drafts in different languages are being created based on the same content version, then we sequentially publish the updated drafts one by one.
The entire sequence of actions is described in the 3rd line itself.
| ); | ||
| } | ||
|
|
||
| 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.
Sztig
changed the title
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.