fix(review): #4580 review findings, round 2

lib/AppInfo/Application.php

@@ -729,6 +729,12 @@ function () {
 		// driver-level 500 an unresolvable column name used to produce.
 		$context->registerMiddleware(\OCA\OpenRegister\Middleware\UnknownMetadataFieldMiddleware::class);
 
+		// Register the RelatedRowFilterMiddleware: maps
+		// RelatedRowFilterNotSupportedException — a `_related`/`_referenced`
+		// filter on a multi-schema facet route that cannot honour it — onto an
+		// HTTP 400 with the exception's message, not an uncaught 500.
+		$context->registerMiddleware(\OCA\OpenRegister\Middleware\RelatedRowFilterMiddleware::class);
+
 		// Register the MaintenanceModeMiddleware (admin-operations-console
 		// D-6): while maintenance mode holds, every controller but the
 		// operations console is refused with the administered message, so the

lib/Controller/FlowRunController.php

@@ -29,6 +29,7 @@
 namespace OCA\OpenRegister\Controller;
 
 use DateTime;
+use DateTimeZone;
 use OCA\OpenRegister\Db\AuditFlowAttribution;
 use OCA\OpenRegister\Db\FlowRun;
 use OCA\OpenRegister\Db\FlowRunMapper;
@@ -254,7 +255,7 @@ public function summary(): JSONResponse {
 		}
 
 		if ($since === null) {
-			$since = new DateTime('-7 days');
+			$since = (new DateTime('-7 days'))->setTimezone(new DateTimeZone('UTC'));
 		}
 
 		$flowIds = array_slice($this->flowIdsParam(), 0, 200);
@@ -329,7 +330,11 @@ private function sinceParam(): DateTime|false|null {
 		}
 
 		try {
-			return new DateTime(trim($raw));
+			// 🔴 NORMALISED TO UTC BEFORE IT IS BOUND. The mapper binds the moment
+			// with PARAM_DATETIME_MUTABLE, which formats the object's own wall
+			// clock without converting, and the `created` column is UTC:
+			// `12:00:00+02:00` was compared as 12:00:00, two hours off the instant.
+			return (new DateTime(trim($raw)))->setTimezone(new DateTimeZone('UTC'));
 		} catch (Throwable) {
 			return false;
 		}

lib/Controller/ViewsController.php

@@ -216,16 +216,16 @@ private function refusalFor(InvalidArgumentException $e): JSONResponse {
 	/**
 	 * Refuse an update that changes fields this caller does not own.
 	 *
-	 * Answers a response to RETURN, or null when the update may proceed. The
-	 * refusal NAMES the fields, because the message a member needs is which
-	 * field was refused rather than that something was.
+	 * Answers a response to RETURN, or null when the update may proceed. The refusal
+	 * NAMES the fields, because a member needs to know which field was refused.
 	 *
 	 * Only fields whose VALUE changes are judged: the edit screen sends the
 	 * whole view. The caller resolves the view with requireReachableView().
 	 *
 	 * @param View $view The stored view.
 	 * @param string $userId The caller.
 	 * @param array<string, mixed> $data The request body.
+	 * @param array<string, mixed>|null $savedAs The body as the save will write it, when that differs (PUT writes `isPublic` false when omitted).
 	 *
 	 * @return JSONResponse|null The refusal, or null when allowed.
 	 *
@@ -234,7 +234,7 @@ private function refusalFor(InvalidArgumentException $e): JSONResponse {
 	 * @spec openspec/specs/saved-search-views/spec.md
 	 * @spec openspec/changes/view-alert-recipients/specs/saved-search-views/spec.md
 	 */
-	private function refuseForbiddenViewFields(View $view, string $userId, array $data): ?JSONResponse {
+	private function refuseForbiddenViewFields(View $view, string $userId, array $data, ?array $savedAs=null): ?JSONResponse {
 		// Only keys that name a view property are judged, so a `_limit` or
 		// routing key on the body cannot refuse an update a member may make.
 		$fields = array_intersect_key(
@@ -261,7 +261,7 @@ private function refuseForbiddenViewFields(View $view, string $userId, array $da
 		);
 
 		if ($refused === []) {
-			$this->viewers->assertMayNameRecipients(view: $view->jsonSerialize(), reach: $this->viewers->reachOf(userId: $userId), data: $data);
+			$this->viewers->assertMayNameRecipients(view: $view->jsonSerialize(), reach: $this->viewers->reachOf(userId: $userId), data: ($savedAs ?? $data));
 			return null;
 		}
 
@@ -592,7 +592,7 @@ public function update(string $id): JSONResponse {
 			// an administrator are unaffected: `mayAdminister()` answers true
 			// for both and the guard returns null.
 			$stored = $this->requireReachableView(id: $id, userId: $userId);
-			$refusal = $this->refuseForbiddenViewFields(view: $stored, userId: $userId, data: $data);
+			$refusal = $this->refuseForbiddenViewFields(view: $stored, userId: $userId, data: $data, savedAs: ($data + ['isPublic' => false]));
 			if ($refusal !== null) {
 				return $refusal;
 			}

lib/Db/MagicMapper.php

@@ -69,6 +69,8 @@
 use OCA\OpenRegister\Service\FieldEncryptionHandler;
 use OCA\OpenRegister\Service\Object\DraftStatusPolicy;
 use OCA\OpenRegister\Service\Object\ObjectEventGate;
+use OCA\OpenRegister\Service\Query\PropertyReadGuard;
+use OCA\OpenRegister\Service\Query\RelatedRowAccess;
 use OCA\OpenRegister\Service\Query\RelatedRowQueryApplier;
 use OCA\OpenRegister\Service\SettingsService;
 use OCA\OpenRegister\Support\QueryLimit;
@@ -611,12 +613,21 @@ private function initializeHandlers(): void {
 		// Assembled by hand rather than resolved from the container, for the
 		// same reason the cache handler above is not: the container would walk
 		// MagicMapper → applier → MagicTableHandler → MagicMapper and recurse.
+		// The applier also answers the property read question for the search
+		// handler's nested-path filters: a filter that reaches through a
+		// reference and one on a path inside an object property are the same
+		// oracle on a field the caller may not read.
+		$relatedRowAccess = new RelatedRowAccess(
+			rbac: $this->rbacHandler,
+			logger: $this->logger,
+			guard: new PropertyReadGuard(container: $this->container, logger: $this->logger)
+		);
 		$relatedRowApplier = new RelatedRowQueryApplier(
 			schemaMapper: $this->schemaMapper,
 			tableHandler: $this->tableHandler,
-			rbacHandler: $this->rbacHandler,
 			db: $this->db,
-			logger: $this->logger
+			logger: $this->logger,
+			access: $relatedRowAccess
 		);
 
 		$this->searchHandler = new MagicSearchHandler(
@@ -1676,6 +1687,7 @@ private function countUnionBatch(array $query, array $registerSchemaPairs): int
 	 * UNION ALL is faster but has limitations:
 	 * - All tables must exist
 	 * - No complex aggregations
+	 * - No related-row filters (`_related`, `_referenced`)
 	 * - Simpler query structure
 	 *
 	 * @param array $query Search query parameters.
@@ -1690,6 +1702,14 @@ private function shouldUseUnionQuery(array $query): bool {
 			return false;
 		}
 
+		// 🔴 `_related` and `_referenced` are EXISTS subqueries that only the
+		// QueryBuilder path (the sequential one) builds. The raw-SQL UNION builder
+		// refuses them, which on this route would end in an uncaught exception
+		// (HTTP 500), so a query carrying either takes the sequential path.
+		if (array_key_exists('_related', $query) === true || array_key_exists('_referenced', $query) === true) {
+			return false;
+		}
+
 		// UNION ALL is safe for simple searches.
 		return true;
 	}//end shouldUseUnionQuery()
@@ -7704,13 +7724,17 @@ static function (array $first, array $second): int {
 	 *
 	 * @return list<int> The folder file ids.
 	 *
+	 * @throws RuntimeException When a table could not be read: a sum over the tables that could be read would
+	 *                           be too low and look measured, so the whole measurement fails (and is logged).
+	 *
 	 * @spec openspec/changes/organisation-usage-measured/specs/organisation-usage-measured/spec.md
 	 */
 	public function findFolderIdsByOrganisation(string $organisationUuid): array {
 		$organisationCol = 't.' . self::METADATA_PREFIX . 'organisation';
 		$folderCol = 't.' . self::METADATA_PREFIX . 'folder';
 		$deletedCol = 't.' . self::METADATA_PREFIX . 'deleted';
 		$folderIds = [];
+		$failed    = [];
 
 		foreach (array_keys($this->discoverMagicTables()) as $fullTableName) {
 			$bareTableName = substr($fullTableName, strlen($this->getTablePrefix()));
@@ -7730,10 +7754,32 @@ public function findFolderIdsByOrganisation(string $organisationUuid): array {
 
 				$result->closeCursor();
 			} catch (\Exception $e) {
-				continue;
-			}
+				// 🔴 NEVER SKIPPED IN SILENCE. A table that fails (a missing
+				// _folder/_organisation column, a lock, a timeout) drops its folders
+				// from the sum, and the caller would show a measured-looking number
+				// that is too low. The service contract is "a figure that cannot be
+				// measured is null, never 0", so this fails after the scan, naming
+				// every table it could not read.
+				$this->logger->warning(
+					message: '[MagicMapper] findFolderIdsByOrganisation: could not read a table',
+					context: [
+						'file' => __FILE__,
+						'line' => __LINE__,
+						'table' => $fullTableName,
+						'organisation' => $organisationUuid,
+						'error' => $e->getMessage(),
+					]
+				);
+				$failed[] = $fullTableName;
+			}//end try
 		}//end foreach
 
+		if ($failed !== []) {
+			throw new RuntimeException(
+				'The folders of organisation ' . $organisationUuid . ' could not be read from: ' . implode(', ', $failed)
+			);
+		}
+
 		return array_keys($folderIds);
 	}//end findFolderIdsByOrganisation()
 

lib/Db/MagicMapper/MagicFacetHandler.php

@@ -433,6 +433,9 @@ function ($key) {
 	 * @param array $query The search query with filters and facet config.
 	 *
 	 * @return array Merged facet results across all tables.
+	 *
+	 * @throws \OCA\OpenRegister\Exception\RelatedRowFilterNotSupportedException When the query carries `_related` or `_referenced`.
+	 *
 	 * @spec openspec/specs/row-field-level-security/spec.md#requirement-a-property-read-rule-holds-on-every-route-that-returns-its-value
 	 */
 	public function getSimpleFacetsUnion(array $tableConfigs, array $query): array {
@@ -442,6 +445,11 @@ public function getSimpleFacetsUnion(array $tableConfigs, array $query): array {
 			return [];
 		}
 
+		// A `_related`/`_referenced` filter cannot be honoured in these raw-SQL
+		// arms. Refuse HERE, before the per-facet queries, whose own catch blocks
+		// would turn the refusal into empty buckets with only a log line.
+		$this->searchHandler?->refuseRelatedRowFilters(query: $query);
+
 		// Extract facet configuration.
 		$facetConfig = $query['_facets'] ?? [];
 		if (empty($facetConfig) === true) {

lib/Db/MagicMapper/MagicRbacHandler.php

@@ -1672,7 +1672,7 @@ private function deniesHere(
 	 * @spec openspec/changes/query-related-schema-rows/specs/zoeken-filteren/spec.md
 	 */
 	public function buildRbacPredicateForAlias(Schema $schema, string $alias, string $action = 'read', ?string $tableName = null): string {
-		if (preg_match('/^[A-Za-z_][A-Za-z0-9_]*$/', $alias) !== 1) {
+		if (preg_match('/^[A-Za-z_][A-Za-z0-9_]*\z/', $alias) !== 1) {
 			throw new InvalidArgumentException(
 				sprintf('\'%s\' is not a table alias an access predicate may be built for.', $alias)
 			);