diff --git a/lib/Controller/ConvertController.php b/lib/Controller/ConvertController.php index 010c9cc..68ee2b1 100644 --- a/lib/Controller/ConvertController.php +++ b/lib/Controller/ConvertController.php @@ -23,13 +23,16 @@ namespace OCA\DocumentServer\Controller; +use OCA\DocumentServer\Document\DocumentConversionException; use OCA\DocumentServer\Document\DocumentStore; +use OCA\DocumentServer\Document\PasswordRequiredException; use OCA\DocumentServer\OnlyOffice\URLDecoder; use OCA\DocumentServer\JSONResponse; use OCP\AppFramework\Controller; use OCP\AppFramework\Http\Attribute\NoAdminRequired; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; use OCP\AppFramework\Http\Attribute\PublicPage; +use OCP\IRequest; use OCP\IURLGenerator; class ConvertController extends Controller { @@ -37,7 +40,14 @@ class ConvertController extends Controller { private $urlDecoder; private $urlGenerator; - public function __construct(DocumentStore $documentStore, URLDecoder $urlDecoder, IURLGenerator $urlGenerator) { + public function __construct( + string $appName, + IRequest $request, + DocumentStore $documentStore, + URLDecoder $urlDecoder, + IURLGenerator $urlGenerator + ) { + parent::__construct($appName, $request); $this->documentStore = $documentStore; $this->urlDecoder = $urlDecoder; $this->urlGenerator = $urlGenerator; @@ -57,8 +67,22 @@ public function convert(bool $async, string $url, string $outputtype, string $fi } else { $documentId = (int)$key; $documentFile = $this->urlDecoder->getFileForUrl($url); - $this->documentStore->getDocumentForEditor($documentId, $documentFile, $filetype); - $this->documentStore->convert($documentId, $outputtype); + + // A failed conversion must come back as a structured {"error": } + // body, not as an uncaught exception: this is a plain HTTP endpoint + // with no other handler, so without this catch the converter's + // exception escapes as a 500 the caller can't interpret. This endpoint + // answers the connector over the server-side conversion API, whose + // error codes differ from the editor's c_oAscError space used in the + // socket replies: -5 incorrect password, -3 conversion error. + try { + $this->documentStore->getDocumentForEditor($documentId, $documentFile, $filetype); + $this->documentStore->convert($documentId, $outputtype); + } catch (PasswordRequiredException $e) { + return new JSONResponse(['error' => -5]); + } catch (DocumentConversionException $e) { + return new JSONResponse(['error' => -3]); + } $url = $this->urlGenerator->linkToRouteAbsolute( 'documentserver_community.Document.documentFile', [ diff --git a/lib/Controller/DocumentController.php b/lib/Controller/DocumentController.php index e1db263..deaa0f6 100644 --- a/lib/Controller/DocumentController.php +++ b/lib/Controller/DocumentController.php @@ -187,7 +187,47 @@ public function upload(int $docId, string $index) { public function download(int $docId, string $cmd) { $cmd = json_decode($cmd, true); $content = fopen('php://input', 'r'); - $title = $this->documentStore->convertForDownload($docId, $content, $cmd); + + // A failed save-as conversion must be reported to the editor as a 'save' + // error, not escape as an uncaught 500: this endpoint has no other + // handler, and the editor is waiting on a documentOpen 'save' reply. Tell + // the session it failed (-4 = sdkjs DownloadError) so the user sees a + // failed download instead of a silent hang, then return the same error to + // the HTTP caller. The reply push is itself guarded: if the IPC backend is + // what failed, getChannel() would throw and re-raise an uncaught 500. + try { + $title = $this->documentStore->convertForDownload($docId, $content, $cmd); + } catch (\Exception $e) { + $this->logger->warning('documentserver download conversion error: {error}', [ + 'error' => $e->getMessage(), + 'exception' => $e, + ]); + + try { + $session = $this->sessionManager->getSessionForUser($cmd['userconnectionid']); + if ($session) { + $this->ipcFactory->getChannel($session->getSessionId())->pushMessage(json_encode([ + 'type' => 'documentOpen', + 'data' => [ + 'type' => 'save', + 'status' => 'err', + 'data' => -4, + ], + ])); + } + } catch (\Exception $replyError) { + $this->logger->warning('documentserver download could not deliver error reply: {error}', [ + 'error' => $replyError->getMessage(), + 'exception' => $replyError, + ]); + } + + return new DataResponse([ + 'type' => 'save', + 'status' => 'err', + 'data' => -4, + ]); + } $session = $this->sessionManager->getSessionForUser($cmd['userconnectionid']); if ($session) { @@ -361,6 +401,35 @@ private function handleEngineIOPacket(string $packet, string $sid, string $docum 'error' => $e->getMessage(), 'exception' => $e, ]); + + // Don't let a failed command (e.g. the document failing to + // open) disappear into the log: with no reply the editor stays + // on "Loading document" forever. Push a documentOpen error so + // the client surfaces it instead of spinning. Same message shape + // the client already handles for the password case in + // OpenDocument::openDocument; sdkjs parses data as a + // c_oAscError.ID and shows the matching error, -4 is + // DownloadError ("download failed"), the closest generic + // open-failure code. Guard the reply itself: if the original + // failure was the IPC backend being unavailable, getChannel() + // throws too, and an unguarded re-throw here would turn the + // swallowed error back into an uncaught 500. + try { + $sessionChannel = $this->ipcFactory->getChannel($sid); + $sessionChannel->pushMessage(json_encode([ + 'type' => 'documentOpen', + 'data' => [ + 'type' => 'open', + 'status' => 'err', + 'data' => -4, + ], + ])); + } catch (\Exception $replyError) { + $this->logger->warning('documentserver socketIO could not deliver error reply: {error}', [ + 'error' => $replyError->getMessage(), + 'exception' => $replyError, + ]); + } } } // sio types '0' (CONNECT) and '1' (DISCONNECT) require no action here. diff --git a/lib/Document/ConverterBinary.php b/lib/Document/ConverterBinary.php index 47a44d2..5e55b29 100644 --- a/lib/Document/ConverterBinary.php +++ b/lib/Document/ConverterBinary.php @@ -53,6 +53,14 @@ public function run(string $param, string $password = null): string { } $process = proc_open($cmd, $descriptorSpec, $pipes, self::BINARY_DIRECTORY, ["LD_LIBRARY_PATH" => "."]); + // proc_open returns false if x2t couldn't be spawned at all (missing + // binary, fork failure). Without this the next lines fclose/read null + // pipes and proc_close(false) warns, and the caller silently gets no + // output instead of a clear failure. + if ($process === false) { + throw new DocumentConversionException("failed to start x2t"); + } + @fclose($pipes[0]); $output = @stream_get_contents($pipes[1]); $error = @stream_get_contents($pipes[2]); @@ -65,9 +73,18 @@ public function run(string $param, string $password = null): string { if ($error) { throw new DocumentConversionException($error); - } else { - return $output; } + + // x2t can fail with a nonzero exit status while writing nothing to stderr; + // without this the caller treats a failed conversion as success and later + // trips over the missing Editor.bin, see #70. Stderr is checked first so its + // (more specific) message wins, including the "Empty sFileFrom or sFileTo" + // string that test() relies on. + if ($status !== 0) { + throw new DocumentConversionException("x2t exited with status $status"); + } + + return $output; } public function test(): bool { diff --git a/lib/LocalAppData.php b/lib/LocalAppData.php index be191e9..9a1bd72 100644 --- a/lib/LocalAppData.php +++ b/lib/LocalAppData.php @@ -23,6 +23,7 @@ namespace OCA\DocumentServer; +use OCP\Files\Cache\IScanner; use OCP\Files\Folder; use OCP\Files\File; use OCP\Files\IAppData; @@ -30,6 +31,8 @@ use OCP\Files\SimpleFS\ISimpleFolder; use OCP\IConfig; use OCP\ITempManager; +use OCP\Lock\LockedException; +use Psr\Log\LoggerInterface; /** * Provide local access to appdata folders @@ -38,15 +41,18 @@ class LocalAppData { private $appData; private $config; private $tmpManager; + private LoggerInterface $logger; public function __construct( IAppData $appData, IConfig $config, - ITempManager $tmpManager + ITempManager $tmpManager, + LoggerInterface $logger ) { $this->appData = $appData; $this->config = $config; $this->tmpManager = $tmpManager; + $this->logger = $logger; } /** @@ -100,6 +106,33 @@ public function getReadWriteLocalPath(ISimpleFolder $folder, callable $callback) if (is_dir($localPath)) { $callback($localPath); + // The callback (e.g. x2t producing Editor.bin) writes straight to the + // on-disk appdata folder, bypassing the Nextcloud storage layer, so the + // file cache never learns about the new files and a later getFile() would + // throw NotFoundException. Rescan the folder so the writes become visible, + // see #70 (editor stuck on "Loading document" because Editor.bin is on disk + // but absent from oc_filecache). The temp-folder branch below does not need + // this because copyFromLocal() writes back through File::putContent(), + // which updates the cache itself. The scan must be RECURSIVE: x2t emits + // not just Editor.bin but a media/ subfolder of embedded images, and + // the open flow reads those back through the file API + // (DocumentStore::getEmbeddedFiles -> media/.getDirectoryListing()). A + // shallow scan would register Editor.bin but leave media/'s children + // uncached, so embedded images would silently fail to load. A + // concurrent writer can hold the scan lock; mirror NC core's + // backgroundScan and skip rather than fail the open, the next access + // rescans. + try { + $fullFolder->getStorage()->getScanner()->scan( + $fullFolder->getInternalPath(), + IScanner::SCAN_RECURSIVE + ); + } catch (LockedException $e) { + $this->logger->warning( + 'documentserver: appdata rescan after conversion skipped, folder locked', + ['exception' => $e, 'app' => 'documentserver_community'] + ); + } } else { $localPath = $this->tmpManager->getTemporaryFolder(); $this->copyToLocal($fullFolder, $localPath);