Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/Controller/RowOCSController.php
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ public function createRow(string $nodeCollection, int $nodeId, mixed $data): Dat
}

try {
return new DataResponse($this->rowService->create($tableId, $viewId, $newRowData)->jsonSerialize());
return new DataResponse($this->rowService->create($tableId, $viewId, $newRowData)->toResponseArray());
} catch (BadRequestError $e) {
return $this->handleBadRequestError($e);
} catch (NotFoundError $e) {
Expand Down
36 changes: 36 additions & 0 deletions lib/Db/Row2.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ class Row2 implements JsonSerializable {
private ?string $lastEditBy = null;
private ?string $lastEditAt = null;
private ?array $data = [];
private array $cellMeta = [];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should need to be added to a VIRTUAL_PROPERTIES const, otherwise the Mapper will complain. Have a look at https://github.com/nextcloud/tables/blob/main/lib/Db/Column.php#L161C2-L161C114 for comparison.

Also, the name is too broad and not specific (same problem with data already…), but right now I am also missing a better suggestion.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ill rename it cellMetadata as the array holds metadata for each cell.

private array $changedColumnIds = []; // collect column ids that have changed after $loaded = true

private bool $loaded = false; // set to true if model is loaded, after that changed column ids will be collected
Expand Down Expand Up @@ -133,6 +134,16 @@ public function filterDataByColumns(array $columns): array {
return $this->data;
}

/**
* add response-only metadata for a specific column
*/
public function addCellMeta(int $columnId, array $meta): void {
if (!isset($this->cellMeta[$columnId])) {
$this->cellMeta[$columnId] = [];
}
$this->cellMeta[$columnId] = array_merge($this->cellMeta[$columnId], $meta);
}

/**
* @psalm-return TablesRow
*/
Expand All @@ -148,6 +159,31 @@ public function jsonSerialize(): array {
];
}

/**
* return merged response-only metadata into the data cells.
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method names says this already, no need to duplicate :)

What we want is however a @return annotation with the array shape. Or @psalm-return as done at jsonSerialize(), which itself would need to be adjusted afterwards.

Did you double check, cannot we only extend jsonSerialize() instead of adding a similar method?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was a bit hesitant on using 'jsonSerialize' as i thought i might interfere in another place, but did more testing and it's fine.

public function toResponseArray(): array {
$data = [];
foreach ($this->data as $cell) {
$colId = $cell['columnId'];
$merged = $cell;
if (isset($this->cellMeta[$colId])) {
$merged = array_merge($merged, $this->cellMeta[$colId]);
}
$data[] = $merged;
}

return [
'id' => $this->id,
'tableId' => $this->tableId,
'createdBy' => $this->createdBy,
'createdAt' => $this->createdAt,
'lastEditBy' => $this->lastEditBy,
'lastEditAt' => $this->lastEditAt,
'data' => $data,
];
}

public function toPublicRow(?array $previousValues = null): Row {
return new Row(
tableId: $this->tableId,
Expand Down
30 changes: 29 additions & 1 deletion lib/Service/RowService.php
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,28 @@ public function create(?int $tableId, ?int $viewId, RowDataInput|array $data): R
throw new InternalError('Cannot create row without table or view in context');
}

$fullRowData = [];
$columnNames = [];

foreach ($columns as $c) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
foreach ($columns as $c) {
foreach ($columns as $column) {

Please don't save bytes where it is not necessary :)

$columnNames[$c->getId()] = $c->getTitle();
}

$rows = $data instanceof RowDataInput ? iterator_to_array($data) : $data;

foreach ($rows as $r) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

$colId = (int)$r['columnId'];
if (!isset($columnNames[$colId])) {
continue;
}

$fullRowData[] = [
'columnId' => $colId,
'value' => $r['value'],
'columnName' => $columnNames[$colId],
];
}

$tableId = $tableId ?? $view->getTableId();

$data = $data instanceof RowDataInput ? $data : RowDataInput::fromArray($data);
Expand All @@ -220,6 +242,12 @@ public function create(?int $tableId, ?int $viewId, RowDataInput|array $data): R
$row2->setData($data);
try {
$insertedRow = $this->row2Mapper->insert($row2);
// attached columnname to returned row for the response only
foreach ($fullRowData as $meta) {
if (isset($meta['columnId']) && array_key_exists('columnName', $meta)) {
$insertedRow->addCellMeta((int)$meta['columnId'], ['columnName' => $meta['columnName']]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we generate it in Row2 instead? data is populated there. Having all logic in Row2 would capsulate it there and not leak logic to other classes around.

}
}

$this->eventDispatcher->dispatchTyped(new RowAddedEvent($insertedRow));
$this->activityManager->triggerEvent(
Expand Down Expand Up @@ -334,7 +362,7 @@ private function cleanupAndValidateData(RowDataInput $data, array $columns, ?int
if (!$column && $viewId) {
throw new InternalError('Column with id ' . $entry['columnId'] . ' is not part of view with id ' . $viewId);
} elseif (!$column && $tableId) {
throw new InternalError('Column with id ' . $entry['columnId'] . ' is not part of table with id ' . $tableId);
throw new BadRequestError('Column with id ' . $entry['columnId'] . ' is not part of table with id ' . $tableId);
}

if (!$column) {
Expand Down
2 changes: 1 addition & 1 deletion phpunit.xml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
- SPDX-FileCopyrightText: 2021 Florian Steffens
- SPDX-License-Identifier: AGPL-3.0-or-later
-->
<phpunit bootstrap="tests/bootstrap.php" colors="true">
<phpunit bootstrap="tests/Unit/bootstrap.php" colors="true">
<testsuites>
<testsuite name="unit">
<directory>./tests/Unit</directory>
Expand Down
32 changes: 32 additions & 0 deletions tests/unit/Db/Row2Test.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?php

declare(strict_types=1);

namespace OCA\Tables\Tests\Unit\Db;

use OCA\Tables\Db\Row2;
use PHPUnit\Framework\TestCase;

class Row2Test extends TestCase {
public function testToResponseArrayMergesMetaButJsonSerializeDoesNot(): void {
$row = new Row2();
$row->setTableId(1);

$row->setData([
['columnId' => 57, 'value' => 'foo'],
['columnId' => 58, 'value' => 'bar'],
]);

$json = $row->jsonSerialize();
$this->assertArrayNotHasKey('columnName', $json['data'][0]);

$row->addCellMeta(57, ['columnName' => 'Title 57']);

$resp = $row->toResponseArray();
$this->assertArrayHasKey('columnName', $resp['data'][0]);
$this->assertSame('Title 57', $resp['data'][0]['columnName']);

$json2 = $row->jsonSerialize();
$this->assertArrayNotHasKey('columnName', $json2['data'][0]);
}
}
Loading