Unified Sharing Legacy Sync by provokateurin · Pull Request #65972 · nextcloud/server

provokateurin · 2026-10-27T15:35:06Z

There are some TODOs left in the code, but I just need to get this merged, so future changes can be done in smaller PRs.

The syncing logic is not nice to understand, because it's an encoding of all the quirks and limitations the legacy sharing system has. I tried my best to leave useful comments whenever possible, but I'm happy to add more if there are questions during the review.

icewind1991 · 2026-10-27T16:43:59Z

+
+		$sourceNodeTargetTable = $schema->createTable('sharing_source_node_target');
+		$sourceNodeTargetTable->addColumn('user_id', Types::TEXT, ['length' => 64]);
+		$sourceNodeTargetTable->addColumn('source_instance', Types::STRING, ['length' => 128, 'notnull' => false]);


this and any code using it will need to be adjusted once #65512 is in

icewind1991 · 2026-10-27T16:47:20Z

+
+	#[Override]
+	public function handle(Event $event): void {
+		foreach ($event->shareIds as $shareId) {


can we send the share objects with the event to prevent having to re-fetch them here.

The problem is that in the place where the event is emitted the share is from the perspective of the user. At least in the sync part we need to fetch the full share. I only sent the IDs, to avoid this kind of confusion, because listeners would likely assume that they see the full share.

So I would like to keep it like this to make sure the event is not used in the wrong way. Alternatively the shares could be re-fetched before the event is emitted, so the case of multiple listeners would be optimized, because they don't need to fetch the shares each.

icewind1991 · 2026-10-27T17:13:27Z

+	/**
+	 * Get user ids for a recipient value.
+	 *
+	 * @return list<string>


I would prefer list<IUser>

icewind1991 · 2026-10-27T17:25:24Z

+
+		$legacyMappingTable = $schema->createTable('sharing_share_legacy_mapping');
+		$legacyMappingTable->addColumn('id', Types::BIGINT);
+		$legacyMappingTable->addColumn('legacy_provider', Types::STRING, ['length' => 128]);


Maybe use class names and the class-mapper so we can store this as ints?

icewind1991 · 2026-10-27T17:26:17Z

+		$legacyMappingTable->addColumn('recipient_secrets', Types::TEXT);
+		$legacyMappingTable->addColumn('recipient_value', Types::STRING, ['length' => 255]);
+		$legacyMappingTable->setPrimaryKey(['legacy_provider', 'legacy_id']);
+		$legacyMappingTable->addIndex(['id']);


is id + recipient_value unique? if so, we should probably have an index for it

Unfortunately it's not.

icewind1991 · 2026-10-27T17:45:14Z

+	 * @experimental 35.0.0
+	 */
+	public function getCreatedAt(): DateTimeImmutable {
+		return $this->createdAt ??= Server::get(ISnowflakeDecoder::class)->decode($this->id)->getCreatedAt();


inject the decoder in the constructor instead?

I think it's better to only lookup the class once when needed and not every time a Share object is instantiated. The result is cached anyway, so the class won't be looked up twice if the method is called twice.

Share objects are manually constructed by the backend right, not automatically constructed. So the backend only needs to lookup the decoder once

icewind1991 · 2026-10-27T17:51:18Z

+	}
+
+	// TODO: Test
+	public function mapLegacyShares(?IUser $user, ?int $limit = null): void {


mapUnmappedLegacyShares to make it more clear that it doesn't update existing mappings

I'll remove that code for now, because it's neither used nor tested yet.

icewind1991 · 2026-10-27T17:55:03Z

+			$legacyMapping = $this->legacyMapper->createLegacyMapping(
+				$id,
+				$provider,
+				$row['id'],
+				new DateTimeImmutable(),
+				[$this->getSharingManager()->generateSecret()],
+				$this->getSharingManager()->generateSecret(),
+			);
+
+			$this->syncLegacySharesToShare($id, [$legacyMapping]);


these two should be in a transaction to prevent an error halfway from causing a desync

icewind1991 · 2026-10-27T18:14:43Z

+		if ($event instanceof SharesUpdatedEvent) {
+			foreach ($event->shareIds as $id) {
+				$this->wrapSync($id, function () use ($id): void {
+					$share = $this->getSharingManager()->getShare(new ShareAccessContext(overrideChecks: true), $id);


another place where having the share in the event would save work

icewind1991 · 2026-10-27T18:41:12Z

+					new ShareUser(
+						$shareInitiator['value'],
+						$shareInitiator['instance'],
+					),


minor nitpick: creating the ShareUser can be de-duplicated

icewind1991 · 2026-10-27T19:11:48Z

+				$this->ignoreNewLegacyShare = false;
+			} catch (Exception $exception) {
+				$this->ignoreNewLegacyShare = false;
+				throw $exception;
+			}


} finally { $this->ignoreNewLegacyShare = false; }

… the controller Signed-off-by: provokateurin <kate@provokateurin.de>

Signed-off-by: provokateurin <kate@provokateurin.de>

… a hash already Signed-off-by: provokateurin <kate@provokateurin.de>

…ead of load Signed-off-by: provokateurin <kate@provokateurin.de>

Signed-off-by: provokateurin <kate@provokateurin.de>

…ailable Signed-off-by: provokateurin <kate@provokateurin.de>

…licit recipients Signed-off-by: provokateurin <kate@provokateurin.de>

Signed-off-by: provokateurin <kate@provokateurin.de>

provokateurin added this to the Nextcloud 36 milestone Oct 27, 2026

provokateurin requested review from icewind1991 and susnux October 27, 2026 15:35

provokateurin added enhancement 2. developing Work in progress labels Oct 27, 2026

provokateurin self-assigned this Oct 27, 2026