From e3dc9681bacd79ba8bf8f8e1e3f81951e367964a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9o=20Jan?= Date: Mon, 26 Oct 2026 20:15:43 +0100 Subject: [PATCH 1/6] feat: enhance import logging and error reporting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add structured logging throughout the recipe import pipeline: - Tag ImportExceptions with a step property (download/parse_html/extract_recipe) - Log per-parser failures with reasons in RecipeExtractionService - Add debug logs in HTML parsers (JSON-LD element count, decode errors, microdata itemscope count) - Log HTTP status, content-type, and response size after download - Return structured {msg, step} error responses from the import API - Remove file/line leak from all error responses (import, update, create) - Catch generic exceptions as HTTP 500 with safe translated message Assisted-by: Claude Code (claude-opus-4-6) Co-Authored-By: Claude Opus 4.6 Signed-off-by: Léo Jan --- .../Implementation/RecipeImplementation.php | 67 +++++-------------- lib/Exception/ImportException.php | 9 ++- lib/Helper/HTMLParser/AbstractHtmlParser.php | 6 +- lib/Helper/HTMLParser/HttpJsonLdParser.php | 14 +++- lib/Helper/HTMLParser/HttpMicrodataParser.php | 10 ++- lib/Helper/HtmlToDomParser.php | 4 +- lib/Service/HtmlDownloadService.php | 16 +++-- lib/Service/RecipeExtractionService.php | 22 +++++- lib/Service/RecipeService.php | 8 ++- src/components/AppNavi.vue | 23 +++---- .../RecipeImplementationTest.php | 19 ++---- .../HTMLParser/HttpJsonLdParserTest.php | 11 ++- .../HTMLParser/HttpMicrodataParserTest.php | 20 ++++-- .../Service/RecipeExtractionServiceTest.php | 4 +- 14 files changed, 126 insertions(+), 107 deletions(-) diff --git a/lib/Controller/Implementation/RecipeImplementation.php b/lib/Controller/Implementation/RecipeImplementation.php index a672e5d59..90a867002 100644 --- a/lib/Controller/Implementation/RecipeImplementation.php +++ b/lib/Controller/Implementation/RecipeImplementation.php @@ -7,6 +7,7 @@ namespace OCA\Cookbook\Controller\Implementation; use Exception; +use OCA\Cookbook\Exception\ImportException; use OCA\Cookbook\Exception\InvalidDownloadURLException; use OCA\Cookbook\Exception\InvalidJSONFileException; use OCA\Cookbook\Exception\NoRecipeNameGivenException; @@ -133,21 +134,9 @@ public function update($id) { try { $file = $this->service->addRecipe($recipeData); } catch (RecipeExistsException $ex) { - $json = [ - 'msg' => $ex->getMessage(), - 'file' => $ex->getFile(), - 'line' => $ex->getLine(), - ]; - - return new JSONResponse($json, Http::STATUS_CONFLICT); + return new JSONResponse(['msg' => $ex->getMessage()], Http::STATUS_CONFLICT); } catch (NoRecipeNameGivenException $ex) { - $json = [ - 'msg' => $ex->getMessage(), - 'file' => $ex->getFile(), - 'line' => $ex->getLine(), - ]; - - return new JSONResponse($json, Http::STATUS_UNPROCESSABLE_ENTITY); + return new JSONResponse(['msg' => $ex->getMessage()], Http::STATUS_UNPROCESSABLE_ENTITY); } try { @@ -160,13 +149,7 @@ public function update($id) { $this->logger->warning('Cannot remove file after failed parsing: {ex}', ['ex' => $ex]); } - $json = [ - 'msg' => $ex->getMessage(), - 'file' => $ex->getFile(), - 'line' => $ex->getLine(), - ]; - - return new JSONResponse($json, Http::STATUS_UNPROCESSABLE_ENTITY); + return new JSONResponse(['msg' => $ex->getMessage()], Http::STATUS_UNPROCESSABLE_ENTITY); } return new JSONResponse($file->getParent()->getId(), Http::STATUS_OK); @@ -184,21 +167,9 @@ public function create() { try { $file = $this->service->addRecipe($recipeData); } catch (RecipeExistsException $ex) { - $json = [ - 'msg' => $ex->getMessage(), - 'file' => $ex->getFile(), - 'line' => $ex->getLine(), - ]; - - return new JSONResponse($json, Http::STATUS_CONFLICT); + return new JSONResponse(['msg' => $ex->getMessage()], Http::STATUS_CONFLICT); } catch (NoRecipeNameGivenException $ex) { - $json = [ - 'msg' => $ex->getMessage(), - 'file' => $ex->getFile(), - 'line' => $ex->getLine(), - ]; - - return new JSONResponse($json, Http::STATUS_UNPROCESSABLE_ENTITY); + return new JSONResponse(['msg' => $ex->getMessage()], Http::STATUS_UNPROCESSABLE_ENTITY); } try { @@ -211,13 +182,7 @@ public function create() { $this->logger->warning('Cannot remove file after failed parsing: {ex}', ['ex' => $ex]); } - $json = [ - 'msg' => $ex->getMessage(), - 'file' => $ex->getFile(), - 'line' => $ex->getLine(), - ]; - - return new JSONResponse($json, Http::STATUS_UNPROCESSABLE_ENTITY); + return new JSONResponse(['msg' => $ex->getMessage()], Http::STATUS_UNPROCESSABLE_ENTITY); } return new JSONResponse($file->getParent()->getId(), Http::STATUS_OK); @@ -345,15 +310,17 @@ public function import() { $recipe_json = $this->service->parseRecipeFile($recipe_file); } catch (RecipeExistsException $ex) { - $json = [ - 'msg' => $ex->getMessage(), - 'line' => $ex->getLine(), - 'file' => $ex->getFile(), - ]; - - return new JSONResponse($json, Http::STATUS_CONFLICT); + return new JSONResponse(['msg' => $ex->getMessage()], Http::STATUS_CONFLICT); + } catch (ImportException $e) { + $this->logger->warning('Import failed at step "{step}": {reason}', [ + 'url' => $data['url'], 'step' => $e->getStep(), 'reason' => $e->getMessage(), 'exception' => $e, + ]); + return new JSONResponse(['msg' => $e->getMessage(), 'step' => $e->getStep()], Http::STATUS_BAD_REQUEST); } catch (\Exception $e) { - return new JSONResponse($e->getMessage(), 400); + $this->logger->error('Unexpected error during import: {reason}', [ + 'url' => $data['url'], 'reason' => $e->getMessage(), 'exception' => $e, + ]); + return new JSONResponse(['msg' => $this->l->t('An unexpected error occurred during import.')], Http::STATUS_INTERNAL_SERVER_ERROR); } try { diff --git a/lib/Exception/ImportException.php b/lib/Exception/ImportException.php index 1d7a4a9c3..e084d4a54 100644 --- a/lib/Exception/ImportException.php +++ b/lib/Exception/ImportException.php @@ -7,7 +7,14 @@ namespace OCA\Cookbook\Exception; class ImportException extends \Exception { - public function __construct($message = '', $code = 0, $previous = null) { + private string $step; + + public function __construct($message = '', $code = 0, $previous = null, string $step = '') { parent::__construct($message, $code, $previous); + $this->step = $step; + } + + public function getStep(): string { + return $this->step; } } diff --git a/lib/Helper/HTMLParser/AbstractHtmlParser.php b/lib/Helper/HTMLParser/AbstractHtmlParser.php index 9043154a2..406a4b3bd 100644 --- a/lib/Helper/HTMLParser/AbstractHtmlParser.php +++ b/lib/Helper/HTMLParser/AbstractHtmlParser.php @@ -8,6 +8,7 @@ use OCA\Cookbook\Exception\HtmlParsingException; use OCP\IL10N; +use Psr\Log\LoggerInterface; abstract class AbstractHtmlParser { /** @@ -15,8 +16,11 @@ abstract class AbstractHtmlParser { */ protected $l; - public function __construct(IL10N $l10n) { + protected LoggerInterface $logger; + + public function __construct(IL10N $l10n, LoggerInterface $logger) { $this->l = $l10n; + $this->logger = $logger; } /** diff --git a/lib/Helper/HTMLParser/HttpJsonLdParser.php b/lib/Helper/HTMLParser/HttpJsonLdParser.php index 27de6567c..cd36c9f86 100644 --- a/lib/Helper/HTMLParser/HttpJsonLdParser.php +++ b/lib/Helper/HTMLParser/HttpJsonLdParser.php @@ -9,6 +9,7 @@ use OCA\Cookbook\Exception\HtmlParsingException; use OCA\Cookbook\Service\JsonService; use OCP\IL10N; +use Psr\Log\LoggerInterface; /** * This class is an AbstractHtmlParser which tries to extract a JSON+LD script from the HTML page. @@ -20,8 +21,8 @@ class HttpJsonLdParser extends AbstractHtmlParser { */ private $jsonService; - public function __construct(IL10N $l10n, JsonService $jsonService) { - parent::__construct($l10n); + public function __construct(IL10N $l10n, LoggerInterface $logger, JsonService $jsonService) { + parent::__construct($l10n, $logger); $this->jsonService = $jsonService; } @@ -32,6 +33,10 @@ public function parse(\DOMDocument $document, ?string $url): array { $json_ld_elements = $xpath->query("//*[@type='application/ld+json']"); + $this->logger->debug('JSON-LD parser found {count} ld+json elements for {url}', [ + 'count' => $json_ld_elements->length, 'url' => $url, + ]); + foreach ($json_ld_elements as $json_ld_element) { if (!$json_ld_element || !$json_ld_element->nodeValue) { continue; @@ -40,7 +45,9 @@ public function parse(\DOMDocument $document, ?string $url): array { try { return $this->parseJsonLdElement($json_ld_element); } catch (HtmlParsingException $ex) { - // Parsing failed for this element. Let's see if there are more... + $this->logger->debug('JSON-LD element did not contain a valid recipe: {reason}', [ + 'reason' => $ex->getMessage(), + ]); } } @@ -62,6 +69,7 @@ private function parseJsonLdElement(\DOMNode $node): array { $json = json_decode($string, true); if ($json === null) { + $this->logger->debug('JSON-LD decode failed: {error}', ['error' => json_last_error_msg()]); throw new HtmlParsingException($this->l->t('JSON cannot be decoded.')); } diff --git a/lib/Helper/HTMLParser/HttpMicrodataParser.php b/lib/Helper/HTMLParser/HttpMicrodataParser.php index e2d981f4f..5b90f1187 100644 --- a/lib/Helper/HTMLParser/HttpMicrodataParser.php +++ b/lib/Helper/HTMLParser/HttpMicrodataParser.php @@ -13,6 +13,7 @@ use DOMXPath; use OCA\Cookbook\Exception\HtmlParsingException; use OCP\IL10N; +use Psr\Log\LoggerInterface; /** * This class is an AbstractHtmlParser which tries to extract micro data from the HTML page. @@ -32,8 +33,8 @@ class HttpMicrodataParser extends AbstractHtmlParser { */ private $recipe; - public function __construct(IL10N $l10n) { - parent::__construct($l10n); + public function __construct(IL10N $l10n, LoggerInterface $logger) { + parent::__construct($l10n, $logger); } #[\Override] @@ -49,9 +50,14 @@ public function parse(DOMDocument $document, ?string $url): array { $recipes = $this->xpath->query($xpathSelector); if (count($recipes) === 0) { + $this->logger->info('Microdata parser found no Recipe itemscope elements for {url}', ['url' => $url]); throw new HtmlParsingException($this->l->t('No recipe was found.')); } + $this->logger->debug('Microdata parser found {count} Recipe itemscope elements for {url}', [ + 'count' => count($recipes), 'url' => $url, + ]); + $this->recipe = [ '@type' => 'Recipe', '@context' => 'http://schema.org' diff --git a/lib/Helper/HtmlToDomParser.php b/lib/Helper/HtmlToDomParser.php index 8a25fca1d..089db7152 100644 --- a/lib/Helper/HtmlToDomParser.php +++ b/lib/Helper/HtmlToDomParser.php @@ -87,13 +87,13 @@ public function loadHtmlString(DOMDocument $dom, string $url, string $html): DOM try { $this->checkXMLErrors($errors, $url); } catch (Exception $ex) { - throw new ImportException($this->l->t('Parsing of HTML failed.'), null, $ex); + throw new ImportException($this->l->t('Parsing of HTML failed.'), null, $ex, step: 'parse_html'); } libxml_clear_errors(); if (!$parsedSuccessfully) { - throw new ImportException($this->l->t('Parsing of HTML failed.')); + throw new ImportException($this->l->t('Parsing of HTML failed.'), step: 'parse_html'); } } finally { libxml_use_internal_errors($libxml_previous_state); diff --git a/lib/Service/HtmlDownloadService.php b/lib/Service/HtmlDownloadService.php index 704d3570b..103e62228 100644 --- a/lib/Service/HtmlDownloadService.php +++ b/lib/Service/HtmlDownloadService.php @@ -86,9 +86,17 @@ public function __construct( * @throws ImportException If obtaining of the URL was not possible */ public function downloadRecipe(string $url): int { + $this->logger->info('Starting recipe download from {url}', ['url' => $url]); + $html = $this->fetchHtmlPage($url); - // Filter the HTML code + $this->logger->debug('Downloaded {url}: HTTP {status}, content-type {ct}, size {size} bytes', [ + 'url' => $url, + 'status' => $this->downloadHelper->getStatus(), + 'ct' => $this->downloadHelper->getContentType() ?? 'unknown', + 'size' => strlen($html), + ]); + /** @var AbstractHtmlFilter $filter */ foreach ($this->htmlFilters as $filter) { $filter->apply($html); @@ -121,7 +129,7 @@ private function fetchHtmlPage(string $url): string { $host = parse_url($url); if (!$host) { - throw new ImportException($this->l->t('Could not parse URL')); + throw new ImportException($this->l->t('Could not parse URL'), step: 'download'); } $opt = [ @@ -150,13 +158,13 @@ private function fetchHtmlPage(string $url): string { try { $this->downloadHelper->downloadFile($url, $opt, $headers); } catch (NoDownloadWasCarriedOutException $ex) { - throw new ImportException($this->l->t('Exception while downloading recipe from %s.: %s', [$url, $ex->getMessage()]), 0, $ex); + throw new ImportException($this->l->t('Exception while downloading recipe from %s.: %s', [$url, $ex->getMessage()]), 0, $ex, step: 'download'); } $status = $this->downloadHelper->getStatus(); if ($status < 200 || $status >= 300) { - throw new ImportException($this->l->t('Download from %s failed as HTTP status code %d is not in expected range.', [$url, $status])); + throw new ImportException($this->l->t('Download from %s failed as HTTP status code %d is not in expected range.', [$url, $status]), step: 'download'); } $html = $this->downloadHelper->getContent(); diff --git a/lib/Service/RecipeExtractionService.php b/lib/Service/RecipeExtractionService.php index f9150343c..7c2d82ee6 100644 --- a/lib/Service/RecipeExtractionService.php +++ b/lib/Service/RecipeExtractionService.php @@ -10,6 +10,7 @@ use OCA\Cookbook\Helper\HTMLParser\HttpJsonLdParser; use OCA\Cookbook\Helper\HTMLParser\HttpMicrodataParser; use OCP\IL10N; +use Psr\Log\LoggerInterface; class RecipeExtractionService { /** @@ -22,10 +23,13 @@ class RecipeExtractionService { */ private $l; + private LoggerInterface $logger; + public function __construct(HttpJsonLdParser $jsonParser, HttpMicrodataParser $microdataParser, - IL10N $l10n) { + IL10N $l10n, LoggerInterface $logger) { $this->parsers = [$jsonParser, $microdataParser]; $this->l = $l10n; + $this->logger = $logger; } /** @@ -37,14 +41,26 @@ public function __construct(HttpJsonLdParser $jsonParser, HttpMicrodataParser $m * @return array The data as returned from the parser */ public function parse(\DOMDocument $document, ?string $url): array { + $failures = []; + foreach ($this->parsers as $parser) { try { return $parser->parse($document, $url); } catch (HtmlParsingException $ex) { - // Silently ignore failure as there might be other parsers better suited + $parserName = (new \ReflectionClass($parser))->getShortName(); + $failures[] = "$parserName: {$ex->getMessage()}"; + $this->logger->info('Parser {parser} failed for URL {url}: {reason}', [ + 'parser' => $parserName, 'url' => $url, 'reason' => $ex->getMessage(), + ]); } } - throw new HtmlParsingException($this->l->t('No parser found for the given import.')); + $detail = implode('; ', $failures); + $this->logger->warning('All parsers failed for URL {url}. Details: {detail}', [ + 'url' => $url, 'detail' => $detail, + ]); + throw new HtmlParsingException( + $this->l->t('No parser found for the given import.') . ' ' . $detail + ); } } diff --git a/lib/Service/RecipeService.php b/lib/Service/RecipeService.php index c8205ab73..e7aeac8c7 100755 --- a/lib/Service/RecipeService.php +++ b/lib/Service/RecipeService.php @@ -365,15 +365,19 @@ public function downloadRecipe(string $url): File { $json = $this->recipeExtractionService->parse($this->htmlDownloadService->getDom(), $url); $importedHtml = $this->htmlDownloadService->getDom()->saveHTML(); } catch (HtmlParsingException $ex) { - throw new ImportException($ex->getMessage(), null, $ex); + throw new ImportException($ex->getMessage(), null, $ex, step: 'extract_recipe'); } + $this->logger->info('Recipe extracted from {url}: name="{name}"', [ + 'url' => $url, 'name' => $json['name'] ?? '(unnamed)', + ]); + $json = $this->checkRecipe($json); if (!$json) { $this->logger->error('Importing parsers resulted in null recipe.' . 'This is most probably a bug. Please report.'); - throw new ImportException($this->il10n->t('No recipe data found. This is a bug')); + throw new ImportException($this->il10n->t('No recipe data found. This is a bug'), step: 'extract_recipe'); } $json['url'] = $url; diff --git a/src/components/AppNavi.vue b/src/components/AppNavi.vue index ff62f6667..8093fef27 100644 --- a/src/components/AppNavi.vue +++ b/src/components/AppNavi.vue @@ -271,20 +271,15 @@ const downloadRecipe = async () => { if (e2.response) { if (e2.response.status >= 400 && e2.response.status < 500) { - if (e2.response.status === 409) { - // There was a recipe found with the same name - - const message = - typeof e2.response.data === 'string' - ? e2.response.data - : e2.response.data.msg; - await showSimpleAlertModal(t('cookbook', 'Error'), message); - } else { - await showSimpleAlertModal( - t('cookbook', 'Error'), - e2.response.data, - ); - } + const data = e2.response.data; + const message = + typeof data === 'string' + ? data + : (data.msg ?? t('cookbook', 'Import failed.')); + await showSimpleAlertModal( + t('cookbook', 'Import Error'), + message, + ); } else { console.error(e2); await showSimpleAlertModal( diff --git a/tests/Unit/Controller/Implementation/RecipeImplementationTest.php b/tests/Unit/Controller/Implementation/RecipeImplementationTest.php index 58981ea2d..97c7ebe12 100644 --- a/tests/Unit/Controller/Implementation/RecipeImplementationTest.php +++ b/tests/Unit/Controller/Implementation/RecipeImplementationTest.php @@ -146,14 +146,8 @@ public function testImportExisting(): void { */ $ret = $this->sut->import(); - $expected = [ - 'msg' => $ex->getMessage(), - 'line' => $ex->getLine(), - 'file' => $ex->getFile(), - ]; - $this->assertEquals(409, $ret->getStatus()); - $this->assertEquals($expected, $ret->getData()); + $this->assertEquals(['msg' => $ex->getMessage()], $ret->getData()); } public function testImportOther(): void { @@ -171,8 +165,8 @@ public function testImportOther(): void { */ $ret = $this->sut->import(); - $this->assertEquals(400, $ret->getStatus()); - $this->assertEquals($errorMsg, $ret->getData()); + $this->assertEquals(500, $ret->getStatus()); + $this->assertEquals(['msg' => 'An unexpected error occurred during import.'], $ret->getData()); } /** @@ -506,12 +500,7 @@ public function testCreateExisting(): void { $ret = $this->sut->create(); $this->assertEquals(409, $ret->getStatus()); - $expected = [ - 'msg' => 'message', - 'file' => __FILE__, - 'line' => $ex->getLine(), - ]; - $this->assertEquals($expected, $ret->getData()); + $this->assertEquals(['msg' => 'message'], $ret->getData()); } public function testShow(): void { diff --git a/tests/Unit/Helper/HTMLParser/HttpJsonLdParserTest.php b/tests/Unit/Helper/HTMLParser/HttpJsonLdParserTest.php index d429b2044..519500d34 100644 --- a/tests/Unit/Helper/HTMLParser/HttpJsonLdParserTest.php +++ b/tests/Unit/Helper/HTMLParser/HttpJsonLdParserTest.php @@ -11,6 +11,7 @@ use OCA\Cookbook\Service\JsonService; use OCP\IL10N; use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerInterface; /** * @coversDefaultClass \OCA\Cookbook\Helper\HTMLParser\HttpJsonLdParser @@ -49,7 +50,9 @@ public function testConstructor(): void { */ $l = $this->createStub(IL10N::class); - $parser = new HttpJsonLdParser($l, $jsonService); + $logger = $this->createStub(LoggerInterface::class); + + $parser = new HttpJsonLdParser($l, $logger, $jsonService); $lProperty = new \ReflectionProperty(HttpJsonLdParser::class, 'l'); $lProperty->setAccessible(true); @@ -71,7 +74,9 @@ public function testHTMLFile($file, $valid, $jsonFile): void { */ $l = $this->createStub(IL10N::class); - $parser = new HttpJsonLdParser($l, $jsonService); + $logger = $this->createStub(LoggerInterface::class); + + $parser = new HttpJsonLdParser($l, $logger, $jsonService); $content = file_get_contents(__DIR__ . "/res_JsonLd/$file"); @@ -113,7 +118,7 @@ public static function dataProviderImageReference(): array { * @covers ::parse */ public function testImageReference(array $input, array $expected): void { - $parser = new HttpJsonLdParser($this->createStub(IL10N::class), new JsonService()); + $parser = new HttpJsonLdParser($this->createStub(IL10N::class), $this->createStub(LoggerInterface::class), new JsonService()); $document = new \DOMDocument(); $document->loadHTML(''); diff --git a/tests/Unit/Helper/HTMLParser/HttpMicrodataParserTest.php b/tests/Unit/Helper/HTMLParser/HttpMicrodataParserTest.php index 7762d392c..25ef30752 100644 --- a/tests/Unit/Helper/HTMLParser/HttpMicrodataParserTest.php +++ b/tests/Unit/Helper/HTMLParser/HttpMicrodataParserTest.php @@ -10,6 +10,7 @@ use OCA\Cookbook\Helper\HTMLParser\HttpMicrodataParser; use OCP\IL10N; use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerInterface; /** * @coversDefaultClass \OCA\Cookbook\Helper\HTMLParser\HttpMicrodataParser @@ -25,7 +26,9 @@ class HttpMicrodataParserTest extends TestCase { public function testConstructor(): void { $l = $this->createStub(IL10N::class); - $parser = new HttpMicrodataParser($l); + $logger = $this->createStub(LoggerInterface::class); + + $parser = new HttpMicrodataParser($l, $logger); $lProperty = new \ReflectionProperty(HttpMicrodataParser::class, 'l'); $lProperty->setAccessible(true); @@ -64,8 +67,9 @@ public function testHTMLFile($filename, $valid, $jsonFile, $skipped = false): vo } $l = $this->createStub(IL10N::class); + $logger = $this->createStub(LoggerInterface::class); - $parser = new HttpMicrodataParser($l); + $parser = new HttpMicrodataParser($l, $logger); $content = file_get_contents(__DIR__ . "/res_Microdata/$filename"); @@ -99,7 +103,8 @@ public static function imageAttributes() { */ public function testImageVariantsAsAttribute($attribute): void { $l = $this->createStub(IL10N::class); - $parser = new HttpMicrodataParser($l); + $logger = $this->createStub(LoggerInterface::class); + $parser = new HttpMicrodataParser($l, $logger); $content = file_get_contents(__DIR__ . '/res_Microdata/caseImageAttribute.html'); $content = str_replace('%IMAGE_NAME%', $attribute, $content); @@ -112,7 +117,8 @@ public function testImageVariantsAsAttribute($attribute): void { */ public function testImageVariantsAsContent($attribute): void { $l = $this->createStub(IL10N::class); - $parser = new HttpMicrodataParser($l); + $logger = $this->createStub(LoggerInterface::class); + $parser = new HttpMicrodataParser($l, $logger); $content = file_get_contents(__DIR__ . '/res_Microdata/caseImageContent.html'); $content = str_replace('%IMAGE_NAME%', $attribute, $content); @@ -131,7 +137,8 @@ public static function ingredientVariantAttributes() { */ public function testIngredientVariants($attribute): void { $l = $this->createStub(IL10N::class); - $parser = new HttpMicrodataParser($l); + $logger = $this->createStub(LoggerInterface::class); + $parser = new HttpMicrodataParser($l, $logger); $content = file_get_contents(__DIR__ . '/res_Microdata/caseIngredient.html'); $content = str_replace('%INGREDIENT_NAME%', $attribute, $content); @@ -151,7 +158,8 @@ public static function instructionVariantAttributes() { */ public function testInstructionVariants($attribute): void { $l = $this->createStub(IL10N::class); - $parser = new HttpMicrodataParser($l); + $logger = $this->createStub(LoggerInterface::class); + $parser = new HttpMicrodataParser($l, $logger); $content = file_get_contents(__DIR__ . '/res_Microdata/caseInstruction.html'); $content = str_replace('%INSTRUCTION_NAME%', $attribute, $content); diff --git a/tests/Unit/Service/RecipeExtractionServiceTest.php b/tests/Unit/Service/RecipeExtractionServiceTest.php index b5a29cb2b..3c953f3d8 100644 --- a/tests/Unit/Service/RecipeExtractionServiceTest.php +++ b/tests/Unit/Service/RecipeExtractionServiceTest.php @@ -13,6 +13,7 @@ use OCP\IL10N; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerInterface; class RecipeExtractionServiceTest extends TestCase { /** @@ -68,7 +69,8 @@ public function testParsingDelegation($jsonSuccess, $microdataSuccess, $exceptio } } - $sut = new RecipeExtractionService($jsonParser, $microdataParser, $this->l); + $logger = $this->createStub(LoggerInterface::class); + $sut = new RecipeExtractionService($jsonParser, $microdataParser, $this->l, $logger); try { $ret = $sut->parse($document, $url); From 30880c03c8e9667dd13d295fbbe341d787ab8127 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9o=20Jan?= Date: Thu, 29 Oct 2026 19:59:19 +0100 Subject: [PATCH 2/6] docs: document structured import errors in the API spec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Assisted-by: Claude Code:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 Signed-off-by: Léo Jan --- docs/dev/api/changelog/0.md | 6 ++++++ docs/dev/api/trunk/internal-cookbook.yaml | 14 +++++++++++--- docs/dev/api/trunk/objects.yaml | 11 ++++------- docs/dev/api/trunk/openapi-cookbook.yaml | 14 +++++++++++--- 4 files changed, 32 insertions(+), 13 deletions(-) diff --git a/docs/dev/api/changelog/0.md b/docs/dev/api/changelog/0.md index bce5ca1b4..2090bc08c 100644 --- a/docs/dev/api/changelog/0.md +++ b/docs/dev/api/changelog/0.md @@ -14,6 +14,12 @@ This serves only for developers to find the changes better. ## Unpublished +### Structured import errors + +- `POST /api/v1/import` (and `/webapp/import`): a failed import now returns `400` with an `Error` object containing `msg` and `step` (`download`, `parse_html` or `extract_recipe`) instead of a plain string. Missing or disallowed URLs still return a plain string. +- Unexpected import errors now return `500` with a generic `msg` instead of `400` with the raw exception message. +- The `Error` object no longer contains `file` and `line`. + ## Version 0.1.3 - cookbook v0.11.5 - 2025-10-30 diff --git a/docs/dev/api/trunk/internal-cookbook.yaml b/docs/dev/api/trunk/internal-cookbook.yaml index b47662102..8f175fda9 100644 --- a/docs/dev/api/trunk/internal-cookbook.yaml +++ b/docs/dev/api/trunk/internal-cookbook.yaml @@ -166,18 +166,26 @@ paths: schema: $ref: "#/components/schemas/Recipe" 400: - description: The URL to be imported was not inserted in the request or any other issue was rosen. + description: The URL is missing or not allowed (plain string), or the import failed at a given step (error object). content: application/json: schema: - type: string - example: Field "url" is required + oneOf: + - type: string + example: Field "url" is required + - $ref: "#/components/schemas/Error" 409: description: There exists a recipe with that name already. Import was aborted. content: application/json: schema: $ref: "#/components/schemas/Error" + 500: + description: An unexpected error occurred during the import. Details are only logged on the server. + content: + application/json: + schema: + $ref: "#/components/schemas/Error" /webapp/recipes/{id}/image: get: tags: [ Recipes ] diff --git a/docs/dev/api/trunk/objects.yaml b/docs/dev/api/trunk/objects.yaml index df05028e2..f86371c37 100644 --- a/docs/dev/api/trunk/objects.yaml +++ b/docs/dev/api/trunk/objects.yaml @@ -79,14 +79,11 @@ Error: type: string description: The error message example: Recipe could not be found - file: + step: type: string - description: The file in which the exception was thrown - example: /var/www/custom_apps/cookbook/Service/RecipeService.php - line: - type: integer - description: The line where the exception is thrown - example: 223 + description: The import step that failed. Only present on import errors. + enum: [download, parse_html, extract_recipe] + example: download Keyword: type: object diff --git a/docs/dev/api/trunk/openapi-cookbook.yaml b/docs/dev/api/trunk/openapi-cookbook.yaml index bcd833710..e58193683 100644 --- a/docs/dev/api/trunk/openapi-cookbook.yaml +++ b/docs/dev/api/trunk/openapi-cookbook.yaml @@ -236,18 +236,26 @@ paths: schema: $ref: "#/components/schemas/Recipe" 400: - description: The URL to be imported was not inserted in the request or any other issue was rosen. + description: The URL is missing or not allowed (plain string), or the import failed at a given step (error object). content: application/json: schema: - type: string - example: Field "url" is required + oneOf: + - type: string + example: Field "url" is required + - $ref: "#/components/schemas/Error" 409: description: There exists a recipe with that name already. Import was aborted. content: application/json: schema: $ref: "#/components/schemas/Error" + 500: + description: An unexpected error occurred during the import. Details are only logged on the server. + content: + application/json: + schema: + $ref: "#/components/schemas/Error" /apps/cookbook/api/v1/recipes/{id}/image: get: tags: [ Recipes ] From ab1434e15b6e3acec25170416eeca334558212af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9o=20Jan?= Date: Thu, 29 Oct 2026 20:42:18 +0100 Subject: [PATCH 3/6] fix: return generic error when caching an imported recipe fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Also document that invalid recipe JSON still returns a plain-string 400. Assisted-by: Claude Code:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 Signed-off-by: Léo Jan --- docs/dev/api/changelog/0.md | 2 +- docs/dev/api/trunk/internal-cookbook.yaml | 2 +- docs/dev/api/trunk/openapi-cookbook.yaml | 2 +- .../Implementation/RecipeImplementation.php | 5 +++++ .../RecipeImplementationTest.php | 20 +++++++++++++++++++ 5 files changed, 28 insertions(+), 3 deletions(-) diff --git a/docs/dev/api/changelog/0.md b/docs/dev/api/changelog/0.md index 2090bc08c..c7b4fb82e 100644 --- a/docs/dev/api/changelog/0.md +++ b/docs/dev/api/changelog/0.md @@ -16,7 +16,7 @@ This serves only for developers to find the changes better. ### Structured import errors -- `POST /api/v1/import` (and `/webapp/import`): a failed import now returns `400` with an `Error` object containing `msg` and `step` (`download`, `parse_html` or `extract_recipe`) instead of a plain string. Missing or disallowed URLs still return a plain string. +- `POST /api/v1/import` (and `/webapp/import`): a failed import now returns `400` with an `Error` object containing `msg` and `step` (`download`, `parse_html` or `extract_recipe`) instead of a plain string. Missing or disallowed URLs and invalid recipe JSON still return a plain string. - Unexpected import errors now return `500` with a generic `msg` instead of `400` with the raw exception message. - The `Error` object no longer contains `file` and `line`. diff --git a/docs/dev/api/trunk/internal-cookbook.yaml b/docs/dev/api/trunk/internal-cookbook.yaml index 8f175fda9..606d39aa0 100644 --- a/docs/dev/api/trunk/internal-cookbook.yaml +++ b/docs/dev/api/trunk/internal-cookbook.yaml @@ -166,7 +166,7 @@ paths: schema: $ref: "#/components/schemas/Recipe" 400: - description: The URL is missing or not allowed (plain string), or the import failed at a given step (error object). + description: The URL is missing or not allowed, or the recipe JSON is invalid (plain string), or the import failed at a given step (error object). content: application/json: schema: diff --git a/docs/dev/api/trunk/openapi-cookbook.yaml b/docs/dev/api/trunk/openapi-cookbook.yaml index e58193683..ebba7247d 100644 --- a/docs/dev/api/trunk/openapi-cookbook.yaml +++ b/docs/dev/api/trunk/openapi-cookbook.yaml @@ -236,7 +236,7 @@ paths: schema: $ref: "#/components/schemas/Recipe" 400: - description: The URL is missing or not allowed (plain string), or the import failed at a given step (error object). + description: The URL is missing or not allowed, or the recipe JSON is invalid (plain string), or the import failed at a given step (error object). content: application/json: schema: diff --git a/lib/Controller/Implementation/RecipeImplementation.php b/lib/Controller/Implementation/RecipeImplementation.php index 1e190d72c..7cf13ba1d 100644 --- a/lib/Controller/Implementation/RecipeImplementation.php +++ b/lib/Controller/Implementation/RecipeImplementation.php @@ -334,6 +334,11 @@ public function import() { } return new DataResponse($ex->getMessage(), 400); + } catch (\Exception $e) { + $this->logger->error('Unexpected error while caching imported recipe: {reason}', [ + 'url' => $data['url'], 'reason' => $e->getMessage(), 'exception' => $e, + ]); + return new JSONResponse(['msg' => $this->l->t('An unexpected error occurred during import.')], Http::STATUS_INTERNAL_SERVER_ERROR); } return new JSONResponse($recipe_json, Http::STATUS_OK); diff --git a/tests/Unit/Controller/Implementation/RecipeImplementationTest.php b/tests/Unit/Controller/Implementation/RecipeImplementationTest.php index b3b9cfbcd..c3fd6dfc4 100644 --- a/tests/Unit/Controller/Implementation/RecipeImplementationTest.php +++ b/tests/Unit/Controller/Implementation/RecipeImplementationTest.php @@ -169,6 +169,26 @@ public function testImportOther(): void { $this->assertEquals(['msg' => 'An unexpected error occurred during import.'], $ret->getData()); } + public function testImportCacheFailure(): void { + $this->ensureCacheCheckTriggered(); + + $url = 'http://example.com/Recipe.html'; + $file = $this->createStub(File::class); + + $this->restParser->method('getParameters')->willReturn([ 'url' => $url ]); + $this->recipeService->method('downloadRecipe')->willReturn($file); + $this->recipeService->method('parseRecipeFile')->willReturn(['name' => 'The recipe name']); + $this->dbCacheService->expects($this->once())->method('addRecipe')->with($file)->willThrowException(new Exception('DB down')); + + /** + * @var JSONResponse $ret + */ + $ret = $this->sut->import(); + + $this->assertEquals(500, $ret->getStatus()); + $this->assertEquals(['msg' => 'An unexpected error occurred during import.'], $ret->getData()); + } + /** * @dataProvider dataProviderCategory * @param mixed $cat From 873bfb703f4c82948797465e170e06ff35cf268b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9o=20Jan?= Date: Thu, 29 Oct 2026 20:49:07 +0100 Subject: [PATCH 4/6] fix: redact credentials and query from logged import URLs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Assisted-by: Claude Code:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 Signed-off-by: Léo Jan --- .../Implementation/RecipeImplementation.php | 7 +++-- lib/Helper/HTMLParser/HttpJsonLdParser.php | 3 +- lib/Helper/HTMLParser/HttpMicrodataParser.php | 5 +-- lib/Helper/UrlRedactionHelper.php | 25 +++++++++++++++ lib/Service/HtmlDownloadService.php | 5 +-- lib/Service/RecipeExtractionService.php | 5 +-- lib/Service/RecipeService.php | 3 +- tests/Unit/Helper/UrlRedactionHelperTest.php | 31 +++++++++++++++++++ 8 files changed, 73 insertions(+), 11 deletions(-) create mode 100644 lib/Helper/UrlRedactionHelper.php create mode 100644 tests/Unit/Helper/UrlRedactionHelperTest.php diff --git a/lib/Controller/Implementation/RecipeImplementation.php b/lib/Controller/Implementation/RecipeImplementation.php index 7cf13ba1d..4b8210f3e 100644 --- a/lib/Controller/Implementation/RecipeImplementation.php +++ b/lib/Controller/Implementation/RecipeImplementation.php @@ -16,6 +16,7 @@ use OCA\Cookbook\Helper\Filter\Output\RecipeJSONOutputFilter; use OCA\Cookbook\Helper\Filter\Output\RecipeStubFilter; use OCA\Cookbook\Helper\RestParameterParser; +use OCA\Cookbook\Helper\UrlRedactionHelper; use OCA\Cookbook\Service\DbCacheService; use OCA\Cookbook\Service\RecipeService; use OCP\AppFramework\Http; @@ -313,12 +314,12 @@ public function import() { return new JSONResponse(['msg' => $ex->getMessage()], Http::STATUS_CONFLICT); } catch (ImportException $e) { $this->logger->warning('Import failed at step "{step}": {reason}', [ - 'url' => $data['url'], 'step' => $e->getStep(), 'reason' => $e->getMessage(), 'exception' => $e, + 'url' => UrlRedactionHelper::redact($data['url']), 'step' => $e->getStep(), 'reason' => $e->getMessage(), 'exception' => $e, ]); return new JSONResponse(['msg' => $e->getMessage(), 'step' => $e->getStep()], Http::STATUS_BAD_REQUEST); } catch (\Exception $e) { $this->logger->error('Unexpected error during import: {reason}', [ - 'url' => $data['url'], 'reason' => $e->getMessage(), 'exception' => $e, + 'url' => UrlRedactionHelper::redact($data['url']), 'reason' => $e->getMessage(), 'exception' => $e, ]); return new JSONResponse(['msg' => $this->l->t('An unexpected error occurred during import.')], Http::STATUS_INTERNAL_SERVER_ERROR); } @@ -336,7 +337,7 @@ public function import() { return new DataResponse($ex->getMessage(), 400); } catch (\Exception $e) { $this->logger->error('Unexpected error while caching imported recipe: {reason}', [ - 'url' => $data['url'], 'reason' => $e->getMessage(), 'exception' => $e, + 'url' => UrlRedactionHelper::redact($data['url']), 'reason' => $e->getMessage(), 'exception' => $e, ]); return new JSONResponse(['msg' => $this->l->t('An unexpected error occurred during import.')], Http::STATUS_INTERNAL_SERVER_ERROR); } diff --git a/lib/Helper/HTMLParser/HttpJsonLdParser.php b/lib/Helper/HTMLParser/HttpJsonLdParser.php index cd36c9f86..5fb211bb2 100644 --- a/lib/Helper/HTMLParser/HttpJsonLdParser.php +++ b/lib/Helper/HTMLParser/HttpJsonLdParser.php @@ -7,6 +7,7 @@ namespace OCA\Cookbook\Helper\HTMLParser; use OCA\Cookbook\Exception\HtmlParsingException; +use OCA\Cookbook\Helper\UrlRedactionHelper; use OCA\Cookbook\Service\JsonService; use OCP\IL10N; use Psr\Log\LoggerInterface; @@ -34,7 +35,7 @@ public function parse(\DOMDocument $document, ?string $url): array { $json_ld_elements = $xpath->query("//*[@type='application/ld+json']"); $this->logger->debug('JSON-LD parser found {count} ld+json elements for {url}', [ - 'count' => $json_ld_elements->length, 'url' => $url, + 'count' => $json_ld_elements->length, 'url' => UrlRedactionHelper::redact($url), ]); foreach ($json_ld_elements as $json_ld_element) { diff --git a/lib/Helper/HTMLParser/HttpMicrodataParser.php b/lib/Helper/HTMLParser/HttpMicrodataParser.php index 5b90f1187..4fdaa1033 100644 --- a/lib/Helper/HTMLParser/HttpMicrodataParser.php +++ b/lib/Helper/HTMLParser/HttpMicrodataParser.php @@ -12,6 +12,7 @@ use DOMNodeList; use DOMXPath; use OCA\Cookbook\Exception\HtmlParsingException; +use OCA\Cookbook\Helper\UrlRedactionHelper; use OCP\IL10N; use Psr\Log\LoggerInterface; @@ -50,12 +51,12 @@ public function parse(DOMDocument $document, ?string $url): array { $recipes = $this->xpath->query($xpathSelector); if (count($recipes) === 0) { - $this->logger->info('Microdata parser found no Recipe itemscope elements for {url}', ['url' => $url]); + $this->logger->info('Microdata parser found no Recipe itemscope elements for {url}', ['url' => UrlRedactionHelper::redact($url)]); throw new HtmlParsingException($this->l->t('No recipe was found.')); } $this->logger->debug('Microdata parser found {count} Recipe itemscope elements for {url}', [ - 'count' => count($recipes), 'url' => $url, + 'count' => count($recipes), 'url' => UrlRedactionHelper::redact($url), ]); $this->recipe = [ diff --git a/lib/Helper/UrlRedactionHelper.php b/lib/Helper/UrlRedactionHelper.php new file mode 100644 index 000000000..f236a187c --- /dev/null +++ b/lib/Helper/UrlRedactionHelper.php @@ -0,0 +1,25 @@ +logger->info('Starting recipe download from {url}', ['url' => $url]); + $this->logger->info('Starting recipe download from {url}', ['url' => UrlRedactionHelper::redact($url)]); $html = $this->fetchHtmlPage($url); $this->logger->debug('Downloaded {url}: HTTP {status}, content-type {ct}, size {size} bytes', [ - 'url' => $url, + 'url' => UrlRedactionHelper::redact($url), 'status' => $this->downloadHelper->getStatus(), 'ct' => $this->downloadHelper->getContentType() ?? 'unknown', 'size' => strlen($html), diff --git a/lib/Service/RecipeExtractionService.php b/lib/Service/RecipeExtractionService.php index 7c2d82ee6..b0f7c9015 100644 --- a/lib/Service/RecipeExtractionService.php +++ b/lib/Service/RecipeExtractionService.php @@ -9,6 +9,7 @@ use OCA\Cookbook\Exception\HtmlParsingException; use OCA\Cookbook\Helper\HTMLParser\HttpJsonLdParser; use OCA\Cookbook\Helper\HTMLParser\HttpMicrodataParser; +use OCA\Cookbook\Helper\UrlRedactionHelper; use OCP\IL10N; use Psr\Log\LoggerInterface; @@ -50,14 +51,14 @@ public function parse(\DOMDocument $document, ?string $url): array { $parserName = (new \ReflectionClass($parser))->getShortName(); $failures[] = "$parserName: {$ex->getMessage()}"; $this->logger->info('Parser {parser} failed for URL {url}: {reason}', [ - 'parser' => $parserName, 'url' => $url, 'reason' => $ex->getMessage(), + 'parser' => $parserName, 'url' => UrlRedactionHelper::redact($url), 'reason' => $ex->getMessage(), ]); } } $detail = implode('; ', $failures); $this->logger->warning('All parsers failed for URL {url}. Details: {detail}', [ - 'url' => $url, 'detail' => $detail, + 'url' => UrlRedactionHelper::redact($url), 'detail' => $detail, ]); throw new HtmlParsingException( $this->l->t('No parser found for the given import.') . ' ' . $detail diff --git a/lib/Service/RecipeService.php b/lib/Service/RecipeService.php index 7459881d4..b571f853c 100755 --- a/lib/Service/RecipeService.php +++ b/lib/Service/RecipeService.php @@ -18,6 +18,7 @@ use OCA\Cookbook\Helper\Filter\JSON\JSONFilter; use OCA\Cookbook\Helper\ImageService\ImageConversionHelper; use OCA\Cookbook\Helper\ImageService\ImageSize; +use OCA\Cookbook\Helper\UrlRedactionHelper; use OCA\Cookbook\Helper\UserConfigHelper; use OCA\Cookbook\Helper\UserFolderHelper; use OCP\AppFramework\Db\DoesNotExistException; @@ -354,7 +355,7 @@ public function downloadRecipe(string $url): File { } $this->logger->info('Recipe extracted from {url}: name="{name}"', [ - 'url' => $url, 'name' => $json['name'] ?? '(unnamed)', + 'url' => UrlRedactionHelper::redact($url), 'name' => $json['name'] ?? '(unnamed)', ]); $json = $this->checkRecipe($json); diff --git a/tests/Unit/Helper/UrlRedactionHelperTest.php b/tests/Unit/Helper/UrlRedactionHelperTest.php new file mode 100644 index 000000000..12e40bc70 --- /dev/null +++ b/tests/Unit/Helper/UrlRedactionHelperTest.php @@ -0,0 +1,31 @@ + ['https://example.com/recipe/1', 'https://example.com/recipe/1'], + 'credentials' => ['https://user:secret@example.com/recipe', 'https://example.com/recipe'], + 'query and fragment' => ['https://example.com/recipe?token=abc#top', 'https://example.com/recipe'], + 'port' => ['http://example.com:8080/r', 'http://example.com:8080/r'], + 'no path' => ['https://example.com', 'https://example.com'], + 'invalid' => ['not a url', '[invalid URL]'], + 'null' => [null, '[invalid URL]'], + ]; + } + + /** + * @dataProvider dataProvider + */ + public function testRedact(?string $url, string $expected): void { + $this->assertEquals($expected, UrlRedactionHelper::redact($url)); + } +} From 9389b3a7642b7869a40628f60ad6f1e7e54948af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9o=20Jan?= Date: Thu, 29 Oct 2026 20:57:24 +0100 Subject: [PATCH 5/6] fix: report missing recipe name as import failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Validation errors of the extracted recipe returned a generic 500 instead of a 400 with the extract_recipe step. Assisted-by: Claude Code:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 Signed-off-by: Léo Jan --- lib/Service/RecipeService.php | 12 ++++++++++-- .../RecipeImplementationTest.php | 19 +++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/lib/Service/RecipeService.php b/lib/Service/RecipeService.php index b571f853c..7b78dcfcd 100755 --- a/lib/Service/RecipeService.php +++ b/lib/Service/RecipeService.php @@ -358,7 +358,11 @@ public function downloadRecipe(string $url): File { 'url' => UrlRedactionHelper::redact($url), 'name' => $json['name'] ?? '(unnamed)', ]); - $json = $this->checkRecipe($json); + try { + $json = $this->checkRecipe($json); + } catch (Exception $ex) { + throw new ImportException($ex->getMessage(), null, $ex, step: 'extract_recipe'); + } if (!$json) { $this->logger->error('Importing parsers resulted in null recipe.' @@ -368,7 +372,11 @@ public function downloadRecipe(string $url): File { $json['url'] = $url; - return $this->addRecipe($json, $importedHtml); + try { + return $this->addRecipe($json, $importedHtml); + } catch (NoRecipeNameGivenException $ex) { + throw new ImportException($ex->getMessage(), null, $ex, step: 'extract_recipe'); + } } /** diff --git a/tests/Unit/Controller/Implementation/RecipeImplementationTest.php b/tests/Unit/Controller/Implementation/RecipeImplementationTest.php index c3fd6dfc4..327173bec 100644 --- a/tests/Unit/Controller/Implementation/RecipeImplementationTest.php +++ b/tests/Unit/Controller/Implementation/RecipeImplementationTest.php @@ -8,6 +8,7 @@ use Exception; use OCA\Cookbook\Controller\Implementation\RecipeImplementation; +use OCA\Cookbook\Exception\ImportException; use OCA\Cookbook\Exception\NoRecipeNameGivenException; use OCA\Cookbook\Exception\RecipeExistsException; use OCA\Cookbook\Helper\AcceptHeaderParsingHelper; @@ -169,6 +170,24 @@ public function testImportOther(): void { $this->assertEquals(['msg' => 'An unexpected error occurred during import.'], $ret->getData()); } + public function testImportFailedAtStep(): void { + $this->ensureCacheCheckTriggered(); + + $url = 'http://example.com/Recipe.html'; + $ex = new ImportException('Field "name" is required', 0, null, 'extract_recipe'); + + $this->restParser->method('getParameters')->willReturn([ 'url' => $url ]); + $this->recipeService->expects($this->once())->method('downloadRecipe')->with($url)->willThrowException($ex); + + /** + * @var JSONResponse $ret + */ + $ret = $this->sut->import(); + + $this->assertEquals(400, $ret->getStatus()); + $this->assertEquals(['msg' => 'Field "name" is required', 'step' => 'extract_recipe'], $ret->getData()); + } + public function testImportCacheFailure(): void { $this->ensureCacheCheckTriggered(); From b6befe40b3b95e507bbca767c2eefde6748d54e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9o=20Jan?= Date: Thu, 29 Oct 2026 21:35:13 +0100 Subject: [PATCH 6/6] refactor: move structured error responses to a separate PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Keep this change focused on import logging. Response format, API docs and frontend handling follow in a stacked PR. Assisted-by: Claude Code:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 Signed-off-by: Léo Jan --- docs/dev/api/changelog/0.md | 6 -- docs/dev/api/trunk/internal-cookbook.yaml | 14 +--- docs/dev/api/trunk/objects.yaml | 11 ++-- docs/dev/api/trunk/openapi-cookbook.yaml | 14 +--- .../Implementation/RecipeImplementation.php | 65 +++++++++++++++---- lib/Service/RecipeExtractionService.php | 4 +- lib/Service/RecipeService.php | 12 +--- src/components/AppNavi.vue | 23 ++++--- .../RecipeImplementationTest.php | 56 ++++------------ 9 files changed, 95 insertions(+), 110 deletions(-) diff --git a/docs/dev/api/changelog/0.md b/docs/dev/api/changelog/0.md index c7b4fb82e..bce5ca1b4 100644 --- a/docs/dev/api/changelog/0.md +++ b/docs/dev/api/changelog/0.md @@ -14,12 +14,6 @@ This serves only for developers to find the changes better. ## Unpublished -### Structured import errors - -- `POST /api/v1/import` (and `/webapp/import`): a failed import now returns `400` with an `Error` object containing `msg` and `step` (`download`, `parse_html` or `extract_recipe`) instead of a plain string. Missing or disallowed URLs and invalid recipe JSON still return a plain string. -- Unexpected import errors now return `500` with a generic `msg` instead of `400` with the raw exception message. -- The `Error` object no longer contains `file` and `line`. - ## Version 0.1.3 - cookbook v0.11.5 - 2025-10-30 diff --git a/docs/dev/api/trunk/internal-cookbook.yaml b/docs/dev/api/trunk/internal-cookbook.yaml index 606d39aa0..b47662102 100644 --- a/docs/dev/api/trunk/internal-cookbook.yaml +++ b/docs/dev/api/trunk/internal-cookbook.yaml @@ -166,26 +166,18 @@ paths: schema: $ref: "#/components/schemas/Recipe" 400: - description: The URL is missing or not allowed, or the recipe JSON is invalid (plain string), or the import failed at a given step (error object). + description: The URL to be imported was not inserted in the request or any other issue was rosen. content: application/json: schema: - oneOf: - - type: string - example: Field "url" is required - - $ref: "#/components/schemas/Error" + type: string + example: Field "url" is required 409: description: There exists a recipe with that name already. Import was aborted. content: application/json: schema: $ref: "#/components/schemas/Error" - 500: - description: An unexpected error occurred during the import. Details are only logged on the server. - content: - application/json: - schema: - $ref: "#/components/schemas/Error" /webapp/recipes/{id}/image: get: tags: [ Recipes ] diff --git a/docs/dev/api/trunk/objects.yaml b/docs/dev/api/trunk/objects.yaml index f86371c37..df05028e2 100644 --- a/docs/dev/api/trunk/objects.yaml +++ b/docs/dev/api/trunk/objects.yaml @@ -79,11 +79,14 @@ Error: type: string description: The error message example: Recipe could not be found - step: + file: type: string - description: The import step that failed. Only present on import errors. - enum: [download, parse_html, extract_recipe] - example: download + description: The file in which the exception was thrown + example: /var/www/custom_apps/cookbook/Service/RecipeService.php + line: + type: integer + description: The line where the exception is thrown + example: 223 Keyword: type: object diff --git a/docs/dev/api/trunk/openapi-cookbook.yaml b/docs/dev/api/trunk/openapi-cookbook.yaml index ebba7247d..bcd833710 100644 --- a/docs/dev/api/trunk/openapi-cookbook.yaml +++ b/docs/dev/api/trunk/openapi-cookbook.yaml @@ -236,26 +236,18 @@ paths: schema: $ref: "#/components/schemas/Recipe" 400: - description: The URL is missing or not allowed, or the recipe JSON is invalid (plain string), or the import failed at a given step (error object). + description: The URL to be imported was not inserted in the request or any other issue was rosen. content: application/json: schema: - oneOf: - - type: string - example: Field "url" is required - - $ref: "#/components/schemas/Error" + type: string + example: Field "url" is required 409: description: There exists a recipe with that name already. Import was aborted. content: application/json: schema: $ref: "#/components/schemas/Error" - 500: - description: An unexpected error occurred during the import. Details are only logged on the server. - content: - application/json: - schema: - $ref: "#/components/schemas/Error" /apps/cookbook/api/v1/recipes/{id}/image: get: tags: [ Recipes ] diff --git a/lib/Controller/Implementation/RecipeImplementation.php b/lib/Controller/Implementation/RecipeImplementation.php index 4b8210f3e..2f9cdea09 100644 --- a/lib/Controller/Implementation/RecipeImplementation.php +++ b/lib/Controller/Implementation/RecipeImplementation.php @@ -135,9 +135,21 @@ public function update($id) { try { $file = $this->service->addRecipe($recipeData); } catch (RecipeExistsException $ex) { - return new JSONResponse(['msg' => $ex->getMessage()], Http::STATUS_CONFLICT); + $json = [ + 'msg' => $ex->getMessage(), + 'file' => $ex->getFile(), + 'line' => $ex->getLine(), + ]; + + return new JSONResponse($json, Http::STATUS_CONFLICT); } catch (NoRecipeNameGivenException $ex) { - return new JSONResponse(['msg' => $ex->getMessage()], Http::STATUS_UNPROCESSABLE_ENTITY); + $json = [ + 'msg' => $ex->getMessage(), + 'file' => $ex->getFile(), + 'line' => $ex->getLine(), + ]; + + return new JSONResponse($json, Http::STATUS_UNPROCESSABLE_ENTITY); } try { @@ -150,7 +162,13 @@ public function update($id) { $this->logger->warning('Cannot remove file after failed parsing: {ex}', ['ex' => $ex]); } - return new JSONResponse(['msg' => $ex->getMessage()], Http::STATUS_UNPROCESSABLE_ENTITY); + $json = [ + 'msg' => $ex->getMessage(), + 'file' => $ex->getFile(), + 'line' => $ex->getLine(), + ]; + + return new JSONResponse($json, Http::STATUS_UNPROCESSABLE_ENTITY); } return new JSONResponse($file->getParent()->getId(), Http::STATUS_OK); @@ -168,9 +186,21 @@ public function create() { try { $file = $this->service->addRecipe($recipeData); } catch (RecipeExistsException $ex) { - return new JSONResponse(['msg' => $ex->getMessage()], Http::STATUS_CONFLICT); + $json = [ + 'msg' => $ex->getMessage(), + 'file' => $ex->getFile(), + 'line' => $ex->getLine(), + ]; + + return new JSONResponse($json, Http::STATUS_CONFLICT); } catch (NoRecipeNameGivenException $ex) { - return new JSONResponse(['msg' => $ex->getMessage()], Http::STATUS_UNPROCESSABLE_ENTITY); + $json = [ + 'msg' => $ex->getMessage(), + 'file' => $ex->getFile(), + 'line' => $ex->getLine(), + ]; + + return new JSONResponse($json, Http::STATUS_UNPROCESSABLE_ENTITY); } try { @@ -183,7 +213,13 @@ public function create() { $this->logger->warning('Cannot remove file after failed parsing: {ex}', ['ex' => $ex]); } - return new JSONResponse(['msg' => $ex->getMessage()], Http::STATUS_UNPROCESSABLE_ENTITY); + $json = [ + 'msg' => $ex->getMessage(), + 'file' => $ex->getFile(), + 'line' => $ex->getLine(), + ]; + + return new JSONResponse($json, Http::STATUS_UNPROCESSABLE_ENTITY); } return new JSONResponse($file->getParent()->getId(), Http::STATUS_OK); @@ -311,17 +347,23 @@ public function import() { $recipe_json = $this->service->parseRecipeFile($recipe_file); } catch (RecipeExistsException $ex) { - return new JSONResponse(['msg' => $ex->getMessage()], Http::STATUS_CONFLICT); + $json = [ + 'msg' => $ex->getMessage(), + 'line' => $ex->getLine(), + 'file' => $ex->getFile(), + ]; + + return new JSONResponse($json, Http::STATUS_CONFLICT); } catch (ImportException $e) { $this->logger->warning('Import failed at step "{step}": {reason}', [ 'url' => UrlRedactionHelper::redact($data['url']), 'step' => $e->getStep(), 'reason' => $e->getMessage(), 'exception' => $e, ]); - return new JSONResponse(['msg' => $e->getMessage(), 'step' => $e->getStep()], Http::STATUS_BAD_REQUEST); + return new JSONResponse($e->getMessage(), 400); } catch (\Exception $e) { $this->logger->error('Unexpected error during import: {reason}', [ 'url' => UrlRedactionHelper::redact($data['url']), 'reason' => $e->getMessage(), 'exception' => $e, ]); - return new JSONResponse(['msg' => $this->l->t('An unexpected error occurred during import.')], Http::STATUS_INTERNAL_SERVER_ERROR); + return new JSONResponse($e->getMessage(), 400); } try { @@ -335,11 +377,6 @@ public function import() { } return new DataResponse($ex->getMessage(), 400); - } catch (\Exception $e) { - $this->logger->error('Unexpected error while caching imported recipe: {reason}', [ - 'url' => UrlRedactionHelper::redact($data['url']), 'reason' => $e->getMessage(), 'exception' => $e, - ]); - return new JSONResponse(['msg' => $this->l->t('An unexpected error occurred during import.')], Http::STATUS_INTERNAL_SERVER_ERROR); } return new JSONResponse($recipe_json, Http::STATUS_OK); diff --git a/lib/Service/RecipeExtractionService.php b/lib/Service/RecipeExtractionService.php index b0f7c9015..914167233 100644 --- a/lib/Service/RecipeExtractionService.php +++ b/lib/Service/RecipeExtractionService.php @@ -60,8 +60,6 @@ public function parse(\DOMDocument $document, ?string $url): array { $this->logger->warning('All parsers failed for URL {url}. Details: {detail}', [ 'url' => UrlRedactionHelper::redact($url), 'detail' => $detail, ]); - throw new HtmlParsingException( - $this->l->t('No parser found for the given import.') . ' ' . $detail - ); + throw new HtmlParsingException($this->l->t('No parser found for the given import.')); } } diff --git a/lib/Service/RecipeService.php b/lib/Service/RecipeService.php index 7b78dcfcd..b571f853c 100755 --- a/lib/Service/RecipeService.php +++ b/lib/Service/RecipeService.php @@ -358,11 +358,7 @@ public function downloadRecipe(string $url): File { 'url' => UrlRedactionHelper::redact($url), 'name' => $json['name'] ?? '(unnamed)', ]); - try { - $json = $this->checkRecipe($json); - } catch (Exception $ex) { - throw new ImportException($ex->getMessage(), null, $ex, step: 'extract_recipe'); - } + $json = $this->checkRecipe($json); if (!$json) { $this->logger->error('Importing parsers resulted in null recipe.' @@ -372,11 +368,7 @@ public function downloadRecipe(string $url): File { $json['url'] = $url; - try { - return $this->addRecipe($json, $importedHtml); - } catch (NoRecipeNameGivenException $ex) { - throw new ImportException($ex->getMessage(), null, $ex, step: 'extract_recipe'); - } + return $this->addRecipe($json, $importedHtml); } /** diff --git a/src/components/AppNavi.vue b/src/components/AppNavi.vue index 8093fef27..ff62f6667 100644 --- a/src/components/AppNavi.vue +++ b/src/components/AppNavi.vue @@ -271,15 +271,20 @@ const downloadRecipe = async () => { if (e2.response) { if (e2.response.status >= 400 && e2.response.status < 500) { - const data = e2.response.data; - const message = - typeof data === 'string' - ? data - : (data.msg ?? t('cookbook', 'Import failed.')); - await showSimpleAlertModal( - t('cookbook', 'Import Error'), - message, - ); + if (e2.response.status === 409) { + // There was a recipe found with the same name + + const message = + typeof e2.response.data === 'string' + ? e2.response.data + : e2.response.data.msg; + await showSimpleAlertModal(t('cookbook', 'Error'), message); + } else { + await showSimpleAlertModal( + t('cookbook', 'Error'), + e2.response.data, + ); + } } else { console.error(e2); await showSimpleAlertModal( diff --git a/tests/Unit/Controller/Implementation/RecipeImplementationTest.php b/tests/Unit/Controller/Implementation/RecipeImplementationTest.php index 327173bec..fbafd5618 100644 --- a/tests/Unit/Controller/Implementation/RecipeImplementationTest.php +++ b/tests/Unit/Controller/Implementation/RecipeImplementationTest.php @@ -8,7 +8,6 @@ use Exception; use OCA\Cookbook\Controller\Implementation\RecipeImplementation; -use OCA\Cookbook\Exception\ImportException; use OCA\Cookbook\Exception\NoRecipeNameGivenException; use OCA\Cookbook\Exception\RecipeExistsException; use OCA\Cookbook\Helper\AcceptHeaderParsingHelper; @@ -147,8 +146,14 @@ public function testImportExisting(): void { */ $ret = $this->sut->import(); + $expected = [ + 'msg' => $ex->getMessage(), + 'line' => $ex->getLine(), + 'file' => $ex->getFile(), + ]; + $this->assertEquals(409, $ret->getStatus()); - $this->assertEquals(['msg' => $ex->getMessage()], $ret->getData()); + $this->assertEquals($expected, $ret->getData()); } public function testImportOther(): void { @@ -166,46 +171,8 @@ public function testImportOther(): void { */ $ret = $this->sut->import(); - $this->assertEquals(500, $ret->getStatus()); - $this->assertEquals(['msg' => 'An unexpected error occurred during import.'], $ret->getData()); - } - - public function testImportFailedAtStep(): void { - $this->ensureCacheCheckTriggered(); - - $url = 'http://example.com/Recipe.html'; - $ex = new ImportException('Field "name" is required', 0, null, 'extract_recipe'); - - $this->restParser->method('getParameters')->willReturn([ 'url' => $url ]); - $this->recipeService->expects($this->once())->method('downloadRecipe')->with($url)->willThrowException($ex); - - /** - * @var JSONResponse $ret - */ - $ret = $this->sut->import(); - $this->assertEquals(400, $ret->getStatus()); - $this->assertEquals(['msg' => 'Field "name" is required', 'step' => 'extract_recipe'], $ret->getData()); - } - - public function testImportCacheFailure(): void { - $this->ensureCacheCheckTriggered(); - - $url = 'http://example.com/Recipe.html'; - $file = $this->createStub(File::class); - - $this->restParser->method('getParameters')->willReturn([ 'url' => $url ]); - $this->recipeService->method('downloadRecipe')->willReturn($file); - $this->recipeService->method('parseRecipeFile')->willReturn(['name' => 'The recipe name']); - $this->dbCacheService->expects($this->once())->method('addRecipe')->with($file)->willThrowException(new Exception('DB down')); - - /** - * @var JSONResponse $ret - */ - $ret = $this->sut->import(); - - $this->assertEquals(500, $ret->getStatus()); - $this->assertEquals(['msg' => 'An unexpected error occurred during import.'], $ret->getData()); + $this->assertEquals($errorMsg, $ret->getData()); } /** @@ -539,7 +506,12 @@ public function testCreateExisting(): void { $ret = $this->sut->create(); $this->assertEquals(409, $ret->getStatus()); - $this->assertEquals(['msg' => 'message'], $ret->getData()); + $expected = [ + 'msg' => 'message', + 'file' => __FILE__, + 'line' => $ex->getLine(), + ]; + $this->assertEquals($expected, $ret->getData()); } public function testShow(): void {