🐛(backend) return 400 when restoring a non-deleted document#2225
🐛(backend) return 400 when restoring a non-deleted document#2225maboukerfa wants to merge 1 commit intosuitenumerique:mainfrom
Conversation
Catch RuntimeError raised by Document.restore() and translate it into a DRF ValidationError so callers get a 400 instead of a 500, when trying to restore a non-deleted document. Signed-off-by: Mohamed El Amine BOUKERFA <boukerfa.ma@gmail.com>
WalkthroughThe changes introduce error handling for the document restore endpoint. The Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/core/api/viewsets.py`:
- Around line 993-996: Replace the except that currently raises
drf.exceptions.ValidationError when document.restore() fails with returning a
DRF Response using the same error payload format used elsewhere in this viewset;
specifically, in the except block for RuntimeError from document.restore()
return Response({"detail": str(err)}) with an appropriate client error status
(e.g., status.HTTP_400_BAD_REQUEST) so the response body matches tests and other
handlers in this viewset.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: debd275c-a935-4638-982b-5fead76c31f7
📒 Files selected for processing (2)
src/backend/core/api/viewsets.pysrc/backend/core/tests/documents/test_api_documents_restore.py
| try: | ||
| document.restore() | ||
| except RuntimeError as err: | ||
| raise drf.exceptions.ValidationError({"detail": str(err)}) from err |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there is a custom DRF exception handler that could reshape ValidationError dicts.
rg -nP -C3 'EXCEPTION_HANDLER|exception_handler\s*=|def\s+\w*exception_handler' --type=pyRepository: suitenumerique/docs
Length of output: 1168
🏁 Script executed:
cat -n src/backend/core/api/__init__.pyRepository: suitenumerique/docs
Length of output: 1700
🏁 Script executed:
#!/bin/bash
# Find the test file and look for the specific test
fd -type f -name "*.py" | xargs rg -l "test_api_documents_restore_authenticated_owner_not_deleted" | head -5Repository: suitenumerique/docs
Length of output: 313
🏁 Script executed:
#!/bin/bash
# Check the specific lines mentioned in the review
sed -n '932,935p; 1890,1893p; 2129,2132p; 2143,2146p' src/backend/core/api/viewsets.pyRepository: suitenumerique/docs
Length of output: 746
🏁 Script executed:
cat -n src/backend/core/tests/documents/test_api_documents_restore.pyRepository: suitenumerique/docs
Length of output: 6022
🏁 Script executed:
#!/bin/bash
# Let's also check what drf_views.exception_handler does with ValidationError
# by looking at how other tests handle this
rg -A 10 "test_api_documents_restore_authenticated_owner_not_deleted" src/backend/core/tests/documents/test_api_documents_restore.pyRepository: suitenumerique/docs
Length of output: 501
🏁 Script executed:
sed -n '985,1005p' src/backend/core/api/viewsets.pyRepository: suitenumerique/docs
Length of output: 704
🏁 Script executed:
#!/bin/bash
# Check if there are any other exception handlers or response formatters
rg -n "exception_handler|format_response|render" src/backend/impress/settings.py | head -20Repository: suitenumerique/docs
Length of output: 182
🏁 Script executed:
#!/bin/bash
# Also check if ValidationError gets special handling anywhere else
rg -n "ValidationError" src/backend/core/api/viewsets.py | head -15Repository: suitenumerique/docs
Length of output: 1023
🏁 Script executed:
#!/bin/bash
# Let's check if there's middleware or other configurations affecting responses
rg -n "MIDDLEWARE|REST_FRAMEWORK" src/backend/impress/settings.py | head -30Repository: suitenumerique/docs
Length of output: 112
🏁 Script executed:
#!/bin/bash
# Let's trace what happens when ValidationError is raised with a dict
# Check if there's any custom renderer or response formatting
rg -n "class.*Renderer\|def.*render" src/backend/impress/settings.py src/backend/core/api/Repository: suitenumerique/docs
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Let's look more closely at what drf_views.exception_handler does with ValidationError
# by checking if there are any tests that show the actual response format
rg -A 5 "ValidationError.*detail" src/backend/core/tests/ | head -50Repository: suitenumerique/docs
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Let's check if there are any other exception handlers or decorators in the file
sed -n '1,100p' src/backend/core/api/viewsets.py | grep -n "exception\|decorator\|handler"Repository: suitenumerique/docs
Length of output: 204
🏁 Script executed:
#!/bin/bash
# Let's also check what document.restore() raises to understand the RuntimeError
rg -n "def restore" src/backend --type py -A 10Repository: suitenumerique/docs
Length of output: 1662
🏁 Script executed:
#!/bin/bash
# Let's verify what DRF does with ValidationError by checking imports and understanding the flow
# Check if there's any custom response formatting
rg -n "class ValidationError\|def validate_" src/backend/core/api/viewsets.py | head -20Repository: suitenumerique/docs
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Let's look at what other ValidationError raises look like in the file and what they do
sed -n '680,710p; 700,730p' src/backend/core/api/viewsets.pyRepository: suitenumerique/docs
Length of output: 2651
🏁 Script executed:
#!/bin/bash
# Let's verify the exact pattern used at the referenced lines
sed -n '930,940p; 1888,1898p; 2127,2137p; 2141,2151p' src/backend/core/api/viewsets.pyRepository: suitenumerique/docs
Length of output: 1566
🏁 Script executed:
#!/bin/bash
# Check imports in viewsets.py to see what's available
head -50 src/backend/core/api/viewsets.pyRepository: suitenumerique/docs
Length of output: 1882
🏁 Script executed:
#!/bin/bash
# Let's verify if there's any custom response formatting that might convert lists to strings
rg -n "detail.*\[0\]\|detail\[0\]" src/backend/core/api/Repository: suitenumerique/docs
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Let's check if the test actually runs and passes by looking for test configuration
find src/backend -name "pytest.ini" -o -name "setup.cfg" -o -name "pyproject.toml" | xargs grep -l "pytest" 2>/dev/nullRepository: suitenumerique/docs
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Let's trace through what RestFramework's exception_handler does
# by checking if there's any custom formatting or if the issue is as described
rg -n "get_error_details\|normalize" src/backend/core/api/ --type pyRepository: suitenumerique/docs
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Let's run the test to see what actually happens
cd src/backend && python -m pytest core/tests/documents/test_api_documents_restore.py::test_api_documents_restore_authenticated_owner_not_deleted -v 2>&1 | head -100Repository: suitenumerique/docs
Length of output: 282
🏁 Script executed:
#!/bin/bash
# Let's understand what DRF does by checking the actual documentation
# and looking at how ValidationError is used elsewhere in the codebase
rg -B 3 -A 3 "raise drf.exceptions.ValidationError" src/backend/core/api/viewsets.py | head -80Repository: suitenumerique/docs
Length of output: 2975
🏁 Script executed:
#!/bin/bash
# Let's also check if there are any middleware or response transformers
grep -r "get_error_details\|ErrorDetail" src/backend/core/ --include="*.py" | head -20Repository: suitenumerique/docs
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Let's verify by looking at what DRF does with ValidationError
# Check if there are other similar patterns in tests that show ValidationError responses
rg -B 2 -A 2 "assert response.json()" src/backend/core/tests/documents/test_api_documents_restore.pyRepository: suitenumerique/docs
Length of output: 888
🏁 Script executed:
#!/bin/bash
# Let's also check what happens with ValidationError vs Response patterns in other tests
rg "assert response.json().*detail" src/backend/core/tests/ | head -20Repository: suitenumerique/docs
Length of output: 2692
🏁 Script executed:
#!/bin/bash
# Let's check if there's a pattern of ValidationError being used and what format it produces
# by looking at tests for other ValidationError cases
rg -B 5 -A 2 'assert response.json().*\["' src/backend/core/tests/ | head -50Repository: suitenumerique/docs
Length of output: 4288
🏁 Script executed:
#!/bin/bash
# Let's look for any ErrorDetail or list-based assertions
rg 'assert.*\[\s*"' src/backend/core/tests/documents/test_api_documents_restore.pyRepository: suitenumerique/docs
Length of output: 45
Use Response instead of raising ValidationError to match the response format pattern established throughout this viewset.
The code at lines 993-996 raises drf.exceptions.ValidationError({"detail": str(err)}), but the surrounding viewset consistently returns error responses using Response() (see lines 932-935, 1890-1893, 2129-2132, 2143-2146). The test test_api_documents_restore_authenticated_owner_not_deleted expects {"detail": "This document is not deleted."} as a string, which aligns with the Response() pattern used throughout the file rather than the ValidationError approach that would normalize to a list format.
♻️ Proposed refactor
document = self.get_object()
- try:
- document.restore()
- except RuntimeError as err:
- raise drf.exceptions.ValidationError({"detail": str(err)}) from err
+ try:
+ document.restore()
+ except RuntimeError as err:
+ return drf_response.Response(
+ {"detail": str(err)},
+ status=status.HTTP_400_BAD_REQUEST,
+ )
return drf_response.Response(
{"detail": "Document has been successfully restored."},
status=status.HTTP_200_OK,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| document.restore() | |
| except RuntimeError as err: | |
| raise drf.exceptions.ValidationError({"detail": str(err)}) from err | |
| document = self.get_object() | |
| try: | |
| document.restore() | |
| except RuntimeError as err: | |
| return drf_response.Response( | |
| {"detail": str(err)}, | |
| status=status.HTTP_400_BAD_REQUEST, | |
| ) | |
| return drf_response.Response( | |
| {"detail": "Document has been successfully restored."}, | |
| status=status.HTTP_200_OK, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/core/api/viewsets.py` around lines 993 - 996, Replace the except
that currently raises drf.exceptions.ValidationError when document.restore()
fails with returning a DRF Response using the same error payload format used
elsewhere in this viewset; specifically, in the except block for RuntimeError
from document.restore() return Response({"detail": str(err)}) with an
appropriate client error status (e.g., status.HTTP_400_BAD_REQUEST) so the
response body matches tests and other handlers in this viewset.
1 participant
Purpose
When attempting to restore a non-deleted document, the server would crash with a 500 Internal Server Error.
Proposal
Handling the exception and returning a 400 Bad Request instead of a 500 Internal Server Error.
Before