Skip to content

N°9687 - Fix incorrect return type for the DBObjectSearch::ApplyDataFilter() function #939

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

Open
Molkobain wants to merge 4 commits into
base: support/3.2.3
Choose a base branch
from
+93 −4
Open

N°9687 - Fix incorrect return type for the DBObjectSearch::ApplyDataFilter() function #939

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e1d274b
N°9687 - Fix incorrect return type for the DBObjectSearch::ApplyDataF…
Molkobain Jun 17, 2026
0f7653a
N°9687 - Add unit test
Molkobain Jun 18, 2026
2466406
N°9687 - Improve unit test after code review
Molkobain Jun 18, 2026
14d1862
N°9687 - Improve unit test after code review
Molkobain Jun 18, 2026
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
3 changes: 1 addition & 2 deletions core/dbobjectsearch.class.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1928,9 +1928,8 @@ public function GetExpectedArguments(): array

/**
* @inheritDoc
* @return DBObjectSearch
*/
protected function ApplyDataFilters(): DBObjectSearch
protected function ApplyDataFilters(): DBSearch
{
if ($this->IsAllDataAllowed() || $this->IsDataFiltered()) {

@odain-cbd odain-cbd Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IsDataFiltered() is doing the opposite of what the name seems to tell... hard to understand at first sight.

@odain-cbd odain-cbd Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I strongly advice to complete PHPDOC of all used methods... some are missing or incomplete (mixed).

ie GetSearches, FromEmptySet, GetSelectedClasses, GetJoinedClasses.

@odain-cbd odain-cbd Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

apart from that if we do the minimum to change the code, it could be nice to have full test cover of 3 distincts kinds of ApplyDataFilters (DBUnionSearch, DBsearch, DBObjSearch().

for example security.disable_joined_classes_filter in the conf can change the behaviour. just as UserRightsGetSelectFilter extensibility.... we can discuss it if you want

@odain-cbd odain-cbd Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

last but not least it could be nice to refactor in one common method. where we would pass an array of string (coming from either GetSearches, GetSelectedClasses or GetJoinedClasses).

		// Apply filter (this is similar to the one in DBSearch but the factorization could make it less readable)
		foreach ($aClassesToFilter as $sClassAlias => $sClass) {
			$oVisibleObjects = UserRights::GetSelectFilter($sClass, $this->GetModifierProperties('UserRightsGetSelectFilter'));
			if ($oVisibleObjects === false) {
				$oVisibleObjects = DBObjectSearch::FromEmptySet($sClass);
			}
			if (is_object($oVisibleObjects)) {
				$oVisibleObjects->AllowAllData();
				$oSearch = $oSearch->Filter($sClassAlias, $oVisibleObjects);
				$oSearch->SetDataFiltered();
			}
		}

@odain-cbd odain-cbd Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

no need to compute GetSelectedClass if 'security.disable_joined_classes_filter' is disabled.

return $this;
Expand Down
3 changes: 1 addition & 2 deletions core/dbunionsearch.class.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -676,9 +676,8 @@ public function GetExpectedArguments(): array

/**
* @inheritDoc
* @return DBUnionSearch
*/
protected function ApplyDataFilters(): DBUnionSearch
protected function ApplyDataFilters(): DBSearch

@odain-cbd odain-cbd Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it is a pity that we compute further times IsDataAllowed and IsDataFiltered when dealing with Union....

first time in DBUnion. and 2nd time in the loop when calling each sub item.

{
if ($this->IsAllDataAllowed() || $this->IsDataFiltered()) {
return $this;
Expand Down
67 changes: 67 additions & 0 deletions tests/php-unit-tests/unitary-tests/core/DBSearch/DBSearchApplyDataFiltersTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
<?php

/*
* @copyright Copyright (C) 2010-2026 Combodo SAS
* @license http://opensource.org/licenses/AGPL-3.0
*/

namespace Combodo\iTop\Test\UnitTest\Core;

use Combodo\iTop\Test\UnitTest\ItopDataTestCase;
use DBObjectSearch;
use DBUnionSearch;
use UserRights;

class DBSearchApplyDataFiltersTest extends ItopDataTestCase
{
public const CREATE_TEST_ORG = true;

protected string $sOriginalUserRightsSelectModuleClass = '';

/**
* @throws \Exception
*/
protected function setUp(): void
{
parent::setUp();

// Backup original UserRights select module as it will changed in some tests
$this->sOriginalUserRightsSelectModuleClass = get_class(UserRights::GetModuleInstance());

$this->RequireOnceUnitTestFile('Fixtures/N9687_CustomGetSelectFilterClass.php');
}

/**
* @inheritDoc
*/
public function tearDown(): void
{
// Restore original UserRights select module to not interfere with next tests
UserRights::SelectModule($this->sOriginalUserRightsSelectModuleClass);

parent::tearDown();
}

public function testApplyDataFiltersOnDBObjectSearchShouldAcceptGetSelectFilterClassReturningDBUnionSearch()
{
// Use custom select filter that returns a DBUnionSearch
$sPreviousSelectModuleClass = get_class(UserRights::GetModuleInstance());
UserRights::SelectModule('\\Combodo\\iTop\\Test\\UnitTest\\Core\\Fixtures\\N9687_CustomGetSelectFilterClass');
Comment thread
greptile-apps[bot] marked this conversation as resolved.

// Create a user and login, otherwise the select filter won't apply
self::CreateUser('test_dbsearch_applydatafilters', 3);
UserRights::Login('test_dbsearch_applydatafilters');

// Create a person
$oCreatedPerson = $this->CreatePerson(microtime());

// Try to retrieve it using the select filter
$oSearch = DBObjectSearch::FromOQL("SELECT Person WHERE id = {$oCreatedPerson->GetKey()}");
$oFilteredSearch = $this->InvokeNonPublicMethod(DBObjectSearch::class, 'ApplyDataFilters', $oSearch);

// Restore original select module to not interfere with next tests
UserRights::SelectModule($sPreviousSelectModuleClass);
Comment thread
greptile-apps[bot] marked this conversation as resolved.

$this->assertEquals(DBUnionSearch::class, get_class($oFilteredSearch), "DBObjectSearch::ApplyDataFilters() should be able to return a \DBUnionSearch");
}
}
24 changes: 24 additions & 0 deletions .../php-unit-tests/unitary-tests/core/DBSearch/Fixtures/N9687_CustomGetSelectFilterClass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?php

/*
* @copyright Copyright (C) 2010-2026 Combodo SAS
* @license http://opensource.org/licenses/AGPL-3.0
*/

namespace Combodo\iTop\Test\UnitTest\Core\Fixtures;

use DBObjectSearch;
use DBUnionSearch;
use UserRightsProfile;

class N9687_CustomGetSelectFilterClass extends UserRightsProfile
{
public function GetSelectFilter($oUser, $sClass, $aSettings = [])
{
// We just need the method to return an union search
return new DBUnionSearch([
DBObjectSearch::FromOQL("SELECT $sClass WHERE 1!=2"),
DBObjectSearch::FromOQL("SELECT $sClass WHERE 1=1"),
]);
}
}