Refactor: migrate doc upload info used in chat#14359
Merged
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughReplaces the legacy Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Browser)
participant Frontend as Frontend (hook/service)
participant API as API Server (/documents/upload)
participant FileSvc as FileService
Client->>Frontend: Trigger upload (files or ?url=...)
Frontend->>API: POST /api/v1/documents/upload (FormData or query `url`)
API->>API: assert_url_is_safe(url) %% for URL path
API->>FileSvc: FileService.upload_info(tenant_id, files or url)
FileSvc-->>API: upload metadata/result
API-->>Frontend: JSON result (get_result)
Frontend-->>Client: onSuccess / onError handlers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
dosubot
Bot
added
the
size:L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 platform limitations.
⚠️ Outside diff range comments (1)
web/src/hooks/use-chat-request.ts (1)
491-505:⚠️ Potential issue | 🟠 MajorRemove
conversation_idfrom FormData or use a conversation-scoped upload endpoint instead.The
POST /documents/uploadendpoint is a generic document info extraction service that only acceptsfileandurlparameters. It callsFileService.upload_info()which returns basic file metadata without any conversation or dataset association. Theconversation_idsent in FormData (line 493) is ignored.If chat-scoped document linking is required, this implementation is incomplete. A separate conversation-scoped upload flow exists in
document_app.pythat callsdoc_upload_and_parse(conversation_id, ...)and properly resolves the conversation to its associated dataset before uploading. The frontend should either:
- Use that conversation-scoped endpoint instead, or
- Remove
conversation_idfrom this endpoint and handle dataset routing differently in the chat context🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/hooks/use-chat-request.ts` around lines 491 - 505, The FormData currently includes a conversation_id which is ignored by the generic upload endpoint; update the frontend to either remove the conversation_id from the FormData when calling chatService.documentInfoUpload (api.documentInfoUpload) so only file (and url if used) are sent, or switch to the conversation-scoped upload endpoint used by document_app.py that invokes doc_upload_and_parse(conversation_id, ...) to correctly resolve dataset linking; locate the upload logic in use-chat-request.ts (the FormData append calls and the chatService.documentInfoUpload invocation) and implement one of these two fixes so the backend FileService.upload_info() receives only supported fields or use the proper conversation-scoped flow.
🧹 Nitpick comments (2)
api/apps/restful_apis/document_api.py (1)
74-75: Minor: redundantfiles.get("file")guard.
MultiDict.getlist("file")already returns[]when the key is absent, sofiles and files.get("file")is redundant and slightly misleading (it would also short-circuit to[]if a caller sentfile="", which is arguably fine but handled elsewhere by an explicit empty-name check).- files = await request.files - file_objs = files.getlist("file") if files and files.get("file") else [] + files = await request.files + file_objs = files.getlist("file") if files else []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/apps/restful_apis/document_api.py` around lines 74 - 75, The guard checking files and files.get("file") is redundant; change the assignment so file_objs is obtained directly from the MultiDict using files.getlist("file") (i.e., replace the conditional expression with a direct call to files.getlist("file")), leaving the preceding files = await request.files and any existing explicit empty-name checks intact; this simplifies the logic and relies on MultiDict.getlist to return [] when the key is absent.web/src/hooks/use-chat-request.ts (1)
47-47: Consider renaming staleUploadAndParse/uploadAndParseFileidentifiers for consistency.The endpoint and service mapping were renamed to
documentInfoUpload, but theChatApiAction.UploadAndParse = 'upload_and_parse'enum (Line 47), the mutation key, and the exporteduploadAndParseFile(Line 528) still carry the old name. Callers likeweb/src/pages/next-chats/hooks/use-upload-file.tscontinue to importuseUploadAndParseFile/ destructureuploadAndParseFile. This is functionally fine but will drift further as the migration continues.If you want a clean rename in this PR, update the enum, mutation key, return property, hook name, and the two call sites in
use-upload-file.tsanduse-send-shared-message(if applicable) in one pass. Otherwise, fine to defer.Also applies to: 528-528
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/hooks/use-chat-request.ts` at line 47, Rename the stale UploadAndParse identifiers to documentInfoUpload for consistency: update the enum entry ChatApiAction.UploadAndParse = 'upload_and_parse' to ChatApiAction.DocumentInfoUpload = 'document_info_upload', change the mutation key and the exported function name uploadAndParseFile to documentInfoUploadFile (and the hook useUploadAndParseFile to useDocumentInfoUploadFile), and update all call sites mentioned (use-upload-file.ts and use-send-shared-message imports/destructuring) to use the new names so names match the endpoint/service mapping documentInfoUpload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/apps/restful_apis/document_api.py`:
- Around line 84-94: The handler currently calls the blocking
FileService.upload_info synchronously (and in the URL case triggers asyncio.run
inside an existing event loop), so change all branches (url, single file,
multiple files) to offload FileService.upload_info to the worker thread using
the existing thread_pool_exec helper (same pattern as upload_document): call
await thread_pool_exec(partial(FileService.upload_info, tenant_id,
file_obj_or_None, url_or_None)) for each invocation, collect results for the
list branch, and then return get_result(data=results) (or single result) so the
Quart event loop is not blocked and asyncio.run inside upload_info executes in a
fresh loop in the worker thread.
- Around line 42-94: The upload_info handler is missing logging and filename
validation: add debug/info logs for start, success and error paths in
upload_info (similar to upload_document) and validate each uploaded file's
filename against empty/None and FILE_NAME_LEN_LIMIT before calling
FileService.upload_info; on invalid names return get_error_argument_result with
a 400-like message and log the validation failure, and ensure exceptions still
use server_error_response while logging the exception.
---
Outside diff comments:
In `@web/src/hooks/use-chat-request.ts`:
- Around line 491-505: The FormData currently includes a conversation_id which
is ignored by the generic upload endpoint; update the frontend to either remove
the conversation_id from the FormData when calling
chatService.documentInfoUpload (api.documentInfoUpload) so only file (and url if
used) are sent, or switch to the conversation-scoped upload endpoint used by
document_app.py that invokes doc_upload_and_parse(conversation_id, ...) to
correctly resolve dataset linking; locate the upload logic in
use-chat-request.ts (the FormData append calls and the
chatService.documentInfoUpload invocation) and implement one of these two fixes
so the backend FileService.upload_info() receives only supported fields or use
the proper conversation-scoped flow.
---
Nitpick comments:
In `@api/apps/restful_apis/document_api.py`:
- Around line 74-75: The guard checking files and files.get("file") is
redundant; change the assignment so file_objs is obtained directly from the
MultiDict using files.getlist("file") (i.e., replace the conditional expression
with a direct call to files.getlist("file")), leaving the preceding files =
await request.files and any existing explicit empty-name checks intact; this
simplifies the logic and relies on MultiDict.getlist to return [] when the key
is absent.
In `@web/src/hooks/use-chat-request.ts`:
- Line 47: Rename the stale UploadAndParse identifiers to documentInfoUpload for
consistency: update the enum entry ChatApiAction.UploadAndParse =
'upload_and_parse' to ChatApiAction.DocumentInfoUpload = 'document_info_upload',
change the mutation key and the exported function name uploadAndParseFile to
documentInfoUploadFile (and the hook useUploadAndParseFile to
useDocumentInfoUploadFile), and update all call sites mentioned
(use-upload-file.ts and use-send-shared-message imports/destructuring) to use
the new names so names match the endpoint/service mapping documentInfoUpload.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 187f308b-195a-404e-ac16-31c2dbe7b0cc
📒 Files selected for processing (5)
api/apps/document_app.pyapi/apps/restful_apis/document_api.pyweb/src/hooks/use-chat-request.tsweb/src/services/next-chat-service.tsweb/src/utils/api.ts
💤 Files with no reviewable changes (1)
- api/apps/document_app.py
xugangqiang
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #14359 +/- ##
=======================================
Coverage 95.30% 95.30%
=======================================
Files 10 10
Lines 703 703
Branches 112 112
=======================================
Hits 670 670
Misses 16 16
Partials 17 17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
api/apps/restful_apis/document_api.py (1)
47-100:⚠️ Potential issue | 🔴 CriticalPast-review issues still unresolved on the new
upload_infohandler.Two previously raised concerns are still present in this revision:
- Blocking sync I/O from async handler +
RuntimeErroron URL branch (Lines 89‑100).FileService.upload_infois synchronous and, on the URL branch, callsasyncio.run(adownload())internally. Invoking it directly from thisasync defQuart handler will (a) block the event loop for file uploads and (b) raiseRuntimeError: asyncio.run() cannot be called from a running event loopfor any?url=...request. The sibling handlerupload_documentalready usesthread_pool_execfor the same service layer (Line 441) — this endpoint must do the same.- Missing logging and filename validation (Lines 79‑100). No
loggingcalls on success/error paths (per**/*.py: Add logging for new flows), and no empty-filename /FILE_NAME_LEN_LIMITvalidation onfile_objs(see the established pattern at Lines 420‑427). A file withfilename == ""orNonewill reachFileService.upload_info→DocumentService.check_doc_health(user_id, file.filename)and surface as a 500 instead of a 400.🛠️ Proposed fix
+ from api.constants import FILE_NAME_LEN_LIMIT + files = await request.files file_objs = files.getlist("file") if files and files.get("file") else [] url = request.args.get("url") if file_objs and url: + logging.error("upload_info: both file and url provided") return get_error_argument_result("Provide either multipart file(s) or ?url=..., not both.") if not file_objs and not url: + logging.error("upload_info: missing input") return get_error_argument_result("Missing input: provide multipart file(s) or url") + for f in file_objs: + if f is None or not f.filename: + return get_error_argument_result("No file selected!") + if len(f.filename.encode("utf-8")) > FILE_NAME_LEN_LIMIT: + return get_error_argument_result( + f"File name must be {FILE_NAME_LEN_LIMIT} bytes or less." + ) + try: if url and not file_objs: assert_url_is_safe(url) - return get_result(data=FileService.upload_info(tenant_id, None, url)) + data = await thread_pool_exec(FileService.upload_info, tenant_id, None, url) + return get_result(data=data) if len(file_objs) == 1: - return get_result(data=FileService.upload_info(tenant_id, file_objs[0], None)) + data = await thread_pool_exec(FileService.upload_info, tenant_id, file_objs[0], None) + return get_result(data=data) - results = [FileService.upload_info(tenant_id, f, None) for f in file_objs] + results = [ + await thread_pool_exec(FileService.upload_info, tenant_id, f, None) + for f in file_objs + ] return get_result(data=results) except Exception as e: + logging.exception("upload_info failed") return server_error_response(e)As per coding guidelines:
**/*.py: Add logging for new flows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/apps/restful_apis/document_api.py` around lines 47 - 100, The upload_info Quart handler is calling the synchronous FileService.upload_info directly from an async handler (and will hit asyncio.run() in the URL branch), and it lacks logging and filename validation; update upload_info to run FileService.upload_info calls inside thread_pool_exec (reuse the same pattern as the upload_document handler) for the URL branch and for each file upload to avoid blocking/RuntimeError, add logging calls (info on success, error on exceptions) using the module logger, and validate each file in file_objs for a non-empty filename and length <= FILE_NAME_LEN_LIMIT (return a 400 error via get_error_argument_result and log the validation failure) before invoking FileService.upload_info; reference functions/variables upload_info (handler), FileService.upload_info, thread_pool_exec, upload_document (sibling), FILE_NAME_LEN_LIMIT, get_error_argument_result, and server_error_response to implement these changes.
🧹 Nitpick comments (3)
test/testcases/test_web_api/test_common.py (1)
343-370: Use proper query-string encoding for theurlparameter.Line 357 concatenates
urlinto the request URL without encoding. Anyurlcontaining&,?,=, spaces, or non-ASCII characters will corrupt the request and cause silently misleading test failures. Pass it throughrequests'params=instead so it’s properly percent-encoded.♻️ Proposed refactor
- url_endpoint = f"{HOST_ADDRESS}/api/{VERSION}/documents/upload" + url_endpoint = f"{HOST_ADDRESS}/api/{VERSION}/documents/upload" + params = {"url": url} if url else None fields = [] file_objects = [] try: if files_path: for fp in files_path: p = Path(fp) f = p.open("rb") fields.append(("file", (p.name, f))) file_objects.append(f) - # Add url as query parameter if provided - if url: - url_endpoint = f"{url_endpoint}?url={url}" - # Handle empty fields (no files) - create empty MultipartEncoder if not fields: fields = [("empty", ("", ""))] m = MultipartEncoder(fields=fields) res = requests.post( url=url_endpoint, headers={"Content-Type": m.content_type}, auth=auth, data=m, + params=params, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/testcases/test_web_api/test_common.py` around lines 343 - 370, The test constructs url_endpoint by concatenating the unencoded url string into the URL; instead, build a params dict (e.g., params = {"url": url} when url is truthy) and pass it to requests.post via the params= parameter so the value is properly percent-encoded; keep creating the MultipartEncoder from fields and set headers={"Content-Type": m.content_type} and data=m when calling requests.post (use requests.post(url=url_endpoint, params=params, headers=..., data=m, auth=auth)), and ensure you only add the params entry when url is provided.test/testcases/test_web_api/test_document_app/test_upload_info_unit.py (2)
90-110: Test name overstates coverage; URL branch is never exercised.The method name claims to cover "url, single, and multiple files", but only file uploads are tested — the URL path (which is what triggered the SSRF guard and
assert_url_is_safeaddition in this PR) is left as a comment. Consider either renaming the test to reflect what's actually covered, or adding a small URL-branch test using an HTTP fixture /responses-style mock so the new SSRF gate gets exercised.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/testcases/test_web_api/test_document_app/test_upload_info_unit.py` around lines 90 - 110, The test test_upload_info_supports_url_single_and_multiple_files_e2e currently never exercises the URL branch; either rename it to reflect only file uploads or add a real URL-path assertion: use a local HTTP fixture (e.g., responses or an httpserver fixture) to mock a reachable URL and call upload_info(WebApiAuth, url="http://...") so the SSRF gate and assert_url_is_safe path are exercised, then assert res["code"] and res["data"] (and for multiple file case keep the existing file assertions); ensure you reference the upload_info call and that assert_url_is_safe is hit by the mocked URL rather than leaving the URL as a commented note.
17-61: Remove leftover unit-test scaffolding.
_AwaitableValue,_DummyFiles,_DummyFile,_DummyRequest,_run, and theasyncioimport are leftovers from the old mocked unit-test approach and are no longer referenced after the switch toTestUploadInfoE2E. Drop them to avoid confusion.♻️ Proposed cleanup
-import asyncio - import pytest from test_common import upload_info from configs import INVALID_API_TOKEN from libs.auth import RAGFlowWebApiAuth from utils.file_utils import create_txt_file -class _AwaitableValue: - def __init__(self, value): - self._value = value - - def __await__(self): - async def _co(): - return self._value - - return _co().__await__() - - -class _DummyFiles(dict): - def getlist(self, key): - value = self.get(key, []) - if isinstance(value, list): - return value - return [value] - - -class _DummyFile: - def __init__(self, filename): - self.filename = filename - - -class _DummyRequest: - def __init__(self, *, files=None, args=None): - self._files = files or _DummyFiles() - self.args = args or {} - - `@property` - def files(self): - return _AwaitableValue(self._files) - - -def _run(coro): - return asyncio.run(coro) - - # ============================================================================ # End-to-End Tests # ============================================================================🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/testcases/test_web_api/test_document_app/test_upload_info_unit.py` around lines 17 - 61, Remove the leftover mocked unit-test scaffolding: delete the asyncio import plus the helper class/function definitions _AwaitableValue, _DummyFiles, _DummyFile, _DummyRequest and the _run function from the test file, and keep only the real test imports (e.g. pytest, upload_info, INVALID_API_TOKEN, RAGFlowWebApiAuth, create_txt_file) used by TestUploadInfoE2E; ensure no remaining references to those removed symbols remain in the file and adjust imports if any become unused.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@api/apps/restful_apis/document_api.py`:
- Around line 47-100: The upload_info Quart handler is calling the synchronous
FileService.upload_info directly from an async handler (and will hit
asyncio.run() in the URL branch), and it lacks logging and filename validation;
update upload_info to run FileService.upload_info calls inside thread_pool_exec
(reuse the same pattern as the upload_document handler) for the URL branch and
for each file upload to avoid blocking/RuntimeError, add logging calls (info on
success, error on exceptions) using the module logger, and validate each file in
file_objs for a non-empty filename and length <= FILE_NAME_LEN_LIMIT (return a
400 error via get_error_argument_result and log the validation failure) before
invoking FileService.upload_info; reference functions/variables upload_info
(handler), FileService.upload_info, thread_pool_exec, upload_document (sibling),
FILE_NAME_LEN_LIMIT, get_error_argument_result, and server_error_response to
implement these changes.
---
Nitpick comments:
In `@test/testcases/test_web_api/test_common.py`:
- Around line 343-370: The test constructs url_endpoint by concatenating the
unencoded url string into the URL; instead, build a params dict (e.g., params =
{"url": url} when url is truthy) and pass it to requests.post via the params=
parameter so the value is properly percent-encoded; keep creating the
MultipartEncoder from fields and set headers={"Content-Type": m.content_type}
and data=m when calling requests.post (use requests.post(url=url_endpoint,
params=params, headers=..., data=m, auth=auth)), and ensure you only add the
params entry when url is provided.
In `@test/testcases/test_web_api/test_document_app/test_upload_info_unit.py`:
- Around line 90-110: The test
test_upload_info_supports_url_single_and_multiple_files_e2e currently never
exercises the URL branch; either rename it to reflect only file uploads or add a
real URL-path assertion: use a local HTTP fixture (e.g., responses or an
httpserver fixture) to mock a reachable URL and call upload_info(WebApiAuth,
url="http://...") so the SSRF gate and assert_url_is_safe path are exercised,
then assert res["code"] and res["data"] (and for multiple file case keep the
existing file assertions); ensure you reference the upload_info call and that
assert_url_is_safe is hit by the mocked URL rather than leaving the URL as a
commented note.
- Around line 17-61: Remove the leftover mocked unit-test scaffolding: delete
the asyncio import plus the helper class/function definitions _AwaitableValue,
_DummyFiles, _DummyFile, _DummyRequest and the _run function from the test file,
and keep only the real test imports (e.g. pytest, upload_info,
INVALID_API_TOKEN, RAGFlowWebApiAuth, create_txt_file) used by
TestUploadInfoE2E; ensure no remaining references to those removed symbols
remain in the file and adjust imports if any become unused.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa210061-2c4f-40db-853b-74862c5172fd
📒 Files selected for processing (4)
api/apps/restful_apis/document_api.pytest/testcases/test_web_api/test_common.pytest/testcases/test_web_api/test_document_app/test_upload_info_unit.pyweb/src/utils/api.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/utils/api.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
api/apps/restful_apis/document_api.py (1)
94-105:⚠️ Potential issue | 🔴 CriticalBlocking sync I/O on async handler — URL branch will still raise
RuntimeErrorat runtime.This is the same defect raised in the prior review and not yet fixed.
FileService.upload_infois a synchronous function performing blocking I/O (put_blob, fileread()), and in the URL branch it eventually callsasyncio.run(adownload())(seeapi/db/services/file_service.pyupload_info → crawl path). Callingasyncio.run()from inside this already-running Quart event loop raisesRuntimeError: asyncio.run() cannot be called from a running event loop, so any?url=…request 500s. The file branches "work" but block the loop for the entire upload.The sibling
_upload_local_documentsat Lines 565-568 already usesthread_pool_exec— the new endpoint should follow the same pattern. Offloading also gives the URL branch its own fresh loop in the worker thread, which makesasyncio.runinsideupload_infosafe.Also folding in the still-missing logging hooks per the repo guideline (the prior "addressed" marker doesn't appear to match the current source).
🛠️ Proposed fix
try: if url and not file_objs: assert_url_is_safe(url) - return get_result(data=FileService.upload_info(tenant_id, None, url)) + data = await thread_pool_exec(FileService.upload_info, tenant_id, None, url) + return get_result(data=data) if len(file_objs) == 1: - return get_result(data=FileService.upload_info(tenant_id, file_objs[0], None)) + data = await thread_pool_exec(FileService.upload_info, tenant_id, file_objs[0], None) + return get_result(data=data) - results = [FileService.upload_info(tenant_id, f, None) for f in file_objs] + results = [ + await thread_pool_exec(FileService.upload_info, tenant_id, f, None) + for f in file_objs + ] return get_result(data=results) except Exception as e: + logging.exception("upload_info failed") return server_error_response(e)As per coding guidelines: "
**/*.py: Add logging for new flows".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/apps/restful_apis/document_api.py` around lines 94 - 105, The URL branch in document_api.py calls the synchronous FileService.upload_info (which may invoke asyncio.run), causing RuntimeError when invoked inside the running Quart event loop; refactor the URL branch (and the other file-processing branches) to offload blocking work to the thread pool like the sibling _upload_local_documents uses (call thread_pool_exec to run FileService.upload_info in a worker thread) so the URL path gets a fresh event loop and doesn't block the main loop; also add logging calls (using the same logger pattern used elsewhere) around the start/finish/error of the upload flow and keep existing guards like assert_url_is_safe and the existing get_result/server_error_response logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/apps/restful_apis/document_api.py`:
- Around line 94-97: The current try/except around the URL-upload branch
swallows ValueError from assert_url_is_safe and turns SSRF rejections into 500s;
update the exception handling in the block that checks "if url and not
file_objs" (where assert_url_is_safe and FileService.upload_info are called) to
explicitly catch ValueError and return an argument/bad-request response (use the
existing argument_error_response or equivalent) instead of falling through to
the generic exception handler, while leaving other exceptions to be handled as
before.
---
Duplicate comments:
In `@api/apps/restful_apis/document_api.py`:
- Around line 94-105: The URL branch in document_api.py calls the synchronous
FileService.upload_info (which may invoke asyncio.run), causing RuntimeError
when invoked inside the running Quart event loop; refactor the URL branch (and
the other file-processing branches) to offload blocking work to the thread pool
like the sibling _upload_local_documents uses (call thread_pool_exec to run
FileService.upload_info in a worker thread) so the URL path gets a fresh event
loop and doesn't block the main loop; also add logging calls (using the same
logger pattern used elsewhere) around the start/finish/error of the upload flow
and keep existing guards like assert_url_is_safe and the existing
get_result/server_error_response logic.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ebaf1743-1380-4287-944d-b4af0aace4b5
📒 Files selected for processing (4)
api/apps/document_app.pyapi/apps/restful_apis/document_api.pytest/testcases/test_web_api/test_common.pyweb/src/utils/api.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- api/apps/document_app.py
- test/testcases/test_web_api/test_common.py
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
2 participants
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.
What problem does this PR solve?
Before migration: POST /v1/document/upload_info/
After migration: POST /api/v1/documentss/upload/
Type of change