Conversation
… detail reporting
📝 WalkthroughWalkthroughRenames and refactors a validation-error helper to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/app/crud/config/version.py (1)
67-69: Use the current function name in the log prefix.Line 68 logs
[ConfigVersionCrud.create_from_partial]while the method isConfigVersionCrud.create_or_raise. This makes log filtering inconsistent.As per coding guidelines, Prefix all log messages with the function name in square brackets:
logger.info(f"[function_name] Message {mask_string(sensitive_value)}").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/crud/config/version.py` around lines 67 - 69, The log prefix is using the wrong method name; update the logger.error call inside ConfigVersionCrud.create_or_raise to use "[ConfigVersionCrud.create_or_raise]" instead of "[ConfigVersionCrud.create_from_partial]" so log filtering is consistent (locate the logger.error call in the create_or_raise method and adjust the f-string prefix accordingly; preserve the existing message and interpolation).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/crud/config/version.py`:
- Line 71: The function create_or_raise currently logs with the wrong prefix and
returns raw Pydantic errors (e.errors()), risking sensitive data leakage; update
the log message prefix from "[ConfigVersionCrud.create_from_partial]" to
"[ConfigVersionCrud.create_or_raise]" and replace the direct e.errors() response
with a sanitized list that extracts only non-sensitive fields (for each error
keep loc, msg, and type and omit input, ctx, url, etc.), then raise
HTTPException(status_code=400, detail=sanitized_errors). Ensure you reference
the exception variable e and the create_or_raise function when making these
changes.
---
Nitpick comments:
In `@backend/app/crud/config/version.py`:
- Around line 67-69: The log prefix is using the wrong method name; update the
logger.error call inside ConfigVersionCrud.create_or_raise to use
"[ConfigVersionCrud.create_or_raise]" instead of
"[ConfigVersionCrud.create_from_partial]" so log filtering is consistent (locate
the logger.error call in the create_or_raise method and adjust the f-string
prefix accordingly; preserve the existing message and interpolation).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dca19b01-227a-4790-8e9b-823351221a0e
📒 Files selected for processing (2)
backend/app/core/exception_handlers.pybackend/app/crud/config/version.py
backend/app/crud/config/version.py
Outdated
| status_code=400, | ||
| detail=f"Invalid config after merge: {str(e)}", | ||
| ) | ||
| raise HTTPException(status_code=400, detail=e.errors()) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Pydantic v2, what fields are included by default in ValidationError.errors(), and how do include_input/include_context/include_url flags change the output?
💡 Result:
In Pydantic v2, ValidationError.errors returns a list of ErrorDetails dictionaries. By default (include_input=True, include_context=True, include_url=True), each error includes these fields when available: - 'type': computer-readable error type identifier (always) - 'loc': location tuple (always) - 'msg': human-readable message (always) - 'input': the invalid input value (default True) - 'ctx': validation context (e.g., constraints like gt=42; default True) - 'url': documentation URL (default True) The flags control inclusion as follows: - include_input=False: omits 'input' from each error - include_context=False: omits 'ctx' (called 'context' in some docs) - include_url=False: omits 'url' from each error Example from docs: [ { 'type': 'missing', 'loc': ('is_required',), 'msg': 'Field required', 'input': {...}, # omitted if include_input=False 'url': 'https://errors.pydantic.dev/2/v/missing' # omitted if include_url=False }, { 'type': 'greater_than', ... 'ctx': {'gt': 42}, # omitted if include_context=False ... } ]
Citations:
- 1: https://docs.pydantic.dev/latest/api/pydantic_core/
- 2: https://docs.pydantic.dev/latest/errors/errors/
- 3: https://docs.pydantic.dev/2.3/errors/errors/
🏁 Script executed:
# Read the file context around lines 67-71
cat -n backend/app/crud/config/version.py | sed -n '65,75p'Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 603
🏁 Script executed:
# Find and check requirements files for Pydantic version
find . -name "requirements*.txt" -o -name "pyproject.toml" -o -name "setup.py" | head -20Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 99
🏁 Script executed:
# Check the Python version specified in the project
git ls-files | grep -E "(setup.py|pyproject.toml|.python-version)" | head -10Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 164
🏁 Script executed:
# Read pyproject.toml to check Pydantic version
cat backend/pyproject.toml | grep -A 50 "dependencies"Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 2487
🏁 Script executed:
# Find the function containing line 71
sed -n '50,72p' backend/app/crud/config/version.py | head -30Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 1024
🏁 Script executed:
# Find the function definition containing line 71 by searching backwards from line 71
sed -n '1,71p' backend/app/crud/config/version.py | tac | grep -m 1 "def " | head -1Repository: ProjectTech4DevAI/kaapi-backend
Length of output: 159
🏁 Script executed:
# Get broader context to see the function definition
sed -n '40,72p' backend/app/crud/config/version.pyRepository: ProjectTech4DevAI/kaapi-backend
Length of output: 1366
Fix log prefix and sanitize Pydantic validation errors.
Line 71 returns e.errors() directly; this exposes verbose input, ctx, and url fields that may leak sensitive config values. Additionally, the log message on line 68 uses [ConfigVersionCrud.create_from_partial] but the actual function is create_or_raise, violating the log-prefix guideline.
Suggested fix
logger.error(
- f"[ConfigVersionCrud.create_from_partial] Validation failed | "
+ f"[ConfigVersionCrud.create_or_raise] Validation failed | "
f"{{'config_id': '{self.config_id}', 'error': '{str(e)}'}}"
)
- raise HTTPException(status_code=400, detail=e.errors())
+ safe_errors = e.errors(
+ include_input=False,
+ include_context=False,
+ include_url=False,
+ )
+ raise HTTPException(status_code=400, detail=safe_errors)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/crud/config/version.py` at line 71, The function create_or_raise
currently logs with the wrong prefix and returns raw Pydantic errors
(e.errors()), risking sensitive data leakage; update the log message prefix from
"[ConfigVersionCrud.create_from_partial]" to
"[ConfigVersionCrud.create_or_raise]" and replace the direct e.errors() response
with a sanitized list that extracts only non-sensitive fields (for each error
keep loc, msg, and type and omit input, ctx, url, etc.), then raise
HTTPException(status_code=400, detail=sanitized_errors). Ensure you reference
the exception variable e and the create_or_raise function when making these
changes.
There was a problem hiding this comment.
http_exception_handler calls _filter_union_branch_errors which sanitize the e.errors() so client would not getting any sensitive config values
There was a problem hiding this comment.
🧩 Analysis chain
Let me verify this by checking the actual implementation of _filter_union_branch_errors in the exception handler.
🏁 Script executed:
#!/bin/bash
cat backend/app/core/exception_handlers.py
Length of output: 3986
@vprashrex, I've read the full implementation of _filter_union_branch_errors. While the http_exception_handler does pass the list through it, that function's purpose is specifically union-branch deduplication and loc path cleanup — it does not strip sensitive fields from each error dict.
Here's what _filter_union_branch_errors does and doesn't do:
| Concern | Handled? |
|---|---|
| Selects the most relevant union branch | ✅ Yes |
Strips branch identifiers from loc |
✅ Yes |
Deduplicates by (loc, msg) |
✅ Yes |
Removes input (actual submitted value) |
❌ No |
Removes ctx (constraint context) |
❌ No |
Removes url (Pydantic docs URL) |
❌ No |
Since e.errors() in Pydantic v2 includes input by default — which contains the actual value that failed validation — sensitive config data (e.g. API keys, model parameters, or any nested config blob values) would still be forwarded to the client after passing through _filter_union_branch_errors.
The suggested fix remains relevant:
safe_errors = e.errors(
include_input=False,
include_context=False,
include_url=False,
)
raise HTTPException(status_code=400, detail=safe_errors)This strips input, ctx, and url at the source, before the exception handler even sees the data. The log prefix (create_from_partial → create_or_raise) is also still a valid fix.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
backend/app/crud/config/version.py
Outdated
| f"{{'config_id': '{self.config_id}', 'error_count': {len(e.errors())}, " | ||
| f"'fields': {['.'.join(str(l) for l in err['loc']) for err in e.errors()]}}}" | ||
| ) | ||
| raise HTTPException(status_code=400, detail=e.errors()) |
There was a problem hiding this comment.
When raising validation error please note that don't use str(e). use e.errors(). so that exception_handlers can pick it up and send the validation error in an structured way to the client
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/crud/config/version.py`:
- Around line 67-72: The logger.error call in ConfigVersionCrud.create_or_raise
uses an ambiguous loop variable `l` and calls e.errors() multiple times; fix by
caching e.errors() to a local variable (e.g., errors = e.errors()) and replace
the comprehension to use a descriptive name like loc_part or loc_item when
joining locations, then reference errors and its length (len(errors)) in the
formatted log and use errors when raising the HTTPException (raise
HTTPException(status_code=400, detail=errors)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72a7213b-4ced-477c-a06e-29c43bf5516a
📒 Files selected for processing (3)
backend/app/core/exception_handlers.pybackend/app/crud/config/version.pybackend/app/tests/core/test_exception_handlers.py
1 participant
Summary
Target issue is #688
Explain the motivation for making this change. What existing problem does the pull request solve?
The Version Create API raises validation errors as str(e) instead of e.errors(), causing the exception handler to return unstructured raw error responses instead of properly formatted ones.
like this below:
Now it raises the error like this after fix
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
Please add here if any other information is required for the reviewer.
Summary by CodeRabbit