Skip to content

Commit 8efea9f

Browse files
TheTisibothclaude
TheTisiboth
and
claude
committed
feat: enhance import logging and error reporting
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 - 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 <noreply@anthropic.com> Signed-off-by: Léo Jan <leo.jan@doctolib.com>
1 parent 22e089f commit 8efea9f

14 files changed

Lines changed: 119 additions & 59 deletions

File tree

‎lib/Controller/Implementation/RecipeImplementation.php‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
namespace OCA\Cookbook\Controller\Implementation;
88

99
use Exception;
10+
use OCA\Cookbook\Exception\ImportException;
1011
use OCA\Cookbook\Exception\InvalidDownloadURLException;
1112
use OCA\Cookbook\Exception\InvalidJSONFileException;
1213
use OCA\Cookbook\Exception\NoRecipeNameGivenException;
@@ -345,15 +346,17 @@ public function import() {
345346
$recipe_json = $this->service->parseRecipeFile($recipe_file);
346347

347348
} catch (RecipeExistsException $ex) {
348-
$json = [
349-
'msg' => $ex->getMessage(),
350-
'line' => $ex->getLine(),
351-
'file' => $ex->getFile(),
352-
];
353-
354-
return new JSONResponse($json, Http::STATUS_CONFLICT);
349+
return new JSONResponse(['msg' => $ex->getMessage()], Http::STATUS_CONFLICT);
350+
} catch (ImportException $e) {
351+
$this->logger->warning('Import failed at step "{step}": {reason}', [
352+
'url' => $data['url'], 'step' => $e->getStep(), 'reason' => $e->getMessage(), 'exception' => $e,
353+
]);
354+
return new JSONResponse(['msg' => $e->getMessage(), 'step' => $e->getStep()], Http::STATUS_BAD_REQUEST);
355355
} catch (\Exception $e) {
356-
return new JSONResponse($e->getMessage(), 400);
356+
$this->logger->error('Unexpected error during import: {reason}', [
357+
'url' => $data['url'], 'reason' => $e->getMessage(), 'exception' => $e,
358+
]);
359+
return new JSONResponse(['msg' => $this->l->t('An unexpected error occurred during import.')], Http::STATUS_INTERNAL_SERVER_ERROR);
357360
}
358361

359362
try {

‎lib/Exception/ImportException.php‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,14 @@
77
namespace OCA\Cookbook\Exception;
88

99
class ImportException extends \Exception {
10-
public function __construct($message = '', $code = 0, $previous = null) {
10+
private string $step;
11+
12+
public function __construct($message = '', $code = 0, $previous = null, string $step = '') {
1113
parent::__construct($message, $code, $previous);
14+
$this->step = $step;
15+
}
16+
17+
public function getStep(): string {
18+
return $this->step;
1219
}
1320
}

‎lib/Helper/HTMLParser/AbstractHtmlParser.php‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,15 +8,19 @@
88

99
use OCA\Cookbook\Exception\HtmlParsingException;
1010
use OCP\IL10N;
11+
use Psr\Log\LoggerInterface;
1112

1213
abstract class AbstractHtmlParser {
1314
/**
1415
* @var IL10N
1516
*/
1617
protected $l;
1718

18-
public function __construct(IL10N $l10n) {
19+
protected LoggerInterface $logger;
20+
21+
public function __construct(IL10N $l10n, LoggerInterface $logger) {
1922
$this->l = $l10n;
23+
$this->logger = $logger;
2024
}
2125

2226
/**

‎lib/Helper/HTMLParser/HttpJsonLdParser.php‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
use OCA\Cookbook\Exception\HtmlParsingException;
1010
use OCA\Cookbook\Service\JsonService;
1111
use OCP\IL10N;
12+
use Psr\Log\LoggerInterface;
1213

1314
/**
1415
* 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 {
2021
*/
2122
private $jsonService;
2223

23-
public function __construct(IL10N $l10n, JsonService $jsonService) {
24-
parent::__construct($l10n);
24+
public function __construct(IL10N $l10n, LoggerInterface $logger, JsonService $jsonService) {
25+
parent::__construct($l10n, $logger);
2526

2627
$this->jsonService = $jsonService;
2728
}
@@ -32,6 +33,10 @@ public function parse(\DOMDocument $document, ?string $url): array {
3233

3334
$json_ld_elements = $xpath->query("//*[@type='application/ld+json']");
3435

36+
$this->logger->debug('JSON-LD parser found {count} ld+json elements for {url}', [
37+
'count' => $json_ld_elements->length, 'url' => $url,
38+
]);
39+
3540
foreach ($json_ld_elements as $json_ld_element) {
3641
if (!$json_ld_element || !$json_ld_element->nodeValue) {
3742
continue;
@@ -40,7 +45,9 @@ public function parse(\DOMDocument $document, ?string $url): array {
4045
try {
4146
return $this->parseJsonLdElement($json_ld_element);
4247
} catch (HtmlParsingException $ex) {
43-
// Parsing failed for this element. Let's see if there are more...
48+
$this->logger->debug('JSON-LD element did not contain a valid recipe: {reason}', [
49+
'reason' => $ex->getMessage(),
50+
]);
4451
}
4552
}
4653

@@ -62,6 +69,7 @@ private function parseJsonLdElement(\DOMNode $node): array {
6269
$json = json_decode($string, true);
6370

6471
if ($json === null) {
72+
$this->logger->debug('JSON-LD decode failed: {error}', ['error' => json_last_error_msg()]);
6573
throw new HtmlParsingException($this->l->t('JSON cannot be decoded.'));
6674
}
6775

‎lib/Helper/HTMLParser/HttpMicrodataParser.php‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
use DOMXPath;
1414
use OCA\Cookbook\Exception\HtmlParsingException;
1515
use OCP\IL10N;
16+
use Psr\Log\LoggerInterface;
1617

1718
/**
1819
* This class is an AbstractHtmlParser which tries to extract micro data from the HTML page.
@@ -32,8 +33,8 @@ class HttpMicrodataParser extends AbstractHtmlParser {
3233
*/
3334
private $recipe;
3435

35-
public function __construct(IL10N $l10n) {
36-
parent::__construct($l10n);
36+
public function __construct(IL10N $l10n, LoggerInterface $logger) {
37+
parent::__construct($l10n, $logger);
3738
}
3839

3940
#[\Override]
@@ -49,9 +50,14 @@ public function parse(DOMDocument $document, ?string $url): array {
4950
$recipes = $this->xpath->query($xpathSelector);
5051

5152
if (count($recipes) === 0) {
53+
$this->logger->info('Microdata parser found no Recipe itemscope elements for {url}', ['url' => $url]);
5254
throw new HtmlParsingException($this->l->t('No recipe was found.'));
5355
}
5456

57+
$this->logger->debug('Microdata parser found {count} Recipe itemscope elements for {url}', [
58+
'count' => count($recipes), 'url' => $url,
59+
]);
60+
5561
$this->recipe = [
5662
'@type' => 'Recipe',
5763
'@context' => 'http://schema.org'

‎lib/Helper/HtmlToDomParser.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -87,13 +87,13 @@ public function loadHtmlString(DOMDocument $dom, string $url, string $html): DOM
8787
try {
8888
$this->checkXMLErrors($errors, $url);
8989
} catch (Exception $ex) {
90-
throw new ImportException($this->l->t('Parsing of HTML failed.'), null, $ex);
90+
throw new ImportException($this->l->t('Parsing of HTML failed.'), null, $ex, step: 'parse_html');
9191
}
9292

9393
libxml_clear_errors();
9494

9595
if (!$parsedSuccessfully) {
96-
throw new ImportException($this->l->t('Parsing of HTML failed.'));
96+
throw new ImportException($this->l->t('Parsing of HTML failed.'), step: 'parse_html');
9797
}
9898
} finally {
9999
libxml_use_internal_errors($libxml_previous_state);

‎lib/Service/HtmlDownloadService.php‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -86,9 +86,17 @@ public function __construct(
8686
* @throws ImportException If obtaining of the URL was not possible
8787
*/
8888
public function downloadRecipe(string $url): int {
89+
$this->logger->info('Starting recipe download from {url}', ['url' => $url]);
90+
8991
$html = $this->fetchHtmlPage($url);
9092

91-
// Filter the HTML code
93+
$this->logger->debug('Downloaded {url}: HTTP {status}, content-type {ct}, size {size} bytes', [
94+
'url' => $url,
95+
'status' => $this->downloadHelper->getStatus(),
96+
'ct' => $this->downloadHelper->getContentType() ?? 'unknown',
97+
'size' => strlen($html),
98+
]);
99+
92100
/** @var AbstractHtmlFilter $filter */
93101
foreach ($this->htmlFilters as $filter) {
94102
$filter->apply($html);
@@ -121,7 +129,7 @@ private function fetchHtmlPage(string $url): string {
121129
$host = parse_url($url);
122130

123131
if (!$host) {
124-
throw new ImportException($this->l->t('Could not parse URL'));
132+
throw new ImportException($this->l->t('Could not parse URL'), step: 'download');
125133
}
126134

127135
$opt = [
@@ -150,13 +158,13 @@ private function fetchHtmlPage(string $url): string {
150158
try {
151159
$this->downloadHelper->downloadFile($url, $opt, $headers);
152160
} catch (NoDownloadWasCarriedOutException $ex) {
153-
throw new ImportException($this->l->t('Exception while downloading recipe from %s.: %s', [$url, $ex->getMessage()]), 0, $ex);
161+
throw new ImportException($this->l->t('Exception while downloading recipe from %s.: %s', [$url, $ex->getMessage()]), 0, $ex, step: 'download');
154162
}
155163

156164
$status = $this->downloadHelper->getStatus();
157165

158166
if ($status < 200 || $status >= 300) {
159-
throw new ImportException($this->l->t('Download from %s failed as HTTP status code %d is not in expected range.', [$url, $status]));
167+
throw new ImportException($this->l->t('Download from %s failed as HTTP status code %d is not in expected range.', [$url, $status]), step: 'download');
160168
}
161169

162170
$html = $this->downloadHelper->getContent();

‎lib/Service/RecipeExtractionService.php‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
use OCA\Cookbook\Helper\HTMLParser\HttpJsonLdParser;
1111
use OCA\Cookbook\Helper\HTMLParser\HttpMicrodataParser;
1212
use OCP\IL10N;
13+
use Psr\Log\LoggerInterface;
1314

1415
class RecipeExtractionService {
1516
/**
@@ -22,10 +23,13 @@ class RecipeExtractionService {
2223
*/
2324
private $l;
2425

26+
private LoggerInterface $logger;
27+
2528
public function __construct(HttpJsonLdParser $jsonParser, HttpMicrodataParser $microdataParser,
26-
IL10N $l10n) {
29+
IL10N $l10n, LoggerInterface $logger) {
2730
$this->parsers = [$jsonParser, $microdataParser];
2831
$this->l = $l10n;
32+
$this->logger = $logger;
2933
}
3034

3135
/**
@@ -37,14 +41,26 @@ public function __construct(HttpJsonLdParser $jsonParser, HttpMicrodataParser $m
3741
* @return array The data as returned from the parser
3842
*/
3943
public function parse(\DOMDocument $document, ?string $url): array {
44+
$failures = [];
45+
4046
foreach ($this->parsers as $parser) {
4147
try {
4248
return $parser->parse($document, $url);
4349
} catch (HtmlParsingException $ex) {
44-
// Silently ignore failure as there might be other parsers better suited
50+
$parserName = (new \ReflectionClass($parser))->getShortName();
51+
$failures[] = "$parserName: {$ex->getMessage()}";
52+
$this->logger->info('Parser {parser} failed for URL {url}: {reason}', [
53+
'parser' => $parserName, 'url' => $url, 'reason' => $ex->getMessage(),
54+
]);
4555
}
4656
}
4757

48-
throw new HtmlParsingException($this->l->t('No parser found for the given import.'));
58+
$detail = implode('; ', $failures);
59+
$this->logger->warning('All parsers failed for URL {url}. Details: {detail}', [
60+
'url' => $url, 'detail' => $detail,
61+
]);
62+
throw new HtmlParsingException(
63+
$this->l->t('No parser found for the given import.') . ' ' . $detail
64+
);
4965
}
5066
}

‎lib/Service/RecipeService.php‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -365,15 +365,19 @@ public function downloadRecipe(string $url): File {
365365
$json = $this->recipeExtractionService->parse($this->htmlDownloadService->getDom(), $url);
366366
$importedHtml = $this->htmlDownloadService->getDom()->saveHTML();
367367
} catch (HtmlParsingException $ex) {
368-
throw new ImportException($ex->getMessage(), null, $ex);
368+
throw new ImportException($ex->getMessage(), null, $ex, step: 'extract_recipe');
369369
}
370370

371+
$this->logger->info('Recipe extracted from {url}: name="{name}"', [
372+
'url' => $url, 'name' => $json['name'] ?? '(unnamed)',
373+
]);
374+
371375
$json = $this->checkRecipe($json);
372376

373377
if (!$json) {
374378
$this->logger->error('Importing parsers resulted in null recipe.'
375379
. 'This is most probably a bug. Please report.');
376-
throw new ImportException($this->il10n->t('No recipe data found. This is a bug'));
380+
throw new ImportException($this->il10n->t('No recipe data found. This is a bug'), step: 'extract_recipe');
377381
}
378382

379383
$json['url'] = $url;

‎src/components/AppNavi.vue‎

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -271,20 +271,15 @@ const downloadRecipe = async () => {
271271
272272
if (e2.response) {
273273
if (e2.response.status >= 400 && e2.response.status < 500) {
274-
if (e2.response.status === 409) {
275-
// There was a recipe found with the same name
276-
277-
const message =
278-
typeof e2.response.data === 'string'
279-
? e2.response.data
280-
: e2.response.data.msg;
281-
await showSimpleAlertModal(t('cookbook', 'Error'), message);
282-
} else {
283-
await showSimpleAlertModal(
284-
t('cookbook', 'Error'),
285-
e2.response.data,
286-
);
287-
}
274+
const data = e2.response.data;
275+
const message =
276+
typeof data === 'string'
277+
? data
278+
: (data.msg ?? t('cookbook', 'Import failed.'));
279+
await showSimpleAlertModal(
280+
t('cookbook', 'Import Error'),
281+
message,
282+
);
288283
} else {
289284
console.error(e2);
290285
await showSimpleAlertModal(

0 commit comments

Comments
 (0)