Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates the API layer from Flask/Blueprint to FastAPI/APIRouter, updating request/response utilities and the full test suite to use Starlette/httpx semantics while keeping existing API envelopes and auth behavior.
Changes:
- Replaced Flask app + blueprints with a FastAPI app + routers, using
Dependsfor auth and dependency injection. - Updated request/response utilities to FastAPI/Starlette equivalents (cookies, redirects, validation errors).
- Refactored tests to use
starlette.testclient.TestClient/httpx-style response APIs (.json(),.content,files=...) and removed Flask app-context usage.
Reviewed changes
Copilot reviewed 42 out of 43 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unittest/submission/test_penalty.py | Removes Flask app_context() usage around submission creation. |
| tests/unittest/submission/test_get_output.py | Removes Flask app_context() usage; keeps output assertions. |
| tests/unittest/submission/test_check_code.py | Removes Flask app_context() usage; keeps zip validation assertions. |
| tests/test_utils_request.py | Deletes Flask-specific request parsing utility tests. |
| tests/test_use_announcement.py | Switches response parsing to rv.json() and adjusts DELETE calls for TestClient. |
| tests/test_test.py | Updates logging expectations and logger configuration for FastAPI module logger. |
| tests/test_submission.py | Updates URL shapes, request files= usage, and response parsing for TestClient/httpx. |
| tests/test_scoreboard.py | Removes Flask app_context() usage; uses rv.json() for assertions. |
| tests/test_ranking.py | Updates response parsing to rv.json(). |
| tests/test_profile.py | Updates response parsing to rv.json(). |
| tests/test_problem_stats.py | Removes Flask app_context() usage and updates assertions to rv.json() / rv.content. |
| tests/test_problem.py | Updates multipart upload format (files=), response parsing, and binary response checks. |
| tests/test_post.py | Updates response parsing and DELETE request style for TestClient. |
| tests/test_mongo_utils.py | Removes Flask app_context() usage for warning-log test. |
| tests/test_homework.py | Replaces FlaskClient typing/usages with TestClient cookies + response parsing updates. |
| tests/test_health.py | Updates response parsing to rv.json(). |
| tests/test_get_homewrok_problem.py | Replaces FlaskClient typing/usages with TestClient cookies + response parsing updates. |
| tests/test_course.py | Updates response parsing and DELETE request style for TestClient. |
| tests/test_copycat.py | Updates response parsing and switches request mocking from requests to httpx. |
| tests/test_auth.py | Updates response parsing/cookies, adjusts request parameter passing, and skips Flask-only verify-link tests. |
| tests/conftest.py | Migrates fixtures to FastAPI TestClient and adjusts test seeding and response JSON behavior. |
| tests/base_tester.py | Updates request helper to work with TestClient responses (rv.json()). |
| pyproject.toml | Removes Flask/gunicorn/requests; adds FastAPI/uvicorn/httpx/python-multipart dependencies. |
| mongo/utils.py | Replaces current_app.logger usage with stdlib logging. |
| mongo/submission.py | Migrates HTTP client to httpx and replaces Flask testing flag checks with env var. |
| mongo/base.py | Replaces Flask current_app.logger fallback with module logger. |
| model/utils/response.py | Reimplements HTTP response helpers using FastAPI/Starlette response classes. |
| model/utils/request.py | Replaces Flask decorators with FastAPI Depends-based helpers (get_doc, get_ip). |
| model/user.py | Migrates user endpoints to FastAPI routers and dependency-based auth. |
| model/test.py | Migrates test endpoints to FastAPI routers and module logger usage. |
| model/submission.py | Migrates submission endpoints to FastAPI, including file uploads and config endpoints. |
| model/ranking.py | Migrates ranking endpoint to FastAPI. |
| model/profile.py | Migrates profile endpoints to FastAPI. |
| model/problem.py | Migrates problem endpoints to FastAPI, including multipart handling and streaming downloads. |
| model/post.py | Migrates post endpoints to FastAPI and factors shared targeting logic. |
| model/homework.py | Migrates homework endpoints to FastAPI and mounts course homework list on course router. |
| model/health.py | Migrates health endpoint to FastAPI. |
| model/course.py | Migrates course endpoints to FastAPI and splits CRUD operations into separate handlers. |
| model/copycat.py | Migrates copycat endpoints to FastAPI and switches HTTP fetching to httpx. |
| model/announcement.py | Migrates announcements endpoints to FastAPI and shares announcement formatting. |
| app.py | Replaces Flask app factory with FastAPI app creation + exception handlers + router inclusion. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Replace resp.ok (requests API) with resp.is_success (httpx API) in target_sandbox - Remove global httpx.Response.json monkey-patch; fix test_redirect to not call rv.json() on a 302 response - Inject httpx.Client via method args through submit/send/target_sandbox/ rejudge for testability; add explicit timeouts (5s status check, 5s connect / 30s read for submission POST) - Handle httpx.RequestError in update_config so unreachable sandboxes fail fast instead of hanging the request - Remove legacy `import httpx as rq` alias - Add unit tests for target_sandbox using httpx MockTransport Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 43 out of 44 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
mongo/submission.py:304
target_sandbox()creates anhttpx.Clientwhen none is provided but never closes it, which can leak connections/file descriptors over time. Also,client.get(...)can raisehttpx.RequestError(DNS/connection/timeouts) and currently would bubble up and potentially break submission flows; handle request exceptions by logging and skipping that sandbox.
def target_sandbox(self, client: httpx.Client | None = None):
if client is None:
client = httpx.Client(timeout=5.0)
load = 10**3 # current min load
tar = None # target
for sb in self.config().sandbox_instances:
resp = client.get(f'{sb.url}/status')
if not resp.is_success:
self.logger.warning(f'sandbox {sb.name} status exception')
self.logger.warning(
f'status code: {resp.status_code}\n '
f'body: {resp.text}', )
continue
mongo/submission.py:463
send()constructs anhttpx.Clientwhenclient is Nonebut doesn't close it, and the subsequentclient.post(...)call can raisehttpx.RequestErrorwhich is not handled. Consider using a context-managed client when created internally (or ensure it’s closed in afinally) and convert transport errors into a logged failure/False return.
def send(self, client: httpx.Client | None = None) -> bool:
'''
send code to sandbox
'''
if self.handwritten:
logging.warning(f'try to send a handwritten {self}')
return False
if client is None:
client = httpx.Client(timeout=httpx.Timeout(5.0, read=30.0))
# TODO: Ensure problem is ready to submitted
# if not Problem(self.problem).is_test_case_ready():
# raise TestCaseNotFound(self.problem.problem_id)
# setup post body
files = {
'src': io.BytesIO(b"".join(self._get_code_raw())),
}
# look for the target sandbox
tar = self.target_sandbox(client=client)
if tar is None:
self.logger.error(f'can not target a sandbox for {repr(self)}')
return False
# save token for validation
Submission.assign_token(self.id, tar.token)
post_data = {
'token': tar.token,
'checker': 'print("not implement yet. qaq")',
'problem_id': self.problem_id,
'language': self.language,
}
judge_url = f'{tar.url}/submit/{self.id}'
# send submission to snadbox for judgement
self.logger.info(f'send {self} to {tar.name}')
resp = client.post(
judge_url,
data=post_data,
files=files,
)
self.logger.info(f'recieve {self} resp from sandbox')
return self.sandbox_resp_handler(resp)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def HTTPResponse( | ||
| message: str = '', | ||
| status_code: int = 200, | ||
| status: str = 'ok', | ||
| data: Any = None, | ||
| cookies: dict = {}, | ||
| ) -> JSONResponse: |
There was a problem hiding this comment.
HTTPResponse uses a mutable default argument (cookies: dict = {}). Even if it's not mutated today, this pattern can lead to cross-request state leakage if any future code mutates the dict. Use cookies: dict | None = None and treat None as an empty dict inside the function.
| def HTTPRedirect( | ||
| location: str, | ||
| status_code: int = 302, | ||
| cookies: dict = {}, | ||
| ) -> RedirectResponse: | ||
| resp = RedirectResponse(location, status_code=status_code) | ||
| return _apply_cookies(resp, cookies) |
There was a problem hiding this comment.
HTTPRedirect also uses a mutable default argument (cookies: dict = {}), with the same risk of shared state across calls. Prefer cookies: dict | None = None and normalize internally.
| def app(tmp_path): | ||
| from app import app as flask_app | ||
| app = flask_app() | ||
| app.config['TESTING'] = True | ||
| app.config['SERVER_NAME'] = 'test.test' | ||
| os.environ['TESTING'] = '1' | ||
| from app import app as fastapi_app, _seed_db | ||
| mongomock.gridfs.enable_gridfs_integration() | ||
|
|
||
| # Re-run seed in case a prior setup_class dropped the DB | ||
| _seed_db() | ||
|
|
||
| # modify submission config for testing | ||
| # use tmp dir to save user source code | ||
| submission_tmp_dir = (tmp_path / Submission.config().TMP_DIR).absolute() | ||
| submission_tmp_dir.mkdir(exist_ok=True) | ||
| Submission.config().TMP_DIR = submission_tmp_dir | ||
|
|
||
| return app | ||
| return fastapi_app |
There was a problem hiding this comment.
The app test fixture sets os.environ['TESTING'] = '1' but never restores the prior value. This can leak test-mode behavior across modules/tests (or when reusing the same process). Prefer using pytest’s monkeypatch.setenv/delenv (or a yield fixture that restores the previous env value).
概述大規模且一致的遷移:Flask Blueprints → 正確性可能的破壞性變更 — trailing slash。 舊版 app 有 External link 產生方式有 regression。
小問題: 小問題:mutable default args。 專案慣例契合度
效能
測試覆蓋
安全性
建議Approve with changes。合併前必修:
Nice-to-have(後續 PR):
此 review 由 Claude Opus 4.6 Max Effort 產生,明早會再人工 review。 |
| raise NOJException('Invalid Token', 403) | ||
| user = User(json['data']['username']) | ||
| if json['data'].get('userId') != user.user_id: | ||
| raise NOJException('Authorization Expired', 403) |
There was a problem hiding this comment.
Does it affect front end?
There was a problem hiding this comment.
why does the Auth Expiration check if userIds are matched?
There was a problem hiding this comment.
That's copied from original flask implementation. IIRC we wrote this error message because user_id is derived from username and password. user_id mismatch means the password has been changed since last time this jwt issued.
Lines 69 to 70 in fa79115
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 49 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
mongo/submission.py:305
target_sandbox()creates a newhttpx.Clientwhenclientis None but never closes it, andclient.get(...)can raisehttpx.RequestErrorwhich will currently bubble up and potentially fail the whole submission flow. Prefer requiring a caller-managed client (from app lifespan) or creating the client with a context manager/try/finallyto close it, and handle request exceptions by treating the sandbox as unhealthy.
def target_sandbox(self, client: httpx.Client | None = None):
if client is None:
client = httpx.Client(timeout=5.0)
load = 10**3 # current min load
tar = None # target
for sb in self.config().sandbox_instances:
resp = client.get(f'{sb.url}/status')
if not resp.is_success:
self.logger.warning(f'sandbox {sb.name} status exception')
self.logger.warning(
f'status code: {resp.status_code}\n '
f'body: {resp.text}', )
continue
resp = resp.json()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @course_router.get('/{course_name}/ann') | ||
| def get_course_announcements(course_name: str, user=Depends(login_required)): | ||
| try: | ||
| anns = Announcement.ann_list(user.obj, course_name or 'Public') | ||
| anns = Announcement.ann_list(user.obj, course_name) | ||
| except (DoesNotExist, ValidationError): | ||
| return HTTPError('Cannot Access a Announcement', 403) | ||
| if anns is None: | ||
| return HTTPError('Announcement Not Found', 404) | ||
| data = [{ | ||
| 'annId': str(an.id), | ||
| 'title': an.title, | ||
| 'createTime': int(an.create_time.timestamp()), | ||
| 'updateTime': int(an.update_time.timestamp()), | ||
| 'creator': an.creator.info, | ||
| 'updater': an.updater.info, | ||
| 'markdown': an.markdown, | ||
| 'pinned': an.pinned | ||
| } for an in anns if ann_id is None or str(an.id) == ann_id] | ||
| data = [_format_ann(an) for an in anns] | ||
| return HTTPResponse('Announcement List', data=data) | ||
|
|
||
|
|
||
| @ann_api.post('/') | ||
| @login_required | ||
| @parse_body(CreateAnnouncementBody) | ||
| def create_announcement(user, body: CreateAnnouncementBody): | ||
| # Create a new announcement | ||
| @ann_router.get('/{course_name}/{ann_id}') | ||
| def get_ann_by_id(course_name: str, ann_id: str, user=Depends(login_required)): | ||
| try: | ||
| anns = Announcement.ann_list(user.obj, course_name) | ||
| except (DoesNotExist, ValidationError): | ||
| return HTTPError('Cannot Access a Announcement', 403) | ||
| if anns is None: | ||
| return HTTPError('Announcement Not Found', 404) | ||
| data = [_format_ann(an) for an in anns if str(an.id) == ann_id] | ||
| return HTTPResponse('Announcement List', data=data) |
There was a problem hiding this comment.
The route @ann_router.get('/{course_name}/{ann_id}') currently captures requests like /ann/Public/ann (previously the course announcement list endpoint) and then filters announcements by ann_id == 'ann', which will typically return an empty list and changes the API semantics. Add an explicit list route for /{course_name}/ann (or move list handling fully under /course/{course_name}/ann and update callers/tests), and ensure the more-specific list route is registered before the generic /{course_name}/{ann_id} route to avoid shadowing.
| def get_report_by_url(url: str): | ||
| try: | ||
| response = requests.get(url) | ||
| response = httpx.get(url) | ||
| return response.text | ||
| except (requests.exceptions.MissingSchema, | ||
| requests.exceptions.InvalidSchema): | ||
| except httpx.InvalidURL: | ||
| return 'No report.' |
There was a problem hiding this comment.
get_report_by_url() calls httpx.get(url) without a timeout and only handles httpx.InvalidURL. Network errors/timeouts (e.g., httpx.RequestError) will currently raise and can fail the request; also a missing timeout can hang a worker. Pass an explicit timeout and handle httpx.RequestError by returning a safe fallback (or surfacing a controlled HTTPError).
| CMD ["uvicorn", "app:app"] | ||
|
|
||
| # 'production' stage uses the clean 'python-base' stage and copyies | ||
| # in only our runtime deps that were installed in the 'builder-base' | ||
| FROM python-base AS production | ||
| ENV UVICORN_HOST=0.0.0.0 \ | ||
| UVICORN_PORT=8080 \ | ||
| UVICORN_WORKERS=4 | ||
|
|
||
| COPY --from=builder-base $VENV_PATH $VENV_PATH | ||
| COPY ./ /app | ||
|
|
||
| WORKDIR /app | ||
| EXPOSE 8080 | ||
| CMD ["gunicorn", "app:app()", "-c", "gunicorn.conf.py"] | ||
| CMD ["uvicorn", "app:app"] |
There was a problem hiding this comment.
CMD ["uvicorn", "app:app"] ignores the UVICORN_HOST/UVICORN_PORT/UVICORN_WORKERS env vars; uvicorn will default to 127.0.0.1:8000, which typically makes the container unreachable and also runs a single worker in production. Pass --host/--port (and --workers in production, --reload in development) explicitly in the CMD/ENTRYPOINT, or add a small entrypoint script that maps these env vars to uvicorn flags.
| app.state.http_client = httpx.Client(timeout=httpx.Timeout(5.0, read=30.0)) | ||
| yield | ||
| app.state.http_client.close() |
There was a problem hiding this comment.
lifespan() creates a synchronous httpx.Client and stores it on app.state. Any async endpoints that use this client will perform blocking I/O on the event loop. Prefer httpx.AsyncClient here (and close it with await aclose()), or ensure all handlers that use it are def (sync) so Starlette runs them in a threadpool.
| app.state.http_client = httpx.Client(timeout=httpx.Timeout(5.0, read=30.0)) | |
| yield | |
| app.state.http_client.close() | |
| app.state.http_client = httpx.AsyncClient( | |
| timeout=httpx.Timeout(5.0, read=30.0)) | |
| yield | |
| await app.state.http_client.aclose() |
| @submission_router.put('/{submission_id}') | ||
| async def update_submission( | ||
| submission_id: str, | ||
| code: Optional[UploadFile] = File(default=None), | ||
| user=Depends(login_required), | ||
| submission: Submission = get_doc('submission_id', Submission), | ||
| http_client: httpx.Client = Depends(get_http_client), | ||
| ): | ||
| if submission.status >= 0: | ||
| return HTTPError( | ||
| f'{submission} has finished judgement.', | ||
| 403, | ||
| ) | ||
| # if user not equal, reject | ||
| return HTTPError(f'{submission} has finished judgement.', 403) | ||
| if not secrets.compare_digest(submission.user.username, user.username): | ||
| return HTTPError('user not equal!', 403) | ||
| # if source code not found | ||
| if code is None: | ||
| return HTTPError( | ||
| f'can not find the source file', | ||
| 400, | ||
| ) | ||
| # or empty file | ||
| if len(code.read()) == 0: | ||
| return HTTPError('can not find the source file', 400) | ||
| content = await code.read() | ||
| if len(content) == 0: | ||
| return HTTPError('empty file', 400) | ||
| code.seek(0) | ||
| # has been uploaded | ||
| code_file = io.BytesIO(content) | ||
| if submission.has_code(): | ||
| return HTTPError( | ||
| f'{submission} has been uploaded source file!', | ||
| 403, | ||
| ) | ||
| return HTTPError(f'{submission} has been uploaded source file!', 403) | ||
| try: | ||
| success = submission.submit(code) | ||
| success = submission.submit(code_file, client=http_client) | ||
| except FileExistsError: |
There was a problem hiding this comment.
update_submission is async def but calls submission.submit(..., client=http_client) where http_client is a synchronous httpx.Client and submit/send/target_sandbox do synchronous network I/O. This can block the event loop under load. Consider switching to an httpx.AsyncClient + async implementations, or run the blocking submit/send logic in a threadpool (e.g., run_in_threadpool) after reading the upload bytes.
| WORKDIR /app | ||
| EXPOSE 8080 | ||
| CMD ["gunicorn", "app:app()", "-c", "gunicorn.conf.py"] | ||
| CMD ["uvicorn", "app:app"] |
There was a problem hiding this comment.
CMD ["uvicorn", "app:app", "--proxy-headers", "--forwarded-allow-ips=*"]
| minio = "^7.2.15" | ||
| python-ulid = "^3.0.0" | ||
| pydantic = "^2.0" | ||
| fastapi = ">=0.115" |
| { | ||
| 'status': 'err', | ||
| 'message': 'Invalid request body', | ||
| 'data': None, |
There was a problem hiding this comment.
'data': jsonable_encoder(exc.errors())
Ref: https://fastapi.tiangolo.com/tutorial/handling-errors/#use-the-requestvalidationerror-body
| request: Request, | ||
| user: User = identity_verify(0, 1), | ||
| ): | ||
| problem = Problem(problem_id) |
| problem = Problem(problem_id) | ||
| if not problem: | ||
| raise NOJException('Problem not found', 404) | ||
| if not problem.permission(user, problem.Permission.MANAGE): |
| except Exception: | ||
| return HTTPError('Invalid or missing arguments.', 400) | ||
| try: | ||
| Problem.edit_problem( |
| return HTTPError('missing or invalid form field: case', 400) | ||
| case = BytesIO(await case_upload.read()) | ||
| try: | ||
| problem.update_test_case(case) |
| def update_submission(user, submission: Submission, code): | ||
| # validate this reques | ||
| @submission_router.put('/{submission_id}') | ||
| async def update_submission( |
Replace Flask/Blueprint with FastAPI/APIRouter across all route modules. Switch to Pydantic v2 request schemas, FastAPI Depends for auth, and httpx/Starlette TestClient in tests. All 485 tests pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rv.json() raised on binary responses (e.g. ZIP testcase download) after the global httpx.Response.json monkey-patch was removed. Move the safe fallback to BaseTester.request where it belongs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace gunicorn with uvicorn to match the FastAPI (ASGI) migration. Use UVICORN_* env vars for runtime configuration so workers and other flags can be tuned per deployment without rebuilding the image. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Using mutable objects (dict/list) as default argument values is a well-known Python footgun — the same object is shared across all calls. Replace with None and normalise to an empty collection in the body. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
request.form() returns starlette.datastructures.UploadFile, not fastapi.UploadFile — they are different classes, so isinstance would always return False for valid file uploads. Also returns a clear 400 when the field is missing rather than propagating an AttributeError. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
A single httpx.Client is created at app startup via lifespan and stored in app.state. Route handlers that trigger sandbox communication receive it via Depends(get_http_client) and pass it down to submission.submit() and submission.rejudge(). This ensures consistent timeout configuration (Timeout(5.0, read=30.0)) and allows connection pool reuse across requests. The mongo layer keeps its client | None = None signatures so it remains usable outside the FastAPI context (tests, scripts, copycat). Update the client fixture to use a context manager so the lifespan runs during tests and app.state.http_client is properly initialised. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Use jsonable_encoder(exc.errors()) instead of None so callers can see which fields failed validation and why. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- manage_problem: wrap MongoEngine calls in asyncio.to_thread() so the event loop is not blocked while waiting on DB operations - update_submission: drop async since the only await was code.read(); use code.file.read() (sync) and let FastAPI run the route in a threadpool Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…f >= Using ^ (compatible release) is safer than >= as it prevents accidentally picking up breaking major-version bumps. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…load FileExistsError previously called exit(10086), killing the worker process on a duplicate upload instead of returning an error response. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
GCS's S3-compatible API rejects the GetBucketLocation call that the MinIO SDK makes when no region is specified, returning AccessDenied. Passing an explicit region bypasses that auto-detection step. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace Flask/Blueprint with FastAPI/APIRouter across all route modules. Switch to Pydantic v2 request schemas, FastAPI Depends for auth, and httpx/Starlette TestClient in tests. All 485 tests pass.