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.

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

EZP-30823: Fixed possible race condition on version loading #2739

Merged
merged 5 commits into from
Sep 18, 2019

Conversation

andrerom
Copy link
Contributor

@andrerom andrerom commented Aug 14, 2019

Question Answer
JIRA issue EZP-30823
Bug/Improvement yes
Target version 6.7
BC breaks no
Tests pass yes
Doc needed no

Change version loading to not rely on first having to load content info to get current version number (risking it having changed in between), just like done for content load some time ago.

BENEFIT: Avoids having to load ContentInfo within loadVersionInfo() when loading current version.

TODO:

  • Implement feature / fix a bug.
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

ON MERGE:

  • Update SQL for version load to also add new columns: (ezcontentobject_is_hidden, ...), patch:
diff --git a/eZ/Publish/Core/Persistence/Legacy/Content/Gateway/DoctrineDatabase/QueryBuilder.php b/eZ/Publish/Core/Persistence/Legacy/Content/Gateway/DoctrineDatabase/QueryBuilder.php
index 920785536b..09f6b7b1f3 100644
--- a/eZ/Publish/Core/Persistence/Legacy/Content/Gateway/DoctrineDatabase/QueryBuilder.php
+++ b/eZ/Publish/Core/Persistence/Legacy/Content/Gateway/DoctrineDatabase/QueryBuilder.php
@@ -279,6 +279,7 @@ class QueryBuilder
                 'c.status AS ezcontentobject_status',
                 'c.name AS ezcontentobject_name',
                 'c.language_mask AS ezcontentobject_language_mask',
+                'c.is_hidden AS ezcontentobject_is_hidden',
                 'v.id AS ezcontentobject_version_id',
                 'v.version AS ezcontentobject_version_version',
                 'v.modified AS ezcontentobject_version_modified',

Change version loading to not rely on first having to load content info to get current version number, just like done for content load some time ago.
@andrerom andrerom requested review from alongosz and adamwojs August 14, 2019 00:37
@alongosz alongosz changed the title EZP-30823: Fix possible race condition on version loading EZP-30823: Fixed possible race condition on version loading Aug 19, 2019
@andrerom
Copy link
Contributor Author

andrerom commented Aug 28, 2019

@ezsystems/qa-team : This one is ready for testing, but the two others on cache transaction isolation (#2740, #2749) are still waiting for review. Two options:

  • Help bug Engineering to review so you can test 😄
  • Ask me to split the bug into 2, one on load issue and another on publish/updates (aka transactions) issue

@micszo
Copy link
Member

micszo commented Aug 29, 2019

From my pov the most effective way to test both issues would be to have branches for testing with the combined changes for each stable branch. Is that feasible?

@andrerom
Copy link
Contributor Author

andrerom commented Aug 29, 2019

From my pov the most effective way to test both issues would be to have branches for testing with the combined changes for each stable branch. Is that feasible?

yes it is, I'll ping on slack qa channel when I have pushed it

@micszo micszo self-assigned this Sep 4, 2019
Copy link
Member

@micszo micszo left a comment

Choose a reason for hiding this comment

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

QA Approved.
Retest successful for Content List block issue and wrong current version issue on eZ Platform EE v1.7.9 with 6.7_race_cond_testing branch. Verified with sanities.
As agreed testing on v1.13.5 will be done post merge.
Testing on v2.5 is ongoing in scope of #2749 / 7.5_race_cond_testing branch.

@andrerom andrerom merged commit fb89579 into 6.7 Sep 18, 2019
@andrerom andrerom deleted the version_load_race branch September 18, 2019 19:15
@micszo micszo removed their assignment Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants