Repository navigation
feat: enhance import logging - #3510
Open
TheTisiboth wants to merge 9 commits into
Open
TheTisiboth wants to merge 9 commits into
TheTisiboth wants to merge 9 commits into
Conversation
Contributor
Test Results 12 files 604 suites 3m 29s ⏱️ Results for commit a17895b. ♻️ This comment has been updated with latest results. |
TheTisiboth
force-pushed
the
feat/enhance-import-logging
branch
from
October 26, 2026 19:38
9ff8dc5 to
8efea9f
Compare
2 of 4 tasks
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 <noreply@anthropic.com>
Signed-off-by: Léo Jan <leo.jan@doctolib.com>
TheTisiboth
force-pushed
the
feat/enhance-import-logging
branch
from
October 26, 2026 21:55
8efea9f to
e3dc968
Compare
…-logging Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Léo Jan <leo.jan@doctolib.com>
Assisted-by: Claude Code:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Léo Jan <leo.jan@doctolib.com>
TheTisiboth
marked this pull request as ready for review
October 29, 2026 18:59
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
Merge Risk:
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 76.92% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title "feat: enhance import logging" matches the main change. The changeset adds logging across the recipe import pipeline (download, HTML parsing, extraction), adds an ImportException step field,… Description check The description follows the template's main sections. It has a Topic and Scope section that explains the logging additions, the step field on ImportException, and URL redaction. It has a Concerns/issu… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title "feat: enhance import logging" matches the main change. The changeset adds logging across the recipe import pipeline (download, HTML parsing, extraction), adds an ImportException step field, and adds a URL redaction helper for logged URLs. The title is short and specific, and it uses the conventional commit style.
Full details: Description check
-
Explanation
The description follows the template's main sections. It has a Topic and Scope section that explains the logging additions, the step field on ImportException, and URL redaction. It has a Concerns/issues section that notes the backward-compatible constructor parameter and the out-of-scope debug dump feature. All four formal requirement boxes are checked. The description is mostly complete. However, the formal requirement checkboxes claim OpenAPI spec updates and a Matrix notification, while the stated scope says no API response change. The commit messages also say the structured error response changes moved to a separate PR, which conflicts with the description's list of those changes. These conflicts do not make the description fail, but the author should reconcile them.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 76.92% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 13 functions across 14 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
RecipeImplementation.php:326-328
lib/Controller/Implementation/RecipeImplementation.php:326-328
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winApply the generic import handler to cache insertion.
DbCacheService::addRecipe()remains outside the new generic handler. Its database and cache-update operations can raise exceptions that escapeRecipeImplementation::import()and bypass the documented generic{msg}response.This block existed before the PR, but the PR adds a broad contract that unexpected import errors return HTTP 500 with a generic message. Extend that handling to cache insertion while preserving the existing
InvalidJSONFileExceptioncleanup.
-
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
4df5fdd1-4015-410b-899d-507f8b67f401
-
docs/dev/api/changelog/0.mddocs/dev/api/trunk/internal-cookbook.yamldocs/dev/api/trunk/objects.yamldocs/dev/api/trunk/openapi-cookbook.yamllib/Controller/Implementation/RecipeImplementation.phplib/Exception/ImportException.phplib/Helper/HTMLParser/AbstractHtmlParser.phplib/Helper/HTMLParser/HttpJsonLdParser.phplib/Helper/HTMLParser/HttpMicrodataParser.phplib/Helper/HtmlToDomParser.phplib/Service/HtmlDownloadService.phplib/Service/RecipeExtractionService.phplib/Service/RecipeService.phpsrc/components/AppNavi.vuetests/Unit/Controller/Implementation/RecipeImplementationTest.phptests/Unit/Helper/HTMLParser/HttpJsonLdParserTest.phptests/Unit/Helper/HTMLParser/HttpMicrodataParserTest.phptests/Unit/Service/RecipeExtractionServiceTest.php
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.
-
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 <noreply@anthropic.com> Signed-off-by: Léo Jan <leo.jan@doctolib.com>
TheTisiboth
enabled stacked auto-merge (squash)
October 29, 2026 19:43
There was a problem hiding this comment.
Actionable comments posted: 1
-
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
a8fbf4f0-0f2a-4b91-b528-de93acd6c9b0
-
docs/dev/api/changelog/0.mddocs/dev/api/trunk/internal-cookbook.yamldocs/dev/api/trunk/openapi-cookbook.yamllib/Controller/Implementation/RecipeImplementation.phptests/Unit/Controller/Implementation/RecipeImplementationTest.php
-
- docs/dev/api/changelog/0.md
- docs/dev/api/trunk/openapi-cookbook.yaml
- docs/dev/api/trunk/internal-cookbook.yaml
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
-
Assisted-by: Claude Code:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Léo Jan <leo.jan@doctolib.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🔇 Additional comments: 2
lib/Service/RecipeService.php-358-358: 1
358-358:Minor · Security & Privacy · Quick win
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Do not log the recipe name.
The parsed
nameis user content. This new info log retains it on every successful extraction, including when the name contains private details. Log extraction success without the name. (raw.githubusercontent.com)lib/Helper/UrlRedactionHelper.php-23-23: 1
23-23:Major · Security & Privacy · Quick win
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Remove the path from logged URLs.
If an import URL contains a token in its path,
redact()retains that token. Download and parser logs then persist it. Return the scheme and host, with the port if needed, but not the path. (raw.githubusercontent.com)Based on learnings, full URLs can embed API keys or tokens in the path.
Source: Learnings
-
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
c3b03ef5-3e0e-49b9-a772-3add014cab91
-
lib/Controller/Implementation/RecipeImplementation.phplib/Helper/HTMLParser/HttpJsonLdParser.phplib/Helper/HTMLParser/HttpMicrodataParser.phplib/Helper/UrlRedactionHelper.phplib/Service/HtmlDownloadService.phplib/Service/RecipeExtractionService.phplib/Service/RecipeService.phptests/Unit/Helper/UrlRedactionHelperTest.php
-
- lib/Service/HtmlDownloadService.php
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.
-
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 <noreply@anthropic.com> Signed-off-by: Léo Jan <leo.jan@doctolib.com>
There was a problem hiding this comment.
Actionable comments posted: 1
-
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
2a7d43ac-5502-4fcc-9fa4-02886edc6737
-
lib/Service/RecipeService.phptests/Unit/Controller/Implementation/RecipeImplementationTest.php
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.
-
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 <noreply@anthropic.com> Signed-off-by: Léo Jan <leo.jan@doctolib.com>
3 of 4 tasks
TheTisiboth
changed the title
christianlupus
disabled stacked auto-merge
November 3, 2026 22:28
christianlupus
previously approved these changes
Nov 3, 2026
| 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, |
Collaborator
There was a problem hiding this comment.
Why do you put more entries into the array of snippet (second argument)?
Collaborator
Author
There was a problem hiding this comment.
exception is a special key for the NC logger: it logs the stack trace.
the second argument is the context array: the keys are kept as extra field in the logs, and will look like this:
{
"reqId": "aB3x...",
"level": 2,
"time": "2026-11-09T10:00:00+00:00",
"user": "alice",
"app": "cookbook",
"method": "POST",
"url": "/index.php/apps/cookbook/api/v1/import",
"message": "Import failed at step \"download\": Could not fetch site https://example.com/recipe",
"exception": {
"Exception": "OCA\\Cookbook\\Exception\\ImportException",
"Message": "Could not fetch site https://example.com/recipe",
"Code": 0,
"Trace": [ ... ],
"File": ".../HtmlDownloadService.php",
"Line": 123,
"Previous": { ... },
"message": "Import failed at step \"download\": {reason}",
"url": "https://example.com/recipe",
"CustomMessage": "Import failed at step \"download\": Could not fetch ..."
}
}
Collaborator
Author
There was a problem hiding this comment.
i'm adding the url to the message to make the message more explicit
Collaborator
Author
There was a problem hiding this comment.
and even if exception is not used in the log message, it will be logged as a metada kind of (adding the stracktrace, file and so on
| 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, |
Collaborator
There was a problem hiding this comment.
Url and exception seem not used, isn't it?
Collaborator
Author
There was a problem hiding this comment.
NC logs the stack trace (from exception)
url was only logged as an extra field, but we could also add it to the message for clarity
Bundle ReportChanges will decrease total bundle size by 7.62kB (-0.27%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: cookbook-esmAssets Changed:
|
Assisted-by: Claude Code:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Léo Jan <leo.jan@doctolib.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses #3279
Followed by #3538 (stacked).
Topic and Scope
Adds logging throughout the recipe import pipeline so admins can see in
nextcloud.logwhy an import failed. No change to API responses.ImportExceptioncarries the failingstep(download,parse_html,extract_recipe), logged on failureUrlRedactionHelper)Concerns/issues
ImportExceptionnew last paramstring $step = ''is backward-compatibleFormal requirements
Note
This PR was created with the assistance of AI tools.
Assisted-by: Claude Code (claude-opus-4-6, claude-opus-5-5)
🤖 Generated with Claude Code