MDL-69026 user: Wrap sub-query in brackets
authorAndrew Nicols <andrew@nicols.co.uk>
Fri, 12 Jun 2020 00:46:46 +0000 (08:46 +0800)
committerAndrew Nicols <andrew@nicols.co.uk>
Fri, 12 Jun 2020 02:03:06 +0000 (10:03 +0800)
It is perfectly valid to have a query like:

Match None of the following:
- Role is ANY of the following:
-- 'Teacher'
-- 'Editing teacher'
-- 'Manager'; AND
- Keyword is NONE of the following:
-- 'Kevin'

However, due to the way in which the query is constructed, this leads to
a query which includes

    WHERE NOT ef.id IS NOT NULL
    AND NOT
        u.id IN (SELECT userid FROM {role_assignments} WHERE roleid IN (...) AND contextid IN (...))
    AND NOT
        NOT (u.firstname || ' ' || u.lastname LIKE '%Kevin%')

The use of NOT NOT is valid in Postgres, MariaDB, MySQL, and MSSQL, but
not in Oracle.

To counter this when the outer jointype is of type NONE, we must wrap
each of the inner WHERE clauses in a set of brackets, which makes the
query:

    WHERE NOT ef.id IS NOT NULL
    AND NOT
        (u.id IN (SELECT userid FROM {role_assignments} WHERE roleid IN (...) AND contextid IN (...)))
    AND NOT
        (NOT (u.firstname || ' ' || u.lastname LIKE '%Kevin%'))

Whilst Oracle does not support the use of `AND NOT NOT ...`, it does support
`AND NOT (NOT ...)`

user/classes/table/participants_search.php
user/tests/table/participants_search_test.php

index 33122cc..7946247 100644 (file)
@@ -283,6 +283,14 @@ class participants_search {
                 case $this->filterset::JOINTYPE_NONE:
                     $wherenot = ' NOT ';
                     $wheresjoin = ' AND NOT ';
+
+                    // Some of the $where conditions may begin with `NOT` which results in `AND NOT NOT ...`.
+                    // To prevent this from breaking on Oracle the inner WHERE clause is wrapped in brackets, making it
+                    // `AND NOT (NOT ...)` which is valid in all DBs.
+                    $wheres = array_map(function($where) {
+                        return "({$where})";
+                    }, $wheres);
+
                     break;
                 default:
                     // Default to 'Any' jointype.
index 66ce32c..2722357 100644 (file)
@@ -3203,7 +3203,7 @@ class participants_search_test extends advanced_testcase {
                             'accesssince' => [
                                 'values' => ['-6 months'],
                                 'jointype' => filter::JOINTYPE_ALL,
-                                ],
+                            ],
                         ],
                         'jointype' => filter::JOINTYPE_NONE,
                         'count' => 1,
@@ -3211,6 +3211,27 @@ class participants_search_test extends advanced_testcase {
                             'barbara.bennett',
                         ],
                     ],
+                    'NONE: Filterset combining several filter types and a double-negative on keyword' => (object) [
+                        'jointype' => filter::JOINTYPE_NONE,
+                        'filterdata' => [
+                            // Note: This is a jointype NONE on the parent jointype NONE.
+                            // The result therefore negated in this instance.
+                            // Include Adam and Anthony.
+                            'keywords' => [
+                                'values' => ['ant'],
+                                'jointype' => filter::JOINTYPE_NONE,
+                            ],
+                            // Excludes Tony.
+                            'status' => [
+                                'values' => [ENROL_USER_SUSPENDED],
+                                'jointype' => filter::JOINTYPE_ALL,
+                            ],
+                        ],
+                        'count' => 1,
+                        'expectedusers' => [
+                            'adam.ant',
+                        ],
+                    ],
                 ],
             ],
         ];