Skip to content

IBX-10494: Refactored location filtering logic #682

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.

Sign up for GitHub

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

Draft
mikadamczyk wants to merge 19 commits into
base: 4.6
Choose a base branch
from
Draft

IBX-10494: Refactored location filtering logic #682

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
1b5d477
IBX-10494: Included Postgres 18 on CI
micszo Oct 2, 2025
01bbfc6
Fixed inconsistent test
Steveb-p Oct 6, 2025
30915f9
Postgres 14 & 18 setup
micszo Oct 6, 2025
d6ead1c
Image in matrix for Postgres int tests
micszo Oct 14, 2025
babf6e9
Image in matrix for MySQL int tests
micszo Oct 14, 2025
52c10b3
Dedicated action for installation of composer packages
micszo Oct 14, 2025
468a9ae
reformat code
micszo Oct 14, 2025
6fe6167
fixup! reformat code
micszo Oct 14, 2025
8df9df8
Refactored location sorting logic in query builders and enhance filte…
mikadamczyk Nov 20, 2025
c289838
Refactored sorting field names in query builders for consistency
mikadamczyk Nov 24, 2025
b4b2d21
Enhanced query wrapping to include dynamic select columns based on or…
mikadamczyk Nov 24, 2025
3f4071a
Refactored location filtering logic and removed redundant query wrapping
mikadamczyk Nov 25, 2025
e3fc8e9
Refactored ContentFilteringTest to use getContentInfo() for mainLocat…
mikadamczyk Dec 2, 2025
2b45e8c
Refactored sorting expression methods for location query builders
mikadamczyk Dec 3, 2025
3822fa9
Added unit test for BaseLocationSortClauseQueryBuilder
mikadamczyk Dec 3, 2025
f95eaad
Added legacy location sort clause and query builder for filtering
mikadamczyk Dec 3, 2025
7201d35
Refactored legacy filtering setup and improved type handling in tests
mikadamczyk Dec 3, 2025
8c8e690
Rebuilt service container to ensure test-only services are loaded
mikadamczyk Dec 4, 2025
89af2c8
Refactored LegacyContentFilteringTest to use RepositoryTestCase and i…
mikadamczyk Dec 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
192 changes: 62 additions & 130 deletions .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,22 +16,17 @@ jobs:
php:
- '8.1'
steps:
- uses: actions/checkout@v5

- name: Setup PHP Action
uses: shivammathur/setup-php@v2
with:
php-version: ${{ matrix.php }}
coverage: none
extensions: 'pdo_sqlite, gd'
tools: cs2pr
- uses: actions/checkout@v5

- uses: ramsey/composer-install@v3
with:
dependency-versions: "highest"
- uses: ibexa/gh-workflows/actions/composer-install@main
with:
gh-client-id: ${{ secrets.AUTOMATION_CLIENT_ID }}
gh-client-secret: ${{ secrets.AUTOMATION_CLIENT_SECRET }}
satis-network-key: ${{ secrets.SATIS_NETWORK_KEY }}
satis-network-token: ${{ secrets.SATIS_NETWORK_TOKEN }}

- name: Run code style check
run: composer run-script check-cs -- --format=checkstyle | cs2pr
- name: Run code style check
run: composer run-script check-cs -- --format=checkstyle | cs2pr

tests:
name: Unit tests & SQLite integration tests
Expand All @@ -47,38 +42,33 @@ jobs:
- '8.1'

steps:
- uses: actions/checkout@v5

- name: Setup PHP Action
uses: shivammathur/setup-php@v2
with:
php-version: ${{ matrix.php }}
coverage: none
extensions: pdo_sqlite, gd
tools: cs2pr
- uses: actions/checkout@v5

- uses: ramsey/composer-install@v3
with:
dependency-versions: "highest"
- uses: ibexa/gh-workflows/actions/composer-install@main
with:
gh-client-id: ${{ secrets.AUTOMATION_CLIENT_ID }}
gh-client-secret: ${{ secrets.AUTOMATION_CLIENT_SECRET }}
satis-network-key: ${{ secrets.SATIS_NETWORK_KEY }}
satis-network-token: ${{ secrets.SATIS_NETWORK_TOKEN }}

- name: Setup problem matchers for PHPUnit
run: echo "::add-matcher::${{ runner.tool_cache }}/phpunit.json"
- name: Setup problem matchers for PHPUnit
run: echo "::add-matcher::${{ runner.tool_cache }}/phpunit.json"

- name: Run PHPStan analysis
run: composer run-script phpstan
- name: Run PHPStan analysis
run: composer run-script phpstan

- name: Run unit test suite
run: composer run-script unit
- name: Run unit test suite
run: composer run-script unit

- name: Run integration test suite
run: composer run-script integration
- name: Run integration test suite
run: composer run-script integration

integration-tests-postgres:
name: PostgreSQL integration tests
needs: tests
services:
postgres:
image: postgres:14
image: ${{ matrix.image }}
ports:
- 5432
env:
Expand All @@ -100,21 +90,19 @@ jobs:
- '7.4'
- '8.0'
- '8.1'
image:
- 'postgres:14'
- 'postgres:18'

steps:
- uses: actions/checkout@v5

- name: Setup PHP Action
uses: shivammathur/setup-php@v2
with:
php-version: ${{ matrix.php }}
coverage: none
extensions: pdo_pgsql, gd
tools: cs2pr

- uses: ramsey/composer-install@v3
- uses: ibexa/gh-workflows/actions/composer-install@main
with:
dependency-versions: "highest"
gh-client-id: ${{ secrets.AUTOMATION_CLIENT_ID }}
gh-client-secret: ${{ secrets.AUTOMATION_CLIENT_SECRET }}
satis-network-key: ${{ secrets.SATIS_NETWORK_KEY }}
satis-network-token: ${{ secrets.SATIS_NETWORK_TOKEN }}

- name: Setup problem matchers for PHPUnit
run: echo "::add-matcher::${{ runner.tool_cache }}/phpunit.json"
Expand All @@ -126,12 +114,12 @@ jobs:
# Required by old repository tests
DATABASE: "pgsql://postgres:postgres@localhost:${{ job.services.postgres.ports[5432] }}/testdb"

integration-tests-mysql-80:
name: MySQL integration tests (8.0)
integration-tests-mysql:
name: MySQL integration tests
needs: tests
services:
mysql:
image: mysql:8.0
image: ${{ matrix.image }}
ports:
- 3306/tcp
env:
Expand All @@ -155,75 +143,19 @@ jobs:
- '7.4'
- '8.0'
- '8.1'
image:
- 'mysql:8.0'
- 'mysql:8.4'

steps:
- uses: actions/checkout@v5

- name: Setup PHP Action
uses: shivammathur/setup-php@v2
- uses: ibexa/gh-workflows/actions/composer-install@main
with:
php-version: ${{ matrix.php }}
coverage: none
extensions: pdo_mysql, gd, redis
tools: cs2pr

- uses: ramsey/composer-install@v3
with:
dependency-versions: "highest"

- name: Setup problem matchers for PHPUnit
run: echo "::add-matcher::${{ runner.tool_cache }}/phpunit.json"

- name: Run integration test suite vs MySQL
run: composer run-script integration
env:
DATABASE_URL: "mysql://mysql:mysql@127.0.0.1:${{ job.services.mysql.ports[3306] }}/testdb"
DATABASE: "mysql://mysql:mysql@127.0.0.1:${{ job.services.mysql.ports[3306] }}/testdb"

integration-tests-mysql-84:
name: MySQL integration tests (8.4)
needs: tests
services:
mysql:
image: mysql:8.4
ports:
- 3306/tcp
env:
MYSQL_RANDOM_ROOT_PASSWORD: true
MYSQL_USER: mysql
MYSQL_PASSWORD: mysql
MYSQL_DATABASE: testdb
options: >-
--health-cmd="mysqladmin ping"
--health-interval=10s
--health-timeout=5s
--health-retries=5
--tmpfs=/var/lib/mysql
runs-on: "ubuntu-24.04"
timeout-minutes: 60

strategy:
fail-fast: false
matrix:
php:
- '7.4'
- '8.0'
- '8.1'

steps:
- uses: actions/checkout@v5

- name: Setup PHP Action
uses: shivammathur/setup-php@v2
with:
php-version: ${{ matrix.php }}
coverage: none
extensions: pdo_mysql, gd, redis
tools: cs2pr

- uses: ramsey/composer-install@v3
with:
dependency-versions: "highest"
gh-client-id: ${{ secrets.AUTOMATION_CLIENT_ID }}
gh-client-secret: ${{ secrets.AUTOMATION_CLIENT_SECRET }}
satis-network-key: ${{ secrets.SATIS_NETWORK_KEY }}
satis-network-token: ${{ secrets.SATIS_NETWORK_TOKEN }}

- name: Setup problem matchers for PHPUnit
run: echo "::add-matcher::${{ runner.tool_cache }}/phpunit.json"
Expand Down Expand Up @@ -265,28 +197,28 @@ jobs:
- '8.0'
- '8.1'
steps:
- uses: actions/checkout@v5
with:
fetch-depth: 0
- uses: actions/checkout@v5
with:
fetch-depth: 0

- name: Setup PHP Action
uses: shivammathur/setup-php@v2
with:
php-version: ${{ matrix.php }}
coverage: none
- name: Setup PHP Action
uses: shivammathur/setup-php@v2
with:
php-version: ${{ matrix.php }}
coverage: none

- name: Add solr dependency
run: |
VERSION=$(jq -r '.extra | ."branch-alias" | ."dev-main"' < composer.json)
composer require --no-update "ibexa/solr:$VERSION"
- name: Add solr dependency
run: |
VERSION=$(jq -r '.extra | ."branch-alias" | ."dev-main"' < composer.json)
composer require --no-update "ibexa/solr:$VERSION"

- uses: ramsey/composer-install@v3
with:
dependency-versions: "highest"
- uses: ramsey/composer-install@v3
with:
dependency-versions: "highest"

- name: Run integration test suite
run: composer test-integration-solr
env:
- name: Run integration test suite
run: composer test-integration-solr
env:
CUSTOM_CACHE_POOL: singleredis
CACHE_HOST: 127.0.0.1
CORES_SETUP: single
28 changes: 27 additions & 1 deletion ...stence/Legacy/Filter/CriterionQueryBuilder/Location/BaseLocationCriterionQueryBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use Ibexa\Contracts\Core\Persistence\Filter\Doctrine\FilteringQueryBuilder;
use Ibexa\Contracts\Core\Repository\Values\Filter\CriterionQueryBuilder;
use Ibexa\Contracts\Core\Repository\Values\Filter\FilteringCriterion;
use Ibexa\Core\Persistence\Legacy\Content\Location\Gateway as LocationGateway;

/**
* @internal for internal use by Repository Filtering
Expand All @@ -21,10 +22,35 @@ public function buildQueryConstraint(
FilteringQueryBuilder $queryBuilder,
FilteringCriterion $criterion
): ?string {
$queryBuilder->joinAllLocations();
if ($this->isLocationFilteringContext($queryBuilder)) {
return null;
}

$expressionBuilder = $queryBuilder->expr();
$queryBuilder->joinOnce(
'content',
LocationGateway::CONTENT_TREE_TABLE,
'location',
(string)$expressionBuilder->andX(
'content.id = location.contentobject_id',
'location.node_id = location.main_node_id'
)
);

return null;
}

private function isLocationFilteringContext(FilteringQueryBuilder $queryBuilder): bool
{
$fromParts = $queryBuilder->getQueryPart('from');
foreach ($fromParts as $fromPart) {
if (($fromPart['alias'] ?? null) === 'location') {
return true;
}
}

return false;
}
}

class_alias(BaseLocationCriterionQueryBuilder::class, 'eZ\Publish\Core\Persistence\Legacy\Filter\CriterionQueryBuilder\Location\BaseLocationCriterionQueryBuilder');
25 changes: 3 additions & 22 deletions src/lib/Persistence/Legacy/Filter/Gateway/Content/Doctrine/DoctrineGateway.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
use Doctrine\DBAL\DBALException;
use Doctrine\DBAL\FetchMode;
use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Query\QueryBuilder;
use Ibexa\Contracts\Core\Persistence\Filter\CriterionVisitor;
use Ibexa\Contracts\Core\Persistence\Filter\Doctrine\FilteringQueryBuilder;
use Ibexa\Contracts\Core\Persistence\Filter\SortClauseVisitor;
Expand All @@ -23,7 +22,6 @@
use Ibexa\Core\Persistence\Legacy\Content\Location\Gateway as LocationGateway;
use Ibexa\Core\Persistence\Legacy\Filter\Gateway\Gateway;
use function iterator_to_array;
use function sprintf;
use Traversable;

/**
Expand Down Expand Up @@ -110,14 +108,12 @@ public function find(
$names = $this->bulkFetchVersionNames(clone $query);
$fieldValues = $this->bulkFetchFieldValues(clone $query);

// wrap query to avoid duplicate entries for multiple Locations
$wrappedQuery = $this->wrapMainQuery($query);
$wrappedQuery->setFirstResult($offset);
$query->setFirstResult($offset);
if ($limit > 0) {
$wrappedQuery->setMaxResults($limit);
$query->setMaxResults($limit);
}

$resultStatement = $wrappedQuery->execute();
$resultStatement = $query->execute();
while (false !== ($row = $resultStatement->fetch(FetchMode::ASSOCIATIVE))) {
$contentId = (int)$row['content_id'];
$versionNo = (int)$row['content_version_no'];
Expand Down Expand Up @@ -274,21 +270,6 @@ private function getColumns(): Traversable
yield "{$columnName} AS {$columnAlias}";
}
}

/**
* Wrap query to avoid duplicate entries for multiple Locations.
*/
private function wrapMainQuery(FilteringQueryBuilder $query): QueryBuilder
{
$wrappedQuery = $this->connection->createQueryBuilder();
$wrappedQuery
->select(array_keys(self::COLUMN_MAP))
->distinct()
->from(sprintf('(%s)', $query->getSQL()), 'wrapped')
->setParameters($query->getParameters(), $query->getParameterTypes());

return $wrappedQuery;
}
}

class_alias(DoctrineGateway::class, 'eZ\Publish\Core\Persistence\Legacy\Filter\Gateway\Content\Doctrine\DoctrineGateway');
2 changes: 1 addition & 1 deletion ...tence/Legacy/Filter/SortClauseQueryBuilder/Content/DateModifiedSortClauseQueryBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public function buildQuery(
FilteringSortClause $sortClause
): void {
/** @var \Ibexa\Contracts\Core\Repository\Values\Content\Query\SortClause $sortClause */
$queryBuilder->addOrderBy('content.modified', $sortClause->direction);
$queryBuilder->addOrderBy('content_modified', $sortClause->direction);
Copy link
Member

@alongosz alongosz Dec 2, 2025

Choose a reason for hiding this comment

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

This is kind of unexpected if you got rid of the wrapped query. AFAIR this notation was used by the wrapped query, wasn't it? How does the SQL look like after the changes?

Copy link
Contributor Author

@mikadamczyk mikadamczyk Dec 2, 2025

Choose a reason for hiding this comment

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

SELECT DISTINCT
      content.id AS content_id,
      content.contentclass_id AS content_type_id,
      content.current_version AS content_current_version,
      content.initial_language_id AS content_initial_language_id,
      content.language_mask AS content_language_mask,
      content.modified AS content_modified,
      content.name AS content_name,
      content.owner_id AS content_owner_id,
      content.published AS content_published,
      content.remote_id AS content_remote_id,
      content.section_id AS content_section_id,
      content.status AS content_status,
      content.is_hidden AS content_is_hidden,
      version.id AS content_version_id,
      version.version AS content_version_no,
      version.creator_id AS content_version_creator_id,
      version.created AS content_version_created,
      version.modified AS content_version_modified,
      version.status AS content_version_status,
      version.language_mask AS content_version_language_mask,
      version.initial_language_id AS content_version_initial_language_id,
      main_location.main_node_id AS content_main_location_id,
      ibexa_sort_location.is_invisible AS ibexa_filter_sort_location_visibility
  FROM ezcontentobject content
  INNER JOIN ezcontentobject_version version
    ON (content.id = version.contentobject_id)
   AND (content.current_version = version.version)
   AND (version.status = 1)
  LEFT JOIN ezcontentobject_tree main_location
    ON (content.id = main_location.contentobject_id)
   AND (main_location.main_node_id = main_location.node_id)
  INNER JOIN ezcontentobject_tree ibexa_sort_location
    ON (content.id = ibexa_sort_location.contentobject_id)
   AND (ibexa_sort_location.node_id = ibexa_sort_location.main_node_id)
  WHERE content.id IN (57)
  ORDER BY ibexa_filter_sort_location_visibility DESC;

and

SELECT DISTINCT
      content.id AS content_id,
      content.contentclass_id AS content_type_id,
      content.current_version AS content_current_version,
      content.initial_language_id AS content_initial_language_id,
      content.language_mask AS content_language_mask,
      content.modified AS content_modified,
      content.name AS content_name,
      content.owner_id AS content_owner_id,
      content.published AS content_published,
      content.remote_id AS content_remote_id,
      content.section_id AS content_section_id,
      content.status AS content_status,
      content.is_hidden AS content_is_hidden,
      version.id AS content_version_id,
      version.version AS content_version_no,
      version.creator_id AS content_version_creator_id,
      version.created AS content_version_created,
      version.modified AS content_version_modified,
      version.status AS content_version_status,
      version.language_mask AS content_version_language_mask,
      version.initial_language_id AS content_version_initial_language_id,
      main_location.main_node_id AS content_main_location_id,
      ibexa_sort_location.depth AS ibexa_filter_sort_location_depth
    ON (content.id = version.contentobject_id)
   AND (content.current_version = version.version)
   AND (version.status = 1)
  LEFT JOIN ezcontentobject_tree main_location
    ON (content.id = main_location.contentobject_id)
   AND (main_location.main_node_id = main_location.node_id)
  INNER JOIN ezcontentobject_tree ibexa_sort_location
    ON (content.id = ibexa_sort_location.contentobject_id)
   AND (ibexa_sort_location.node_id = ibexa_sort_location.main_node_id)
  INNER JOIN ezcontent_language language
    ON language.id & version.language_mask = language.id
  WHERE (content.id IN (63, 61))
    AND ((language.locale IN ('eng-US', 'eng-GB', 'ger-DE')) OR (version.language_mask & 1 = 1))
  ORDER BY ibexa_filter_sort_location_depth ASC;

Copy link
Member

@alongosz alongosz Dec 3, 2025

Choose a reason for hiding this comment

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

@mikadamczyk but that means that any custom sort clause will stop working because we've changed field name format from alias.field to alias_field, doesn't it? Which makes it a BC break.

}
}

Expand Down
2 changes: 1 addition & 1 deletion ...ence/Legacy/Filter/SortClauseQueryBuilder/Content/DatePublishedSortClauseQueryBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public function buildQuery(
FilteringSortClause $sortClause
): void {
/** @var \Ibexa\Contracts\Core\Repository\Values\Content\Query\SortClause $sortClause */
$queryBuilder->addOrderBy('content.published', $sortClause->direction);
$queryBuilder->addOrderBy('content_published', $sortClause->direction);
}
}

Expand Down
2 changes: 1 addition & 1 deletion ...lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Content/IdSortClauseQueryBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public function buildQuery(
FilteringSortClause $sortClause
): void {
/** @var \Ibexa\Contracts\Core\Repository\Values\Content\Query\SortClause $sortClause */
$queryBuilder->addOrderBy('content.id', $sortClause->direction);
$queryBuilder->addOrderBy('content_id', $sortClause->direction);
}
}

Expand Down
Loading
Loading