fix(context): do not return document from updateDocument

Changing the object and returning it is confusing.
Use `$document->getUpdatedFields()` to determine what changed.

Signed-off-by: Max <max@nextcloud.com>

Author: max-nextcloud

lib/Context/FileContext.php

@@ -143,22 +143,21 @@ public function isReadOnly(): bool {
 	 * @throws GenericFileException if the file changed and reading the content fails.
 	 * @throws LockedException if the file changed and a lock prevents reading the content.
 	 * @throws NotPermittedException if the file changed and reading is not allowed.
-	 * @return Document|null Updated document if there was an update
 	 */
-	public function updateDocument(Document $document): ?Document {
+	public function updateDocument(Document $document): void {
 		$lastMTime = $document->getLastSavedVersionTime();
 		$lastEtag = $document->getLastSavedVersionEtag();
 
 		if ($this->isReadOnly()) {
-			return null;
+			return;
 		}
 
 		$file = $this->getFile();
 		$fileMtime = $file->getMtime();
 		$fileEtag = $file->getEtag();
 
 		if ($lastEtag === $fileEtag && $lastMTime === $fileMtime) {
-			return null;
+			return;
 		}
 
 		$fileContent = $file->getContent();
@@ -167,7 +166,6 @@ public function updateDocument(Document $document): ?Document {
 		$document->setChecksum($fileChecksum);
 		$document->setLastSavedVersionTime($fileMtime);
 		$document->setLastSavedVersionEtag($fileEtag);
-		return $document;
 	}
 
 	public function loadContent(): ?string {

lib/Context/IContext.php

@@ -25,7 +25,7 @@ public function toString(): string;
 	public function buildDocument(): Document;
 	public function prepareSession(DocumentData $documentData): SessionInfo;
 	public function isReadOnly(): bool;
-	public function updateDocument(Document $document): ?Document;
+	public function updateDocument(Document $document): void;
 	/**
 	 * @throws NotFoundException
 	 * @throws NotPermittedException

lib/Context/UnauthorizedFileContext.php

@@ -72,9 +72,8 @@ public function isReadOnly(): bool {
 	 * @throws GenericFileException if the file changed and reading the content fails.
 	 * @throws LockedException if the file changed and a lock prevents reading the content.
 	 * @throws NotPermittedException if the file changed and reading is not allowed.
-	 * @return Document|null Updated document if there was an update
 	 */
-	public function updateDocument(Document $document): ?Document {
+	public function updateDocument(Document $document): void {
 		$lastMTime = $document->getLastSavedVersionTime();
 		$lastEtag = $document->getLastSavedVersionEtag();
 
@@ -83,7 +82,7 @@ public function updateDocument(Document $document): ?Document {
 		$fileEtag = $file->getEtag();
 
 		if ($lastEtag === $fileEtag && $lastMTime === $fileMtime) {
-			return null;
+			return;
 		}
 
 		$fileContent = $file->getContent();
@@ -92,7 +91,7 @@ public function updateDocument(Document $document): ?Document {
 		$document->setChecksum($fileChecksum);
 		$document->setLastSavedVersionTime($fileMtime);
 		$document->setLastSavedVersionEtag($fileEtag);
-		return $document;
+		return;
 	}
 
 	public function loadContent(): ?string {

lib/Listeners/DocumentContentUpdatedListener.php

@@ -43,24 +43,26 @@ public function handle(Event $event): void {
 		}
 
 		$document = $this->documentMapper->load($context->getType(), $context->getId());
-		if (!$document || $this->documentService->isSaveFromText()) {
-			$this->logger->debug('No document or event triggered by text itself', ['document' => $document]);
+		if (!$document) {
+			$this->logger->debug('No document for context.', ['context' => $context->toString()]);
 			return;
 		}
 
-		$oldChecksum = $document->getChecksum();
-		$updatedDocument = $context->updateDocument($document);
-		if (!$updatedDocument) {
-			$this->logger->info('Nothing changed', ['document' => $document]);
-			// nothing changed.
+		if ($this->documentService->isSaveFromText()) {
+			$this->logger->debug('DocumentContentUpdated triggered by text itself', ['document' => $document->jsonSerialize()]);
 			return;
 		}
 
-		$newChecksum = $updatedDocument->getChecksum();
-		if ($oldChecksum !== null && $newChecksum !== null && $oldChecksum === $newChecksum) {
-			// Same content: no need to reset document session. Still update document mtime and etag as they might have changed
-			$this->logger->info('Same checksum', ['document' => $document->jsonSerialize(), 'updated' => $updatedDocument->jsonSerialize()]);
-			$this->documentMapper->update($updatedDocument);
+		$context->updateDocument($document);
+		if (empty($document->getUpdatedFields())) {
+			$this->logger->debug('Nothing changed', ['document' => $document->jsonSerialize()]);
+			return;
+		}
+
+		if (!isset($document->getUpdatedFields()['checksum'])) {
+			// Same content: no need to reset document session. Still update document mtime and etag.
+			$this->logger->debug('DocumentContentUpdated with same checksum', ['document' => $document->jsonSerialize()]);
+			$this->documentMapper->update($document);
 			return;
 		}
 

lib/Service/DocumentService.php

@@ -355,16 +355,15 @@ public function autosave(Document $document, IContext $context, int $version, st
 
 		$lastMTime = $document->getLastSavedVersionTime();
 		if ($lastMTime > 0 && !$force && !$this->cache->get('document-save-lock-' . $document->id)) {
-			$updatedDocument = $context->updateDocument($document);
-			if ($updatedDocument !== null) {
+			$context->updateDocument($document);
+			if (!empty($document->getUpdatedFields())) {
 				// Content was overwritten in the meantime and content changed.
-				if ($updatedDocument->getChecksum() !== $document->getChecksum()) {
+				if (isset($document->getUpdatedFields()['checksum'])) {
 					$content = $context->loadContent();
 					if ($content !== null) {
 						throw new DocumentSaveConflictException($content);
 					}
 				}
-				$document = $updatedDocument;
 				$lastMTime = $document->getLastSavedVersionTime();
 				$this->documentMapper->update($document);
 			}