Skip to content

feat: add last modified information to response #1844

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
blizzz wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

feat: add last modified information to response #1844

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 6 additions & 1 deletion lib/Controller/Api1Controller.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1146,7 +1146,12 @@ public function indexTableRowsSimple(int $tableId, ?int $limit, ?int $offset): D
#[OpenAPI(scope: OpenAPI::SCOPE_DEFAULT)]
public function indexTableRows(int $tableId, ?int $limit, ?int $offset): DataResponse {
try {
return new DataResponse($this->rowService->formatRows($this->rowService->findAllByTable($tableId, $this->userId, $limit, $offset)));
$rows = $this->rowService->findAllByTable($tableId, $this->userId, $limit, $offset);
$response = new DataResponse($this->rowService->formatRows($rows));
$table = $this->tableService->find($tableId);
Copy link
Member

@juliusknorr juliusknorr Jun 20, 2025

Choose a reason for hiding this comment

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

We could move this line first and return a 304 early in case the if-modified-since matches, before doing the possibly heavier operations of fetching the rows

Copy link
Member Author

@blizzz blizzz Jun 20, 2025

Choose a reason for hiding this comment

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

Yes, definitely. First I thought i had to analyze the rows that were fetched, then i was reminded about the last modificated set in the table (if that really works as i think). I pushed this to not have it rotting away locally and turning to legend in first place 🙂

$lastModified = new \DateTime($table->getLastEditAt());
Copy link
Member

@juliusknorr juliusknorr Jun 20, 2025

Choose a reason for hiding this comment

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

Are we sure that any change of rows would update the getLastEditAt date? Otherwise this would not have much benefit i think.

Copy link
Member Author

@blizzz blizzz Jun 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I did not double check it yet, it is merely my expectation. I was wondering whether this is also updated, when a row is being deleted.

$response->setLastModified($lastModified);
return $response;
} catch (PermissionError $e) {
$this->logger->warning('A permission error occurred: ' . $e->getMessage(), ['exception' => $e]);
$message = ['message' => $e->getMessage()];
Expand Down
Loading