Merge pull request #1351 from nextcloud/fix/use-ocs-row-create

refactor(Rows): switch front-end to use Row OCS API

appinfo/routes.php

@@ -93,7 +93,6 @@
 		['name' => 'row#index', 'url' => '/row/table/{tableId}', 'verb' => 'GET'],
 		['name' => 'row#show', 'url' => '/row/{id}', 'verb' => 'GET'],
 		['name' => 'row#indexView', 'url' => '/row/view/{viewId}', 'verb' => 'GET'],
-		['name' => 'row#create', 'url' => '/row', 'verb' => 'POST'],
 		['name' => 'row#update', 'url' => '/row/{id}/column/{columnId}', 'verb' => 'PUT'],
 		['name' => 'row#updateSet', 'url' => '/row/{id}', 'verb' => 'PUT'],
 		['name' => 'row#destroyByView', 'url' => '/view/{viewId}/row/{id}', 'verb' => 'DELETE'],

lib/Controller/Api1Controller.php

@@ -10,6 +10,7 @@
 namespace OCA\Tables\Controller;
 
 use Exception;
+use InvalidArgumentException;
 use OCA\Tables\Api\V1Api;
 use OCA\Tables\AppInfo\Application;
 use OCA\Tables\Db\ViewMapper;
@@ -376,19 +377,24 @@ public function getView(int $viewId): DataResponse {
 	 *
 	 * @param int $viewId View ID
 	 * @param array{key: 'title'|'emoji'|'description', value: string}|array{key: 'columns', value: int[]}|array{key: 'sort', value: array{columnId: int, mode: 'ASC'|'DESC'}}|array{key: 'filter', value: array{columnId: int, operator: 'begins-with'|'ends-with'|'contains'|'is-equal'|'is-greater-than'|'is-greater-than-or-equal'|'is-lower-than'|'is-lower-than-or-equal'|'is-empty', value: string|int|float}} $data key-value pairs
-	 * @return DataResponse<Http::STATUS_OK, TablesView, array{}>|DataResponse<Http::STATUS_FORBIDDEN|Http::STATUS_INTERNAL_SERVER_ERROR, array{message: string}, array{}>
+	 * @return DataResponse<Http::STATUS_OK, TablesView, array{}>|DataResponse<Http::STATUS_FORBIDDEN|Http::STATUS_BAD_REQUEST|Http::STATUS_INTERNAL_SERVER_ERROR, array{message: string}, array{}>
 	 *
 	 * 200: View updated
+	 * 400: Invalid data
 	 * 403: No permissions
 	 * 404: Not found
 	 */
 	public function updateView(int $viewId, array $data): DataResponse {
 		try {
 			return new DataResponse($this->viewService->update($viewId, $data)->jsonSerialize());
 		} catch (PermissionError $e) {
-			$this->logger->warning('A permission error occurred: '.$e->getMessage(), ['exception' => $e]);
+			$this->logger->warning('A permission error occurred: ' . $e->getMessage(), ['exception' => $e]);
 			$message = ['message' => $e->getMessage()];
 			return new DataResponse($message, Http::STATUS_FORBIDDEN);
+		} catch (InvalidArgumentException $e) {
+			$this->logger->warning('An invalid request occurred: ' . $e->getMessage(), ['exception' => $e]);
+			$message = ['message' => $e->getMessage()];
+			return new DataResponse($message, Http::STATUS_BAD_REQUEST);
 		} catch (InternalError|Exception $e) {
 			$this->logger->error('An internal error or exception occurred: '.$e->getMessage(), ['exception' => $e]);
 			$message = ['message' => $e->getMessage()];
@@ -1127,6 +1133,7 @@ public function indexViewRows(int $viewId, ?int $limit, ?int $offset): DataRespo
 	 * 200: Row returned
 	 * 403: No permissions
 	 */
+	#[RequirePermission(permission: Application::PERMISSION_CREATE, type: Application::NODE_TYPE_VIEW, idParam: 'viewId')]
 	public function createRowInView(int $viewId, $data): DataResponse {
 		if(is_string($data)) {
 			$data = json_decode($data, true);
@@ -1173,6 +1180,7 @@ public function createRowInView(int $viewId, $data): DataResponse {
 	 * 403: No permissions
 	 * 404: Not found
 	 */
+	#[RequirePermission(permission: Application::PERMISSION_CREATE, type: Application::NODE_TYPE_TABLE, idParam: 'tableId')]
 	public function createRowInTable(int $tableId, $data): DataResponse {
 		if(is_string($data)) {
 			$data = json_decode($data, true);
@@ -1360,8 +1368,10 @@ public function deleteRowByView(int $rowId, int $viewId): DataResponse {
 	 * 403: No permissions
 	 * 404: Not found
 	 */
+	#[RequirePermission(permission: Application::PERMISSION_CREATE, type: Application::NODE_TYPE_TABLE, idParam: 'tableId')]
 	public function importInTable(int $tableId, string $path, bool $createMissingColumns = true): DataResponse {
 		try {
+			// minimal permission is checked, creating columns requires MANAGE permissions - currently tested on service layer
 			return new DataResponse($this->importService->import($tableId, null, $path, $createMissingColumns));
 		} catch (PermissionError $e) {
 			$this->logger->warning('A permission error occurred: ' . $e->getMessage(), ['exception' => $e]);
@@ -1393,8 +1403,10 @@ public function importInTable(int $tableId, string $path, bool $createMissingCol
 	 * 403: No permissions
 	 * 404: Not found
 	 */
+	#[RequirePermission(permission: Application::PERMISSION_CREATE, type: Application::NODE_TYPE_VIEW, idParam: 'viewId')]
 	public function importInView(int $viewId, string $path, bool $createMissingColumns = true): DataResponse {
 		try {
+			// minimal permission is checked, creating columns requires MANAGE permissions - currently tested on service layer
 			return new DataResponse($this->importService->import(null, $viewId, $path, $createMissingColumns));
 		} catch (PermissionError $e) {
 			$this->logger->warning('A permission error occurred: ' . $e->getMessage(), ['exception' => $e]);

lib/Controller/Errors.php

@@ -8,6 +8,7 @@
 namespace OCA\Tables\Controller;
 
 use Closure;
+use InvalidArgumentException;
 use OCA\Tables\Errors\InternalError;
 use OCA\Tables\Errors\NotFoundError;
 use OCA\Tables\Errors\PermissionError;
@@ -20,13 +21,17 @@ protected function handleError(Closure $callback): DataResponse {
 		try {
 			return new DataResponse($callback());
 		} catch (PermissionError $e) {
-			$this->logger->warning('A permission error accured: '.$e->getMessage(), ['exception' => $e]);
+			$this->logger->warning('A permission error occurred: '.$e->getMessage(), ['exception' => $e]);
 			$message = ['message' => $e->getMessage()];
 			return new DataResponse($message, Http::STATUS_FORBIDDEN);
 		} catch (NotFoundError $e) {
-			$this->logger->warning('A not found error accured: '.$e->getMessage(), ['exception' => $e]);
+			$this->logger->warning('A not found error occurred: ' . $e->getMessage(), ['exception' => $e]);
 			$message = ['message' => $e->getMessage()];
 			return new DataResponse($message, Http::STATUS_NOT_FOUND);
+		} catch (InvalidArgumentException $e) {
+			$this->logger->warning('An invalid request occurred: ' . $e->getMessage(), ['exception' => $e]);
+			$message = ['message' => $e->getMessage()];
+			return new DataResponse($message, Http::STATUS_BAD_REQUEST);
 		} catch (InternalError|\Exception $e) {
 			$this->logger->error('An internal error or exception occurred: '.$e->getMessage(), ['exception' => $e]);
 			$message = ['message' => $e->getMessage()];

lib/Controller/ImportController.php

@@ -8,6 +8,7 @@
 namespace OCA\Tables\Controller;
 
 use OCA\Tables\AppInfo\Application;
+use OCA\Tables\Middleware\Attribute\RequirePermission;
 use OCA\Tables\Service\ImportService;
 use OCA\Tables\UploadException;
 use OCP\AppFramework\Controller;
@@ -65,8 +66,10 @@ public function previewImportTable(int $tableId, String $path): DataResponse {
 	/**
 	 * @NoAdminRequired
 	 */
+	#[RequirePermission(permission: Application::PERMISSION_CREATE, type: Application::NODE_TYPE_TABLE, idParam: 'tableId')]
 	public function importInTable(int $tableId, String $path, bool $createMissingColumns = true, array $columnsConfig = []): DataResponse {
 		return $this->handleError(function () use ($tableId, $path, $createMissingColumns, $columnsConfig) {
+			// minimal permission is checked, creating columns requires MANAGE permissions - currently tested on service layer
 			return $this->service->import($tableId, null, $path, $createMissingColumns, $columnsConfig);
 		});
 	}
@@ -83,8 +86,10 @@ public function previewImportView(int $viewId, String $path): DataResponse {
 	/**
 	 * @NoAdminRequired
 	 */
+	#[RequirePermission(permission: Application::PERMISSION_CREATE, type: Application::NODE_TYPE_VIEW, idParam: 'viewId')]
 	public function importInView(int $viewId, String $path, bool $createMissingColumns = true, array $columnsConfig = []): DataResponse {
 		return $this->handleError(function () use ($viewId, $path, $createMissingColumns, $columnsConfig) {
+			// minimal permission is checked, creating columns requires MANAGE permissions - currently tested on service layer
 			return $this->service->import(null, $viewId, $path, $createMissingColumns, $columnsConfig);
 		});
 	}
@@ -107,11 +112,13 @@ public function previewUploadImportTable(int $tableId): DataResponse {
 	/**
 	 * @NoAdminRequired
 	 */
+	#[RequirePermission(permission: Application::PERMISSION_CREATE, type: Application::NODE_TYPE_TABLE, idParam: 'tableId')]
 	public function importUploadInTable(int $tableId, bool $createMissingColumns = true, string $columnsConfig = ''): DataResponse {
 		try {
 			$columnsConfigArray = json_decode($columnsConfig, true);
 			$file = $this->getUploadedFile('uploadfile');
 			return $this->handleError(function () use ($tableId, $file, $createMissingColumns, $columnsConfigArray) {
+				// minimal permission is checked, creating columns requires MANAGE permissions - currently tested on service layer
 				return $this->service->import($tableId, null, $file['tmp_name'], $createMissingColumns, $columnsConfigArray);
 			});
 		} catch (UploadException | NotPermittedException $e) {
@@ -138,11 +145,13 @@ public function previewUploadImportView(int $viewId): DataResponse {
 	/**
 	 * @NoAdminRequired
 	 */
+	#[RequirePermission(permission: Application::PERMISSION_CREATE, type: Application::NODE_TYPE_VIEW, idParam: 'viewId')]
 	public function importUploadInView(int $viewId, bool $createMissingColumns = true, string $columnsConfig = ''): DataResponse {
 		try {
 			$columnsConfigArray = json_decode($columnsConfig, true);
 			$file = $this->getUploadedFile('uploadfile');
 			return $this->handleError(function () use ($viewId, $file, $createMissingColumns, $columnsConfigArray) {
+				// minimal permission is checked, creating columns requires MANAGE permissions - currently tested on service layer
 				return $this->service->import(null, $viewId, $file['tmp_name'], $createMissingColumns, $columnsConfigArray);
 			});
 		} catch (UploadException | NotPermittedException $e) {

lib/Controller/RowController.php

@@ -63,22 +63,6 @@ public function show(int $id): DataResponse {
 		});
 	}
 
-	/**
-	 * @NoAdminRequired
-	 */
-	public function create(
-		?int $tableId,
-		?int $viewId,
-		array $data
-	): DataResponse {
-		return $this->handleError(function () use ($tableId, $viewId, $data) {
-			return $this->service->create(
-				$tableId,
-				$viewId,
-				$data);
-		});
-	}
-
 	/**
 	 * @NoAdminRequired
 	 */

lib/Model/RowDataInput.php

@@ -8,17 +8,23 @@
 namespace OCA\Tables\Model;
 
 use ArrayAccess;
+use Iterator;
+use function current;
+use function key;
+use function next;
+use function reset;
 
 /**
  * @template-implements ArrayAccess<mixed, array{'columnId': int, 'value': mixed}>
+ * @template-implements Iterator<mixed, array{'columnId': int, 'value': mixed}>
  */
-class RowDataInput implements ArrayAccess {
+class RowDataInput implements ArrayAccess, Iterator {
 	protected const DATA_KEY = 'columnId';
 	protected const DATA_VAL = 'value';
 	/** @psalm-var array<array{'columnId': int, 'value': mixed}> */
 	protected array $data = [];
 
-	public function add(int $columnId, string $value): self {
+	public function add(int $columnId, string|array $value): self {
 		$this->data[] = [self::DATA_KEY => $columnId, self::DATA_VAL => $value];
 		return $this;
 	}
@@ -45,4 +51,24 @@ public function offsetUnset(mixed $offset): void {
 			unset($this->data[$offset]);
 		}
 	}
+
+	public function current(): mixed {
+		return current($this->data);
+	}
+
+	public function next(): void {
+		next($this->data);
+	}
+
+	public function key(): mixed {
+		return key($this->data);
+	}
+
+	public function valid(): bool {
+		return $this->key() !== null;
+	}
+
+	public function rewind(): void {
+		reset($this->data);
+	}
 }

lib/Service/ImportService.php

@@ -187,8 +187,8 @@ private function getPreviewData(Worksheet $worksheet): array {
 	}
 
 	/**
-	 * @param int|null $tableId
-	 * @param int|null $viewId
+	 * @param ?int $tableId
+	 * @param ?int $viewId
 	 * @param string $path
 	 * @param bool $createMissingColumns
 	 * @return array
@@ -208,7 +208,8 @@ public function import(?int $tableId, ?int $viewId, string $path, bool $createMi
 				throw new PermissionError('create columns at the view id = '.$viewId.' is not allowed.');
 			}
 			$this->viewId = $viewId;
-		} elseif ($tableId) {
+		}
+		if ($tableId) {
 			$table = $this->tableService->find($tableId);
 			if (!$this->permissionsService->canCreateRows($table, 'table')) {
 				throw new PermissionError('create row at the view id = '. (string) $viewId .' is not allowed.');
@@ -217,11 +218,17 @@ public function import(?int $tableId, ?int $viewId, string $path, bool $createMi
 				throw new PermissionError('create columns at the view id = '. (string) $viewId .' is not allowed.');
 			}
 			$this->tableId = $tableId;
-		} else {
+		}
+		if (!$this->tableId && !$this->viewId) {
 			$e = new \Exception('Neither tableId nor viewId is given.');
 			$this->logger->error($e->getMessage(), ['exception' => $e]);
 			throw new InternalError(get_class($this) . ' - ' . __FUNCTION__ . ': '.$e->getMessage());
 		}
+		if ($this->tableId && $this->viewId) {
+			$e = new \LogicException('Both table ID and view ID are provided, but only one of them is allowed');
+			$this->logger->error($e->getMessage(), ['exception' => $e]);
+			throw new InternalError(get_class($this) . ' - ' . __FUNCTION__ . ': '.$e->getMessage());
+		}
 
 		if ($this->userId === null || $this->userManager->get($this->userId) === null) {
 			$error = 'No user in context, can not import data. Cancel.';

lib/Service/RowService.php

@@ -11,7 +11,6 @@
 use OCA\Tables\Db\ColumnMapper;
 use OCA\Tables\Db\Row2;
 use OCA\Tables\Db\Row2Mapper;
-use OCA\Tables\Db\Table;
 use OCA\Tables\Db\TableMapper;
 use OCA\Tables\Db\View;
 use OCA\Tables\Db\ViewMapper;
@@ -192,7 +191,8 @@ public function create(?int $tableId, ?int $viewId, RowDataInput|array $data): R
 			}
 
 			$columns = $this->columnMapper->findMultiple($view->getColumnsArray());
-		} elseif ($tableId) {
+		}
+		if ($tableId) {
 			try {
 				$table = $this->tableMapper->find($tableId);
 			} catch (DoesNotExistException $e) {
@@ -209,7 +209,9 @@ public function create(?int $tableId, ?int $viewId, RowDataInput|array $data): R
 			}
 
 			$columns = $this->columnMapper->findAllByTable($tableId);
-		} else {
+		}
+
+		if (!$viewId && !$tableId) {
 			throw new InternalError('Cannot create row without table or view in context');
 		}
 

lib/Service/ViewService.php

@@ -11,13 +11,12 @@
 
 use DateTime;
 use Exception;
-
+use InvalidArgumentException;
 use OCA\Tables\AppInfo\Application;
+use OCA\Tables\Db\Column;
 use OCA\Tables\Db\Table;
 use OCA\Tables\Db\View;
 use OCA\Tables\Db\ViewMapper;
-
-
 use OCA\Tables\Errors\InternalError;
 use OCA\Tables\Errors\NotFoundError;
 use OCA\Tables\Errors\PermissionError;
@@ -74,7 +73,6 @@ public function __construct(
 		$this->contextService = $contextService;
 	}
 
-
 	/**
 	 * @param Table $table
 	 * @param string|null $userId
@@ -246,6 +244,7 @@ public function updateSingle(int $id, string $key, ?string $value, ?string $user
 	 * @return View
 	 * @throws InternalError
 	 * @throws PermissionError
+	 * @throws InvalidArgumentException
 	 */
 	public function update(int $id, array $data, ?string $userId = null, bool $skipTableEnhancement = false): View {
 		$userId = $this->permissionsService->preCheckUserId($userId);
@@ -255,26 +254,42 @@ public function update(int $id, array $data, ?string $userId = null, bool $skipT
 
 			// security
 			if (!$this->permissionsService->canManageView($view, $userId)) {
-				throw new PermissionError('PermissionError: can not update view with id '.$id);
+				throw new PermissionError('PermissionError: can not update view with id ' . $id);
 			}
 
 			$updatableParameter = ['title', 'emoji', 'description', 'columns', 'sort', 'filter'];
 
 			foreach ($data as $key => $value) {
 				if (!in_array($key, $updatableParameter)) {
-					throw new InternalError('View parameter '.$key.' can not be updated.');
+					throw new InternalError('View parameter ' . $key . ' can not be updated.');
 				}
-				$setterMethod = 'set'.ucfirst($key);
+
+				if ($key === 'columns') {
+					// we have to fetch the service here as ColumnService already depends on the ViewService, i.e. no DI
+					$columnService = \OCP\Server::get(ColumnService::class);
+					$columnIds = \json_decode($value, true);
+					$availableColumns = $columnService->findAllByTable($view->getTableId(), $view->getId(), $this->userId);
+					$availableColumns = array_map(static fn (Column $column) => $column->getId(), $availableColumns);
+					foreach ($columnIds as $columnId) {
+						if (!in_array($columnId, $availableColumns, true)) {
+							throw new InvalidArgumentException('Invalid column ID provided');
+						}
+					}
+				}
+
+				$setterMethod = 'set' . ucfirst($key);
 				$view->$setterMethod($value);
 			}
 			$time = new DateTime();
 			$view->setLastEditBy($userId);
 			$view->setLastEditAt($time->format('Y-m-d H:i:s'));
 			$view = $this->mapper->update($view);
-			if(!$skipTableEnhancement) {
+			if (!$skipTableEnhancement) {
 				$this->enhanceView($view, $userId);
 			}
 			return $view;
+		} catch (InvalidArgumentException $e) {
+			throw $e;
 		} catch (Exception $e) {
 			$this->logger->error($e->getMessage(), ['exception' => $e]);
 			throw new InternalError($e->getMessage());