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.

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

APIv4 - Fix CONTAINS operator to work with more types of serialized fields #26362

Merged
merged 1 commit into from
May 28, 2023
Merged

APIv4 - Fix CONTAINS operator to work with more types of serialized fields #26362

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
23 changes: 22 additions & 1 deletion Civi/Api4/Query/Api4SelectQuery.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -592,8 +592,13 @@ protected function createSQLClause($fieldAlias, $operator, $value, $field, int $
}
return $sql ? implode(' AND ', $sql) : NULL;
}

// The CONTAINS operator matches a substring for strings. For arrays & serialized fields,
// it only matches a complete (not partial) string within the array.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe note that we don't match complete strings for PHP serialized?

Copy link
Member Author

@colemanw colemanw May 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'll be there's a way to support that type of serialization too! Probably something like:

        case \CRM_Core_DAO::SERIALIZE_PHP:
          $operator = 'LIKE';
          $value = '%' . serialize($value) . '%';
          break;

But I haven't tested that. Best to merge this PR first.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I assumed it was a don't bother to support it because there aren't many of them situation.

if ($operator === 'CONTAINS') {
$sep = \CRM_Core_DAO::VALUE_SEPARATOR;
switch ($field['serialize'] ?? NULL) {

case \CRM_Core_DAO::SERIALIZE_JSON:
$operator = 'LIKE';
$value = '%"' . $value . '"%';
Copy link
Contributor

Choose a reason for hiding this comment

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

MIN_INSTALL_MYSQL_VER is now 5.7, so maybe it makes sense to fix this while we're here? Would be good to improve the JSON search, since the current version has some obvious shortcomings.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! I looked at that as well, and tried a few ways of searching JSON but couldn't get it working just right so I thought I'd go ahead and put up this PR without that so at least this can get merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough

Expand All @@ -603,7 +608,23 @@ protected function createSQLClause($fieldAlias, $operator, $value, $field, int $

case \CRM_Core_DAO::SERIALIZE_SEPARATOR_BOOKEND:
$operator = 'LIKE';
$value = '%' . \CRM_Core_DAO::VALUE_SEPARATOR . $value . \CRM_Core_DAO::VALUE_SEPARATOR . '%';
// This is easy to query because the string is always bookended by separators.
$value = '%' . $sep . $value . $sep . '%';
break;

case \CRM_Core_DAO::SERIALIZE_SEPARATOR_TRIMMED:
$operator = 'REGEXP';
// This is harder to query because there's no bookend.
// Use regex to match string within separators or content boundary
// Escaping regex per https://stackoverflow.com/questions/3782379/whats-the-best-way-to-escape-user-input-for-regular-expressions-in-mysql
$value = "(^|$sep)" . preg_quote($value, '&') . "($sep|$)";
break;

case \CRM_Core_DAO::SERIALIZE_COMMA:
$operator = 'REGEXP';
// Match string within commas or content boundary
// Escaping regex per https://stackoverflow.com/questions/3782379/whats-the-best-way-to-escape-user-input-for-regular-expressions-in-mysql
$value = '(^|,)' . preg_quote($value, '&') . '(,|$)';
break;

default:
Expand Down
2 changes: 1 addition & 1 deletion tests/phpunit/api/v4/Api4TestBase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public function tearDown(): void {
/**
* Quick clean by emptying tables created for the test.
*
* @param array $params
* @param array{tablesToTruncate: array} $params
*/
public function cleanup(array $params): void {
$params += [
Expand Down
4 changes: 2 additions & 2 deletions tests/phpunit/api/v4/Custom/CustomContactRefTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,15 +104,15 @@ public function testGetWithJoin() {

$result = Contact::get(FALSE)
->addSelect('id')
->addWhere('MyContactRef.FavPeople.first_name', 'CONTAINS', 'First')
->addWhere('MyContactRef.FavPeople.first_name', 'CONTAINS', 'FirstFav')
->execute()
->single();

$this->assertEquals($contactId1, $result['id']);

$result = Contact::get(FALSE)
->addSelect('id')
->addWhere('MyContactRef.FavPeople.first_name', 'CONTAINS', 'Second')
->addWhere('MyContactRef.FavPeople.first_name', 'CONTAINS', 'SecondFav')
->execute();

$this->assertCount(2, $result);
Expand Down
37 changes: 37 additions & 0 deletions tests/phpunit/api/v4/Entity/GroupTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,43 @@ public function testCreate() {
->execute();
}

public function testParentsInWhereClause() {
// Create 10 groups - at least 1 id will be 2-digit and contain the number 1
$groups = $this->saveTestRecords('Group', [
'records' => array_fill(0, 10, []),
]);

$child1 = $this->createTestRecord('Group', [
'parents' => [$groups[1]['id'], $groups[2]['id']],
]);
$child2 = $this->createTestRecord('Group', [
'parents' => [$groups[8]['id']],
]);
$child3 = $this->createTestRecord('Group', [
'parents' => [$groups[8]['id'], $groups[9]['id']],
]);

// Check that a digit of e.g. "1" doesn't match a value of e.g. "10"
$firstDigit = substr($groups[9]['id'], 0, 1);
$found = Group::get(FALSE)
->addWhere('parents', 'CONTAINS', $firstDigit)
->selectRowCount()
->execute();
$this->assertCount(0, $found);

$found = Group::get(FALSE)
->addWhere('parents', 'CONTAINS', $groups[8]['id'])
->selectRowCount()
->execute();
$this->assertCount(2, $found);

$found = Group::get(FALSE)
->addWhere('parents', 'CONTAINS', $groups[9]['id'])
->execute();
$this->assertCount(1, $found);
$this->assertEquals($child3['id'], $found[0]['id']);
}

public function testGetParents() {
$parent1 = Group::create(FALSE)
->addValue('title', uniqid())
Expand Down