🕵️ Add sharereview support by AndyScherzinger · Pull Request #2711 · nextcloud/tables

AndyScherzinger · 2026-06-07T12:59:48Z

🏁 Checklist

⛑️ Tests (unit and/or integration) are included or not needed
🔙 Backport requests are created or not needed: /backport to stableX.X
📅 Milestone is set
🌸 PR title is meaningful (if it should be in the changelog: is it meaningful to users?)

🤖 AI (if applicable)

The content of this PR was partly or fully generated using AI

Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

…ayer Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

… shares Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

…share review Use last_edit_at as the share time when it is more recent than created_at, falling back to created_at otherwise. This ensures that shares whose permissions were modified after creation show the more recent timestamp, allowing the ShareReview app to surface recently-changed shares accurately. Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

enjeck

To be sure, it's intentional to be able to delete shares from resources that you don't own/manage? And that usually-secret public share tokens are listed too?

blizzz · 2026-06-17T11:11:28Z

+	private const SHARE_TABLE = 'tables_shares';
+	private const TABLES_TABLE = 'tables_tables';
+	private const VIEWS_TABLE = 'tables_views';
+	private const CONTEXTS_TABLE = 'tables_contexts_context';


I do not like that database resource names leak from their actual place, which are the mappers.

Alternative: make it a public class constant in the respective mapper.

blizzz · 2026-06-17T11:16:15Z

+	/** @return list<array<string, mixed>> */
+	private function fetchAllShares(): array {
+		try {
+			$qb = $this->db->getQueryBuilder();
+			$qb->select(
+				'id', 'sender', 'receiver', 'receiver_type', 'node_id', 'node_type',
+				'token', 'password',
+				'permission_read', 'permission_create', 'permission_update',
+				'permission_delete', 'permission_manage',
+				'created_at', 'last_edit_at'
+			)->from(self::SHARE_TABLE)
+				->orderBy('id', 'ASC');
+			$result = $qb->executeQuery();
+			$rows = $result->fetchAll();
+			$result->closeCursor();
+			return $rows;
+		} catch (Exception $e) {
+			$this->logger->error('Tables ShareReview: failed to fetch shares: {message}', ['message' => $e->getMessage()]);
+			return [];
+		}
+	}


Leaks responsibility, should be provided as a method on (Service and) Mapper level.

A think to consider: can be implemented as Generator (we do it to seldomly) for a smaller memory footprint. Especially since this one reads all shares. Think of big instances. In worst case (not sure it is likely, but depends) this can ran into a memory exhaustion – but also on the callee side if really everything is consumed at once.

blizzz · 2026-06-17T11:22:28Z

+				$qb = $this->db->getQueryBuilder();
+				$qb->select('id', $nameColumn)
+					->from($table)
+					->where($qb->expr()->in('id', $qb->createNamedParameter($chunk, IQueryBuilder::PARAM_INT_ARRAY)));
+				$result = $qb->executeQuery();
+				$rows = $result->fetchAll();
+				$result->closeCursor();
+				foreach ($rows as $row) {
+					$map[(int)$row['id']] = (string)$row[$nameColumn];
+				}
+			} catch (Exception $e) {
+				$this->logger->error('Tables ShareReview: failed to fetch names from {table}: {message}', ['table' => $table, 'message' => $e->getMessage()]);
+			}


as mentioned, database queries should be done in the respectives mappers and exposed perhaps/ideally via Service.

blizzz · 2026-06-17T11:24:53Z

+
+		$formatted = [];
+		foreach ($rawShares as $share) {
+			$formatted[] = [


Ideally this is not a random associated array, but a specific class (can be a DTO/data containers without further logic). This is way more maintainable and also has a better memory footprint.

A factory class or callback could be provided by the ShareReview app when calling, then you have it more formalized/API-like as well.

blizzz · 2026-06-17T11:28:45Z

+			return $this->l10n->t('%s (View)', [$viewNames[$nodeId] ?? $this->l10n->t('View %s', [$nodeId])]);
+		}
+		if ($nodeType === self::NODE_TYPE_CONTEXT) {
+			return $this->l10n->t('%s (Context)', [$contextNames[$nodeId] ?? $this->l10n->t('Context %s', [$nodeId])]);


Application is the user facing term, Context is only the internal-technical name.

blizzz · 2026-06-17T11:35:58Z

+		try {
+			$this->contextNavigationMapper->deleteByShareId((int)$shareId);
+		} catch (Exception $e) {
+			$this->logger->error('Tables ShareReview: failed to clean up context navigation for share {id}: {message}', ['id' => $shareId, 'message' => $e->getMessage()]);


This will be more rule than exception, as we can expect this being called for Tables and Views way more often than for Contexts. One or more entries will only be against shared contexts.

Hence it would result in confusing, at worst also spammy, log messages.

AndyScherzinger · 2026-06-19T19:10:28Z

@enjeck regarding:

To be sure, it's intentional to be able to delete shares from resources that you don't own/manage? And that usually-secret public share tokens are listed too?

Yes BUT I will change the implementation in the way that it uses an event mechanism like in other places with similar calling patterns, to ensure the user would have that permission as checked then by the share review app. SO I reset the PR to draft, to reflect the need for further improvements.

AndyScherzinger added this to the v2.3.0 milestone Jun 7, 2026

AndyScherzinger added enhancement New feature or request 2. developing Work in progress AI assisted labels Jun 7, 2026

AndyScherzinger force-pushed the feat/noid/sharereview branch from a74331b to 411e1f7 Compare June 7, 2026 13:34

AndyScherzinger marked this pull request as ready for review June 7, 2026 18:35

AndyScherzinger requested review from blizzz and enjeck as code owners June 7, 2026 18:35

AndyScherzinger changed the title ~~Add sharereview support~~ 🕵️ Add sharereview support Jun 7, 2026

AndyScherzinger added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 7, 2026

AndyScherzinger force-pushed the feat/noid/sharereview branch 2 times, most recently from 569f563 to b35b188 Compare June 11, 2026 15:39

enjeck reviewed Jun 15, 2026

View reviewed changes

Comment thread lib/ShareReview/ShareReviewSource.php Outdated

Comment thread lib/ShareReview/ShareReviewSource.php

Comment thread lib/ShareReview/ShareReviewSource.php Outdated

Comment thread lib/ShareReview/ShareReviewSource.php Outdated

AndyScherzinger added 8 commits June 15, 2026 22:26

feat(sharereview): add listener registering Tables as a share source

140f813

Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

feat(sharereview): add ShareReviewSource with constructor and getName()

480bb71

Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

feat(sharereview): implement getShares() with batched name lookups

c01cbb9

Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

feat(sharereview): implement deleteShare() via direct SQL with logging

e525ba4

Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

feat(sharereview): register ShareReview listener on SourceEvent

48dcd0a

Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

style(sharereview): apply coding standards and Psalm fixes

c4624eb

Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

test(sharereview): add unit tests for ShareReviewSource

5dbba58

Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

fix(sharereview): harden and optimize implementation and testing

7d4ce52

Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

AndyScherzinger force-pushed the feat/noid/sharereview branch 4 times, most recently from 1f6933e to d53e2d8 Compare June 15, 2026 21:50

AndyScherzinger added 4 commits June 16, 2026 00:01

refactor(sharereview): replace raw SQL in deleteShare() with mapper l…

94ce853

…ayer Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

fix(sharereview): remove redundant app key from getShares() output

cbf2ce9

Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

fix(sharereview): localize user-facing strings in ShareReviewSource

c73d566

Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

fix(sharereview): log warnings for unknown node and receiver types of…

d2e8862

… shares Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

AndyScherzinger force-pushed the feat/noid/sharereview branch from d53e2d8 to d2e8862 Compare June 15, 2026 22:08

enjeck reviewed Jun 17, 2026

View reviewed changes

blizzz requested changes Jun 17, 2026

View reviewed changes

AndyScherzinger added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jun 19, 2026

AndyScherzinger marked this pull request as draft June 19, 2026 19:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🕵️ Add sharereview support#2711

🕵️ Add sharereview support#2711
AndyScherzinger wants to merge 13 commits into
mainfrom
feat/noid/sharereview

AndyScherzinger commented Jun 7, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

enjeck left a comment

Uh oh!

blizzz Jun 17, 2026

Uh oh!

blizzz Jun 17, 2026

Uh oh!

blizzz Jun 17, 2026

Uh oh!

blizzz Jun 17, 2026

Uh oh!

blizzz Jun 17, 2026

Uh oh!

blizzz Jun 17, 2026

Uh oh!

AndyScherzinger commented Jun 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

AndyScherzinger commented Jun 7, 2026

🏁 Checklist

🤖 AI (if applicable)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

enjeck left a comment

Choose a reason for hiding this comment

Uh oh!

blizzz Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

blizzz Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

blizzz Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

blizzz Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

blizzz Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

blizzz Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

AndyScherzinger commented Jun 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants