Skip to content

Start moving itemtype-specific SQL criteria out of Search engine #14002

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
cconard96 wants to merge 1 commit into
base: main
Choose a base branch
from
+1,863 −1,565
Open

Start moving itemtype-specific SQL criteria out of Search engine #14002

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
60 changes: 12 additions & 48 deletions .phpstan-baseline.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1009,30 +1009,12 @@
'count' => 1,
'path' => __DIR__ . '/src/CommonITILObject.php',
];
$ignoreErrors[] = [
'message' => '#^PHPDoc tag @var with type CommonDBTM is not subtype of native type null\\.$#',
'identifier' => 'varTag.nativeType',
'count' => 3,
'path' => __DIR__ . '/src/CommonITILObject.php',
];
$ignoreErrors[] = [
'message' => '#^PHPDoc tag @var with type CommonDBTM is not subtype of native type string\\.$#',
'identifier' => 'varTag.nativeType',
'count' => 2,
'path' => __DIR__ . '/src/CommonITILObject.php',
];
$ignoreErrors[] = [
'message' => '#^Right side of && is always true\\.$#',
'identifier' => 'booleanAnd.rightAlwaysTrue',
'count' => 1,
'path' => __DIR__ . '/src/CommonITILObject.php',
];
$ignoreErrors[] = [
'message' => '#^Strict comparison using \\=\\=\\= between CommonDBTM and \'User\' will always evaluate to false\\.$#',
'identifier' => 'identical.alwaysFalse',
'count' => 1,
'path' => __DIR__ . '/src/CommonITILObject.php',
];
$ignoreErrors[] = [
'message' => '#^Strict comparison using \\=\\=\\= between false and int\\|string\\|null will always evaluate to false\\.$#',
'identifier' => 'identical.alwaysFalse',
Expand Down Expand Up @@ -3241,12 +3223,6 @@
'count' => 2,
'path' => __DIR__ . '/src/Glpi/Search/Provider/SQLProvider.php',
];
$ignoreErrors[] = [
'message' => '#^Call to static method getAssignableVisiblityCriteria\\(\\) on an unknown class Glpi\\\\Features\\\\AssignableItem\\.$#',
'identifier' => 'class.notFound',
'count' => 1,
'path' => __DIR__ . '/src/Glpi/Search/Provider/SQLProvider.php',
];
$ignoreErrors[] = [
'message' => '#^Default value of the parameter \\#4 \\$meta \\(int\\) of method Glpi\\\\Search\\\\Provider\\\\SQLProvider\\:\\:giveItem\\(\\) is incompatible with type bool\\.$#',
'identifier' => 'parameter.defaultValue',
Expand Down Expand Up @@ -3307,12 +3283,6 @@
'count' => 1,
'path' => __DIR__ . '/src/Glpi/Search/Provider/SQLProvider.php',
];
$ignoreErrors[] = [
'message' => '#^Method Glpi\\\\Search\\\\Provider\\\\SQLProvider\\:\\:getWhereCriteria\\(\\) should return array\\|null but returns Glpi\\\\DBAL\\\\QueryExpression\\.$#',
'identifier' => 'return.type',
'count' => 1,
'path' => __DIR__ . '/src/Glpi/Search/Provider/SQLProvider.php',
];
$ignoreErrors[] = [
'message' => '#^Method Glpi\\\\Search\\\\Provider\\\\SQLProvider\\:\\:giveItem\\(\\) should return string but returns int\\.$#',
'identifier' => 'return.type',
Expand Down Expand Up @@ -3343,18 +3313,6 @@
'count' => 1,
'path' => __DIR__ . '/src/Glpi/Search/Provider/SQLProvider.php',
];
$ignoreErrors[] = [
'message' => '#^PHPDoc tag @var for variable \\$itemtype has invalid type Glpi\\\\Features\\\\AssignableItem\\.$#',
'identifier' => 'varTag.trait',
'count' => 1,
'path' => __DIR__ . '/src/Glpi/Search/Provider/SQLProvider.php',
];
$ignoreErrors[] = [
'message' => '#^PHPDoc tag @var with type Glpi\\\\Features\\\\AssignableItem is not subtype of native type string\\.$#',
'identifier' => 'varTag.nativeType',
'count' => 1,
'path' => __DIR__ . '/src/Glpi/Search/Provider/SQLProvider.php',
];
$ignoreErrors[] = [
'message' => '#^Strict comparison using \\!\\=\\= between 100\\|float\\|string and null will always evaluate to true\\.$#',
'identifier' => 'notIdentical.alwaysTrue',
Expand All @@ -3373,12 +3331,6 @@
'count' => 2,
'path' => __DIR__ . '/src/Glpi/Search/Provider/SQLProvider.php',
];
$ignoreErrors[] = [
'message' => '#^Strict comparison using \\=\\=\\= between \'Problem\' and \'Problem\' will always evaluate to true\\.$#',
'identifier' => 'identical.alwaysTrue',
'count' => 2,
'path' => __DIR__ . '/src/Glpi/Search/Provider/SQLProvider.php',
];
$ignoreErrors[] = [
'message' => '#^Strict comparison using \\=\\=\\= between 2 and 2 will always evaluate to true\\.$#',
'identifier' => 'identical.alwaysTrue',
Expand Down Expand Up @@ -5755,5 +5707,17 @@
'count' => 2,
'path' => __DIR__ . '/src/autoload/i18n.php',
];
$ignoreErrors[] = [
'message' => '#^Instanceof between static\\(PlanningExternalEvent\\) and ExtraVisibilityCriteria will always evaluate to true\\.$#',
'identifier' => 'instanceof.alwaysTrue',
'count' => 1,
'path' => __DIR__ . '/src/PlanningExternalEvent.php',
];
$ignoreErrors[] = [
'message' => '#^Call to an undefined static method CommonDBTM\\:\\:getVisibilityCriteria\\(\\)\\.$#',
'identifier' => 'staticMethod.notFound',
'count' => 2,
'path' => __DIR__ . '/src/CommonDBTM.php',
];

return ['parameters' => ['ignoreErrors' => $ignoreErrors]];
102 changes: 50 additions & 52 deletions phpunit/functional/SearchTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1756,8 +1756,10 @@ public function testAddOrderBy($itemtype, $sort_fields, $expected)
{
$result = \Search::addOrderBy($itemtype, $sort_fields);
$this->assertEquals($expected, $result);
}

// Complex cases
public function testAddOrderByComplex()
{
$table_addtable = 'glpi_users_af1042e23ce6565cfe58c6db91f84692';
$table_ticket_user = 'glpi_tickets_users_019878060c6d5f06cbe3c4d7c31dec24';

Expand All @@ -1769,17 +1771,17 @@ public function testAddOrderBy($itemtype, $sort_fields, $expected)
]
]);
$this->assertEquals(
" ORDER BY GROUP_CONCAT(DISTINCT CONCAT(
IFNULL(`$table_addtable`.`firstname`, ''),
IFNULL(`$table_addtable`.`realname`, ''),
IFNULL(`$table_addtable`.`name`, ''),
IFNULL(`$table_ticket_user`.`alternative_email`, '')
) ORDER BY CONCAT(
IFNULL(`$table_addtable`.`firstname`, ''),
IFNULL(`$table_addtable`.`realname`, ''),
IFNULL(`$table_addtable`.`name`, ''),
IFNULL(`$table_ticket_user`.`alternative_email`, '')) ASC
) ASC",
" ORDER BY GROUP_CONCAT(DISTINCT CONCAT(" .
"IFNULL(`$table_addtable`.`firstname`, '')," .
" IFNULL(`$table_addtable`.`realname`, '')," .
" IFNULL(`$table_addtable`.`name`, '')," .
" IFNULL(`$table_ticket_user`.`alternative_email`, '')" .
") ORDER BY CONCAT(" .
"IFNULL(`$table_addtable`.`firstname`, '')," .
" IFNULL(`$table_addtable`.`realname`, '')," .
" IFNULL(`$table_addtable`.`name`, '')," .
" IFNULL(`$table_ticket_user`.`alternative_email`, '')) ASC" .
") ASC",
$user_order_1
);
$user_order_2 = \Search::addOrderBy('Ticket', [
Expand All @@ -1789,17 +1791,17 @@ public function testAddOrderBy($itemtype, $sort_fields, $expected)
]
]);
$this->assertEquals(
" ORDER BY GROUP_CONCAT(DISTINCT CONCAT(
IFNULL(`$table_addtable`.`firstname`, ''),
IFNULL(`$table_addtable`.`realname`, ''),
IFNULL(`$table_addtable`.`name`, ''),
IFNULL(`$table_ticket_user`.`alternative_email`, '')
) ORDER BY CONCAT(
IFNULL(`$table_addtable`.`firstname`, ''),
IFNULL(`$table_addtable`.`realname`, ''),
IFNULL(`$table_addtable`.`name`, ''),
IFNULL(`$table_ticket_user`.`alternative_email`, '')) ASC
) DESC",
" ORDER BY GROUP_CONCAT(DISTINCT CONCAT(" .
"IFNULL(`$table_addtable`.`firstname`, '')," .
" IFNULL(`$table_addtable`.`realname`, '')," .
" IFNULL(`$table_addtable`.`name`, '')," .
" IFNULL(`$table_ticket_user`.`alternative_email`, '')" .
") ORDER BY CONCAT(" .
"IFNULL(`$table_addtable`.`firstname`, '')," .
" IFNULL(`$table_addtable`.`realname`, '')," .
" IFNULL(`$table_addtable`.`name`, '')," .
" IFNULL(`$table_ticket_user`.`alternative_email`, '')) ASC" .
") DESC",
$user_order_2
);

Expand All @@ -1811,17 +1813,17 @@ public function testAddOrderBy($itemtype, $sort_fields, $expected)
]
]);
$this->assertEquals(
" ORDER BY GROUP_CONCAT(DISTINCT CONCAT(
IFNULL(`$table_addtable`.`realname`, ''),
IFNULL(`$table_addtable`.`firstname`, ''),
IFNULL(`$table_addtable`.`name`, ''),
IFNULL(`$table_ticket_user`.`alternative_email`, '')
) ORDER BY CONCAT(
IFNULL(`$table_addtable`.`realname`, ''),
IFNULL(`$table_addtable`.`firstname`, ''),
IFNULL(`$table_addtable`.`name`, ''),
IFNULL(`$table_ticket_user`.`alternative_email`, '')) ASC
) ASC",
" ORDER BY GROUP_CONCAT(DISTINCT CONCAT(" .
"IFNULL(`$table_addtable`.`realname`, '')," .
" IFNULL(`$table_addtable`.`firstname`, '')," .
" IFNULL(`$table_addtable`.`name`, '')," .
" IFNULL(`$table_ticket_user`.`alternative_email`, '')" .
") ORDER BY CONCAT(" .
"IFNULL(`$table_addtable`.`realname`, '')," .
" IFNULL(`$table_addtable`.`firstname`, '')," .
" IFNULL(`$table_addtable`.`name`, '')," .
" IFNULL(`$table_ticket_user`.`alternative_email`, '')) ASC" .
") ASC",
$user_order_3
);
$user_order_4 = \Search::addOrderBy('Ticket', [
Expand All @@ -1831,17 +1833,17 @@ public function testAddOrderBy($itemtype, $sort_fields, $expected)
]
]);
$this->assertEquals(
" ORDER BY GROUP_CONCAT(DISTINCT CONCAT(
IFNULL(`$table_addtable`.`realname`, ''),
IFNULL(`$table_addtable`.`firstname`, ''),
IFNULL(`$table_addtable`.`name`, ''),
IFNULL(`$table_ticket_user`.`alternative_email`, '')
) ORDER BY CONCAT(
IFNULL(`$table_addtable`.`realname`, ''),
IFNULL(`$table_addtable`.`firstname`, ''),
IFNULL(`$table_addtable`.`name`, ''),
IFNULL(`$table_ticket_user`.`alternative_email`, '')) ASC
) DESC",
" ORDER BY GROUP_CONCAT(DISTINCT CONCAT(" .
"IFNULL(`$table_addtable`.`realname`, '')," .
" IFNULL(`$table_addtable`.`firstname`, '')," .
" IFNULL(`$table_addtable`.`name`, '')," .
" IFNULL(`$table_ticket_user`.`alternative_email`, '')" .
") ORDER BY CONCAT(" .
"IFNULL(`$table_addtable`.`realname`, '')," .
" IFNULL(`$table_addtable`.`firstname`, '')," .
" IFNULL(`$table_addtable`.`name`, '')," .
" IFNULL(`$table_ticket_user`.`alternative_email`, '')) ASC" .
") DESC",
$user_order_4
);
}
Expand Down Expand Up @@ -2613,7 +2615,7 @@ public static function providerAddWhere()
'searchtype' => 'contains',
'val' => '< 192.168.1.10',
'meta' => false,
'expected' => "AND (INET_ATON(`glpi_ipaddresses`.`name`) < INET_ATON('192.168.1.10'))",
'expected' => "AND INET_ATON(`glpi_ipaddresses`.`name`) < INET_ATON('192.168.1.10')",
],
[
'link' => ' AND ',
Expand All @@ -2623,7 +2625,7 @@ public static function providerAddWhere()
'searchtype' => 'contains',
'val' => '> 192.168.1.10',
'meta' => false,
'expected' => "AND (INET_ATON(`glpi_ipaddresses`.`name`) > INET_ATON('192.168.1.10'))",
'expected' => "AND INET_ATON(`glpi_ipaddresses`.`name`) > INET_ATON('192.168.1.10')",
],
];
}
Expand Down Expand Up @@ -2731,11 +2733,7 @@ public function testSearchWithMultipleFkeysOnSameTable()
"`glpi_users_users_id_recipient`.`id` = '{$user_normal_id}'",

// Check that ORDER applies on corresponding table alias
"CONCAT(
IFNULL(`glpi_users_users_id_recipient`.`realname`, ''),
IFNULL(`glpi_users_users_id_recipient`.`firstname`, ''),
IFNULL(`glpi_users_users_id_recipient`.`name`, '')
) ASC"
"CONCAT(IFNULL(`glpi_users_users_id_recipient`.`realname`, ''), IFNULL(`glpi_users_users_id_recipient`.`firstname`, ''), IFNULL(`glpi_users_users_id_recipient`.`name`, '')) ASC"
];
foreach ($contains as $contain) {
$this->assertStringContainsString($contain, $data['sql']['search']);
Expand Down
7 changes: 3 additions & 4 deletions src/Change.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,11 @@ class Change extends CommonITILObject
protected static $forward_entity_to = ['ChangeValidation', 'ChangeCost'];

// From CommonITIL
/** @var class-string<CommonITILActor> */
public $userlinkclass = 'Change_User';
/** @var class-string<CommonITILActor> */
public $grouplinkclass = 'Change_Group';
/** @var class-string<CommonITILActor> */
public $supplierlinkclass = 'Change_Supplier';

public static $rightname = 'change';
Expand All @@ -59,10 +62,6 @@ class Change extends CommonITILObject
const IMPACT_MASK_FIELD = 'impact_mask';
const STATUS_MATRIX_FIELD = 'change_status';


const READMY = 1;
const READALL = 1024;

// Specific status for changes
const REFUSED = 13;
const CANCELED = 14;
Expand Down
86 changes: 86 additions & 0 deletions src/CommonDBTM.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@

use Glpi\Application\View\TemplateRenderer;
use Glpi\Asset\Asset_PeripheralAsset;
use Glpi\DBAL\QueryExpression;
use Glpi\DBAL\QueryFunction;
use Glpi\DBAL\QueryParam;
use Glpi\Event;
Expand Down Expand Up @@ -6858,4 +6859,89 @@ public static function getByUuid(string $uuid): ?self

return null;
}

/**
* Get the default SELECT criteria for the itemtype.
* This method is called on the itemtype being searched on.
* @return array The SELECT criteria to use
*/
public static function getSQLDefaultSelectCriteria(): array
{
return [];
}

/**
* Get the default WHERE criteria for the itemtype.
* This method is called on the itemtype being searched on.
* @return array The WHERE criteria to use
*/
public static function getSQLDefaultWhereCriteria(): array
{
if (is_subclass_of(static::class, ExtraVisibilityCriteria::class)) {
return static::getVisibilityCriteria()['WHERE'] ?? [];
}
return [];
}

/**
* Get the default JOIN criteria for the itemtype.
* This method is called on the itemtype being searched on.
* @return array The JOIN criteria to use
*/
public static function getSQLDefaultJoinCriteria(string $ref_table, array &$already_link_tables): array
{
if (is_subclass_of(static::class, ExtraVisibilityCriteria::class)) {
$leftjoin = static::getVisibilityCriteria()['LEFT JOIN'] ?? [];
$out = ['LEFT JOIN' => $leftjoin];
foreach ($leftjoin as $table => $criteria) {
$already_link_tables[] = $table;
}
return $out;
}
return [];
}

/**
* Get the SELECT criteria for the provided itemtype and search option.
* This method is called on the class that the search option belongs to (based on the table).
* @param class-string<CommonDBTM> $itemtype The itemtype being searched on
* @param SearchOption $opt The search option being handled
* @param bool $meta Whether the search option is for a meta field
* @param class-string<CommonDBTM>|'' $meta_type The meta itemtype being searched on
* @return array|null The SELECT criteria to use, or null to use the default handling
*/
public static function getSQLSelectCriteria(string $itemtype, SearchOption $opt, bool $meta = false, string $meta_type = ''): ?array
{
return null;
}

/**
* Get an array of criteria to handle the search option in a non-standard way.
* This method is called on the class that the search option belongs to (based on the table).
* @param class-string<CommonDBTM> $itemtype The main itemtype being searched on
* @param SearchOption $opt The search option being handled
* @param bool $nott Whether the search option is negated
* @param string $searchtype The search type (e.g. 'contains')
* @param mixed $val The value to search for
* @param bool $meta Whether the search option is for a meta field
* @param callable $fn_append_with_search A helper function to append a criterion to a criteria array in a standardized way
* @phpstan-param callable(array &$criteria, string|QueryFunction $value): void $fn_append_with_search
* @return array|null The criteria to use, or null to use the default handling
*/
public static function getSQLWhereCriteria(string $itemtype, SearchOption $opt, bool $nott, string $searchtype, mixed $val, bool $meta, callable $fn_append_with_search): ?array
{
return null;
}

/**
* Get the ORDER BY criteria for the provided itemtype and search option.
* @param class-string<CommonDBTM> $itemtype The main itemtype being searched on
* @param SearchOption $opt The search option being handled
* @param 'ASC'|'DESC' $order The order direction ('ASC' or 'DESC')
* @return QueryExpression|null The ORDER BY criteria to use, or null to use the default handling
*/
public static function getSQLOrderByCriteria(string $itemtype, SearchOption $opt, string $order): ?\Glpi\DBAL\QueryExpression
{
return null;
}
}
Loading