From 922d02f5356d5904debaf10003477cdd00306192 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Tue, 19 May 2015 13:39:39 +1200 Subject: [PATCH] API Enable filters to perform 'IS NULL' or 'IS NOT NULL' checks --- .../00_Model/01_Data_Model_and_ORM.md | 34 ++++ docs/en/changelogs/4.0.0.md | 1 + model/DataList.php | 4 + model/connect/Database.php | 14 ++ search/filters/ExactMatchFilter.php | 163 ++++++++++------ tests/model/DataListTest.php | 177 +++++++++++++++++- tests/model/DataObjectTest.php | 3 +- tests/model/DataObjectTest.yml | 5 + tests/model/PolymorphicHasManyListTest.php | 14 ++ 9 files changed, 358 insertions(+), 57 deletions(-) diff --git a/docs/en/02_Developer_Guides/00_Model/01_Data_Model_and_ORM.md b/docs/en/02_Developer_Guides/00_Model/01_Data_Model_and_ORM.md index d7670f262..2179e738d 100644 --- a/docs/en/02_Developer_Guides/00_Model/01_Data_Model_and_ORM.md +++ b/docs/en/02_Developer_Guides/00_Model/01_Data_Model_and_ORM.md @@ -355,6 +355,40 @@ You can use [SearchFilters](searchfilters) to add additional behavior to your `f 'PlayerNumber:GreaterThan' => '10' )); +### Filtering by null values + +Since null values in SQL are special, they are non-comparable with other values, certain filters will add +`IS NULL` or `IS NOT NULL` predicates automatically to your query. As per ANSI SQL-92, any comparison +condition against a field will filter out nulls by default. Therefore, it's necessary to include certain null +checks to ensure that exclusion filters behave predictably. + +For instance, the below code will select only values that do not match the given value, including nulls. + + + :::php + $players = Player::get()->filter('FirstName:not', 'Sam'); + // ... WHERE "FirstName" != 'Sam' OR "FirstName" IS NULL + // Returns rows with any value (even null) other than Sam + + +If null values should be excluded, include the null in your check. + + + :::php + $players = Player::get()->filter('FirstName:not', array('Sam', null)); + // ... WHERE "FirstName" != 'Sam' AND "FirstName" IS NOT NULL + // Only returns non-null values for "FirstName" that aren't Sam. + // Strictly the IS NOT NULL isn't necessary, but is included for explicitness + + +It is also often useful to filter by all rows with either empty or null for a given field. + + + :::php + $players = Player::get()->filter('FirstName', array(null, '')); + // ... WHERE "FirstName" == '' OR "FirstName" IS NULL + // Returns rows with FirstName which is either empty or null + ### filterByCallback diff --git a/docs/en/changelogs/4.0.0.md b/docs/en/changelogs/4.0.0.md index 5c8beee15..b5d7a2702 100644 --- a/docs/en/changelogs/4.0.0.md +++ b/docs/en/changelogs/4.0.0.md @@ -5,6 +5,7 @@ ### Framework * Deprecate `SQLQuery` in favour `SQLSelect` + * `DataList::filter` by null now internally generates "IS NULL" or "IS NOT NULL" conditions appropriately on queries ## Upgrading diff --git a/model/DataList.php b/model/DataList.php index 3ec2d112d..2ca138a45 100644 --- a/model/DataList.php +++ b/model/DataList.php @@ -352,6 +352,10 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab * @example $list = $list->filter(array('Name'=>array('aziz','bob'), 'Age'=>array(21, 43))); * // aziz with the age 21 or 43 and bob with the Age 21 or 43 * + * Note: When filtering on nullable columns, null checks will be automatically added. + * E.g. ->filter('Field:not', 'value) will generate '... OR "Field" IS NULL', and + * ->filter('Field:not', null) will generate '"Field" IS NOT NULL' + * * @todo extract the sql from $customQuery into a SQLGenerator class * * @param string|array Escaped SQL statement. If passed as array, all keys and values will be escaped internally diff --git a/model/connect/Database.php b/model/connect/Database.php index d12896f8a..4bbf036a9 100644 --- a/model/connect/Database.php +++ b/model/connect/Database.php @@ -267,6 +267,20 @@ abstract class SS_Database { $this->query("TRUNCATE \"$table\""); } + /** + * Generates a WHERE clause for null comparison check + * + * @param string $field Quoted field name + * @param bool $isNull Whether to check for NULL or NOT NULL + * @return string Non-parameterised null comparison clause + */ + public function nullCheckClause($field, $isNull) { + $clause = $isNull + ? "%s IS NULL" + : "%s IS NOT NULL"; + return sprintf($clause, $field); + } + /** * Generate a WHERE clause for text matching. * diff --git a/search/filters/ExactMatchFilter.php b/search/filters/ExactMatchFilter.php index 515fa254e..45d0ea216 100644 --- a/search/filters/ExactMatchFilter.php +++ b/search/filters/ExactMatchFilter.php @@ -30,16 +30,51 @@ class ExactMatchFilter extends SearchFilter { * @return DataQuery */ protected function applyOne(DataQuery $query) { + return $this->oneFilter($query, true); + } + + /** + * Excludes an exact match (equals) on a field value. + * + * @return DataQuery + */ + protected function excludeOne(DataQuery $query) { + return $this->oneFilter($query, false); + } + + /** + * Applies a single match, either as inclusive or exclusive + * + * @param DataQuery $query + * @param bool $inclusive True if this is inclusive, or false if exclusive + * @return DataQuery + */ + protected function oneFilter(DataQuery $query, $inclusive) { $this->model = $query->applyRelation($this->relation); + $field = $this->getDbName(); + $value = $this->getValue(); + + // Null comparison check + if($value === null) { + $where = DB::get_conn()->nullCheckClause($field, $inclusive); + return $query->where($where); + } + + // Value comparison check $where = DB::get_conn()->comparisonClause( - $this->getDbName(), + $field, null, true, // exact? - false, // negate? + !$inclusive, // negate? $this->getCaseSensitive(), true ); - return $query->where(array($where => $this->getValue())); + // for != clauses include IS NULL values, since they would otherwise be excluded + if(!$inclusive) { + $nullClause = DB::get_conn()->nullCheckClause($field, true); + $where .= " OR {$nullClause}"; + } + return $query->where(array($where => $value)); } /** @@ -49,50 +84,7 @@ class ExactMatchFilter extends SearchFilter { * @return DataQuery */ protected function applyMany(DataQuery $query) { - $this->model = $query->applyRelation($this->relation); - $caseSensitive = $this->getCaseSensitive(); - $values = $this->getValue(); - if($caseSensitive === null) { - // For queries using the default collation (no explicit case) we can use the WHERE .. IN .. syntax, - // providing simpler SQL than many WHERE .. OR .. fragments. - $column = $this->getDbName(); - $placeholders = DB::placeholders($values); - return $query->where(array( - "$column IN ($placeholders)" => $values - )); - } else { - $whereClause = array(); - $comparisonClause = DB::get_conn()->comparisonClause( - $this->getDbName(), - null, - true, // exact? - false, // negate? - $caseSensitive, - true - ); - foreach($values as $value) { - $whereClause[] = array($comparisonClause => $value); - } - return $query->whereAny($whereClause); - } - } - - /** - * Excludes an exact match (equals) on a field value. - * - * @return DataQuery - */ - protected function excludeOne(DataQuery $query) { - $this->model = $query->applyRelation($this->relation); - $where = DB::get_conn()->comparisonClause( - $this->getDbName(), - null, - true, // exact? - true, // negate? - $this->getCaseSensitive(), - true - ); - return $query->where(array($where => $this->getValue())); + return $this->manyFilter($query, true); } /** @@ -102,32 +94,93 @@ class ExactMatchFilter extends SearchFilter { * @return DataQuery */ protected function excludeMany(DataQuery $query) { + return $this->manyFilter($query, false); + } + + /** + * Applies matches for several values, either as inclusive or exclusive + * + * @param DataQuery $query + * @param bool $inclusive True if this is inclusive, or false if exclusive + * @return DataQuery + */ + protected function manyFilter(DataQuery $query, $inclusive) { $this->model = $query->applyRelation($this->relation); $caseSensitive = $this->getCaseSensitive(); + + // Check values for null + $field = $this->getDbName(); $values = $this->getValue(); - if($caseSensitive === null) { + $hasNull = in_array(null, $values, true); + if($hasNull) { + $values = array_filter($values, function($value) { + return $value !== null; + }); + } + + $connective = ''; + if(empty($values)) { + $predicate = ''; + } elseif($caseSensitive === null) { // For queries using the default collation (no explicit case) we can use the WHERE .. NOT IN .. syntax, // providing simpler SQL than many WHERE .. AND .. fragments. $column = $this->getDbName(); $placeholders = DB::placeholders($values); - return $query->where(array( - "$column NOT IN ($placeholders)" => $values - )); + if($inclusive) { + $predicate = "$column IN ($placeholders)"; + } else { + $predicate = "$column NOT IN ($placeholders)"; + } } else { // Generate reusable comparison clause $comparisonClause = DB::get_conn()->comparisonClause( $this->getDbName(), null, true, // exact? - true, // negate? + !$inclusive, // negate? $this->getCaseSensitive(), true ); - // Since query connective is ambiguous, use AND explicitly here $count = count($values); - $predicate = implode(' AND ', array_fill(0, $count, $comparisonClause)); - return $query->where(array($predicate => $values)); + if($count > 1) { + $connective = $inclusive ? ' OR ' : ' AND '; + $conditions = array_fill(0, $count, $comparisonClause); + $predicate = implode($connective, $conditions); + } else { + $predicate = $comparisonClause; + } } + + // Always check for null when doing exclusive checks (either AND IS NOT NULL / OR IS NULL) + // or when including the null value explicitly (OR IS NULL) + if($hasNull || !$inclusive) { + // If excluding values which don't include null, or including + // values which include null, we should do an `OR IS NULL`. + // Otherwise we are excluding values that do include null, so `AND IS NOT NULL`. + // Simplified from (!$inclusive && !$hasNull) || ($inclusive && $hasNull); + $isNull = !$hasNull || $inclusive; + $nullCondition = DB::get_conn()->nullCheckClause($field, $isNull); + + // Determine merge strategy + if(empty($predicate)) { + $predicate = $nullCondition; + } else { + // Merge null condition with predicate + if($isNull) { + $nullCondition = " OR {$nullCondition}"; + } else { + $nullCondition = " AND {$nullCondition}"; + } + // If current predicate connective doesn't match the same as the null connective + // make sure to group the prior condition + if($connective && (($connective === ' OR ') !== $isNull)) { + $predicate = "({$predicate})"; + } + $predicate .= $nullCondition; + } + } + + return $query->where(array($predicate => $values)); } public function isEmpty() { diff --git a/tests/model/DataListTest.php b/tests/model/DataListTest.php index 1132c3be6..864835ba3 100755 --- a/tests/model/DataListTest.php +++ b/tests/model/DataListTest.php @@ -797,6 +797,180 @@ class DataListTest extends SapphireTest { $this->assertEquals(0, $list->exclude('ID', $obj->ID)->count()); } + public function testFilterByNull() { + $list = DataObjectTest_Fan::get(); + // Force DataObjectTest_Fan/fan5::Email to empty string + $fan5id = $this->idFromFixture('DataObjectTest_Fan', 'fan5'); + DB::prepared_query("UPDATE \"DataObjectTest_Fan\" SET \"Email\" = '' WHERE \"ID\" = ?", array($fan5id)); + + // Filter by null email + $nullEmails = $list->filter('Email', null); + $this->assertDOSEquals(array( + array( + 'Name' => 'Stephen', + ), + array( + 'Name' => 'Mitch', + ) + ), $nullEmails); + + // Filter by non-null + $nonNullEmails = $list->filter('Email:not', null); + $this->assertDOSEquals(array( + array( + 'Name' => 'Damian', + 'Email' => 'damian@thefans.com', + ), + array( + 'Name' => 'Richard', + 'Email' => 'richie@richers.com', + ), + array( + 'Name' => 'Hamish', + ) + ), $nonNullEmails); + + // Filter by empty only + $emptyOnly = $list->filter('Email', ''); + $this->assertDOSEquals(array( + array( + 'Name' => 'Hamish', + ) + ), $emptyOnly); + + // Non-empty only. This should include null values, since ExactMatchFilter works around + // the caveat that != '' also excludes null values in ANSI SQL-92 behaviour. + $nonEmptyOnly = $list->filter('Email:not', ''); + $this->assertDOSEquals(array( + array( + 'Name' => 'Damian', + 'Email' => 'damian@thefans.com', + ), + array( + 'Name' => 'Richard', + 'Email' => 'richie@richers.com', + ), + array( + 'Name' => 'Stephen', + ), + array( + 'Name' => 'Mitch', + ) + ), $nonEmptyOnly); + + // Filter by many including null, empty string, and non-empty + $items1 = $list->filter('Email', array(null, '', 'damian@thefans.com')); + $this->assertDOSEquals(array( + array( + 'Name' => 'Damian', + 'Email' => 'damian@thefans.com', + ), + array( + 'Name' => 'Stephen', + ), + array( + 'Name' => 'Mitch', + ), + array( + 'Name' => 'Hamish', + ) + ), $items1); + + // Filter exclusion of above list + $items2 = $list->filter('Email:not', array(null, '', 'damian@thefans.com')); + $this->assertDOSEquals(array( + array( + 'Name' => 'Richard', + 'Email' => 'richie@richers.com', + ), + ), $items2); + + // Filter by many including empty string and non-empty + $items3 = $list->filter('Email', array('', 'damian@thefans.com')); + $this->assertDOSEquals(array( + array( + 'Name' => 'Damian', + 'Email' => 'damian@thefans.com', + ), + array( + 'Name' => 'Hamish', + ) + ), $items3); + + // Filter by many including empty string and non-empty + // This also relies no the workaround for null comparison as in the $nonEmptyOnly test + $items4 = $list->filter('Email:not', array('', 'damian@thefans.com')); + $this->assertDOSEquals(array( + array( + 'Name' => 'Richard', + 'Email' => 'richie@richers.com', + ), + array( + 'Name' => 'Stephen', + ), + array( + 'Name' => 'Mitch', + ) + ), $items4); + + // Filter by many including empty string and non-empty + // The extra null check isn't necessary, but check that this doesn't fail + $items5 = $list->filterAny(array( + 'Email:not' => array('', 'damian@thefans.com'), + 'Email' => null + )); + $this->assertDOSEquals(array( + array( + 'Name' => 'Richard', + 'Email' => 'richie@richers.com', + ), + array( + 'Name' => 'Stephen', + ), + array( + 'Name' => 'Mitch', + ) + ), $items5); + + // Filter by null or empty values + $items6 = $list->filter('Email', array(null, '')); + $this->assertDOSEquals(array( + array( + 'Name' => 'Stephen', + ), + array( + 'Name' => 'Mitch', + ), + array( + 'Name' => 'Hamish', + ) + ), $items6); + } + + /** + * Test null checks with case modifiers + */ + public function testFilterByNullCase() { + // Test with case (case/nocase both use same code path) + // Test with and without null, and with inclusion/exclusion permutations + $list = DataObjectTest_Fan::get(); + + // Only an explicit NOT NULL should include null values + $items6 = $list->filter('Email:not:case', array(null, '', 'damian@thefans.com')); + $this->assertSQLContains(' AND "DataObjectTest_Fan"."Email" IS NOT NULL', $items6->sql()); + + // These should all include values where Email IS NULL + $items7 = $list->filter('Email:nocase', array(null, '', 'damian@thefans.com')); + $this->assertSQLContains(' OR "DataObjectTest_Fan"."Email" IS NULL', $items7->sql()); + $items8 = $list->filter('Email:not:case', array('', 'damian@thefans.com')); + $this->assertSQLContains(' OR "DataObjectTest_Fan"."Email" IS NULL', $items8->sql()); + + // These should not contain any null checks at all + $items9 = $list->filter('Email:nocase', array('', 'damian@thefans.com')); + $this->assertSQLNotContains('"DataObjectTest_Fan"."Email" IS NULL', $items9->sql()); + $this->assertSQLNotContains('"DataObjectTest_Fan"."Email" IS NOT NULL', $items9->sql()); + } + /** * $list = $list->filterByCallback(function($item, $list) { return $item->Age == 21; }) */ @@ -865,7 +1039,8 @@ class DataListTest extends SapphireTest { $sql = $list->sql($parameters); $this->assertSQLContains( - 'WHERE ("DataObjectTest_TeamComment"."Comment" = ?) AND (("DataObjectTest_TeamComment"."Name" != ?))', + 'WHERE ("DataObjectTest_TeamComment"."Comment" = ?) AND (("DataObjectTest_TeamComment"."Name" != ? ' + . 'OR "DataObjectTest_TeamComment"."Name" IS NULL))', $sql); $this->assertEquals(array('Phil is a unique guy, and comments on team2', 'Bob'), $parameters); } diff --git a/tests/model/DataObjectTest.php b/tests/model/DataObjectTest.php index 4401fe995..dc14e6bb0 100644 --- a/tests/model/DataObjectTest.php +++ b/tests/model/DataObjectTest.php @@ -1887,7 +1887,8 @@ class DataObjectTest_TeamComment extends DataObject implements TestOnly { class DataObjectTest_Fan extends DataObject implements TestOnly { private static $db = array( - 'Name' => 'Varchar(255)' + 'Name' => 'Varchar(255)', + 'Email' => 'Varchar', ); private static $has_one = array( diff --git a/tests/model/DataObjectTest.yml b/tests/model/DataObjectTest.yml index f5065efef..a559f744e 100644 --- a/tests/model/DataObjectTest.yml +++ b/tests/model/DataObjectTest.yml @@ -62,6 +62,7 @@ DataObjectTest_TeamComment: DataObjectTest_Fan: fan1: Name: Damian + Email: damian@thefans.com Favourite: =>DataObjectTest_Team.team1 fan2: Name: Stephen @@ -69,10 +70,14 @@ DataObjectTest_Fan: SecondFavourite: =>DataObjectTest_Team.team2 fan3: Name: Richard + Email: richie@richers.com Favourite: =>DataObjectTest_Team.team1 fan4: Name: Mitch Favourite: =>DataObjectTest_SubTeam.subteam1 + fan5: + Name: Hamish + Email: '' DataObjectTest_Company: company1: Name: Company corp diff --git a/tests/model/PolymorphicHasManyListTest.php b/tests/model/PolymorphicHasManyListTest.php index 0b1fc96fb..81cb32dd6 100644 --- a/tests/model/PolymorphicHasManyListTest.php +++ b/tests/model/PolymorphicHasManyListTest.php @@ -17,8 +17,22 @@ class PolymorphicHasManyListTest extends SapphireTest { protected $extraDataObjects = array( 'DataObjectTest_Team', + 'DataObjectTest_Fixture', 'DataObjectTest_SubTeam', + 'OtherSubclassWithSameField', + 'DataObjectTest_FieldlessTable', + 'DataObjectTest_FieldlessSubTable', + 'DataObjectTest_ValidatedObject', 'DataObjectTest_Player', + 'DataObjectTest_TeamComment', + 'DataObjectTest_EquipmentCompany', + 'DataObjectTest_SubEquipmentCompany', + 'DataObjectTest\NamespacedClass', + 'DataObjectTest\RelationClass', + 'DataObjectTest_ExtendedTeamComment', + 'DataObjectTest_Company', + 'DataObjectTest_Staff', + 'DataObjectTest_CEO', 'DataObjectTest_Fan', );