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

IBX-7172: Fixed Repository Filtering by multiple ObjectStateId criteria #401

Conversation

vidarl
Copy link
Member

@vidarl vidarl commented Jan 9, 2024

Question Answer
JIRA issue IBX-7172
Type bug
Target Ibexa version v3.3
BC breaks no

If user has policy with limitation on multiple object states, the api call for $locationService->loadLocationChildren($location) creates (eZ\Publish\Core\Persistence\Legacy\Filter\CriterionQueryBuilder\Content\ObjectStateIdQueryBuilder) an incorrect sql for checking the object states.

The sql looks as follows :

SELECT
  DISTINCT COUNT(DISTINCT location.node_id)
FROM
  ezcontentobject_tree location
  INNER JOIN ezcontentobject content ON content.id = location.contentobject_id
  INNER JOIN ezcontentobject_version version ON (
    content.id = version.contentobject_id
  )
  AND (
    content.current_version = version.version
  )
  AND (version.status = :dcValue1)
  INNER JOIN ezcobj_state_link object_state_link ON content.id = object_state_link.contentobject_id
  INNER JOIN ezcontent_language language ON language.id & version.language_mask = language.id
WHERE
  (
    location.parent_node_id IN (:dcValue2)
  )
  AND (
    (
      language.locale IN (:dcValue3)
    )
    OR (version.language_mask & 1 = 1)
  )
  AND (
    (
      content.section_id IN (:dcValue4)
    )
    AND (
      (
        object_state_link.contentobject_state_id IN (:dcValue5)
      )
      AND (
        object_state_link.contentobject_state_id IN (:dcValue6)
      )
    )
  )

Obviously, object_state_link.contentobject_state_id cannot be equal both :dcValue5 and :dcValue6 at the same time.
Instead the objet_state_link table should be joined once for each id and the sql should read:

SELECT
  DISTINCT COUNT(DISTINCT location.node_id)
FROM
  ezcontentobject_tree location
  INNER JOIN ezcontentobject content ON content.id = location.contentobject_id
  INNER JOIN ezcontentobject_version version ON (
    content.id = version.contentobject_id
  )
  AND (
    content.current_version = version.version
  )
  AND (version.status = :dcValue1)
  INNER JOIN ezcobj_state_link object_state_link_1 ON content.id = object_state_link.contentobject_id
  INNER JOIN ezcobj_state_link object_state_link_2 ON content.id = object_state_link.contentobject_id
  INNER JOIN ezcontent_language language ON language.id & version.language_mask = language.id
WHERE
  (
    location.parent_node_id IN (:dcValue2)
  )
  AND (
    (
      language.locale IN (:dcValue3)
    )
    OR (version.language_mask & 1 = 1)
  )
  AND (
    (
      content.section_id IN (:dcValue4)
    )
    AND (
      (
        object_state_link_1.contentobject_state_id IN (:dcValue5)
      )
      AND (
        object_state_link_2.contentobject_state_id IN (:dcValue6)
      )
    )
  )

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Provided automated test coverage.
  • Checked that target branch is set correctly (master for features, the oldest supported for bugs).
  • Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
  • Asked for a review (ping @ezsystems/engineering-team).

@vidarl vidarl requested a review from a team January 9, 2024 13:36
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

Foremost, please add an integration test case showing the issue.

@alongosz alongosz requested a review from a team January 9, 2024 14:15
@vidarl
Copy link
Member Author

vidarl commented Jan 12, 2024

Foremost, please add an integration test case showing the issue.

Honestly, a bit unsure how you want that test. Didn't find a existing test I could reuse by just adding more fixture. So created a new test in 3271b67

@vidarl vidarl requested a review from alongosz January 12, 2024 12:31
vidarl and others added 3 commits January 15, 2024 16:07
…r/Content/ObjectStateIdQueryBuilder.php

Co-authored-by: Andrew Longosz <alongosz@users.noreply.github.com>
@vidarl vidarl requested a review from Steveb-p January 16, 2024 13:24
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

Looks good, some minor tweaks are needed:

vidarl and others added 3 commits February 13, 2024 15:07
….php

Co-authored-by: Andrew Longosz <alongosz@users.noreply.github.com>
Co-authored-by: Andrew Longosz <alongosz@users.noreply.github.com>
@vidarl
Copy link
Member Author

vidarl commented Feb 13, 2024

FYI: Review comments implemented, @Steveb-p , @alongosz

@vidarl vidarl requested a review from alongosz February 13, 2024 14:13
@alongosz alongosz changed the title IBX-7172: loadLocationChildren() not working with multiple objectstate permissions IBX-7172: Fixed Repository Filtering by multiple ObjectStateId criteria Feb 13, 2024
@alongosz alongosz requested a review from a team February 13, 2024 15:58
@alongosz alongosz added Bug Something isn't working Ready for review labels Feb 13, 2024
Copy link
Contributor

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

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

Test looks very good. 👍

@@ -29,19 +29,21 @@ public function buildQueryConstraint(
FilteringQueryBuilder $queryBuilder,
FilteringCriterion $criterion
): ?string {
$tableAlias = uniqid('osl_');

/** @var \eZ\Publish\API\Repository\Values\Content\Query\Criterion\ObjectStateId $criterion */
$queryBuilder
->joinOnce(
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider it as a non merge blocking comment:

Just a remark - if we will always generate custom table alias for each ObjectStateId criteria, then it would make sense to switch into simpler QueryBuilder::join, which does not contain additional checks against table uniqueness.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point.. Changed joinOnce() into a join()

Copy link

sonarcloud bot commented Feb 14, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

@tomaszszopinski tomaszszopinski 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 on IbexaDXP 3.3 experience.

@alongosz alongosz merged commit aa31bb2 into 1.3 Feb 19, 2024
23 checks passed
@alongosz alongosz deleted the ibx-7172-loadLocationChildren-not-working-with-multiple-object-state-permissions branch February 19, 2024 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working QA approved
Development

Successfully merging this pull request may close these issues.

6 participants