From 4ff209cb7b5b6763c3cecd4516b5675aaa3cf10d Mon Sep 17 00:00:00 2001 From: Jiajun Xu Date: Thu, 5 Nov 2026 10:31:29 +0800 Subject: [PATCH 1/5] feat(rows): store immutable UUIDv7 identities on row sleeves Add a nullable, uniquely indexed uuid column to tables_row_sleeves. Every sleeve insertion, including legacy transfers with explicit IDs, assigns a UUIDv7 before writing. An assigned identity is normalized to lowercase and cannot change or be cleared. Stored values load unchanged, so rows created before this release keep NULL until they are backfilled. User migration archives now carry each row's UUID. Import preserves an unused UUID, generates one when the archive has none, and creates the row with a fresh UUID when the identity is already taken. A unique violation is retried only after a plain read confirms the UUID collision, so other constraint failures surface unchanged. Invalid archived identities fail before anything is written. Refs: #3093 Assisted-by: Codex:gpt-6 Assisted-by: Codex:gpt-6-astra Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 Signed-off-by: Jiajun Xu --- lib/Db/Row2Mapper.php | 4 + lib/Db/RowSleeve.php | 24 +++++ lib/Db/RowSleeveMapper.php | 56 +++++++++++ .../Version2500Date20261103000000.php | 35 +++++++ lib/Service/RowService.php | 38 +++---- tests/unit/Database/DatabaseTestCase.php | 2 + tests/unit/Db/RowSleeveMapperTest.php | 98 +++++++++++++++++++ tests/unit/Db/RowSleeveTest.php | 74 ++++++++++++++ tests/unit/Service/RowServiceUuidTest.php | 75 ++++++++++++++ 9 files changed, 388 insertions(+), 18 deletions(-) create mode 100644 lib/Migration/Version2500Date20261103000000.php create mode 100644 tests/unit/Db/RowSleeveMapperTest.php create mode 100644 tests/unit/Db/RowSleeveTest.php create mode 100644 tests/unit/Service/RowServiceUuidTest.php diff --git a/lib/Db/Row2Mapper.php b/lib/Db/Row2Mapper.php index aeb7c59c8c..89248ffd34 100644 --- a/lib/Db/Row2Mapper.php +++ b/lib/Db/Row2Mapper.php @@ -43,6 +43,10 @@ public function __construct( ) { } + public function importSleeve(RowSleeve $sleeve): RowSleeve { + return $this->rowSleeveMapper->insertImported($sleeve); + } + /** * @param Row2 $row * @return Row2 diff --git a/lib/Db/RowSleeve.php b/lib/Db/RowSleeve.php index b909a3f44f..82f8b72a37 100644 --- a/lib/Db/RowSleeve.php +++ b/lib/Db/RowSleeve.php @@ -8,10 +8,13 @@ namespace OCA\Tables\Db; use JsonSerializable; +use OCA\Tables\Vendor\Symfony\Component\Uid\Uuid; +use OCA\Tables\Vendor\Symfony\Component\Uid\UuidV7; use OCP\AppFramework\Db\Entity; /** * @psalm-suppress PropertyNotSetInConstructor + * @method getUuid(): ?string * @method getTableId(): ?int * @method setTableId(int $columnId) * @method getCreatedBy(): string @@ -25,6 +28,7 @@ */ class RowSleeve extends Entity implements JsonSerializable { protected ?int $tableId = null; + protected ?string $uuid = null; protected ?string $createdBy = null; protected ?string $createdAt = null; protected ?string $lastEditBy = null; @@ -33,11 +37,31 @@ class RowSleeve extends Entity implements JsonSerializable { public function __construct() { $this->addType('id', 'integer'); $this->addType('tableId', 'integer'); + $this->addType('uuid', 'string'); + } + + /** + * Hydration uses Entity::setter() instead, so stored values load unchanged. + */ + public function setUuid(?string $uuid): void { + $uuid = $uuid !== null ? (new UuidV7($uuid))->toRfc4122() : null; + if ($this->uuid !== null && $uuid !== $this->uuid) { + throw new \InvalidArgumentException('Row UUIDs are immutable.'); + } + $this->uuid = $uuid; + $this->markFieldUpdated('uuid'); + } + + public function assignUuid(): void { + if ($this->uuid === null) { + $this->setUuid(Uuid::v7()->toRfc4122()); + } } public function jsonSerialize(): array { return [ 'id' => $this->id, + 'uuid' => $this->uuid, 'tableId' => $this->tableId, 'createdBy' => $this->createdBy, 'createdAt' => $this->createdAt, diff --git a/lib/Db/RowSleeveMapper.php b/lib/Db/RowSleeveMapper.php index b627424ba5..a0b64b1c07 100644 --- a/lib/Db/RowSleeveMapper.php +++ b/lib/Db/RowSleeveMapper.php @@ -7,7 +7,9 @@ namespace OCA\Tables\Db; +use OCA\Tables\Vendor\Symfony\Component\Uid\Uuid; use OCP\AppFramework\Db\DoesNotExistException; +use OCP\AppFramework\Db\Entity; use OCP\AppFramework\Db\MultipleObjectsReturnedException; use OCP\AppFramework\Db\QBMapper; use OCP\DB\Exception; @@ -28,6 +30,15 @@ public function __construct( parent::__construct($db, $this->table, RowSleeve::class); } + /** + * @param RowSleeve $entity + * @return RowSleeve + */ + public function insert(Entity $entity): Entity { + $entity->assignUuid(); + return parent::insert($entity); + } + /** * @throws MultipleObjectsReturnedException * @throws DoesNotExistException @@ -41,6 +52,50 @@ public function find(int $id): RowSleeve { return $this->findEntity($qb); } + public function findByUuid(string $uuid): RowSleeve { + $qb = $this->db->getQueryBuilder(); + $qb->select('*')->from($this->table) + ->where($qb->expr()->eq('uuid', $qb->createNamedParameter($uuid))); + return $this->findEntity($qb); + } + + /** + * Import creates a copy when the archived identity is already in use. + */ + public function insertImported(RowSleeve $sleeve): RowSleeve { + $tableId = $sleeve->getTableId(); + if ($tableId === null) { + throw new \InvalidArgumentException('An imported row requires a destination table.'); + } + $uuid = $sleeve->getUuid() ?? Uuid::v7()->toRfc4122(); + for ($attempt = 0; ; $attempt++) { + $candidate = new RowSleeve(); + $candidate->setTableId($tableId); + $candidate->setCreatedBy($sleeve->getCreatedBy()); + $candidate->setCreatedAt($sleeve->getCreatedAt()); + $candidate->setLastEditBy($sleeve->getLastEditBy()); + $candidate->setLastEditAt($sleeve->getLastEditAt()); + $candidate->setUuid($uuid); + $this->db->beginTransaction(); + try { + $this->insert($candidate); + $this->db->commit(); + return $candidate; + } catch (\Throwable $e) { + $this->db->rollBack(); + if (!$e instanceof Exception || $e->getReason() !== Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION || $attempt >= 3) { + throw $e; + } + try { + $this->findByUuid($uuid); + } catch (DoesNotExistException) { + throw $e; + } + $uuid = Uuid::v7()->toRfc4122(); + } + } + } + /** * @param int[] $ids * @return RowSleeve[] @@ -51,6 +106,7 @@ public function findMultiple(array $ids): array { $qb = $this->db->getQueryBuilder(); $qb->select( $sleeveAlias . '.id', + $sleeveAlias . '.uuid', $sleeveAlias . '.table_id', $sleeveAlias . '.created_by', $sleeveAlias . '.created_at', diff --git a/lib/Migration/Version2500Date20261103000000.php b/lib/Migration/Version2500Date20261103000000.php new file mode 100644 index 0000000000..494af51762 --- /dev/null +++ b/lib/Migration/Version2500Date20261103000000.php @@ -0,0 +1,35 @@ +getTable('tables_row_sleeves'); + if (!$table->hasColumn('uuid')) { + $table->addColumn('uuid', Types::STRING, [ + 'notnull' => false, + 'default' => null, + 'length' => 36, + ]); + } + if (!$table->hasIndex('tables_row_uuid_uniq')) { + $table->addUniqueIndex(['uuid'], 'tables_row_uuid_uniq'); + } + return $schema; + } +} diff --git a/lib/Service/RowService.php b/lib/Service/RowService.php index 3e98e7d4a2..b7cabdd7aa 100644 --- a/lib/Service/RowService.php +++ b/lib/Service/RowService.php @@ -12,6 +12,7 @@ use OCA\Tables\Db\ColumnMapper; use OCA\Tables\Db\Row2; use OCA\Tables\Db\Row2Mapper; +use OCA\Tables\Db\RowSleeve; use OCA\Tables\Db\Table; use OCA\Tables\Db\TableMapper; use OCA\Tables\Db\View; @@ -32,9 +33,7 @@ use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\MultipleObjectsReturnedException; use OCP\DB\Exception; -use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\EventDispatcher\IEventDispatcher; -use OCP\IDBConnection; use Psr\Container\ContainerExceptionInterface; use Psr\Container\NotFoundExceptionInterface; use Psr\Log\LoggerInterface; @@ -58,7 +57,6 @@ public function __construct( private readonly ColumnsHelper $columnsHelper, private readonly ActivityManager $activityManager, private readonly NotificationHelper $notificationHelper, - private readonly IDBConnection $connection, ) { parent::__construct($logger, $userId, $permissionsService); @@ -1003,23 +1001,27 @@ private function buildAliasByColumnId(array $columns): array { * @throws InternalError */ public function importRow(Table $table, array $row): int { - $qb = $this->connection->getQueryBuilder(); + $tableId = $table->getId(); + $owner = $table->getOwnership(); + if ($tableId === null || $owner === null) { + throw new \InvalidArgumentException('An imported row requires a stored destination table and owner.'); + } + + $sleeve = new RowSleeve(); + if (isset($row['uuid']) && !is_string($row['uuid'])) { + throw new \InvalidArgumentException('Archived row UUID must be a string or null.'); + } + $sleeve->setUuid($row['uuid'] ?? null); + $sleeve->setTableId($tableId); + $sleeve->setCreatedBy($owner); + $sleeve->setCreatedAt($row['createdAt']); + $sleeve->setLastEditBy($owner); + $sleeve->setLastEditAt($row['lastEditAt']); try { - $qb->insert('tables_row_sleeves') - ->values([ - 'table_id' => $qb->createNamedParameter($table->getId(), IQueryBuilder::PARAM_INT), - 'created_by' => $qb->createNamedParameter($table->getOwnership()), - 'created_at' => $qb->createNamedParameter($row['createdAt']), - 'last_edit_by' => $qb->createNamedParameter($table->getOwnership()), - 'last_edit_at' => $qb->createNamedParameter($row['lastEditAt']), - ]); - $qb->executeStatement(); - $id = $this->connection->lastInsertId('*PREFIX*tables_row_sleeves'); - - return $id; - } catch (\Exception $e) { + return $this->row2Mapper->importSleeve($sleeve)->getId(); + } catch (Exception $e) { $this->logger->error('userMigrationImport insert error: ' . $e->getMessage()); - throw new InternalError('userMigrationImport insert error: ' . $e->getMessage()); + throw new InternalError('userMigrationImport insert error: ' . $e->getMessage(), 0, $e); } } diff --git a/tests/unit/Database/DatabaseTestCase.php b/tests/unit/Database/DatabaseTestCase.php index 4519504a84..ff09e4bff3 100644 --- a/tests/unit/Database/DatabaseTestCase.php +++ b/tests/unit/Database/DatabaseTestCase.php @@ -15,6 +15,7 @@ use OC\DB\MigrationService; use OC\Migration\NullOutput; use OC\SystemConfig; +use OCA\Tables\Vendor\Symfony\Component\Uid\Uuid; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\Server; use PHPUnit\Framework\TestCase; @@ -234,6 +235,7 @@ protected function createTestColumn(int $tableId, array $data = []) { protected function createTestRow(int $tableId, array $data = []) { $defaultData = [ 'table_id' => $tableId, + 'uuid' => Uuid::v7()->toRfc4122(), 'created_by' => 'user1', 'created_at' => date('Y-m-d H:i:s'), 'last_edit_by' => 'user1', diff --git a/tests/unit/Db/RowSleeveMapperTest.php b/tests/unit/Db/RowSleeveMapperTest.php new file mode 100644 index 0000000000..b035e3c4cd --- /dev/null +++ b/tests/unit/Db/RowSleeveMapperTest.php @@ -0,0 +1,98 @@ +mapper = new RowSleeveMapper($this->connectionAdapter, new NullLogger()); + $this->connectionAdapter->beginTransaction(); + } + + protected function tearDown(): void { + $this->connectionAdapter->rollBack(); + parent::tearDown(); + } + + private function sleeve(int $tableId = 1): RowSleeve { + $sleeve = new RowSleeve(); + $sleeve->setTableId($tableId); + $sleeve->setCreatedBy('alice'); + $sleeve->setLastEditBy('alice'); + $sleeve->setCreatedAt('2020-01-01 00:00:00'); + $sleeve->setLastEditAt('2020-01-02 00:00:00'); + return $sleeve; + } + + public function testEveryInsertionGetsStoredUuidAndProjectionIncludesIt(): void { + $first = $this->mapper->insert($this->sleeve()); + $second = $this->mapper->insert($this->sleeve(2)); + $this->assertNotNull($first->getUuid()); + $this->assertNotSame($first->getUuid(), $second->getUuid()); + $this->assertSame($first->getUuid(), $this->mapper->find($first->getId())->getUuid()); + $this->assertSame($first->getUuid(), $this->mapper->findMultiple([$first->getId()])[0]->getUuid()); + $this->assertSame($first->getId(), $this->mapper->findByUuid($first->getUuid())->getId()); + } + + public function testExplicitLegacyIdAlsoGetsUuid(): void { + $sleeve = $this->sleeve(); + $sleeve->setId(900001); + $this->mapper->insert($sleeve); + $this->assertNotNull($this->mapper->find(900001)->getUuid()); + } + + public function testImportPreservesUnusedIdentityAndCopiesCollisionsAcrossTables(): void { + $sleeve = $this->sleeve(); + $uuid = '019f90c5-8576-7f0b-9b14-9bf49b2826cf'; + $sleeve->setUuid($uuid); + $first = $this->mapper->insertImported($sleeve); + $this->assertSame($uuid, $first->getUuid()); + $sleeve->setTableId(2); + $copy = $this->mapper->insertImported($sleeve); + $this->assertNotSame($uuid, $copy->getUuid()); + $this->assertTrue($this->connectionAdapter->inTransaction()); + $this->assertSame($uuid, $this->mapper->find($first->getId())->getUuid()); + $this->assertSame('2020-01-01 00:00:00', $copy->getCreatedAt()); + $this->assertSame($copy->getUuid(), $this->mapper->find($copy->getId())->getUuid()); + } + + public function testUniqueIndexAppliesAcrossTables(): void { + $first = $this->mapper->insert($this->sleeve()); + $second = $this->sleeve(2); + $second->setUuid($first->getUuid()); + $this->expectException(Exception::class); + $this->mapper->insert($second); + } + + public function testMultipleHistoricalNullsRemainReadable(): void { + $insert = $this->connectionAdapter->getQueryBuilder(); + $insert->insert('tables_row_sleeves')->values([ + 'table_id' => $insert->createNamedParameter(1), + 'created_by' => $insert->createNamedParameter('alice'), + 'created_at' => $insert->createNamedParameter('2020-01-01 00:00:00'), + 'last_edit_by' => $insert->createNamedParameter('alice'), + 'last_edit_at' => $insert->createNamedParameter('2020-01-01 00:00:00'), + ]); + $insert->executeStatement(); + $first = $insert->getLastInsertId(); + $insert->executeStatement(); + $second = $insert->getLastInsertId(); + $this->assertNull($this->mapper->find($first)->getUuid()); + $this->assertNull($this->mapper->find($second)->getUuid()); + } +} diff --git a/tests/unit/Db/RowSleeveTest.php b/tests/unit/Db/RowSleeveTest.php new file mode 100644 index 0000000000..55ff23956c --- /dev/null +++ b/tests/unit/Db/RowSleeveTest.php @@ -0,0 +1,74 @@ + 17, 'uuid' => null]); + $this->assertNull($sleeve->getUuid()); + $this->assertNull($sleeve->jsonSerialize()['uuid']); + $this->assertSame([], $sleeve->getUpdatedFields()); + } + + public function testHydrationPreservesAssignedUuid(): void { + $sleeve = RowSleeve::fromRow(['id' => 17, 'uuid' => self::UUID]); + $this->assertSame(self::UUID, $sleeve->getUuid()); + $this->assertSame([], $sleeve->getUpdatedFields()); + } + + public function testHydrationDoesNotRejectStoredValues(): void { + $sleeve = RowSleeve::fromRow(['id' => 17, 'uuid' => 'not-a-uuid']); + $this->assertSame('not-a-uuid', $sleeve->getUuid()); + $this->assertSame([], $sleeve->getUpdatedFields()); + } + + public function testAssignmentIsExplicitAndIdempotent(): void { + $sleeve = new RowSleeve(); + $this->assertNull($sleeve->getUuid()); + $sleeve->assignUuid(); + $uuid = $sleeve->getUuid(); + $this->assertMatchesRegularExpression('/^[0-9a-f]{8}-[0-9a-f]{4}-7[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/D', $uuid); + $this->assertArrayHasKey('uuid', $sleeve->getUpdatedFields()); + $sleeve->assignUuid(); + $this->assertSame($uuid, $sleeve->jsonSerialize()['uuid']); + } + + public function testArchiveUuidIsNormalized(): void { + $sleeve = new RowSleeve(); + $sleeve->setUuid(strtoupper(self::UUID)); + $this->assertSame(self::UUID, $sleeve->getUuid()); + } + + /** @dataProvider immutableChanges */ + public function testAssignedIdentityCannotChange(?string $uuid): void { + $sleeve = RowSleeve::fromRow(['uuid' => self::UUID]); + $this->expectException(\InvalidArgumentException::class); + $sleeve->setUuid($uuid); + } + + public static function immutableChanges(): array { + return [[null], ['019f90c5-8576-7f0b-9b14-9bf49b2826c0']]; + } + + /** @dataProvider invalidUuids */ + public function testInvalidUuidFails(string $uuid): void { + $this->expectException(\InvalidArgumentException::class); + (new RowSleeve())->setUuid($uuid); + } + + public static function invalidUuids(): array { + return [[''], ['invalid'], ['019f90c5-8576-4f0b-9b14-9bf49b2826cf'], ['019f90c5-8576-7f0b-0b14-9bf49b2826cf']]; + } +} diff --git a/tests/unit/Service/RowServiceUuidTest.php b/tests/unit/Service/RowServiceUuidTest.php new file mode 100644 index 0000000000..5ca56c01a3 --- /dev/null +++ b/tests/unit/Service/RowServiceUuidTest.php @@ -0,0 +1,75 @@ +rows = $this->createMock(Row2Mapper::class); + $this->service = new RowService(new NullLogger(), 'alice', $this->createMock(PermissionsService::class), $this->createMock(ColumnMapper::class), + $this->createMock(ViewMapper::class), $this->createMock(TableMapper::class), $this->rows, $this->createMock(IEventDispatcher::class), + $this->createMock(ColumnsHelper::class), $this->createMock(ActivityManager::class), $this->createMock(NotificationHelper::class)); + } + + /** @dataProvider archiveIdentities */ + public function testArchiveIdentityGoesThroughSleeveInsertionBoundary(mixed $uuid, ?string $expected, bool $includeUuid = true): void { + $table = new Table(); + $table->setId(5); + $table->setOwnership('alice'); + $this->rows->expects($this->once())->method('importSleeve')->willReturnCallback(function (RowSleeve $sleeve) use ($expected): RowSleeve { + $this->assertSame($expected, $sleeve->getUuid()); + $this->assertSame(5, $sleeve->getTableId()); + $this->assertSame('alice', $sleeve->getCreatedBy()); + $sleeve->setId(99); + $sleeve->assignUuid(); + return $sleeve; + }); + $archive = ['createdAt' => '2020-01-01 00:00:00', 'lastEditAt' => '2020-01-02 00:00:00']; + if ($includeUuid) { + $archive['uuid'] = $uuid; + } + $this->assertSame(99, $this->service->importRow($table, $archive)); + } + + public static function archiveIdentities(): array { + return [[null, null, false], [null, null], ['019F90C5-8576-7F0B-9B14-9BF49B2826CF', '019f90c5-8576-7f0b-9b14-9bf49b2826cf']]; + } + + /** @dataProvider invalidArchiveIdentities */ + public function testInvalidArchiveIdentityFailsBeforeWriting(mixed $uuid): void { + $this->rows->expects($this->never())->method('importSleeve'); + $this->expectException(\InvalidArgumentException::class); + $table = new Table(); + $table->setId(5); + $table->setOwnership('alice'); + $this->service->importRow($table, ['uuid' => $uuid]); + } + + public static function invalidArchiveIdentities(): array { + return [[3], [[]], [''], ['019f90c5-8576-4f0b-9b14-9bf49b2826cf']]; + } +} From 6167879b33295ca26c2f06146084b5ecce2e62b3 Mon Sep 17 00:00:00 2001 From: Jiajun Xu Date: Thu, 5 Nov 2026 10:31:29 +0800 Subject: [PATCH 2/5] feat(rows): backfill UUIDs for existing rows in background batches Register a periodic job from the migration that assigns UUIDv7 values to sleeves still holding NULL. A run processes batches of up to 500 rows for about five seconds and commits each batch. The update only touches rows whose uuid is still NULL, so concurrent assignments and deletions are respected and audit metadata stays unchanged. Parallel runs are disabled. Progress is a lazy app-config cursor written once per run. A lagging cursor only repeats a cheap NULL scan, while writing it after every batch would invalidate the shared app-config cache. When a pass finds nothing left, the job rescans from the start for rows below the cursor before it removes itself. On SQLite the job pauses briefly between batches so requests can take the single writer. Refs: #3093 Assisted-by: Codex:gpt-6 Assisted-by: Codex:gpt-6-astra Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 Signed-off-by: Jiajun Xu --- lib/BackgroundJob/BackfillRowUuids.php | 108 +++++++++ .../Version2500Date20261103000000.php | 11 + .../BackgroundJob/BackfillRowUuidsTest.php | 221 ++++++++++++++++++ 3 files changed, 340 insertions(+) create mode 100644 lib/BackgroundJob/BackfillRowUuids.php create mode 100644 tests/unit/BackgroundJob/BackfillRowUuidsTest.php diff --git a/lib/BackgroundJob/BackfillRowUuids.php b/lib/BackgroundJob/BackfillRowUuids.php new file mode 100644 index 0000000000..243a29aa3e --- /dev/null +++ b/lib/BackgroundJob/BackfillRowUuids.php @@ -0,0 +1,108 @@ +setInterval(60); + $this->setAllowParallelRuns(false); + } + + public function run(mixed $argument): void { + $deadline = $this->time->getTime() + self::TIME_BUDGET; + $cursor = $this->config->getValueInt(Application::APP_ID, self::CURSOR_KEY, 0, true); + $select = $this->connection->getQueryBuilder(); + $select->select('id')->from('tables_row_sleeves') + ->where($select->expr()->isNull('uuid')) + ->andWhere($select->expr()->gt('id', $select->createParameter('cursor'))) + ->orderBy('id', 'ASC')->setMaxResults(self::BATCH_SIZE); + $update = $this->connection->getQueryBuilder(); + $update->update('tables_row_sleeves')->set('uuid', $update->createParameter('uuid')) + ->where($update->expr()->eq('id', $update->createParameter('id'))) + ->andWhere($update->expr()->isNull('uuid')); + + try { + do { + $select->setParameter('cursor', $cursor, IQueryBuilder::PARAM_INT); + $result = $select->executeQuery(); + $ids = array_map('intval', $result->fetchFirstColumn()); + $result->closeCursor(); + if ($ids === []) { + if ($cursor === 0) { + $this->jobList->remove(self::class); + $this->config->deleteKey(Application::APP_ID, self::CURSOR_KEY); + return; + } + $cursor = 0; + continue; + } + + $cursor = $this->processBatch($ids, $update, $deadline); + if ($this->connection->getDatabaseProvider() === IDBConnection::PLATFORM_SQLITE && $this->time->getTime() < $deadline) { + // Yield SQLite's single writer between committed batches. + usleep(self::SQLITE_BATCH_PAUSE_MICROSECONDS); + } + } while ($this->time->getTime() < $deadline); + // A lagging cursor only repeats a NULL scan, so one write per run spares the shared config cache. + $this->config->setValueInt(Application::APP_ID, self::CURSOR_KEY, $cursor, true); + } catch (\Throwable $e) { + $this->logger->error('Row UUID backfill will retry on its next run.', ['exception' => $e]); + } + } + + /** @param non-empty-list $ids */ + private function processBatch(array $ids, IQueryBuilder $update, int $deadline): int { + for ($attempt = 0; ; $attempt++) { + $this->connection->beginTransaction(); + try { + foreach ($ids as $id) { + $update->setParameter('id', $id, IQueryBuilder::PARAM_INT); + $update->setParameter('uuid', Uuid::v7()->toRfc4122()); + $update->executeStatement(); + if ($this->time->getTime() >= $deadline) { + break; + } + } + $this->connection->commit(); + return $id; + } catch (\Throwable $e) { + $this->connection->rollBack(); + // Only uuid changes in this statement, so any unique violation is a UUID collision. + if (!$e instanceof Exception || $e->getReason() !== Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION || $attempt >= 3) { + throw $e; + } + } + } + } +} diff --git a/lib/Migration/Version2500Date20261103000000.php b/lib/Migration/Version2500Date20261103000000.php index 494af51762..e51f96f3ff 100644 --- a/lib/Migration/Version2500Date20261103000000.php +++ b/lib/Migration/Version2500Date20261103000000.php @@ -10,12 +10,19 @@ namespace OCA\Tables\Migration; use Closure; +use OCA\Tables\BackgroundJob\BackfillRowUuids; +use OCP\BackgroundJob\IJobList; use OCP\DB\ISchemaWrapper; use OCP\DB\Types; use OCP\Migration\IOutput; use OCP\Migration\SimpleMigrationStep; class Version2500Date20261103000000 extends SimpleMigrationStep { + public function __construct( + private readonly IJobList $jobList, + ) { + } + public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper { /** @var ISchemaWrapper $schema */ $schema = $schemaClosure(); @@ -32,4 +39,8 @@ public function changeSchema(IOutput $output, Closure $schemaClosure, array $opt } return $schema; } + + public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options): void { + $this->jobList->add(BackfillRowUuids::class); + } } diff --git a/tests/unit/BackgroundJob/BackfillRowUuidsTest.php b/tests/unit/BackgroundJob/BackfillRowUuidsTest.php new file mode 100644 index 0000000000..0310b5f2d7 --- /dev/null +++ b/tests/unit/BackgroundJob/BackfillRowUuidsTest.php @@ -0,0 +1,221 @@ +connectionAdapter->beginTransaction(); + $delete = $this->connectionAdapter->getQueryBuilder(); + $delete->delete('tables_row_sleeves')->where($delete->expr()->gt('id', $delete->createNamedParameter(0)))->executeStatement(); + $this->settings = []; + $this->config = $this->createMock(IAppConfig::class); + $this->config->method('getValueInt')->willReturnCallback(fn ($app, $key, $default) => $this->settings[$key] ?? $default); + $this->config->method('setValueInt')->willReturnCallback(function ($app, $key, $value): bool { + $this->settings[$key] = $value; + return true; + }); + $this->config->method('deleteKey')->willReturnCallback(function ($app, $key): void { + unset($this->settings[$key]); + }); + $this->jobList = $this->createMock(IJobList::class); + $this->mapper = new RowSleeveMapper($this->connectionAdapter, new NullLogger()); + } + + protected function tearDown(): void { + $this->connectionAdapter->rollBack(); + parent::tearDown(); + } + + private function historicalRows(int $count): array { + $insert = $this->connectionAdapter->getQueryBuilder(); + $insert->insert('tables_row_sleeves')->values([ + 'table_id' => $insert->createNamedParameter(991), + 'created_by' => $insert->createNamedParameter('original-author'), + 'created_at' => $insert->createNamedParameter('2020-01-01 00:00:00'), + 'last_edit_by' => $insert->createNamedParameter('original-editor'), + 'last_edit_at' => $insert->createNamedParameter('2020-01-02 00:00:00'), + ]); + $ids = []; + for ($i = 0; $i < $count; $i++) { + $insert->executeStatement(); + $ids[] = $insert->getLastInsertId(); + } + return $ids; + } + + private function job(ITimeFactory $clock, ?LoggerInterface $logger = null): BackfillRowUuids { + return new BackfillRowUuids($clock, $this->connectionAdapter, $this->config, $this->jobList, $logger ?? new NullLogger()); + } + + public function testTimeBudgetCheckpointsProcessedPrefixAndResumePreservesMetadata(): void { + $ids = $this->historicalRows(3); + $clock = $this->createMock(ITimeFactory::class); + $clock->method('getTime')->willReturnOnConsecutiveCalls(0, 5, 5, 5); + $this->jobList->expects($this->once())->method('remove')->with(BackfillRowUuids::class); + $this->job($clock)->run(null); + $this->assertSame($ids[0], $this->settings['row_uuid_backfill_cursor']); + $uuid = $this->mapper->find($ids[0])->getUuid(); + $this->assertNotNull($uuid); + $this->assertNull($this->mapper->find($ids[1])->getUuid()); + + $clock = $this->createMock(ITimeFactory::class); + $clock->method('getTime')->willReturn(10); + $this->job($clock)->run(null); + $this->assertSame($uuid, $this->mapper->find($ids[0])->getUuid()); + foreach ($ids as $id) { + $row = $this->mapper->find($id); + $this->assertNotNull($row->getUuid()); + $this->assertSame('original-author', $row->getCreatedBy()); + $this->assertSame('original-editor', $row->getLastEditBy()); + $this->assertSame('2020-01-01 00:00:00', $row->getCreatedAt()); + $this->assertSame('2020-01-02 00:00:00', $row->getLastEditAt()); + } + $this->assertSame([], $this->settings); + } + + public function testFinalPassRevisitsNullsBelowCursor(): void { + $ids = $this->historicalRows(2); + $this->settings['row_uuid_backfill_cursor'] = $ids[1] + 100; + $clock = $this->createMock(ITimeFactory::class); + $clock->method('getTime')->willReturn(0); + $this->jobList->expects($this->once())->method('remove'); + $this->job($clock)->run(null); + $this->assertNotNull($this->mapper->find($ids[0])->getUuid()); + $this->assertNotNull($this->mapper->find($ids[1])->getUuid()); + } + + public function testCommittedWorkSurvivesCheckpointFailure(): void { + $ids = $this->historicalRows(1); + $config = $this->createMock(IAppConfig::class); + $config->method('getValueInt')->willReturn(0); + $config->method('setValueInt')->willThrowException(new \RuntimeException('Checkpoint unavailable')); + $logger = $this->createMock(LoggerInterface::class); + $logger->expects($this->once())->method('error'); + $clock = $this->createMock(ITimeFactory::class); + $clock->method('getTime')->willReturnOnConsecutiveCalls(0, 5, 5, 5); + $job = new BackfillRowUuids($clock, $this->connectionAdapter, $config, $this->jobList, $logger); + $job->run(null); + $uuid = $this->mapper->find($ids[0])->getUuid(); + $this->assertNotNull($uuid); + $resumeClock = $this->createMock(ITimeFactory::class); + $resumeClock->method('getTime')->willReturn(10); + $this->jobList->expects($this->once())->method('remove'); + $this->job($resumeClock)->run(null); + $this->assertSame($uuid, $this->mapper->find($ids[0])->getUuid()); + } + + public function testCheckpointIsWrittenLazilyOncePerExecution(): void { + $ids = $this->historicalRows(501); + $config = $this->createMock(IAppConfig::class); + $config->method('getValueInt')->with(Application::APP_ID, 'row_uuid_backfill_cursor', 0, true)->willReturn(0); + $config->expects($this->once())->method('setValueInt')->with(Application::APP_ID, 'row_uuid_backfill_cursor', $ids[500], true)->willReturn(true); + $calls = 0; + $clock = $this->createMock(ITimeFactory::class); + $clock->method('getTime')->willReturnCallback(function () use (&$calls): int { + return ++$calls > 503 ? 5 : 0; + }); + $job = new BackfillRowUuids($clock, $this->connectionAdapter, $config, $this->jobList, new NullLogger()); + $job->run(null); + $this->assertNotNull($this->mapper->find($ids[500])->getUuid()); + } + + public function testFailedBatchRollsBackItsPrefixBeforeRetrying(): void { + $ids = $this->historicalRows(2); + $actualUpdate = $this->connectionAdapter->getQueryBuilder(); + $update = $this->createMock(IQueryBuilder::class); + foreach (['update', 'set', 'where', 'andWhere', 'setParameter'] as $method) { + $update->method($method)->willReturnCallback(function (...$arguments) use ($actualUpdate, $update, $method) { + $actualUpdate->$method(...$arguments); + return $update; + }); + } + $update->method('expr')->willReturn($actualUpdate->expr()); + $update->method('createParameter')->willReturnCallback(fn ($name) => $actualUpdate->createParameter($name)); + $calls = 0; + $update->method('executeStatement')->willReturnCallback(function () use ($actualUpdate, &$calls): int { + if (++$calls === 2) { + throw new \RuntimeException('Database write interrupted'); + } + return $actualUpdate->executeStatement(); + }); + $connection = $this->createMock(IDBConnection::class); + $connection->method('getQueryBuilder')->willReturnOnConsecutiveCalls($this->connectionAdapter->getQueryBuilder(), $update); + $connection->expects($this->once())->method('beginTransaction')->willReturnCallback(fn () => $this->connectionAdapter->beginTransaction()); + $connection->expects($this->once())->method('rollBack')->willReturnCallback(fn () => $this->connectionAdapter->rollBack()); + $connection->expects($this->never())->method('commit'); + $clock = $this->createMock(ITimeFactory::class); + $clock->method('getTime')->willReturn(0); + $logger = $this->createMock(LoggerInterface::class); + $logger->expects($this->once())->method('error'); + $job = new BackfillRowUuids($clock, $connection, $this->config, $this->jobList, $logger); + $job->run(null); + $this->assertSame([], $this->settings); + $this->assertNull($this->mapper->find($ids[0])->getUuid()); + $this->assertNull($this->mapper->find($ids[1])->getUuid()); + $this->jobList->expects($this->once())->method('remove'); + $this->job($clock)->run(null); + $this->assertNotNull($this->mapper->find($ids[0])->getUuid()); + $this->assertNotNull($this->mapper->find($ids[1])->getUuid()); + } + + public function testInterleavedAssignmentAndDeletionAreNotOverwritten(): void { + $ids = $this->historicalRows(3); + $uuid = '019f90c5-8576-7f0b-9b14-9bf49b2826cf'; + $assign = $this->connectionAdapter->getQueryBuilder(); + $assign->update('tables_row_sleeves')->set('uuid', $assign->createNamedParameter($uuid)) + ->where($assign->expr()->eq('id', $assign->createNamedParameter($ids[1]))); + $delete = $this->connectionAdapter->getQueryBuilder(); + $delete->delete('tables_row_sleeves')->where($delete->expr()->eq('id', $delete->createNamedParameter($ids[2]))); + $clock = $this->createMock(ITimeFactory::class); + $calls = 0; + $clock->method('getTime')->willReturnCallback(function () use (&$calls, $assign, $delete): int { + if (++$calls === 2) { + $assign->executeStatement(); + $delete->executeStatement(); + } + return 0; + }); + $this->jobList->expects($this->once())->method('remove'); + $this->job($clock)->run(null); + $this->assertNotNull($this->mapper->find($ids[0])->getUuid()); + $this->assertSame($uuid, $this->mapper->find($ids[1])->getUuid()); + $this->expectException(\OCP\AppFramework\Db\DoesNotExistException::class); + $this->mapper->find($ids[2]); + } + + public function testMoreThanOneBatchCompletesWithUniqueIdentities(): void { + $ids = $this->historicalRows(501); + $clock = $this->createMock(ITimeFactory::class); + $clock->method('getTime')->willReturn(0); + $this->jobList->expects($this->once())->method('remove'); + $this->job($clock)->run(null); + $uuids = array_map(fn ($id) => $this->mapper->find($id)->getUuid(), $ids); + $this->assertNotContains(null, $uuids); + $this->assertCount(501, array_unique($uuids)); + } +} From c57c0ddf54692b97f626d20158a2d89add9f37e1 Mon Sep 17 00:00:00 2001 From: Jiajun Xu Date: Thu, 5 Nov 2026 10:31:29 +0800 Subject: [PATCH 3/5] feat(rows): expose stored UUIDs in row responses Row responses now include the persisted uuid next to the integer id, including public-share rows and the row returned by POST. Rows created before the backfill reaches them report null. Row responses forwarded from a federated server keep the remote uuid, or report null when the remote server does not send one. Regenerate openapi.json and the TypeScript types for the new field. Refs: #3093 Assisted-by: Codex:gpt-6 Assisted-by: Codex:gpt-6-astra Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 Signed-off-by: Jiajun Xu --- lib/Controller/RowOCSController.php | 12 ++++++------ lib/Db/Row2.php | 10 ++++++++++ lib/Db/Row2Mapper.php | 3 +++ lib/ResponseDefinitions.php | 2 ++ openapi.json | 10 ++++++++++ src/types/openapi/openapi.ts | 2 ++ tests/unit/Service/LegacyRowMapperTest.php | 3 ++- tests/unit/Service/RowServiceUuidTest.php | 19 +++++++++++++++++++ 8 files changed, 54 insertions(+), 7 deletions(-) diff --git a/lib/Controller/RowOCSController.php b/lib/Controller/RowOCSController.php index 13ddd4e946..265966d66b 100644 --- a/lib/Controller/RowOCSController.php +++ b/lib/Controller/RowOCSController.php @@ -87,11 +87,11 @@ public function createRow(string $nodeCollection, int $nodeId, mixed $data): Dat try { if ($tableId !== null && $this->federationService->isNodeFederated($tableId, 'table')) { $table = $this->tableService->find($nodeId, true); - return new DataResponse($this->federationService->createRow($table, $data)); + return new DataResponse($this->federationService->createRow($table, $data) + ['uuid' => null]); } if ($viewId !== null && $this->federationService->isNodeFederated($viewId, 'view')) { $view = $this->viewService->find($nodeId, false, $this->userId); - return new DataResponse($this->federationService->createRow($view, $data)); + return new DataResponse($this->federationService->createRow($view, $data) + ['uuid' => null]); } return new DataResponse($this->rowService->create($tableId, $viewId, $newRowData)->jsonSerialize()); } catch (BadRequestError $e) { @@ -140,11 +140,11 @@ public function updateRow(string $nodeCollection, int $nodeId, int $rowId, mixed try { if ($tableId !== null && $this->federationService->isNodeFederated($tableId, 'table')) { $table = $this->tableService->find($nodeId, true); - return new DataResponse($this->federationService->updateRow($table, $rowId, $data)); + return new DataResponse($this->federationService->updateRow($table, $rowId, $data) + ['uuid' => null]); } if ($viewId !== null && $this->federationService->isNodeFederated($viewId, 'view')) { $view = $this->viewService->find($nodeId, false, $this->userId); - return new DataResponse($this->federationService->updateRow($view, $rowId, $data)); + return new DataResponse($this->federationService->updateRow($view, $rowId, $data) + ['uuid' => null]); } return new DataResponse($this->rowService->updateSet($rowId, $viewId, $data, $this->userId, $tableId)->jsonSerialize()); } catch (BadRequestError $e) { @@ -185,11 +185,11 @@ public function deleteRow(string $nodeCollection, int $nodeId, int $rowId): Data try { if ($tableId !== null && $this->federationService->isNodeFederated($tableId, 'table')) { $table = $this->tableService->find($nodeId, true); - return new DataResponse($this->federationService->deleteRow($table, $rowId)); + return new DataResponse($this->federationService->deleteRow($table, $rowId) + ['uuid' => null]); } if ($viewId !== null && $this->federationService->isNodeFederated($viewId, 'view')) { $view = $this->viewService->find($nodeId, false, $this->userId); - return new DataResponse($this->federationService->deleteRow($view, $rowId)); + return new DataResponse($this->federationService->deleteRow($view, $rowId) + ['uuid' => null]); } return new DataResponse($this->rowService->delete($rowId, $viewId, $this->userId, $tableId)->jsonSerialize()); } catch (NotFoundError $e) { diff --git a/lib/Db/Row2.php b/lib/Db/Row2.php index 0755bb325a..67f93164bb 100644 --- a/lib/Db/Row2.php +++ b/lib/Db/Row2.php @@ -17,6 +17,7 @@ */ class Row2 implements JsonSerializable { private ?int $id = null; + private ?string $uuid = null; private ?int $tableId = null; private ?string $createdBy = null; private ?string $createdAt = null; @@ -36,6 +37,14 @@ public function setId(int $id): void { $this->id = $id; } + public function getUuid(): ?string { + return $this->uuid; + } + + public function setUuid(?string $uuid): void { + $this->uuid = $uuid; + } + public function getTableId(): ?int { return $this->tableId; } @@ -165,6 +174,7 @@ public function filterDataByAliasByColumns(array $columns): array { public function jsonSerialize(): array { return [ 'id' => $this->id, + 'uuid' => $this->uuid, 'tableId' => $this->tableId, 'createdBy' => $this->createdBy, 'createdAt' => $this->createdAt, diff --git a/lib/Db/Row2Mapper.php b/lib/Db/Row2Mapper.php index 89248ffd34..02b3dd9ecd 100644 --- a/lib/Db/Row2Mapper.php +++ b/lib/Db/Row2Mapper.php @@ -726,6 +726,7 @@ private function parseEntities(IResult $result, array $sleeves): array { foreach ($sleeves as $sleeve) { $rows[$sleeve->getId()] = new Row2(); $rows[$sleeve->getId()]->setId($sleeve->getId()); + $rows[$sleeve->getId()]->setUuid($sleeve->getUuid()); $rows[$sleeve->getId()]->setCreatedBy($sleeve->getCreatedBy()); $rows[$sleeve->getId()]->setCreatedAt($sleeve->getCreatedAt()); $rows[$sleeve->getId()]->setLastEditBy($sleeve->getLastEditBy()); @@ -795,6 +796,8 @@ public function insert(Row2 $row, ?string $userId = null): Row2 { $row->setId($rowSleeve->getId()); } + $row->setUuid($rowSleeve->getUuid()); + // write all cells to its db-table foreach ($row->getData() as $cell) { $this->insertCell($rowSleeve->getId(), $cell['columnId'], $cell['value'], $rowSleeve->getLastEditAt(), $rowSleeve->getLastEditBy()); diff --git a/lib/ResponseDefinitions.php b/lib/ResponseDefinitions.php index dd41a0bf4f..ad037ebfeb 100644 --- a/lib/ResponseDefinitions.php +++ b/lib/ResponseDefinitions.php @@ -87,6 +87,7 @@ * * @psalm-type TablesRow = array{ * id: int, + * uuid: ?string, * tableId: int, * createdBy: string, * createdAt: string, @@ -98,6 +99,7 @@ * * @psalm-type TablesPublicRow = array{ * id: int, + * uuid: ?string, * createdAt: string, * lastEditAt: string, * data: ?array{columnId: int, value: mixed}, diff --git a/openapi.json b/openapi.json index 9fd3ae317c..874cc025c0 100644 --- a/openapi.json +++ b/openapi.json @@ -657,6 +657,7 @@ "type": "object", "required": [ "id", + "uuid", "createdAt", "lastEditAt", "data", @@ -667,6 +668,10 @@ "type": "integer", "format": "int64" }, + "uuid": { + "type": "string", + "nullable": true + }, "createdAt": { "type": "string" }, @@ -715,6 +720,7 @@ "type": "object", "required": [ "id", + "uuid", "tableId", "createdBy", "createdAt", @@ -728,6 +734,10 @@ "type": "integer", "format": "int64" }, + "uuid": { + "type": "string", + "nullable": true + }, "tableId": { "type": "integer", "format": "int64" diff --git a/src/types/openapi/openapi.ts b/src/types/openapi/openapi.ts index 85437d7fca..85d4ed9719 100644 --- a/src/types/openapi/openapi.ts +++ b/src/types/openapi/openapi.ts @@ -1306,6 +1306,7 @@ export type components = { readonly PublicRow: { /** Format: int64 */ readonly id: number; + readonly uuid: string | null; readonly createdAt: string; readonly lastEditAt: string; readonly data: { @@ -1324,6 +1325,7 @@ export type components = { readonly Row: { /** Format: int64 */ readonly id: number; + readonly uuid: string | null; /** Format: int64 */ readonly tableId: number; readonly createdBy: string; diff --git a/tests/unit/Service/LegacyRowMapperTest.php b/tests/unit/Service/LegacyRowMapperTest.php index 7822d50556..4933feb744 100644 --- a/tests/unit/Service/LegacyRowMapperTest.php +++ b/tests/unit/Service/LegacyRowMapperTest.php @@ -139,7 +139,8 @@ public function testMigrateLegacyRow() { self::assertTrue($data === $data2); $row2Serialized = $row2->jsonSerialize(); - unset($row2Serialized['dataByAlias']); + $this->assertNull($row2Serialized['uuid']); + unset($row2Serialized['dataByAlias'], $row2Serialized['uuid']); self::assertTrue($legacyRow->jsonSerialize() === $row2Serialized); } } diff --git a/tests/unit/Service/RowServiceUuidTest.php b/tests/unit/Service/RowServiceUuidTest.php index 5ca56c01a3..b3b2b17b3a 100644 --- a/tests/unit/Service/RowServiceUuidTest.php +++ b/tests/unit/Service/RowServiceUuidTest.php @@ -11,6 +11,7 @@ use OCA\Tables\Activity\ActivityManager; use OCA\Tables\Db\ColumnMapper; +use OCA\Tables\Db\Row2; use OCA\Tables\Db\Row2Mapper; use OCA\Tables\Db\RowSleeve; use OCA\Tables\Db\Table; @@ -35,6 +36,15 @@ protected function setUp(): void { $this->createMock(ColumnsHelper::class), $this->createMock(ActivityManager::class), $this->createMock(NotificationHelper::class)); } + private function row(): Row2 { + $row = new Row2(); + $row->setId(13); + $row->setTableId(5); + $row->setUuid('019f90c5-8576-7f0b-9b14-9bf49b2826cf'); + $row->setData([['columnId' => 1, 'value' => 'visible'], ['columnId' => 2, 'value' => 'hidden']]); + return $row; + } + /** @dataProvider archiveIdentities */ public function testArchiveIdentityGoesThroughSleeveInsertionBoundary(mixed $uuid, ?string $expected, bool $includeUuid = true): void { $table = new Table(); @@ -72,4 +82,13 @@ public function testInvalidArchiveIdentityFailsBeforeWriting(mixed $uuid): void public static function invalidArchiveIdentities(): array { return [[3], [[]], [''], ['019f90c5-8576-4f0b-9b14-9bf49b2826cf']]; } + + public function testPublicFormatterKeepsUuidAndRemovesPrivateMetadata(): void { + $row = $this->row(); + $result = $this->service->formatRowsForPublicShare([$row])[0]; + $this->assertSame($row->getUuid(), $result['uuid']); + $this->assertArrayNotHasKey('tableId', $result); + $this->assertArrayNotHasKey('createdBy', $result); + $this->assertArrayNotHasKey('lastEditBy', $result); + } } From ea0740795527957c238d36caccfb0489c58f41ab Mon Sep 17 00:00:00 2001 From: Jiajun Xu Date: Thu, 5 Nov 2026 10:31:29 +0800 Subject: [PATCH 4/5] feat(rows): resolve row identifiers within table and view scopes Add a RowIdentifier value object for positive integer IDs within the signed 32-bit range of the id column and for UUIDv7 strings. The row service resolves it inside the requested table or view, evaluating view filters with a bounded single-row query and the same user placeholders as listings, so knowing an identifier grants no access to other rows. Rows that are missing, deleted, in another table or outside a view are now reported as not found instead of as internal errors, including a row removed between loading and updating it. A spreadsheet import counts such a row as one failed line and continues. View projections work on a copy instead of shortening a shared row object, and listings of nodes without columns still return no rows. Row updates accept a RowDataInput so that callers can validate cells first. Refs: #3093 Assisted-by: Codex:gpt-6 Assisted-by: Codex:gpt-6-astra Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 Signed-off-by: Jiajun Xu --- lib/Db/Row2Mapper.php | 40 +++++- lib/Model/RowIdentifier.php | 48 +++++++ lib/Service/ImportService.php | 21 ++- lib/Service/RowService.php | 98 ++++++++++++-- tests/unit/Db/Row2MapperTest.php | 57 ++++++++ tests/unit/Model/RowIdentifierTest.php | 57 ++++++++ .../Service/ImportServiceRowUpdateTest.php | 128 ++++++++++++++++++ tests/unit/Service/RowServiceUuidTest.php | 85 +++++++++++- 8 files changed, 509 insertions(+), 25 deletions(-) create mode 100644 lib/Model/RowIdentifier.php create mode 100644 tests/unit/Model/RowIdentifierTest.php create mode 100644 tests/unit/Service/ImportServiceRowUpdateTest.php diff --git a/lib/Db/Row2Mapper.php b/lib/Db/Row2Mapper.php index 02b3dd9ecd..43b39f7c96 100644 --- a/lib/Db/Row2Mapper.php +++ b/lib/Db/Row2Mapper.php @@ -14,6 +14,7 @@ use OCA\Tables\Errors\NotFoundError; use OCA\Tables\Helper\ColumnsHelper; use OCA\Tables\Helper\UserHelper; +use OCA\Tables\Model\RowIdentifier; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\MultipleObjectsReturnedException; use OCP\AppFramework\Db\TTransactional; @@ -128,13 +129,19 @@ public function setUserId(string $userId): void { * @return int[] * @throws InternalError */ - private function getWantedRowIds(string $userId, int $tableId, ?array $filter = null, ?array $sort = null, ?int $limit = null, ?int $offset = null): array { + private function getWantedRowIds(string $userId, int $tableId, ?array $filter = null, ?array $sort = null, ?int $limit = null, ?int $offset = null, ?RowIdentifier $identifier = null): array { $qb = $this->db->getQueryBuilder(); $qb->select('sleeves.id') ->from('tables_row_sleeves', 'sleeves') ->where($qb->expr()->eq('table_id', $qb->createNamedParameter($tableId, IQueryBuilder::PARAM_INT))); + if ($identifier !== null) { + $column = $identifier->isUuid() ? 'sleeves.uuid' : 'sleeves.id'; + $type = $identifier->isUuid() ? IQueryBuilder::PARAM_STR : IQueryBuilder::PARAM_INT; + $qb->andWhere($qb->expr()->eq($column, $qb->createNamedParameter($identifier->value, $type))); + } + if ($filter) { $this->addFilterToQuery($qb, $filter, $userId); } @@ -175,6 +182,10 @@ private function getWantedRowIds(string $userId, int $tableId, ?array $filter = * @throws InternalError */ public function findAll(array $showColumnIds, int $tableId, ?int $limit = null, ?int $offset = null, ?array $filter = null, ?array $sort = null, ?string $userId = null): array { + if ($showColumnIds === []) { + return []; + } + try { $this->columnMapper->preloadColumns($showColumnIds, $filter, $sort); @@ -198,7 +209,7 @@ public function findAll(array $showColumnIds, int $tableId, ?int $limit = null, * @throws InternalError */ private function getRows(array $rowIds, array $columnIds): array { - if (empty($rowIds) || empty($columnIds)) { + if (empty($rowIds)) { return []; } @@ -216,6 +227,10 @@ private function getRows(array $rowIds, array $columnIds): array { * @throws InternalError */ private function getRowsChunk(array $rowIds, array $columnIds): array { + if ($columnIds === []) { + return $this->parseEntities(null, $this->rowSleeveMapper->findMultiple($rowIds)); + } + $qb = $this->db->getQueryBuilder(); $qbSqlForColumnTypes = null; @@ -716,12 +731,12 @@ private function getSqlOperator(string $operator, IQueryBuilder $qb, string $col } /** - * @param IResult $result + * @param IResult|null $result * @param RowSleeve[] $sleeves * @return Row2[] * @throws InternalError */ - private function parseEntities(IResult $result, array $sleeves): array { + private function parseEntities(?IResult $result, array $sleeves): array { $rows = []; foreach ($sleeves as $sleeve) { $rows[$sleeve->getId()] = new Row2(); @@ -739,7 +754,7 @@ private function parseEntities(IResult $result, array $sleeves): array { $keyToRowId = []; $cellMapperCache = []; - while ($rowData = $result->fetchAssociative()) { + while ($result !== null && ($rowData = $result->fetchAssociative())) { if (!isset($rowData['row_id'], $rows[$rowData['row_id']])) { break; } @@ -773,7 +788,14 @@ private function parseEntities(IResult $result, array $sleeves): array { * @throws InternalError */ public function isRowInViewPresent(int $rowId, View $view, string $userId): bool { - return in_array($rowId, $this->getWantedRowIds($userId, $view->getTableId(), $view->getFilterArray())); + $identifier = RowIdentifier::tryFromInt($rowId); + return $identifier !== null && $this->findScopedId($identifier, $view->getTableId(), $view->getFilterArray(), $userId) !== null; + } + + public function findScopedId(RowIdentifier $identifier, int $tableId, ?array $filter, string $userId): ?int { + $this->columnMapper->preloadColumns([], $filter, null); + $ids = $this->getWantedRowIds($userId, $tableId, $filter, null, 1, null, $identifier); + return $ids === [] ? null : (int)$ids[0]; } /** @@ -811,6 +833,7 @@ public function insert(Row2 $row, ?string $userId = null): Row2 { * @param string|null $userId * @return Row2 * @throws InternalError + * @throws NotFoundError */ public function update(Row2 $row, ?string $userId = null): Row2 { if ($userId) { @@ -827,7 +850,10 @@ public function update(Row2 $row, ?string $userId = null): Row2 { $sleeve = $this->rowSleeveMapper->find($row->getId()); $this->updateMetaData($sleeve); $this->rowSleeveMapper->update($sleeve); - } catch (DoesNotExistException|MultipleObjectsReturnedException|Exception $e) { + } catch (DoesNotExistException $e) { + $this->logger->warning($e->getMessage(), ['exception' => $e]); + throw new NotFoundError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage(), 0, $e); + } catch (MultipleObjectsReturnedException|Exception $e) { $this->logger->error($e->getMessage(), ['exception' => $e]); throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); } diff --git a/lib/Model/RowIdentifier.php b/lib/Model/RowIdentifier.php new file mode 100644 index 0000000000..65ab6bcee0 --- /dev/null +++ b/lib/Model/RowIdentifier.php @@ -0,0 +1,48 @@ + strlen($maximum) + || (strlen($digits) === strlen($maximum) && strcmp($digits, $maximum) > 0)) { + throw new \InvalidArgumentException('Row ID must be a positive integer within the supported range.'); + } + return new self((int)$digits); + } + + try { + return new self((new UuidV7($value))->toRfc4122()); + } catch (\InvalidArgumentException $e) { + throw new \InvalidArgumentException('Row identifier must be a positive integer or UUIDv7.', 0, $e); + } + } + + public static function tryFromInt(int $rowId): ?self { + return $rowId > 0 && $rowId <= self::MAX_ROW_ID ? new self($rowId) : null; + } + + public function isUuid(): bool { + return is_string($this->value); + } +} diff --git a/lib/Service/ImportService.php b/lib/Service/ImportService.php index fbc0054832..58f2f58b3d 100644 --- a/lib/Service/ImportService.php +++ b/lib/Service/ImportService.php @@ -691,8 +691,7 @@ private function upsertRow(Row $row, array $columnBusinesses): void { } if ($rowId) { - $this->rowService->updateSet($rowId, $this->viewId, $data, $this->userId, $this->tableId); - $this->countUpdatedRows++; + $this->updateImportedRow($rowId, $data); } else { $this->rowService->create($this->tableId, $this->viewId, $data, $this->userId); $this->countInsertedRows++; @@ -712,6 +711,24 @@ private function upsertRow(Row $row, array $columnBusinesses): void { } } + /** + * An ID that matches no row in the destination fails that line, not the whole import. + * + * @param list $data + * @throws BadRequestError + * @throws InternalError + * @throws PermissionError + */ + private function updateImportedRow(int $rowId, array $data): void { + try { + $this->rowService->updateSet($rowId, $this->viewId, $data, $this->userId, $this->tableId); + $this->countUpdatedRows++; + } catch (NotFoundError $e) { + $this->logger->warning('Row to update during import was not found: ' . $rowId, ['exception' => $e]); + $this->countErrors++; + } + } + /** * @param null|string $value */ diff --git a/lib/Service/RowService.php b/lib/Service/RowService.php index b7cabdd7aa..fd79563fda 100644 --- a/lib/Service/RowService.php +++ b/lib/Service/RowService.php @@ -8,6 +8,7 @@ namespace OCA\Tables\Service; use OCA\Tables\Activity\ActivityManager; +use OCA\Tables\AppInfo\Application; use OCA\Tables\Db\Column; use OCA\Tables\Db\ColumnMapper; use OCA\Tables\Db\Row2; @@ -25,7 +26,9 @@ use OCA\Tables\Event\RowDeletedEvent; use OCA\Tables\Event\RowUpdatedEvent; use OCA\Tables\Helper\ColumnsHelper; +use OCA\Tables\Helper\ConversionHelper; use OCA\Tables\Model\RowDataInput; +use OCA\Tables\Model\RowIdentifier; use OCA\Tables\Notification\NotificationHelper; use OCA\Tables\ResponseDefinitions; use OCA\Tables\Service\ColumnTypes\IColumnTypeBusiness; @@ -192,6 +195,56 @@ public function find(int $rowId): Row2 { return $row; } + /** + * Resolve a validated identifier to the stored row ID inside the requested table or view. + * + * @throws PermissionError + * @throws NotFoundError + * @throws InternalError + */ + public function resolveRowId(RowIdentifier $identifier, int $nodeType, int $nodeId, string $userId): int { + if (!$this->permissionsService->canReadRowsByElementId($nodeId, ConversionHelper::constNodeType2String($nodeType), $userId)) { + throw new PermissionError('Cannot read rows in the requested node.'); + } + try { + $view = $nodeType === Application::NODE_TYPE_VIEW ? $this->viewMapper->find($nodeId) : null; + } catch (DoesNotExistException $e) { + throw new NotFoundError('Row not found in the requested node.', 0, $e); + } catch (Exception|MultipleObjectsReturnedException $e) { + throw new InternalError('Cannot load the requested view.', 0, $e); + } + $rowId = $this->row2Mapper->findScopedId( + $identifier, + $view !== null ? $view->getTableId() : $nodeId, + $view?->getFilterArray(), + $view !== null ? $this->resolveFilterUserId($userId, $view) : $userId, + ); + if ($rowId === null) { + throw new NotFoundError('Row not found in the requested node.'); + } + return $rowId; + } + + /** + * @throws PermissionError + * @throws NotFoundError + * @throws InternalError + */ + public function findByIdentifier(RowIdentifier $identifier, int $nodeType, int $nodeId, string $userId): Row2 { + $rowId = $this->resolveRowId($identifier, $nodeType, $nodeId, $userId); + try { + $view = $nodeType === Application::NODE_TYPE_VIEW ? $this->viewMapper->find($nodeId) : null; + $columns = $view !== null ? $this->columnMapper->findAll($view->getColumnIds()) : $this->columnMapper->findAllByTable($nodeId); + $row = $this->row2Mapper->find($rowId, $columns); + $this->attachAliasPayload($row, $columns); + return $this->filterRowResult($view, $row); + } catch (DoesNotExistException $e) { + throw new NotFoundError('Row not found in the requested node.', 0, $e); + } catch (Exception|MultipleObjectsReturnedException $e) { + throw new InternalError('Cannot load the requested row.', 0, $e); + } + } + /** * @param int|null $tableId * @param int|null $viewId @@ -556,10 +609,10 @@ private function getRowById(int $rowId): Row2 { } $row = $this->row2Mapper->find($rowId, $this->columnMapper->findAllByTable($this->row2Mapper->getTableIdForRow($rowId))); $row->markAsLoaded(); - } catch (InternalError|DoesNotExistException|MultipleObjectsReturnedException|Exception $e) { + } catch (InternalError|MultipleObjectsReturnedException|Exception $e) { $this->logger->error($e->getMessage(), ['exception' => $e]); throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); - } catch (NotFoundError $e) { + } catch (NotFoundError|DoesNotExistException $e) { $this->logger->error($e->getMessage(), ['exception' => $e]); throw new NotFoundError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); } @@ -582,10 +635,29 @@ private function getRowById(int $rowId): Row2 { * @throws PermissionError * @noinspection DuplicatedCode */ - public function updateSet( + public function updateSet(int $id, ?int $viewId, array $data, string $userId, ?int $tableId): Row2 { + return $this->updateRowData($id, $viewId, RowDataInput::fromArray($data), $userId, $tableId); + } + + /** + * Update multiple cells in a row + * + * @param int $id + * @param int|null $viewId + * @param RowDataInput $data + * @param string $userId + * @return Row2 + * + * @throws BadRequestError + * @throws InternalError + * @throws NotFoundError + * @throws PermissionError + * @noinspection DuplicatedCode + */ + public function updateRowData( int $id, ?int $viewId, - array $data, + RowDataInput $data, string $userId, ?int $tableId, ): Row2 { @@ -632,10 +704,10 @@ public function updateSet( } // is row in view? - if (!$this->row2Mapper->isRowInViewPresent($id, $view, $userId)) { + if (!$this->row2Mapper->isRowInViewPresent($id, $view, $this->resolveFilterUserId($userId, $view))) { $e = new \Exception('Update row is not allowed.'); $this->logger->error($e->getMessage(), ['exception' => $e]); - throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); + throw new NotFoundError('Row not found in the requested view.'); } // fetch all needed columns @@ -650,7 +722,7 @@ public function updateSet( if ($tableId !== $item->getTableId()) { $e = new \Exception('Row does not belong to table with id ' . $tableId); $this->logger->error($e->getMessage(), ['exception' => $e]); - throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); + throw new NotFoundError('Row not found in the requested node.'); } // security @@ -673,7 +745,6 @@ public function updateSet( } $previousData = $item->getData(); - $data = RowDataInput::fromArray($data); $data = $this->cleanupAndValidateData($data, $columns, $item->getTableId(), $viewId, $id); foreach ($data as $entry) { @@ -746,7 +817,7 @@ public function delete(int $id, ?int $viewId, string $userId, ?int $tableId = nu $this->logger->error($e->getMessage(), ['exception' => $e]); throw new NotFoundError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); } - if (!$this->permissionsService->canDeleteRowsByViewId($viewId)) { + if (!$this->permissionsService->canDeleteRowsByViewId($viewId, $userId)) { $e = new \Exception('Update row is not allowed.'); $this->logger->error($e->getMessage(), ['exception' => $e]); throw new PermissionError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); @@ -757,16 +828,14 @@ public function delete(int $id, ?int $viewId, string $userId, ?int $tableId = nu $this->logger->error($e->getMessage(), ['exception' => $e]); throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); } - if (!$this->row2Mapper->isRowInViewPresent($id, $view, $userId)) { - $e = new \Exception('Update row is not allowed.'); - $this->logger->error($e->getMessage(), ['exception' => $e]); - throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); + if (!$this->row2Mapper->isRowInViewPresent($id, $view, $this->resolveFilterUserId($userId, $view))) { + throw new NotFoundError('Row not found in the requested view.'); } } else { if ($tableId !== null && $tableId !== $item->getTableId()) { $e = new \Exception('Row does not belong to table with id ' . $tableId); $this->logger->error($e->getMessage(), ['exception' => $e]); - throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); + throw new NotFoundError('Row not found in the requested node.'); } // security @@ -920,6 +989,7 @@ private function filterRowResult(?View $view, Row2 $row): Row2 { return $row; } + $row = clone $row; $columnIds = $view->getColumnIds(); $row->filterDataByColumns($columnIds); $row->filterDataByAliasByColumns($columnIds); diff --git a/tests/unit/Db/Row2MapperTest.php b/tests/unit/Db/Row2MapperTest.php index a5a82b25ab..18771eea21 100644 --- a/tests/unit/Db/Row2MapperTest.php +++ b/tests/unit/Db/Row2MapperTest.php @@ -10,6 +10,10 @@ namespace OCA\Tables\Tests\Unit\Db; use OCA\Tables\Db\Column; +use OCA\Tables\Db\Row2; +use OCA\Tables\Db\View; +use OCA\Tables\Errors\NotFoundError; +use OCA\Tables\Model\RowIdentifier; use OCA\Tables\Tests\Unit\Database\DatabaseTestCase; use PHPUnit\Framework\Attributes\DataProvider; @@ -28,6 +32,59 @@ protected function setUp(): void { $this->setupRealColumnMapper(self::$testTableId); } + public function testEmptyRowKeepsStoredUuidWithoutVisibleColumns(): void { + $this->connectionAdapter->beginTransaction(); + try { + $row = new Row2(); + $row->setTableId(self::$testTableId); + $created = $this->mapper->insert($row, 'alice'); + $this->assertNotNull($created->getUuid()); + $loaded = $this->mapper->find($created->getId(), []); + $this->assertSame($created->getUuid(), $loaded->jsonSerialize()['uuid']); + $this->assertSame([], $loaded->getData()); + $identifier = RowIdentifier::fromString($created->getUuid()); + $this->assertSame($created->getId(), $this->mapper->findScopedId($identifier, self::$testTableId, null, 'alice')); + $this->assertNull($this->mapper->findScopedId($identifier, self::$testTableId + 1, null, 'alice')); + } finally { + $this->connectionAdapter->rollBack(); + } + } + + public function testListingWithoutColumnsReturnsNoRows(): void { + $this->assertSame([], $this->mapper->findAll([], self::$testTableId, null, null, null, null, 'alice')); + } + + /** @dataProvider idsNoRowCanHave */ + public function testPresenceCheckIsFalseForIdsNoRowCanHave(int $rowId): void { + $view = new View(); + $view->setTableId(self::$testTableId); + $this->assertFalse($this->mapper->isRowInViewPresent($rowId, $view, 'alice')); + } + + public static function idsNoRowCanHave(): array { + return [[0], [-1], [2147483648]]; + } + + public function testUpdatingRemovedRowIsNotFound(): void { + $row = new Row2(); + $row->setId(2147483647); + $row->markAsLoaded(); + $row->insertOrUpdateCell(['columnId' => 1, 'value' => 'changed']); + $this->expectException(NotFoundError::class); + $this->mapper->update($row, 'alice'); + } + + public function testTargetLookupAppliesSavedFiltersAndTableScope(): void { + $firstId = self::$testRowIds[0]; + $identifier = RowIdentifier::fromString((string)$firstId); + $this->assertSame($firstId, $this->mapper->findScopedId($identifier, self::$testTableId, null, 'alice')); + $filter = [[['columnId' => Column::TYPE_META_ID, 'operator' => 'is-equal', 'value' => (string)($firstId + 1)]]]; + $this->assertNull($this->mapper->findScopedId($identifier, self::$testTableId, $filter, 'alice')); + $filter[0][0]['value'] = (string)$firstId; + $this->assertSame($firstId, $this->mapper->findScopedId($identifier, self::$testTableId, $filter, 'alice')); + $this->assertNull($this->mapper->findScopedId($identifier, self::$testTableId + 1, null, 'alice')); + } + /** * Data provider for sorting tests * diff --git a/tests/unit/Model/RowIdentifierTest.php b/tests/unit/Model/RowIdentifierTest.php new file mode 100644 index 0000000000..e5f79b98d9 --- /dev/null +++ b/tests/unit/Model/RowIdentifierTest.php @@ -0,0 +1,57 @@ +assertSame($expected, $identifier->value); + $this->assertSame(is_string($expected), $identifier->isUuid()); + } + + public static function validIdentifiers(): array { + return [ + ['1', 1], ['00123', 123], ['2147483647', 2147483647], + ['019F90C5-8576-7F0B-9B14-9BF49B2826CF', '019f90c5-8576-7f0b-9b14-9bf49b2826cf'], + ]; + } + + /** @dataProvider invalidIdentifiers */ + public function testInvalidIdentifier(string $input): void { + $this->expectException(\InvalidArgumentException::class); + RowIdentifier::fromString($input); + } + + public static function invalidIdentifiers(): array { + return array_map(static fn (string $input): array => [$input], [ + '', '0', '000', '-1', '+1', ' 1', '1 ', '1.0', '1e3', "1\n", '１２', + '2147483648', (string)PHP_INT_MAX, (string)PHP_INT_MAX . '0', '019f90c5-8576-4f0b-9b14-9bf49b2826cf', + '019f90c5-8576-7f0b-0b14-9bf49b2826cf', '019f90c585767f0b9b149bf49b2826cf', + ]); + } + + public function testIntegerWithinColumnRangeBecomesIdentifier(): void { + $this->assertSame(13, RowIdentifier::tryFromInt(13)?->value); + $this->assertSame(2147483647, RowIdentifier::tryFromInt(2147483647)?->value); + } + + /** @dataProvider integersOutsideColumnRange */ + public function testIntegerOutsideColumnRangeHasNoIdentifier(int $rowId): void { + $this->assertNull(RowIdentifier::tryFromInt($rowId)); + } + + public static function integersOutsideColumnRange(): array { + return [[0], [-1], [2147483648], [PHP_INT_MAX]]; + } +} diff --git a/tests/unit/Service/ImportServiceRowUpdateTest.php b/tests/unit/Service/ImportServiceRowUpdateTest.php new file mode 100644 index 0000000000..8e86f58feb --- /dev/null +++ b/tests/unit/Service/ImportServiceRowUpdateTest.php @@ -0,0 +1,128 @@ +temporaryFile = sys_get_temp_dir() . '/' . uniqid('tables-import-', true) . '.csv'; + $this->rows = $this->createMock(RowService::class); + } + + protected function tearDown(): void { + if (file_exists($this->temporaryFile)) { + unlink($this->temporaryFile); + } + parent::tearDown(); + } + + private function column(int $id, string $title): Column { + $column = new Column(); + $column->setId($id); + $column->setTitle($title); + $column->setType(Column::TYPE_TEXT); + $column->setSubtype(Column::SUBTYPE_TEXT_LINE); + $column->setMandatory(false); + return $column; + } + + private function createService(): ImportService { + $file = $this->createMock(ISimpleFile::class); + $file->method('getExtension')->willReturn('csv'); + $file->method('getContent')->willReturn(self::CSV); + $folder = $this->createMock(ISimpleFolder::class); + $folder->method('getFile')->willReturn($file); + $appData = $this->createMock(IAppData::class); + $appData->method('getFolder')->willReturn($folder); + $appDataFactory = $this->createMock(IAppDataFactory::class); + $appDataFactory->method('get')->willReturn($appData); + $tempManager = $this->createMock(ITempManager::class); + $tempManager->method('getTemporaryFile')->willReturn($this->temporaryFile); + + $columns = $this->createMock(ColumnService::class); + $columns->method('findOrCreateColumnsByTitleForTableAsArray')->willReturn([ + $this->column(Column::TYPE_META_ID, 'ID'), + $this->column(7, 'Title'), + ]); + $business = $this->createMock(IColumnTypeBusiness::class); + $business->method('canBeParsedDisplayValue')->willReturn(true); + $business->method('parseDisplayValue')->willReturn('"parsed"'); + $columnsHelper = $this->createMock(ColumnsHelper::class); + $columnsHelper->method('getColumnBusinessObject')->willReturn($business); + + return new ImportService( + $this->createMock(PermissionsService::class), + new NullLogger(), + 'alice', + $this->createMock(IRootFolder::class), + $columns, + $this->rows, + $this->createMock(TableService::class), + $this->createMock(ViewService::class), + $this->createMock(IUserManager::class), + $columnsHelper, + $this->createMock(IJobList::class), + $tempManager, + $this->createMock(IFilenameValidator::class), + $appDataFactory, + $this->createMock(AuditLogServiceInterface::class), + ); + } + + public function testRowMissingFromDestinationFailsOnlyThatLine(): void { + $this->rows->expects($this->once())->method('updateSet') + ->with(999, null, [['columnId' => 7, 'value' => 'parsed']], 'alice', 5) + ->willThrowException(new NotFoundError('Row not found.')); + $this->rows->expects($this->once())->method('create'); + + $stats = $this->createService()->importV2('alice', ImportService::IMPORT_TYPE_APP_FILE, 'import.csv', 5, null); + + $this->assertSame(1, $stats->errorsCount); + $this->assertSame(0, $stats->updatedRowsCount); + $this->assertSame(1, $stats->insertedRowsCount); + } + + public function testMissingDestinationStillAbortsTheImport(): void { + $this->rows->method('updateSet'); + $this->rows->method('create')->willThrowException(new NotFoundError('Table not found.')); + + $this->expectException(NotFoundError::class); + + $this->createService()->importV2('alice', ImportService::IMPORT_TYPE_APP_FILE, 'import.csv', 5, null); + } +} diff --git a/tests/unit/Service/RowServiceUuidTest.php b/tests/unit/Service/RowServiceUuidTest.php index b3b2b17b3a..5dd44ad5a4 100644 --- a/tests/unit/Service/RowServiceUuidTest.php +++ b/tests/unit/Service/RowServiceUuidTest.php @@ -10,14 +10,20 @@ namespace OCA\Tables\Tests\Unit\Service; use OCA\Tables\Activity\ActivityManager; +use OCA\Tables\AppInfo\Application; +use OCA\Tables\Db\Column; use OCA\Tables\Db\ColumnMapper; use OCA\Tables\Db\Row2; use OCA\Tables\Db\Row2Mapper; use OCA\Tables\Db\RowSleeve; use OCA\Tables\Db\Table; use OCA\Tables\Db\TableMapper; +use OCA\Tables\Db\View; use OCA\Tables\Db\ViewMapper; +use OCA\Tables\Errors\NotFoundError; +use OCA\Tables\Errors\PermissionError; use OCA\Tables\Helper\ColumnsHelper; +use OCA\Tables\Model\RowIdentifier; use OCA\Tables\Notification\NotificationHelper; use OCA\Tables\Service\PermissionsService; use OCA\Tables\Service\RowService; @@ -27,15 +33,30 @@ class RowServiceUuidTest extends TestCase { private RowService $service; + private PermissionsService $permissions; private Row2Mapper $rows; + private ColumnMapper $columns; + private ViewMapper $views; protected function setUp(): void { + $this->permissions = $this->createMock(PermissionsService::class); $this->rows = $this->createMock(Row2Mapper::class); - $this->service = new RowService(new NullLogger(), 'alice', $this->createMock(PermissionsService::class), $this->createMock(ColumnMapper::class), - $this->createMock(ViewMapper::class), $this->createMock(TableMapper::class), $this->rows, $this->createMock(IEventDispatcher::class), + $this->columns = $this->createMock(ColumnMapper::class); + $this->views = $this->createMock(ViewMapper::class); + $this->service = new RowService(new NullLogger(), 'alice', $this->permissions, $this->columns, $this->views, + $this->createMock(TableMapper::class), $this->rows, $this->createMock(IEventDispatcher::class), $this->createMock(ColumnsHelper::class), $this->createMock(ActivityManager::class), $this->createMock(NotificationHelper::class)); } + private function view(): View { + $view = new View(); + $view->setId(8); + $view->setTableId(5); + $view->setColumnsArray([['columnId' => 1, 'order' => 0]]); + $view->setFilterArray([[['columnId' => -1, 'operator' => 'is-equal', 'value' => '13']]]); + return $view; + } + private function row(): Row2 { $row = new Row2(); $row->setId(13); @@ -45,6 +66,51 @@ private function row(): Row2 { return $row; } + public function testViewOnlyAccessUsesViewScopeAndProjectsBothCellRepresentations(): void { + $view = $this->view(); + $this->permissions->expects($this->once())->method('canReadRowsByElementId')->with(8, 'view', 'alice')->willReturn(true); + $this->views->method('find')->with(8)->willReturn($view); + $this->rows->expects($this->once())->method('findScopedId')->with($this->isInstanceOf(RowIdentifier::class), 5, $view->getFilterArray(), 'alice')->willReturn(13); + $column = new Column(); + $column->setId(1); + $column->setTechnicalName('visible'); + $this->columns->method('findAll')->with([1])->willReturn([$column]); + $this->columns->expects($this->never())->method('findAllByTable'); + $original = $this->row(); + $this->rows->method('find')->with(13, [$column])->willReturn($original); + $result = $this->service->findByIdentifier(RowIdentifier::fromString('13'), Application::NODE_TYPE_VIEW, 8, 'alice'); + $this->assertSame([['columnId' => 1, 'value' => 'visible']], $result->getData()); + $this->assertSame(['visible' => ['columnId' => 1, 'value' => 'visible']], $result->getDataByAlias()); + $this->assertCount(2, $original->getData()); + $this->assertNotSame($original, $result); + $this->assertSame($original->getUuid(), $result->getUuid()); + } + + public function testMissingRowReadPermissionIsDeniedBeforeLookup(): void { + $this->permissions->method('canReadRowsByElementId')->willReturn(false); + $this->rows->expects($this->never())->method('findScopedId'); + $this->expectException(PermissionError::class); + $this->service->findByIdentifier(RowIdentifier::fromString('13'), Application::NODE_TYPE_TABLE, 5, 'alice'); + } + + public function testResolvingRowIdDoesNotLoadTheRow(): void { + $view = $this->view(); + $this->permissions->expects($this->once())->method('canReadRowsByElementId')->with(8, 'view', 'alice')->willReturn(true); + $this->views->method('find')->with(8)->willReturn($view); + $this->rows->expects($this->once())->method('findScopedId')->with($this->isInstanceOf(RowIdentifier::class), 5, $view->getFilterArray(), 'alice')->willReturn(13); + $this->rows->expects($this->never())->method('find'); + $this->columns->expects($this->never())->method('findAll'); + $this->assertSame(13, $this->service->resolveRowId(RowIdentifier::fromString('13'), Application::NODE_TYPE_VIEW, 8, 'alice')); + } + + public function testScopeMismatchIsNotFoundWithoutLoadingCells(): void { + $this->permissions->method('canReadRowsByElementId')->willReturn(true); + $this->rows->method('findScopedId')->willReturn(null); + $this->rows->expects($this->never())->method('find'); + $this->expectException(NotFoundError::class); + $this->service->findByIdentifier(RowIdentifier::fromString('13'), Application::NODE_TYPE_TABLE, 5, 'alice'); + } + /** @dataProvider archiveIdentities */ public function testArchiveIdentityGoesThroughSleeveInsertionBoundary(mixed $uuid, ?string $expected, bool $includeUuid = true): void { $table = new Table(); @@ -83,6 +149,21 @@ public static function invalidArchiveIdentities(): array { return [[3], [[]], [''], ['019f90c5-8576-4f0b-9b14-9bf49b2826cf']]; } + /** @dataProvider mutationMethods */ + public function testRowRemovedBeforeMutationIsNotFound(string $method): void { + $this->rows->method('getTableIdForRow')->willThrowException(new \OCP\AppFramework\Db\DoesNotExistException('Deleted row')); + $this->expectException(NotFoundError::class); + if ($method === 'updateSet') { + $this->service->updateSet(13, null, [], 'alice', 5); + } else { + $this->service->delete(13, null, 'alice', 5); + } + } + + public static function mutationMethods(): array { + return [['updateSet'], ['delete']]; + } + public function testPublicFormatterKeepsUuidAndRemovesPrivateMetadata(): void { $row = $this->row(); $result = $this->service->formatRowsForPublicShare([$row])[0]; From 0e70ed8c9e48364a9a2c029b1db5728292b6f568 Mon Sep 17 00:00:00 2001 From: Jiajun Xu Date: Thu, 5 Nov 2026 10:31:29 +0800 Subject: [PATCH 5/5] feat(api): address API v2 row item routes by numeric ID or UUIDv7 Add GET /api/2/{nodeCollection}/{nodeId}/rows/{rowId} and let the existing PUT and DELETE routes accept either a positive numeric ID or a UUIDv7. Invalid identifiers and malformed cell lists return 400 before any lookup, and rows outside the requested table or view return 404. PUT and DELETE resolve only the row ID before mutating, and service failures are no longer reported as client errors. Federated nodes keep numeric PUT and DELETE forwarding. UUID addressing and the new GET return 400 there, because the federation protocol has no equivalent yet. The existing row routes stay without a rate limit, since the web UI saves every cell and deletes each selected row through them. Regenerate openapi.json and the TypeScript types. Refs: #3093 Assisted-by: Codex:gpt-6 Assisted-by: Codex:gpt-6-astra Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 Signed-off-by: Jiajun Xu --- lib/Controller/RowOCSController.php | 105 +++++- lib/Model/RowDataInput.php | 27 ++ openapi.json | 348 +++++++++++++++++- src/types/openapi/openapi.ts | 149 +++++++- .../unit/Controller/RowOCSControllerTest.php | 164 +++++++++ tests/unit/Model/RowDataInputTest.php | 46 +++ 6 files changed, 808 insertions(+), 31 deletions(-) create mode 100644 tests/unit/Controller/RowOCSControllerTest.php create mode 100644 tests/unit/Model/RowDataInputTest.php diff --git a/lib/Controller/RowOCSController.php b/lib/Controller/RowOCSController.php index 265966d66b..aa29343e11 100644 --- a/lib/Controller/RowOCSController.php +++ b/lib/Controller/RowOCSController.php @@ -15,6 +15,7 @@ use OCA\Tables\Helper\ConversionHelper; use OCA\Tables\Middleware\Attribute\RequirePermission; use OCA\Tables\Model\RowDataInput; +use OCA\Tables\Model\RowIdentifier; use OCA\Tables\ResponseDefinitions; use OCA\Tables\Service\FederationService; use OCA\Tables\Service\RowService; @@ -105,13 +106,49 @@ public function createRow(string $nodeCollection, int $nodeId, mixed $data): Dat } } + /** + * [api v2] Read a local row in a table or view + * + * @param 'tables'|'views' $nodeCollection The targeted node collection + * @param int $nodeId The table or view identifier + * @param string $rowId A positive decimal row ID or a hyphenated UUIDv7 + * @return DataResponse|DataResponse + * + * 200: Row returned + * 400: Invalid identifier or unsupported federated operation + * 403: No permissions + * 404: Not found in the requested node + * 500: Internal error + */ + #[NoAdminRequired] + #[RequirePermission(permission: Application::PERMISSION_READ, typeParam: 'nodeCollection')] + #[ApiRoute(verb: 'GET', url: '/api/2/{nodeCollection}/{nodeId}/rows/{rowId}', requirements: ['nodeCollection' => '(tables|views)', 'nodeId' => '(\d+)'])] + public function getRow(string $nodeCollection, int $nodeId, string $rowId): DataResponse { + $iNodeType = ConversionHelper::stringNodeType2Const($nodeCollection); + try { + $identifier = $this->parseRowIdentifier($rowId); + if ($this->federationService->isNodeFederated($nodeId, ConversionHelper::constNodeType2String($iNodeType))) { + throw new BadRequestError('Single-row reads are not supported on federated nodes.'); + } + return new DataResponse($this->rowService->findByIdentifier($identifier, $iNodeType, $nodeId, $this->userId)->jsonSerialize()); + } catch (BadRequestError $e) { + return $this->handleBadRequestError($e); + } catch (PermissionError $e) { + return $this->handlePermissionError($e); + } catch (NotFoundError $e) { + return $this->handleNotFoundError($e); + } catch (\Exception $e) { + return $this->handleError($e); + } + } + /** * [api v2] Update a row in a table or a view * * @param 'tables'|'views' $nodeCollection Indicates whether to update a row on a table or view * @param int $nodeId The identifier of the targeted table or view - * @param int $rowId The identifier of the row to update - * @param string|array $data An array containing the column identifiers and their values + * @param string $rowId A positive decimal row ID or a hyphenated UUIDv7 + * @param string|list $data A list of cells containing columnId and value * @return DataResponse|DataResponse * * 200: Row updated @@ -123,7 +160,7 @@ public function createRow(string $nodeCollection, int $nodeId, mixed $data): Dat #[NoAdminRequired] #[RequirePermission(permission: Application::PERMISSION_UPDATE, typeParam: 'nodeCollection')] #[ApiRoute(verb: 'PUT', url: '/api/2/{nodeCollection}/{nodeId}/rows/{rowId}', requirements: ['nodeCollection' => '(tables|views)', 'nodeId' => '(\d+)'])] - public function updateRow(string $nodeCollection, int $nodeId, int $rowId, mixed $data): DataResponse { + public function updateRow(string $nodeCollection, int $nodeId, string $rowId, mixed $data): DataResponse { if (is_string($data)) { $data = json_decode($data, true); } @@ -138,15 +175,20 @@ public function updateRow(string $nodeCollection, int $nodeId, int $rowId, mixed $viewId = $nodeId; } try { + $identifier = $this->parseRowIdentifier($rowId); + $rowData = $this->parseRowData($data); if ($tableId !== null && $this->federationService->isNodeFederated($tableId, 'table')) { + $federatedRowId = $this->requireNumericRowId($identifier); $table = $this->tableService->find($nodeId, true); - return new DataResponse($this->federationService->updateRow($table, $rowId, $data) + ['uuid' => null]); + return new DataResponse($this->federationService->updateRow($table, $federatedRowId, $data) + ['uuid' => null]); } if ($viewId !== null && $this->federationService->isNodeFederated($viewId, 'view')) { + $federatedRowId = $this->requireNumericRowId($identifier); $view = $this->viewService->find($nodeId, false, $this->userId); - return new DataResponse($this->federationService->updateRow($view, $rowId, $data) + ['uuid' => null]); + return new DataResponse($this->federationService->updateRow($view, $federatedRowId, $data) + ['uuid' => null]); } - return new DataResponse($this->rowService->updateSet($rowId, $viewId, $data, $this->userId, $tableId)->jsonSerialize()); + $localRowId = $this->rowService->resolveRowId($identifier, $iNodeType, $nodeId, $this->userId); + return new DataResponse($this->rowService->updateRowData($localRowId, $viewId, $rowData, $this->userId, $tableId)->jsonSerialize()); } catch (BadRequestError $e) { return $this->handleBadRequestError($e); } catch (NotFoundError $e) { @@ -163,10 +205,11 @@ public function updateRow(string $nodeCollection, int $nodeId, int $rowId, mixed * * @param 'tables'|'views' $nodeCollection Indicates whether to delete a row on a table or view * @param int $nodeId The identifier of the targeted table or view - * @param int $rowId The identifier of the row to delete - * @return DataResponse|DataResponse + * @param string $rowId A positive decimal row ID or a hyphenated UUIDv7 + * @return DataResponse|DataResponse * * 200: Row deleted + * 400: Invalid identifier or unsupported federated operation * 403: No permissions * 404: Not found * 500: Internal error @@ -174,7 +217,7 @@ public function updateRow(string $nodeCollection, int $nodeId, int $rowId, mixed #[NoAdminRequired] #[RequirePermission(permission: Application::PERMISSION_DELETE, typeParam: 'nodeCollection')] #[ApiRoute(verb: 'DELETE', url: '/api/2/{nodeCollection}/{nodeId}/rows/{rowId}', requirements: ['nodeCollection' => '(tables|views)', 'nodeId' => '(\d+)'])] - public function deleteRow(string $nodeCollection, int $nodeId, int $rowId): DataResponse { + public function deleteRow(string $nodeCollection, int $nodeId, string $rowId): DataResponse { $iNodeType = ConversionHelper::stringNodeType2Const($nodeCollection); $tableId = $viewId = null; if ($iNodeType === Application::NODE_TYPE_TABLE) { @@ -183,15 +226,21 @@ public function deleteRow(string $nodeCollection, int $nodeId, int $rowId): Data $viewId = $nodeId; } try { + $identifier = $this->parseRowIdentifier($rowId); if ($tableId !== null && $this->federationService->isNodeFederated($tableId, 'table')) { + $federatedRowId = $this->requireNumericRowId($identifier); $table = $this->tableService->find($nodeId, true); - return new DataResponse($this->federationService->deleteRow($table, $rowId) + ['uuid' => null]); + return new DataResponse($this->federationService->deleteRow($table, $federatedRowId) + ['uuid' => null]); } if ($viewId !== null && $this->federationService->isNodeFederated($viewId, 'view')) { + $federatedRowId = $this->requireNumericRowId($identifier); $view = $this->viewService->find($nodeId, false, $this->userId); - return new DataResponse($this->federationService->deleteRow($view, $rowId) + ['uuid' => null]); + return new DataResponse($this->federationService->deleteRow($view, $federatedRowId) + ['uuid' => null]); } - return new DataResponse($this->rowService->delete($rowId, $viewId, $this->userId, $tableId)->jsonSerialize()); + $localRowId = $this->rowService->resolveRowId($identifier, $iNodeType, $nodeId, $this->userId); + return new DataResponse($this->rowService->delete($localRowId, $viewId, $this->userId, $tableId)->jsonSerialize()); + } catch (BadRequestError $e) { + return $this->handleBadRequestError($e); } catch (NotFoundError $e) { return $this->handleNotFoundError($e); } catch (PermissionError $e) { @@ -200,4 +249,36 @@ public function deleteRow(string $nodeCollection, int $nodeId, int $rowId): Data return $this->handleError($e); } } + + /** + * @throws BadRequestError + */ + private function parseRowIdentifier(string $rowId): RowIdentifier { + try { + return RowIdentifier::fromString($rowId); + } catch (\InvalidArgumentException $e) { + throw new BadRequestError($e->getMessage(), 0, $e); + } + } + + /** + * @throws BadRequestError + */ + private function parseRowData(array $data): RowDataInput { + try { + return RowDataInput::createFromInputArray($data); + } catch (\InvalidArgumentException $e) { + throw new BadRequestError($e->getMessage(), 0, $e); + } + } + + /** + * @throws BadRequestError + */ + private function requireNumericRowId(RowIdentifier $identifier): int { + if (!is_int($identifier->value)) { + throw new BadRequestError('Federated rows do not support UUID addressing.'); + } + return $identifier->value; + } } diff --git a/lib/Model/RowDataInput.php b/lib/Model/RowDataInput.php index 21f54cd2f9..b5d7a4fa7d 100644 --- a/lib/Model/RowDataInput.php +++ b/lib/Model/RowDataInput.php @@ -84,6 +84,33 @@ public function rewind(): void { reset($this->data); } + public static function createFromInputArray(array $cells): self { + if (!array_is_list($cells)) { + throw new \InvalidArgumentException('Row data must be a list of cells.'); + } + $input = new self(); + foreach ($cells as $cell) { + if (!is_array($cell) || !array_key_exists('columnId', $cell) || !array_key_exists('value', $cell)) { + throw new \InvalidArgumentException('Each cell requires columnId and value.'); + } + $columnId = $cell['columnId']; + if (is_string($columnId) && preg_match('/^-?[0-9]+$/D', $columnId)) { + $digits = ltrim(ltrim($columnId, '-'), '0'); + $canonical = ($digits !== '' && str_starts_with($columnId, '-') ? '-' : '') . ($digits === '' ? '0' : $digits); + $columnId = filter_var($canonical, FILTER_VALIDATE_INT); + } + if (!is_int($columnId)) { + throw new \InvalidArgumentException('Cell columnId must be an integer within the supported range.'); + } + $value = $cell['value']; + if ($value !== null && !is_scalar($value) && !is_array($value)) { + throw new \InvalidArgumentException('Cell values must be JSON values.'); + } + $input->add($columnId, is_bool($value) ? (int)$value : $value); + } + return $input; + } + public static function fromArray(array $data): self { $newRowData = new RowDataInput(); foreach ($data as $value) { diff --git a/openapi.json b/openapi.json index 874cc025c0..1dd9b21758 100644 --- a/openapi.json +++ b/openapi.json @@ -16452,6 +16452,278 @@ } }, "/ocs/v2.php/apps/tables/api/2/{nodeCollection}/{nodeId}/rows/{rowId}": { + "get": { + "operationId": "rowocs-get-row", + "summary": "[api v2] Read a local row in a table or view", + "tags": [ + "rowocs" + ], + "security": [ + { + "bearer_auth": [] + }, + { + "basic_auth": [] + } + ], + "parameters": [ + { + "name": "nodeCollection", + "in": "path", + "description": "The targeted node collection", + "required": true, + "schema": { + "type": "string", + "enum": [ + "tables", + "views" + ], + "pattern": "^(tables|views)$" + } + }, + { + "name": "nodeId", + "in": "path", + "description": "The table or view identifier", + "required": true, + "schema": { + "type": "integer", + "format": "int64" + } + }, + { + "name": "rowId", + "in": "path", + "description": "A positive decimal row ID or a hyphenated UUIDv7", + "required": true, + "schema": { + "type": "string" + } + }, + { + "name": "OCS-APIRequest", + "in": "header", + "description": "Required to be true for the API request to pass", + "required": true, + "schema": { + "type": "boolean", + "default": true + } + } + ], + "responses": { + "200": { + "description": "Row returned", + "content": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "ocs" + ], + "properties": { + "ocs": { + "type": "object", + "required": [ + "meta", + "data" + ], + "properties": { + "meta": { + "$ref": "#/components/schemas/OCSMeta" + }, + "data": { + "$ref": "#/components/schemas/Row" + } + } + } + } + } + } + } + }, + "400": { + "description": "Invalid identifier or unsupported federated operation", + "content": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "ocs" + ], + "properties": { + "ocs": { + "type": "object", + "required": [ + "meta", + "data" + ], + "properties": { + "meta": { + "$ref": "#/components/schemas/OCSMeta" + }, + "data": { + "type": "object", + "required": [ + "message" + ], + "properties": { + "message": { + "type": "string" + } + } + } + } + } + } + } + } + } + }, + "403": { + "description": "No permissions", + "content": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "ocs" + ], + "properties": { + "ocs": { + "type": "object", + "required": [ + "meta", + "data" + ], + "properties": { + "meta": { + "$ref": "#/components/schemas/OCSMeta" + }, + "data": { + "type": "object", + "required": [ + "message" + ], + "properties": { + "message": { + "type": "string" + } + } + } + } + } + } + } + } + } + }, + "404": { + "description": "Not found in the requested node", + "content": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "ocs" + ], + "properties": { + "ocs": { + "type": "object", + "required": [ + "meta", + "data" + ], + "properties": { + "meta": { + "$ref": "#/components/schemas/OCSMeta" + }, + "data": { + "type": "object", + "required": [ + "message" + ], + "properties": { + "message": { + "type": "string" + } + } + } + } + } + } + } + } + } + }, + "500": { + "description": "Internal error", + "content": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "ocs" + ], + "properties": { + "ocs": { + "type": "object", + "required": [ + "meta", + "data" + ], + "properties": { + "meta": { + "$ref": "#/components/schemas/OCSMeta" + }, + "data": { + "type": "object", + "required": [ + "message" + ], + "properties": { + "message": { + "type": "string" + } + } + } + } + } + } + } + } + } + }, + "401": { + "description": "Current user is not logged in", + "content": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "ocs" + ], + "properties": { + "ocs": { + "type": "object", + "required": [ + "meta", + "data" + ], + "properties": { + "meta": { + "$ref": "#/components/schemas/OCSMeta" + }, + "data": {} + } + } + } + } + } + } + } + } + }, "put": { "operationId": "rowocs-update-row", "summary": "[api v2] Update a row in a table or a view", @@ -16477,15 +16749,35 @@ ], "properties": { "data": { - "description": "An array containing the column identifiers and their values", + "description": "A list of cells containing columnId and value", "oneOf": [ { "type": "string" }, { - "type": "object", - "additionalProperties": { - "type": "object" + "type": "array", + "items": { + "type": "object", + "required": [ + "columnId", + "value" + ], + "properties": { + "columnId": { + "oneOf": [ + { + "type": "integer", + "format": "int64" + }, + { + "type": "string" + } + ] + }, + "value": { + "type": "object" + } + } } } ] @@ -16523,11 +16815,10 @@ { "name": "rowId", "in": "path", - "description": "The identifier of the row to update", + "description": "A positive decimal row ID or a hyphenated UUIDv7", "required": true, "schema": { - "type": "integer", - "format": "int64" + "type": "string" } }, { @@ -16796,11 +17087,10 @@ { "name": "rowId", "in": "path", - "description": "The identifier of the row to delete", + "description": "A positive decimal row ID or a hyphenated UUIDv7", "required": true, "schema": { - "type": "integer", - "format": "int64" + "type": "string" } }, { @@ -16883,6 +17173,44 @@ } } }, + "400": { + "description": "Invalid identifier or unsupported federated operation", + "content": { + "application/json": { + "schema": { + "type": "object", + "required": [ + "ocs" + ], + "properties": { + "ocs": { + "type": "object", + "required": [ + "meta", + "data" + ], + "properties": { + "meta": { + "$ref": "#/components/schemas/OCSMeta" + }, + "data": { + "type": "object", + "required": [ + "message" + ], + "properties": { + "message": { + "type": "string" + } + } + } + } + } + } + } + } + } + }, "404": { "description": "Not found", "content": { diff --git a/src/types/openapi/openapi.ts b/src/types/openapi/openapi.ts index 85d4ed9719..4ef16c8725 100644 --- a/src/types/openapi/openapi.ts +++ b/src/types/openapi/openapi.ts @@ -1081,7 +1081,8 @@ export type paths = { readonly path?: never; readonly cookie?: never; }; - readonly get?: never; + /** [api v2] Read a local row in a table or view */ + readonly get: operations["rowocs-get-row"]; /** [api v2] Update a row in a table or a view */ readonly put: operations["rowocs-update-row"]; readonly post?: never; @@ -8850,6 +8851,119 @@ export interface operations { }; }; }; + readonly "rowocs-get-row": { + readonly parameters: { + readonly query?: never; + readonly header: { + /** @description Required to be true for the API request to pass */ + readonly "OCS-APIRequest": boolean; + }; + readonly path: { + /** @description The targeted node collection */ + readonly nodeCollection: "tables" | "views"; + /** @description The table or view identifier */ + readonly nodeId: number; + /** @description A positive decimal row ID or a hyphenated UUIDv7 */ + readonly rowId: string; + }; + readonly cookie?: never; + }; + readonly requestBody?: never; + readonly responses: { + /** @description Row returned */ + readonly 200: { + headers: { + readonly [name: string]: unknown; + }; + content: { + readonly "application/json": { + readonly ocs: { + readonly meta: components["schemas"]["OCSMeta"]; + readonly data: components["schemas"]["Row"]; + }; + }; + }; + }; + /** @description Invalid identifier or unsupported federated operation */ + readonly 400: { + headers: { + readonly [name: string]: unknown; + }; + content: { + readonly "application/json": { + readonly ocs: { + readonly meta: components["schemas"]["OCSMeta"]; + readonly data: { + readonly message: string; + }; + }; + }; + }; + }; + /** @description Current user is not logged in */ + readonly 401: { + headers: { + readonly [name: string]: unknown; + }; + content: { + readonly "application/json": { + readonly ocs: { + readonly meta: components["schemas"]["OCSMeta"]; + readonly data: unknown; + }; + }; + }; + }; + /** @description No permissions */ + readonly 403: { + headers: { + readonly [name: string]: unknown; + }; + content: { + readonly "application/json": { + readonly ocs: { + readonly meta: components["schemas"]["OCSMeta"]; + readonly data: { + readonly message: string; + }; + }; + }; + }; + }; + /** @description Not found in the requested node */ + readonly 404: { + headers: { + readonly [name: string]: unknown; + }; + content: { + readonly "application/json": { + readonly ocs: { + readonly meta: components["schemas"]["OCSMeta"]; + readonly data: { + readonly message: string; + }; + }; + }; + }; + }; + /** @description Internal error */ + readonly 500: { + headers: { + readonly [name: string]: unknown; + }; + content: { + readonly "application/json": { + readonly ocs: { + readonly meta: components["schemas"]["OCSMeta"]; + readonly data: { + readonly message: string; + }; + }; + }; + }; + }; + }; + }; readonly "rowocs-update-row": { readonly parameters: { readonly query?: never; @@ -8862,18 +8976,19 @@ export interface operations { readonly nodeCollection: "tables" | "views"; /** @description The identifier of the targeted table or view */ readonly nodeId: number; - /** @description The identifier of the row to update */ - readonly rowId: number; + /** @description A positive decimal row ID or a hyphenated UUIDv7 */ + readonly rowId: string; }; readonly cookie?: never; }; readonly requestBody: { readonly content: { readonly "application/json": { - /** @description An array containing the column identifiers and their values */ - readonly data: string | { - readonly [key: string]: Record; - }; + /** @description A list of cells containing columnId and value */ + readonly data: string | readonly { + readonly columnId: number | string; + readonly value: Record; + }[]; }; }; }; @@ -8984,8 +9099,8 @@ export interface operations { readonly nodeCollection: "tables" | "views"; /** @description The identifier of the targeted table or view */ readonly nodeId: number; - /** @description The identifier of the row to delete */ - readonly rowId: number; + /** @description A positive decimal row ID or a hyphenated UUIDv7 */ + readonly rowId: string; }; readonly cookie?: never; }; @@ -9005,6 +9120,22 @@ export interface operations { }; }; }; + /** @description Invalid identifier or unsupported federated operation */ + readonly 400: { + headers: { + readonly [name: string]: unknown; + }; + content: { + readonly "application/json": { + readonly ocs: { + readonly meta: components["schemas"]["OCSMeta"]; + readonly data: { + readonly message: string; + }; + }; + }; + }; + }; /** @description Current user is not logged in */ readonly 401: { headers: { diff --git a/tests/unit/Controller/RowOCSControllerTest.php b/tests/unit/Controller/RowOCSControllerTest.php new file mode 100644 index 0000000000..85ede89ce1 --- /dev/null +++ b/tests/unit/Controller/RowOCSControllerTest.php @@ -0,0 +1,164 @@ +rows = $this->createMock(RowService::class); + $this->federation = $this->createMock(FederationService::class); + $this->tables = $this->createMock(TableService::class); + $this->views = $this->createMock(ViewService::class); + $l10n = $this->createMock(IL10N::class); + $l10n->method('t')->willReturnArgument(0); + $this->controller = new RowOCSController($this->createMock(IRequest::class), new NullLogger(), $l10n, 'alice', $this->rows, $this->tables, $this->views, $this->federation); + } + + private function row(): Row2 { + $row = new Row2(); + $row->setId(13); + $row->setTableId(5); + $row->setUuid(self::UUID); + return $row; + } + + /** @dataProvider localOperations */ + public function testLocalItemOperationsConvergeOnStoredIdentity(string $method, string $collection, string $identifier): void { + $row = $this->row(); + $nodeType = $collection === 'views' ? Application::NODE_TYPE_VIEW : Application::NODE_TYPE_TABLE; + $matchesIdentifier = $this->callback(fn (RowIdentifier $id) => $id->value === RowIdentifier::fromString($identifier)->value); + if ($method === 'getRow') { + $this->rows->expects($this->once())->method('findByIdentifier')->with($matchesIdentifier, $nodeType, 5, 'alice')->willReturn($row); + } else { + $this->rows->expects($this->never())->method('findByIdentifier'); + $this->rows->expects($this->once())->method('resolveRowId')->with($matchesIdentifier, $nodeType, 5, 'alice')->willReturn(13); + } + if ($method === 'updateRow') { + $this->rows->expects($this->once())->method('updateRowData')->with(13, $collection === 'views' ? 5 : null, $this->isInstanceOf(RowDataInput::class), 'alice', $collection === 'tables' ? 5 : null)->willReturn($row); + } elseif ($method === 'deleteRow') { + $this->rows->expects($this->once())->method('delete')->with(13, $collection === 'views' ? 5 : null, 'alice', $collection === 'tables' ? 5 : null)->willReturn($row); + } + $response = $method === 'updateRow' + ? $this->controller->updateRow($collection, 5, $identifier, []) + : $this->controller->$method($collection, 5, $identifier); + $this->assertSame(200, $response->getStatus()); + $this->assertSame(self::UUID, $response->getData()['uuid']); + $this->assertSame(13, $response->getData()['id']); + } + + public static function localOperations(): array { + $cases = []; + foreach (['getRow', 'updateRow', 'deleteRow'] as $method) { + foreach (['tables', 'views'] as $collection) { + foreach (['00013', strtoupper(self::UUID)] as $identifier) { + $cases[] = [$method, $collection, $identifier]; + } + } + } + return $cases; + } + + /** @dataProvider invalidOperations */ + public function testInvalidIdentifiersReturnBadRequestBeforeResolving(string $method): void { + $this->rows->expects($this->never())->method('findByIdentifier'); + $this->rows->expects($this->never())->method('resolveRowId'); + $this->federation->expects($this->never())->method('isNodeFederated'); + $response = $method === 'updateRow' ? $this->controller->updateRow('tables', 5, 'not-a-row', []) : $this->controller->$method('tables', 5, 'not-a-row'); + $this->assertSame(400, $response->getStatus()); + } + + public static function invalidOperations(): array { + return [['getRow'], ['updateRow'], ['deleteRow']]; + } + + public function testMalformedUpdateCellsReturnBadRequestWithoutLoadingRow(): void { + $this->rows->expects($this->never())->method('resolveRowId'); + $this->assertSame(400, $this->controller->updateRow('tables', 5, '13', ['column_1' => 'value'])->getStatus()); + $this->assertSame(400, $this->controller->updateRow('tables', 5, '13', [['columnId' => 1]])->getStatus()); + } + + /** @dataProvider federatedOperations */ + public function testFederatedBoundary(string $method, string $collection, bool $uuid): void { + $this->federation->method('isNodeFederated')->willReturn(true); + $this->rows->expects($this->never())->method('findByIdentifier'); + $this->rows->expects($this->never())->method('resolveRowId'); + $forward = !$uuid && $method !== 'getRow'; + if ($forward) { + $node = $collection === 'tables' ? new Table() : new View(); + ($collection === 'tables' ? $this->tables : $this->views)->method('find')->willReturn($node); + if ($method === 'updateRow') { + $this->federation->expects($this->once())->method($method)->with($node, 13, [])->willReturn(['id' => 13]); + } else { + $this->federation->expects($this->once())->method($method)->with($node, 13)->willReturn(['id' => 13]); + } + } else { + $this->federation->expects($this->never())->method('updateRow'); + $this->federation->expects($this->never())->method('deleteRow'); + } + $identifier = $uuid ? self::UUID : '13'; + $response = $method === 'updateRow' ? $this->controller->updateRow($collection, 5, $identifier, []) : $this->controller->$method($collection, 5, $identifier); + $this->assertSame($forward ? 200 : 400, $response->getStatus()); + if ($forward) { + $this->assertArrayHasKey('uuid', $response->getData()); + $this->assertNull($response->getData()['uuid']); + } + } + + public static function federatedOperations(): array { + $cases = []; + foreach (['getRow', 'updateRow', 'deleteRow'] as $method) { + foreach (['tables', 'views'] as $collection) { + foreach ([false, true] as $uuid) { + $cases[] = [$method, $collection, $uuid]; + } + } + } + return $cases; + } + + /** @dataProvider serviceFailures */ + public function testServiceErrorsMapToHttpStatus(\Exception $error, int $status): void { + $this->rows->method('findByIdentifier')->willThrowException($error); + $this->assertSame($status, $this->controller->getRow('tables', 5, self::UUID)->getStatus()); + } + + public static function serviceFailures(): array { + return [ + [new NotFoundError('Missing'), 404], + [new PermissionError('Denied'), 403], + [new \RuntimeException('Failed'), 500], + [new \InvalidArgumentException('Stored state is invalid'), 500], + ]; + } +} diff --git a/tests/unit/Model/RowDataInputTest.php b/tests/unit/Model/RowDataInputTest.php new file mode 100644 index 0000000000..82be41d714 --- /dev/null +++ b/tests/unit/Model/RowDataInputTest.php @@ -0,0 +1,46 @@ + '0012', 'value' => null], + ['columnId' => '-2', 'value' => 'alice'], + ['columnId' => 13, 'value' => true], + ]); + $this->assertSame([ + ['columnId' => 12, 'value' => null], + ['columnId' => -2, 'value' => 'alice'], + ['columnId' => 13, 'value' => 1], + ], iterator_to_array($input)); + } + + /** @dataProvider malformedCells */ + public function testMalformedCellsAreRejected(array $cells): void { + $this->expectException(\InvalidArgumentException::class); + RowDataInput::createFromInputArray($cells); + } + + public static function malformedCells(): array { + return [ + [['column_1' => 'value']], + [[['columnId' => 1]]], + [[['value' => 'value']]], + [[['columnId' => 'garbage', 'value' => null]]], + [[['columnId' => '92233720368547758070', 'value' => null]]], + [[['columnId' => 1.5, 'value' => null]]], + [[['columnId' => true, 'value' => null]]], + ]; + } +}