From 54eee99e6adf5e6a32b7b3822cd0e68495d8c5f0 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Sat, 17 Oct 2026 14:15:52 +0200 Subject: [PATCH] fix(aggregation): filters accept IS NULL, IS NOT NULL and [isnull], as the list does The grouped and plain aggregations compared IS NULL as literal text and refused the isnull operator, so chip counts on a page filtered on an empty field (dossiq's Queue, assignee IS NULL) drew nothing. Emptiness is now read by one NullFilter, shared with the list search and nested-path filters, on both the native SQL path and the PHP fallback. Also adds Dutch for the flow step names and descriptions on the palette. Assisted-by: ClaudeCode:claude-opus-5-5 --- l10n/nl.js | 55 +++++++- l10n/nl.json | 55 +++++++- lib/Db/MagicMapper/MagicSearchHandler.php | 4 +- lib/Db/MagicMapper/NestedPathFilter.php | 7 +- lib/Service/Aggregation/AggregationRunner.php | 129 +++++++++++++----- lib/Support/NullFilter.php | 103 ++++++++++++++ .../aggregation-null-filters/proposal.md | 33 +++++ .../specs/aggregation-api/spec.md | 19 +++ .../changes/aggregation-null-filters/tasks.md | 5 + .../AggregationRunnerMultiValueFilterTest.php | 73 ++++++++++ tests/Unit/Support/NullFilterTest.php | 39 ++++++ 11 files changed, 481 insertions(+), 41 deletions(-) create mode 100644 lib/Support/NullFilter.php create mode 100644 openspec/changes/aggregation-null-filters/proposal.md create mode 100644 openspec/changes/aggregation-null-filters/specs/aggregation-api/spec.md create mode 100644 openspec/changes/aggregation-null-filters/tasks.md create mode 100644 tests/Unit/Support/NullFilterTest.php diff --git a/l10n/nl.js b/l10n/nl.js index 54ebbc735e..4ce523edb1 100644 --- a/l10n/nl.js +++ b/l10n/nl.js @@ -3576,7 +3576,60 @@ OC.L10N.register( "Transform of row {number}:": "Omzetting van rij {number}:", "You are not signed in. Sign in and try again.": "Je bent niet aangemeld. Meld je aan en probeer het opnieuw.", "{label} of row {number}": "{label} van rij {number}", - "Another pack already has this slug. Choose a different slug.": "Een ander pakket heeft deze slug al. Kies een andere slug." + "Another pack already has this slug. Choose a different slug.": "Een ander pakket heeft deze slug al. Kies een andere slug.", + "Wait for an answer": "Wachten op een antwoord", + "Pause until someone approves, rejects, or another system reports back.": "Pauzeer tot iemand goedkeurt of afwijst, of tot een ander systeem iets terugmeldt.", + "Evaluate a decision table": "Beslistabel toepassen", + "Apply a table of rules to each item and write the outcome onto it.": "Pas een tabel met regels toe op elk item en schrijf de uitkomst erop.", + "End": "Einde", + "End the flow here, optionally as an error with a message.": "Beëindig de flow hier, als je wilt als fout met een melding.", + "Explode": "Opsplitsen", + "Turn a list on the item into one item per entry, so later steps act on each.": "Maak van een lijst op het item één item per regel, zodat volgende stappen elk apart behandelen.", + "Filter": "Filteren", + "Keep only the items matching a condition.": "Houd alleen de items over die aan een voorwaarde voldoen.", + "Flow state": "Flowgeheugen", + "Remember something between runs of this flow — a counter, a cursor, or a table of slots.": "Onthoud iets tussen uitvoeringen van deze flow: een teller, een positie of een tabel met plekken.", + "Repeat until done": "Herhalen tot klaar", + "Run a chain of steps once per batch, until the source runs out.": "Voer een reeks stappen uit per portie, tot de bron leeg is.", + "Lock an object": "Object vergrendelen", + "Hold an object for this run so nobody else can change it. If another run holds it, this step waits and tries again.": "Houd een object vast voor deze uitvoering, zodat niemand anders het wijzigt. Houdt een andere uitvoering het vast, dan wacht deze stap en probeert het opnieuw.", + "Batch items": "Items in porties delen", + "Split the items into fixed-size batches for a step that needs them a slice at a time.": "Verdeel de items in porties van vaste grootte, voor een stap die ze per deel verwerkt.", + "Map": "Mappen", + "Reshape each item through a stored mapping.": "Zet elk item om met een opgeslagen mapping.", + "Combine the items arriving from several branches into one list.": "Voeg de items uit meerdere takken samen tot één lijst.", + "Read objects": "Objecten lezen", + "Find objects in a register and put them on the item.": "Zoek objecten in een register en zet ze op het item.", + "Write an object": "Object schrijven", + "Create, update, upsert or delete an OpenRegister object for every item — as the person who started the run, with their permissions.": "Maak, wijzig of verwijder een OpenRegister-object voor elk item. Dat gebeurt als de persoon die de uitvoering startte, met diens rechten.", + "Ask a party outside the organisation": "Een partij buiten de organisatie iets vragen", + "Ask the applicant or another party on the case, through the portal, and wait for their answer or upload. For someone in the organisation use \"Ask a person\"; for a system that will call back use \"Wait for an answer\".": "Vraag de aanvrager of een andere partij op de zaak iets via het portaal en wacht op het antwoord of de upload. Voor iemand in de organisatie gebruik je \"Een persoon of groep iets vragen\". Voor een systeem dat terugbelt gebruik je \"Wachten op een antwoord\".", + "Route items": "Items verdelen", + "Send each item down a different branch, chosen per item by a rule.": "Stuur elk item naar een eigen tak, per item gekozen met een regel.", + "Send an email": "E-mail versturen", + "Send an email to people, at this point in the flow.": "Stuur op dit punt in de flow een e-mail naar mensen.", + "Send a notification": "Melding versturen", + "Send an in-app notification to people, at this point in the flow.": "Stuur op dit punt in de flow een melding in Nextcloud naar mensen.", + "Send a Talk message": "Talk-bericht versturen", + "Post a chat message to a Talk conversation, as the user this flow runs as.": "Plaats een chatbericht in een Talk-gesprek, als de gebruiker waarmee deze flow draait.", + "Edit fields": "Velden bewerken", + "Set, rename or remove fields on every item passing through.": "Vul velden, hernoem ze of haal ze weg op elk item dat langskomt.", + "Run a flow": "Flow uitvoeren", + "Run another flow as a step — wait for its result, or start it and carry on.": "Voer een andere flow uit als stap. Wacht op het resultaat, of start hem en ga door.", + "Switch": "Wissel", + "Send the flow down one of several branches, chosen by the condition on each.": "Stuur de flow naar een van meerdere takken, gekozen op de voorwaarde van elke tak.", + "When someone runs it": "Als iemand hem start", + "Start the flow on demand. Says \"a person starts this\" out loud, which a flow with no trigger at all cannot.": "Start de flow wanneer iemand dat wil. Zo staat er zichtbaar dat een persoon hem start, wat een flow zonder trigger niet zegt.", + "When an object changes": "Als een object verandert", + "Start the flow when one kind of object is created, updated or deleted. One event, one register, one schema — use a mapping node to normalise other shapes.": "Start de flow als een soort object wordt aangemaakt, gewijzigd of verwijderd. Eén gebeurtenis, één register, één schema. Gebruik een mappingstap om andere vormen gelijk te trekken.", + "On a schedule": "Volgens schema", + "Start the flow on a cron schedule. The run begins with no object — the first node has to fetch what it works on.": "Start de flow volgens een cronschema. De uitvoering begint zonder object, dus de eerste stap haalt op waarmee hij werkt.", + "Unlock an object": "Object ontgrendelen", + "Release an object this run is holding, so others can change it again. The run releases its locks when it ends anyway; this frees them sooner.": "Laat een object los dat deze uitvoering vasthoudt, zodat anderen het weer kunnen wijzigen. Aan het eind laat de uitvoering het toch los, deze stap doet dat eerder.", + "Ask a person or group": "Een persoon of groep iets vragen", + "Ask a person, a group or an agent to do something, and wait for the answer. For a system that will call back, use \"Wait for an answer\" instead.": "Vraag een persoon, groep of agent iets te doen en wacht op het antwoord. Voor een systeem dat terugbelt gebruik je \"Wachten op een antwoord\".", + "Wait": "Wachten", + "Pause the flow for a while, then carry on where it left off.": "Pauzeer de flow een tijd en ga daarna verder waar hij was." }, "nplurals=2; plural=(n != 1);" ) diff --git a/l10n/nl.json b/l10n/nl.json index f4e28ff085..b8b495caed 100644 --- a/l10n/nl.json +++ b/l10n/nl.json @@ -3463,7 +3463,60 @@ "You may not save this object.": "U mag dit object niet opslaan.", "This draft is not complete yet. Check the fields marked below.": "Dit concept is nog niet compleet. Controleer de gemarkeerde velden.", "Please sign in to make this draft definitive.": "Log in om dit concept definitief te maken.", - "The draft could not be made definitive right now. Nothing was changed; try again later.": "Het concept kon nu niet definitief worden gemaakt. Er is niets gewijzigd; probeer het later opnieuw." + "The draft could not be made definitive right now. Nothing was changed; try again later.": "Het concept kon nu niet definitief worden gemaakt. Er is niets gewijzigd; probeer het later opnieuw.", + "Wait for an answer": "Wachten op een antwoord", + "Pause until someone approves, rejects, or another system reports back.": "Pauzeer tot iemand goedkeurt of afwijst, of tot een ander systeem iets terugmeldt.", + "Evaluate a decision table": "Beslistabel toepassen", + "Apply a table of rules to each item and write the outcome onto it.": "Pas een tabel met regels toe op elk item en schrijf de uitkomst erop.", + "End": "Einde", + "End the flow here, optionally as an error with a message.": "Beëindig de flow hier, als je wilt als fout met een melding.", + "Explode": "Opsplitsen", + "Turn a list on the item into one item per entry, so later steps act on each.": "Maak van een lijst op het item één item per regel, zodat volgende stappen elk apart behandelen.", + "Filter": "Filteren", + "Keep only the items matching a condition.": "Houd alleen de items over die aan een voorwaarde voldoen.", + "Flow state": "Flowgeheugen", + "Remember something between runs of this flow — a counter, a cursor, or a table of slots.": "Onthoud iets tussen uitvoeringen van deze flow: een teller, een positie of een tabel met plekken.", + "Repeat until done": "Herhalen tot klaar", + "Run a chain of steps once per batch, until the source runs out.": "Voer een reeks stappen uit per portie, tot de bron leeg is.", + "Lock an object": "Object vergrendelen", + "Hold an object for this run so nobody else can change it. If another run holds it, this step waits and tries again.": "Houd een object vast voor deze uitvoering, zodat niemand anders het wijzigt. Houdt een andere uitvoering het vast, dan wacht deze stap en probeert het opnieuw.", + "Batch items": "Items in porties delen", + "Split the items into fixed-size batches for a step that needs them a slice at a time.": "Verdeel de items in porties van vaste grootte, voor een stap die ze per deel verwerkt.", + "Map": "Mappen", + "Reshape each item through a stored mapping.": "Zet elk item om met een opgeslagen mapping.", + "Combine the items arriving from several branches into one list.": "Voeg de items uit meerdere takken samen tot één lijst.", + "Read objects": "Objecten lezen", + "Find objects in a register and put them on the item.": "Zoek objecten in een register en zet ze op het item.", + "Write an object": "Object schrijven", + "Create, update, upsert or delete an OpenRegister object for every item — as the person who started the run, with their permissions.": "Maak, wijzig of verwijder een OpenRegister-object voor elk item. Dat gebeurt als de persoon die de uitvoering startte, met diens rechten.", + "Ask a party outside the organisation": "Een partij buiten de organisatie iets vragen", + "Ask the applicant or another party on the case, through the portal, and wait for their answer or upload. For someone in the organisation use \"Ask a person\"; for a system that will call back use \"Wait for an answer\".": "Vraag de aanvrager of een andere partij op de zaak iets via het portaal en wacht op het antwoord of de upload. Voor iemand in de organisatie gebruik je \"Een persoon of groep iets vragen\". Voor een systeem dat terugbelt gebruik je \"Wachten op een antwoord\".", + "Route items": "Items verdelen", + "Send each item down a different branch, chosen per item by a rule.": "Stuur elk item naar een eigen tak, per item gekozen met een regel.", + "Send an email": "E-mail versturen", + "Send an email to people, at this point in the flow.": "Stuur op dit punt in de flow een e-mail naar mensen.", + "Send a notification": "Melding versturen", + "Send an in-app notification to people, at this point in the flow.": "Stuur op dit punt in de flow een melding in Nextcloud naar mensen.", + "Send a Talk message": "Talk-bericht versturen", + "Post a chat message to a Talk conversation, as the user this flow runs as.": "Plaats een chatbericht in een Talk-gesprek, als de gebruiker waarmee deze flow draait.", + "Edit fields": "Velden bewerken", + "Set, rename or remove fields on every item passing through.": "Vul velden, hernoem ze of haal ze weg op elk item dat langskomt.", + "Run a flow": "Flow uitvoeren", + "Run another flow as a step — wait for its result, or start it and carry on.": "Voer een andere flow uit als stap. Wacht op het resultaat, of start hem en ga door.", + "Switch": "Wissel", + "Send the flow down one of several branches, chosen by the condition on each.": "Stuur de flow naar een van meerdere takken, gekozen op de voorwaarde van elke tak.", + "When someone runs it": "Als iemand hem start", + "Start the flow on demand. Says \"a person starts this\" out loud, which a flow with no trigger at all cannot.": "Start de flow wanneer iemand dat wil. Zo staat er zichtbaar dat een persoon hem start, wat een flow zonder trigger niet zegt.", + "When an object changes": "Als een object verandert", + "Start the flow when one kind of object is created, updated or deleted. One event, one register, one schema — use a mapping node to normalise other shapes.": "Start de flow als een soort object wordt aangemaakt, gewijzigd of verwijderd. Eén gebeurtenis, één register, één schema. Gebruik een mappingstap om andere vormen gelijk te trekken.", + "On a schedule": "Volgens schema", + "Start the flow on a cron schedule. The run begins with no object — the first node has to fetch what it works on.": "Start de flow volgens een cronschema. De uitvoering begint zonder object, dus de eerste stap haalt op waarmee hij werkt.", + "Unlock an object": "Object ontgrendelen", + "Release an object this run is holding, so others can change it again. The run releases its locks when it ends anyway; this frees them sooner.": "Laat een object los dat deze uitvoering vasthoudt, zodat anderen het weer kunnen wijzigen. Aan het eind laat de uitvoering het toch los, deze stap doet dat eerder.", + "Ask a person or group": "Een persoon of groep iets vragen", + "Ask a person, a group or an agent to do something, and wait for the answer. For a system that will call back, use \"Wait for an answer\" instead.": "Vraag een persoon, groep of agent iets te doen en wacht op het antwoord. Voor een systeem dat terugbelt gebruik je \"Wachten op een antwoord\".", + "Wait": "Wachten", + "Pause the flow for a while, then carry on where it left off.": "Pauzeer de flow een tijd en ga daarna verder waar hij was." }, "pluralForm": "nplurals=2; plural=(n != 1);", "plurals": { diff --git a/lib/Db/MagicMapper/MagicSearchHandler.php b/lib/Db/MagicMapper/MagicSearchHandler.php index f427ed44da..aeadaebb70 100644 --- a/lib/Db/MagicMapper/MagicSearchHandler.php +++ b/lib/Db/MagicMapper/MagicSearchHandler.php @@ -61,6 +61,7 @@ use OCA\OpenRegister\Service\Search\SearchTermParser; use OCA\OpenRegister\Service\Search\SearchTermSqlCompiler; use OCA\OpenRegister\Support\FilterParams; +use OCA\OpenRegister\Support\NullFilter; use OCA\OpenRegister\Support\QueryLimit; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\DB\QueryBuilder\IQueryFunction; @@ -1715,9 +1716,10 @@ private function likeConditionSql(string $column, mixed $value, object $connecti * @param mixed $value The raw operator value as the request layer delivered it. * * @return bool True when the caller asked for IS NULL. + * @spec openspec/changes/aggregation-null-filters/specs/aggregation-api/spec.md */ private function isNullOperatorAsksForNull(mixed $value): bool { - return filter_var($value, FILTER_VALIDATE_BOOLEAN) === true; + return NullFilter::isNullOperatorAsksForNull(value: $value); }//end isNullOperatorAsksForNull() /** diff --git a/lib/Db/MagicMapper/NestedPathFilter.php b/lib/Db/MagicMapper/NestedPathFilter.php index 3b4e27a6f1..edd36d7b70 100644 --- a/lib/Db/MagicMapper/NestedPathFilter.php +++ b/lib/Db/MagicMapper/NestedPathFilter.php @@ -25,6 +25,7 @@ namespace OCA\OpenRegister\Db\MagicMapper; use OCA\OpenRegister\Db\LikeOperator; +use OCA\OpenRegister\Support\NullFilter; /** * Reads nested-path filters out of a filter map and writes their SQL. @@ -221,12 +222,16 @@ private static function setConditions(string $expression, array $value, callable * @param callable $bind Value to SQL. * * @return list The conditions. + * + * @SuppressWarnings(PHPMD.StaticAccess) NullFilter is the one reader of the isnull value, shared with the list and aggregation paths. + * + * @spec openspec/changes/aggregation-null-filters/specs/aggregation-api/spec.md */ private static function presenceConditions(string $expression, array $value, callable $bind): array { $conditions = []; if (array_key_exists('isnull', $value) === true) { $predicate = 'IS NOT NULL'; - if (filter_var($value['isnull'], FILTER_VALIDATE_BOOLEAN) === true) { + if (NullFilter::isNullOperatorAsksForNull(value: $value['isnull']) === true) { $predicate = 'IS NULL'; } diff --git a/lib/Service/Aggregation/AggregationRunner.php b/lib/Service/Aggregation/AggregationRunner.php index 21b181592b..876233cb87 100644 --- a/lib/Service/Aggregation/AggregationRunner.php +++ b/lib/Service/Aggregation/AggregationRunner.php @@ -56,6 +56,7 @@ use OCA\OpenRegister\Service\OrganisationService; use OCA\OpenRegister\Service\RegisterScopedSchemaResolver; use OCA\OpenRegister\Service\Search\PlaceholderResolver; +use OCA\OpenRegister\Support\NullFilter; use OCP\IDBConnection; use OCP\IUserSession; use Psr\Log\LoggerInterface; @@ -2117,54 +2118,71 @@ private function avg(array $rows, string $field): ?float { * @return array> Filtered rows. * * @spec openspec/specs/aggregation-api/spec.md + * @spec openspec/changes/aggregation-null-filters/specs/aggregation-api/spec.md */ private function applyFilter(array $rows, array $filter): array { $result = []; foreach ($rows as $row) { $keep = true; foreach ($filter as $field => $criterion) { - $value = $row[$field] ?? null; - - // Bare array criterion (a plain, non-associative list) is an - // implicit `in` — any-of match against the candidate list. - if (is_array($criterion) === true && array_is_list($criterion) === true) { - if ($this->valueMatchesAnyOf(rowValue: $value, candidates: $criterion) === false) { - $keep = false; - break; - } - - continue; - } - - if (is_array($criterion) === false) { - // Scalar equality. valueMatchesAnyOf() folds in the - // BUG-SVC-9 string-cast coercion (magic-table columns - // come back as strings while the criterion may be - // int/bool/float) AND the multi-value row-overlap test. - if ($this->valueMatchesAnyOf(rowValue: $value, candidates: [$criterion]) === false) { - $keep = false; - break; - } - - continue; - } - - foreach ($criterion as $op => $onValue) { - if ($this->checkOn(value: $value, op: (string)$op, onValue: $onValue) === false) { - $keep = false; - break 2; - } + if ($this->criterionMatches(value: ($row[$field] ?? null), criterion: $criterion) === false) { + $keep = false; + break; } - }//end foreach + } if ($keep === true) { $result[] = $row; } - }//end foreach + } return $result; }//end applyFilter() + /** + * Whether one row value satisfies one filter criterion. + * + * @param mixed $value The value extracted from the row. + * @param mixed $criterion Scalar = eq, plain list = implicit `in`, assoc array = operator map, + * or the list endpoint's `IS NULL` / `IS NOT NULL` / `{isnull: bool}`. + * + * @return bool True when the value matches. + * + * @spec openspec/specs/aggregation-api/spec.md + * @spec openspec/changes/aggregation-null-filters/specs/aggregation-api/spec.md + */ + private function criterionMatches(mixed $value, mixed $criterion): bool { + // `IS NULL` / `IS NOT NULL` / `{isnull: bool}`, the list endpoint's + // spelling of "empty" and "filled". As a scalar it would otherwise be + // compared as the literal text. + $wantsNull = NullFilter::nullCheck(criterion: $criterion); + if ($wantsNull !== null) { + return ($value === null) === $wantsNull; + } + + // Bare array criterion (a plain, non-associative list) is an + // implicit `in` — any-of match against the candidate list. + if (is_array($criterion) === true && array_is_list($criterion) === true) { + return $this->valueMatchesAnyOf(rowValue: $value, candidates: $criterion); + } + + if (is_array($criterion) === false) { + // Scalar equality. valueMatchesAnyOf() folds in the + // BUG-SVC-9 string-cast coercion (magic-table columns + // come back as strings while the criterion may be + // int/bool/float) AND the multi-value row-overlap test. + return $this->valueMatchesAnyOf(rowValue: $value, candidates: [$criterion]); + } + + foreach ($criterion as $op => $onValue) { + if ($this->checkOn(value: $value, op: (string)$op, onValue: $onValue) === false) { + return false; + } + } + + return true; + }//end criterionMatches() + /** * Apply a single operator check. * @@ -2178,20 +2196,25 @@ private function applyFilter(array $rows, array $filter): array { * multi-value row aren't part of this fix's scope. * * @param mixed $value The value extracted from the row. - * @param string $op Operator name ('eq','ne','gt','gte','lt','lte','in','notIn'). + * @param string $op Operator name ('eq','ne','gt','gte','lt','lte','in','notIn','isnull'). * @param mixed $onValue The operand value to compare against. * * @return bool True when the value satisfies the operator. * - * @throws RuntimeException When the operator is not one of the eight implemented + * @throws RuntimeException When the operator is not one of the nine implemented * spellings. It previously matched every row instead, * silently widening the filter. * * @SuppressWarnings(PHPMD.CyclomaticComplexity) * * @spec openspec/specs/aggregation-api/spec.md + * @spec openspec/changes/aggregation-null-filters/specs/aggregation-api/spec.md */ private function checkOn(mixed $value, string $op, mixed $onValue): bool { + if ($op === NullFilter::ISNULL_OPERATOR) { + return ($value === null) === NullFilter::isNullOperatorAsksForNull(value: $onValue); + } + if ($op === 'eq') { return $this->valueMatchesAnyOf(rowValue: $value, candidates: [$onValue]); } @@ -2244,11 +2267,28 @@ private function checkOn(mixed $value, string $op, mixed $onValue): bool { // or rewritten. throw new RuntimeException( 'Unsupported aggregation filter operator "' . $op . '". Supported: ' - . 'eq, ne, gt, gte, lt, lte, in, notIn. An unknown operator used to match every ' + . 'eq, ne, gt, gte, lt, lte, in, notIn, isnull. An unknown operator used to match every ' . 'row, which silently widened the filter instead of narrowing it.' ); }//end checkOp() + /** + * The SQL that asks for an empty or a filled column. + * + * @param bool $wantsNull True for an empty column. + * + * @return string ` IS NULL` or ` IS NOT NULL`. + * + * @spec openspec/changes/aggregation-null-filters/specs/aggregation-api/spec.md + */ + private function nullPredicate(bool $wantsNull): string { + if ($wantsNull === true) { + return ' IS NULL'; + } + + return ' IS NOT NULL'; + }//end nullPredicate() + /** * Test whether a row value matches any of the given candidates. * @@ -2405,6 +2445,7 @@ private function normaliseForCompare(mixed $v): mixed { * * @spec openspec/changes/retrofit-2026-05-24-annotate-openregister/tasks.md#task-19 * @spec openspec/specs/aggregation-api/spec.md + * @spec openspec/changes/aggregation-null-filters/specs/aggregation-api/spec.md */ private function tryNativeAggregation( Register $register, @@ -2493,11 +2534,12 @@ private function tryNativeAggregation( // {field: {notIn: [...]}} → field NOT IN (?, ?, ?) // {field: {gt|gte|lt|lte: x}} → field > / >= / < / <= ? // {field: {ne: x}} → field <> ? + // {field: 'IS NULL'}, {field: {isnull: true}} → field IS NULL (and the NOT NULL forms) // Reject anything else. foreach ($filter as $value) { if (is_array($value) === true && array_is_list($value) === false) { foreach (array_keys($value) as $op) { - if (in_array((string)$op, ['in', 'notIn', 'gt', 'gte', 'lt', 'lte', 'ne'], true) === false) { + if (in_array((string)$op, ['in', 'notIn', 'gt', 'gte', 'lt', 'lte', 'ne', NullFilter::ISNULL_OPERATOR], true) === false) { return null; } } @@ -2649,6 +2691,12 @@ private function tryNativeAggregation( foreach ($filter as $f => $v) { $col = $this->sanitizeColumnName(name: (string)$f); + $wantsNull = NullFilter::nullCheck(criterion: $v); + if ($wantsNull !== null) { + $whereParts[] = $quote . $col . $quote . $this->nullPredicate(wantsNull: $wantsNull); + continue; + } + // Bare array value — implicit `in` (any-of), REQ-AGG-106 / #2027. // A plain list (non-associative) is distinguished from an // operator map (`{in: [...]}`) by array_is_list(); the operator @@ -2721,6 +2769,13 @@ private function tryNativeAggregation( continue; }//end if + if ((string)$op === NullFilter::ISNULL_OPERATOR) { + $whereParts[] = $quote . $col . $quote . $this->nullPredicate( + wantsNull: NullFilter::isNullOperatorAsksForNull(value: $onValue) + ); + continue; + } + $sqlOn = match ((string)$op) { 'gt' => '>', 'gte' => '>=', diff --git a/lib/Support/NullFilter.php b/lib/Support/NullFilter.php new file mode 100644 index 0000000000..8335e19ad7 --- /dev/null +++ b/lib/Support/NullFilter.php @@ -0,0 +1,103 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://OpenRegister.app + */ + +declare(strict_types=1); + +namespace OCA\OpenRegister\Support; + +/** + * Reads `IS NULL`, `IS NOT NULL` and `{isnull: bool}` filter criteria. + * + * The list search, nested-path filters and the aggregation runner all read + * emptiness through this class, so the endpoints that count a list and the + * one that lists it cannot disagree about what "empty" means. + * + * @spec openspec/changes/aggregation-null-filters/specs/aggregation-api/spec.md + */ +final class NullFilter { + + /** + * The scalar filter value that asks for an empty field (`assignee=IS NULL`). + */ + public const IS_NULL = 'IS NULL'; + + /** + * The scalar filter value that asks for a filled field (`assignee=IS NOT NULL`). + */ + public const IS_NOT_NULL = 'IS NOT NULL'; + + /** + * The operator that asks the same with a boolean (`assignee[isnull]=true`). + */ + public const ISNULL_OPERATOR = 'isnull'; + + /** + * Whether an `isnull` operator value asks for IS NULL rather than IS NOT NULL. + * + * A query string only delivers strings, so a strict `=== true` can never + * match and raw truthiness would read "false" as true. + * `FILTER_VALIDATE_BOOLEAN` reads true/"true"/"1"/"on"/"yes" as true and + * everything else, "false"/"0"/"" included, as false. + * + * @param mixed $value The raw operator value. + * + * @return bool True when the caller asked for IS NULL. + * + * @spec openspec/changes/aggregation-null-filters/specs/aggregation-api/spec.md + */ + public static function isNullOperatorAsksForNull(mixed $value): bool { + return filter_var($value, FILTER_VALIDATE_BOOLEAN) === true; + }//end isNullOperatorAsksForNull() + + /** + * What a filter criterion says about emptiness, in the list endpoint's syntax. + * + * `IS NULL` and `{isnull: true}` ask for an empty field, `IS NOT NULL` and + * `{isnull: false}` for a filled one. Any other criterion says nothing about + * emptiness. An operator bag that carries `isnull` beside other operators + * is not read here; its other operators still apply. + * + * @param mixed $criterion A filter value: a scalar or an operator bag. + * + * @return bool|null True for "must be empty", false for "must be filled", null otherwise. + * + * @spec openspec/changes/aggregation-null-filters/specs/aggregation-api/spec.md + */ + public static function nullCheck(mixed $criterion): ?bool { + if ($criterion === self::IS_NULL) { + return true; + } + + if ($criterion === self::IS_NOT_NULL) { + return false; + } + + if (is_array($criterion) === true + && array_keys($criterion) === [self::ISNULL_OPERATOR] + ) { + return self::isNullOperatorAsksForNull(value: $criterion[self::ISNULL_OPERATOR]); + } + + return null; + }//end nullCheck() +}//end class diff --git a/openspec/changes/aggregation-null-filters/proposal.md b/openspec/changes/aggregation-null-filters/proposal.md new file mode 100644 index 0000000000..9a46fd2fe5 --- /dev/null +++ b/openspec/changes/aggregation-null-filters/proposal.md @@ -0,0 +1,33 @@ +--- +kind: code +--- + +# Proposal: aggregation-null-filters + +## Summary + +The aggregation endpoints (`/api/objects/aggregations/{register}/{schema}`, +`.../grouped`, named and timeseries aggregations) accept the list endpoint's +"empty" / "filled" filter syntax: `field=IS NULL`, `field=IS NOT NULL`, +`field[isnull]=true|false`. The meaning is read in one place, +`NullFilter::nullCheck()` / `NullFilter::isNullOperatorAsksForNull()` (`lib/Support/NullFilter.php`), +which the list search (`MagicSearchHandler`, `NestedPathFilter`) and the +aggregation runner now share. + +## Why + +Dossiq's Queue page filters `assignee: IS NULL`. nextcloud-vue counts its +quick-filter chips through the grouped aggregation with the page filter, and +there `IS NULL` was compared as the literal text (empty groups) while +`[isnull]=true` was refused as an unsupported operator. The list showed rows; +the chips showed nothing. + +## Scope + +- PHP fallback (`applyFilter` / `checkOn`) and the native SQL path + (`tryNativeAggregation`) both honour the three spellings; the native path no + longer bails on `isnull`. +- Row RBAC and organisation scoping are unchanged (the native path keeps its + row predicate; the PHP path keeps its hydration). +- "Empty" means SQL NULL / a missing value, exactly as on the list endpoint; + an empty string is a value. diff --git a/openspec/changes/aggregation-null-filters/specs/aggregation-api/spec.md b/openspec/changes/aggregation-null-filters/specs/aggregation-api/spec.md new file mode 100644 index 0000000000..e5a3bf0c1f --- /dev/null +++ b/openspec/changes/aggregation-null-filters/specs/aggregation-api/spec.md @@ -0,0 +1,19 @@ +## ADDED Requirements + +### Requirement: Aggregation filters accept the list endpoint's empty and filled syntax + +An aggregation filter on a field SHALL accept `IS NULL`, `IS NOT NULL` and the +operator `isnull` (a boolean, read as the list endpoint reads it: true, "true", +"1", "on", "yes" ask for empty; anything else asks for filled), with the +meaning the list endpoint gives them: empty is a missing or SQL NULL value. +Both the native SQL path and the PHP fallback SHALL apply them, and the +reader's row-level access rules SHALL apply as for any other filter. + +#### Scenario: chip counts on a queue of unassigned work +- GIVEN five cases, two of them without an assignee +- WHEN the grouped count runs with `assignee=IS NULL` (or `assignee[isnull]=true`) +- THEN it counts the two unassigned cases, grouped as asked + +#### Scenario: the filled form +- WHEN the count runs with `assignee=IS NOT NULL` (or `assignee[isnull]=false`) +- THEN it counts the three assigned cases diff --git a/openspec/changes/aggregation-null-filters/tasks.md b/openspec/changes/aggregation-null-filters/tasks.md new file mode 100644 index 0000000000..ff4bcf2655 --- /dev/null +++ b/openspec/changes/aggregation-null-filters/tasks.md @@ -0,0 +1,5 @@ +# Tasks: aggregation-null-filters + +- [x] `NullFilter::nullCheck()` and `isNullOperatorAsksForNull()`, shared by list search, nested paths and aggregation +- [x] Aggregation PHP fallback and native SQL honour `IS NULL`, `IS NOT NULL`, `[isnull]` +- [x] Unit tests: NullFilter, PHP fallback counts, native grouped SQL and counts diff --git a/tests/Unit/Service/Aggregation/AggregationRunnerMultiValueFilterTest.php b/tests/Unit/Service/Aggregation/AggregationRunnerMultiValueFilterTest.php index a90303e60e..fe40e7ae87 100644 --- a/tests/Unit/Service/Aggregation/AggregationRunnerMultiValueFilterTest.php +++ b/tests/Unit/Service/Aggregation/AggregationRunnerMultiValueFilterTest.php @@ -39,6 +39,9 @@ declare(strict_types=1); +// phpcs:disable PEAR.Commenting.FunctionComment.Missing -- arrange/act/assert PHPUnit conventions. +// phpcs:disable CustomSniffs.Functions.NamedParameters.RequireNamedParameters -- PHPUnit assertion helpers use positional args. + namespace Unit\Service\Aggregation; use OCA\OpenRegister\Db\MagicMapper\MagicOrganizationHandler; @@ -184,6 +187,76 @@ public function testNativeWrappedInOperatorStillCountsAnyOf(): void { }//end testNativeWrappedInOperatorStillCountsAnyOf() + // ----------------------------------------------------------------------- + // "Empty" and "filled": the list endpoint's IS NULL syntax. + // ----------------------------------------------------------------------- + + /** + * A queue: two unassigned rows, three assigned. + * + * @return array> + */ + private function queueDataset(): array { + return [ + ['status' => 'open', 'region' => null, 'amount' => 10], + ['status' => 'open', 'region' => 'nl', 'amount' => 20], + ['status' => 'in-progress', 'region' => null, 'amount' => 30], + ['status' => 'closed', 'region' => 'be', 'amount' => 40], + ['status' => 'closed', 'region' => 'be', 'amount' => 50], + ]; + }//end queueDataset() + + public function testPhpFallbackCountsEmptyAndFilledFieldsInEverySpelling(): void { + $runner = $this->makePhpFallbackRunner(dataset: $this->queueDataset()); + $count = fn (array $filter): mixed => $runner->runAdhoc( + register: $this->makeRegister(), + schema: $this->makeStatusSchema(), + query: AggregationQuery::create(metric: 'count', filter: $filter) + )['value']; + + $this->assertSame(2, $count(['region' => 'IS NULL']), 'the literal text "IS NULL" matched nothing before'); + $this->assertSame(2, $count(['region' => ['isnull' => 'true']]), 'the isnull operator was refused before'); + $this->assertSame(3, $count(['region' => 'IS NOT NULL'])); + $this->assertSame(3, $count(['region' => ['isnull' => 'false']])); + $this->assertSame(1, $count(['region' => 'IS NULL', 'status' => 'in-progress'])); + }//end testPhpFallbackCountsEmptyAndFilledFieldsInEverySpelling() + + public function testNativeGroupedCountOverEmptyFieldsEmitsIsNull(): void { + $runner = $this->makeNativeRunner(dataset: $this->queueDataset(), capturedSql: $captured); + + $result = $runner->runAdhoc( + register: $this->makeRegister(), + schema: $this->makeStatusSchema(), + query: AggregationQuery::create( + metric: 'count', + filter: ['region' => 'IS NULL'], + groupBy: ['field' => 'status'] + ) + ); + + $this->assertSame('sqlite', $result['backend']); + $this->assertStringContainsString('"region" IS NULL', $captured['sql']); + $this->assertEquals(['open' => 1, 'in-progress' => 1], $this->foldSingle($result['groups'])); + }//end testNativeGroupedCountOverEmptyFieldsEmitsIsNull() + + public function testNativeGroupedCountWithTheIsnullOperatorEmitsIsNotNull(): void { + $runner = $this->makeNativeRunner(dataset: $this->queueDataset(), capturedSql: $captured); + + $result = $runner->runAdhoc( + register: $this->makeRegister(), + schema: $this->makeStatusSchema(), + query: AggregationQuery::create( + metric: 'count', + filter: ['region' => ['isnull' => 'false']], + groupBy: ['field' => 'status'] + ) + ); + + $this->assertSame('sqlite', $result['backend'], 'isnull must stay on the native path, not bail'); + $this->assertStringContainsString('"region" IS NOT NULL', $captured['sql']); + $this->assertEquals(['open' => 1, 'closed' => 2], $this->foldSingle($result['groups'])); + }//end testNativeGroupedCountWithTheIsnullOperatorEmitsIsNotNull() + /** * Fold a single-field grouped result into a `key => value` map. * diff --git a/tests/Unit/Support/NullFilterTest.php b/tests/Unit/Support/NullFilterTest.php new file mode 100644 index 0000000000..64ef500e93 --- /dev/null +++ b/tests/Unit/Support/NullFilterTest.php @@ -0,0 +1,39 @@ + + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +// phpcs:disable PEAR.Commenting.FunctionComment.Missing -- arrange/act/assert PHPUnit conventions. +// phpcs:disable CustomSniffs.Functions.NamedParameters.RequireNamedParameters -- PHPUnit assertion helpers use positional args. + +namespace Unit\Support; + +use OCA\OpenRegister\Support\NullFilter; +use PHPUnit\Framework\TestCase; + +class NullFilterTest extends TestCase { + + public function testTheScalarSpellingsAskForEmptyAndFilled(): void { + $this->assertTrue(NullFilter::nullCheck(criterion: 'IS NULL')); + $this->assertFalse(NullFilter::nullCheck(criterion: 'IS NOT NULL')); + } + + public function testTheOperatorReadsItsValueAsAQueryStringBoolean(): void { + $this->assertTrue(NullFilter::nullCheck(criterion: ['isnull' => 'true'])); + $this->assertTrue(NullFilter::nullCheck(criterion: ['isnull' => '1'])); + $this->assertFalse(NullFilter::nullCheck(criterion: ['isnull' => 'false'])); + $this->assertFalse(NullFilter::nullCheck(criterion: ['isnull' => ''])); + } + + public function testAnyOtherCriterionSaysNothingAboutEmptiness(): void { + $this->assertNull(NullFilter::nullCheck(criterion: 'open')); + $this->assertNull(NullFilter::nullCheck(criterion: ['in' => ['a']])); + $this->assertNull(NullFilter::nullCheck(criterion: ['isnull' => 'true', 'ne' => 'x'])); + } +}