Conversation
|
use to test. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (3)
api/controllers/console/init_validate.py:76
- Like
setup.py, this module calls synchronous, Flask-SQLAlchemy-backed services (TenantService.get_tenant_count()and theSession(db.engine)fallback). In anasync defFastAPI handler this will both (1) block the event loop and (2) fail without an active Flask app context. Consider making these handlers sync (def) until the underlying DB layer is async, and ensure ASGI requests enter a Flask app context (or migrate to the standalone SQLAlchemy session factory).
async def get_init_status(request: Request) -> InitStatusResponse:
"""Get initialization validation status."""
init_status = get_init_validate_status(request)
if init_status:
return InitStatusResponse(status="finished")
return InitStatusResponse(status="not_started")
@console_async_router.post(
"/init",
response_model=InitValidateResponse,
tags=["console"],
status_code=201,
)
async def validate_init_password(
payload: InitValidatePayload,
request: Request,
_: Annotated[None, Depends(require_self_hosted_edition)],
) -> InitValidateResponse:
"""Validate initialization password."""
tenant_count = TenantService.get_tenant_count()
if tenant_count > 0:
raise AlreadySetupError()
if payload.password != os.environ.get("INIT_PASSWORD"):
request.session["is_init_validated"] = False
raise InitValidateFailedError()
request.session["is_init_validated"] = True
return InitValidateResponse(result="success")
def get_init_validate_status(request: Request) -> bool:
if dify_config.EDITION == "SELF_HOSTED":
if os.environ.get("INIT_PASSWORD"):
if request.session.get("is_init_validated"):
return True
with Session(db.engine) as db_session:
return db_session.execute(select(DifySetup)).scalar_one_or_none() is not None
api/tests/unit_tests/controllers/console/test_fastapi_setup.py:8
DIFY_ASGI_MOUNT_FLASKis set withsetdefault()at import time; if the variable is already set (e.g., by another test), importingasgimay still mount the Flask app via the module-levelapp = create_asgi_app(), making this test order-dependent. Prefer setting the env var unconditionally or configuring it once in a conftest before importingasgianywhere.
api/controllers/console/setup.py:101- These endpoints are declared
async defbut call synchronous services/DB access (e.g.,TenantService.get_tenant_count(),RegisterService.setup(), andget_setup_status()which usesdb.session). In FastAPI, blocking work insideasync defwill block the event loop; until the DB/services are async, preferdefendpoints (threadpool) or explicitly offload blocking calls to a threadpool.
async def get_setup_status_api() -> SetupStatusResponse:
"""Get system setup status.
NOTE: This endpoint is unauthenticated by design.
During first-time bootstrap there is no admin account yet, so frontend initialization must be
able to query setup progress before any login flow exists.
Only bootstrap-safe status information should be returned by this endpoint.
"""
if dify_config.EDITION == "SELF_HOSTED":
setup_status = get_setup_status()
if setup_status and not isinstance(setup_status, bool):
return SetupStatusResponse(step="finished", setup_at=setup_status.setup_at.isoformat())
if setup_status:
return SetupStatusResponse(step="finished")
return SetupStatusResponse(step="not_started")
return SetupStatusResponse(step="finished")
@console_async_router.post(
"/setup",
response_model=SetupResponse,
tags=["console"],
status_code=201,
)
async def setup_system(
payload: SetupRequestPayload,
request: Request,
_: Annotated[None, Depends(require_self_hosted_edition)],
) -> SetupResponse:
"""Initialize system setup with admin account.
NOTE: This endpoint is unauthenticated by design for first-time bootstrap.
Access is restricted by deployment mode (`SELF_HOSTED`), one-time setup guards,
and init-password validation rather than user session authentication.
"""
if get_setup_status():
raise AlreadySetupError()
tenant_count = TenantService.get_tenant_count()
if tenant_count > 0:
raise AlreadySetupError()
if not get_init_validate_status(request):
raise NotInitValidateError()
normalized_email = payload.email.lower()
RegisterService.setup(
email=normalized_email,
name=payload.name,
password=payload.password,
ip_address=extract_remote_ip(request),
language=payload.language,
)
return SetupResponse(result="success")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 23 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
api/pyproject.toml:1
- FastAPI version 0.122.0 does not exist. As of January 2025, the latest FastAPI version is 0.115.x. Please verify and use an existing version number.
[project]
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0313e82 to
4517174
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 23 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
this need some redesign... Discussed with gemini 3 pro, get a new plan. Will update later. |
01bdec2 to
6d9ca2a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| server_name ${NGINX_SERVER_NAME}; | ||
|
|
||
| location = /console/api/ping { | ||
| proxy_pass http://fastapi:5004; |
There was a problem hiding this comment.
The nginx configuration routes /console/api/ping specifically to the fastapi service (port 5004), while all other /console/api/* routes go to the api service (port 5001). This creates inconsistent routing where one endpoint goes to a different service. This could lead to confusion, especially since the tests expect the ping endpoint to be available in the ASGI app. Consider whether this separation is intentional or if all /console/api/* routes should go to the same service.
| proxy_pass http://fastapi:5004; | |
| proxy_pass http://api:5001; |
27632bc to
817bc71
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces FastAPI into the project by adding a new fastapi service and migrating the /console/api/ping endpoint as a first step. The changes are well-structured, using a new fastapi run mode and updating the Docker and Nginx configurations to route traffic appropriately for the new endpoint. I have two main points of feedback: one regarding the API documentation for the new FastAPI app, and another about the lack of tests for the new endpoint after the old ones were removed.
[autofix.ci] apply automated fixes
2a42a21 to
ed8ddca
Compare
|
first migrate easy part. and the doc will be better. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| --host "${DIFY_BIND_ADDRESS:-0.0.0.0}" \ | ||
| --port "${DIFY_PORT:-5004}" | ||
|
|
||
| else |
There was a problem hiding this comment.
The FastAPI service uses the same image (langgenius/dify-api:1.13.0) as the main API service, but there's no error handling for the MODE environment variable in the entrypoint script. If MODE is set to an unsupported value, the service will fall through to the default Flask behavior without clear indication that FastAPI mode was intended. Consider adding validation or logging for the MODE value.
| else | |
| else | |
| # If MODE is set but not one of the recognized values, log a warning before falling back. | |
| if [[ -n "${MODE}" && "${MODE}" != "worker" && "${MODE}" != "beat" && "${MODE}" != "job" && "${MODE}" != "fastapi" && "${MODE}" != "migration" ]]; then | |
| echo "Warning: Unsupported MODE '${MODE}', falling back to default Flask API mode." | |
| fi |
|
|
||
| location = /console/api/ping { | ||
| proxy_pass http://fastapi:5004; | ||
| include proxy.conf; | ||
| } | ||
|
|
There was a problem hiding this comment.
The nginx configuration routes /console/api/ping specifically to the FastAPI service, but all other /console/api requests continue to go to the Flask service. This creates a split-brain scenario where clients cannot rely on consistent behavior for all /console/api endpoints. Consider documenting this routing split or ensuring that the FastAPI service can handle all /console/api requests to avoid confusion.
| location = /console/api/ping { | |
| proxy_pass http://fastapi:5004; | |
| include proxy.conf; | |
| } | |
| # NOTE: | |
| # The `/console/api/ping` endpoint is intentionally routed to the FastAPI service. | |
| # This is typically used as a health-check or a FastAPI-specific endpoint. | |
| location = /console/api/ping { | |
| proxy_pass http://fastapi:5004; | |
| include proxy.conf; | |
| } | |
| # NOTE: | |
| # All other `/console/api` endpoints are intentionally routed to the Flask API service. | |
| # This split routing means `/console/api/ping` is served by FastAPI while the rest of | |
| # `/console/api/*` continues to be served by the Flask backend at `api:5001`. |
| result: str = Field(description="Health check result", examples=["pong"]) | ||
|
|
||
|
|
||
| @app.get("/console/api/ping", response_model=PingResponse, tags=["console"]) |
There was a problem hiding this comment.
The test file for the ping endpoint was deleted, but no equivalent test for the new FastAPI ping endpoint was created. The codebase has test coverage for other console endpoints (e.g., test_fastopenapi_init_validate.py, test_fastopenapi_setup.py). A corresponding test should be added to maintain test coverage consistency for the FastAPI ping endpoint.
Important
Fixes #<issue number>.Summary
just skip to fastapi lol
begin #30269
Screenshots
fastapi has redoc
Checklist
make lintandmake type-check(backend) andcd web && npx lint-staged(frontend) to appease the lint gods