feat: refactor user management API to RESTful conventions#1980
feat: refactor user management API to RESTful conventions#1980NJX-njx wants to merge 1 commit intovllm-project:mainfrom
Conversation
Refactor the metadata user management API from non-standard POST-based
endpoints to proper RESTful conventions while maintaining full backward
compatibility.
New RESTful endpoints:
- POST /users - Create a new user
- GET /users/{name} - Read user by name
- PUT /users/{name} - Update user by name
- DELETE /users/{name} - Delete user by name
Legacy endpoints (deprecated, kept for backward compatibility):
- POST /CreateUser -> delegates to POST /users
- POST /ReadUser -> delegates to GET /users/{name}
- POST /UpdateUser -> delegates to PUT /users/{name}
- POST /DeleteUser -> delegates to DELETE /users/{name}
Tests updated to cover both RESTful and legacy endpoints.
Fixes vllm-project#1640
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request modernizes the metadata user management API by transitioning from non-standard POST-based endpoints to a fully RESTful design. The core change involves implementing new, semantically clear HTTP methods and URL structures for user operations. Crucially, existing integrations are safeguarded through the introduction of a backward-compatible legacy router that delegates to the new implementations while issuing deprecation warnings, ensuring a smooth transition for consumers of the API. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull request overview
Refactors the user management API to follow RESTful conventions while preserving deprecated legacy POST endpoints for backward compatibility.
Changes:
- Introduced RESTful
/usersand/users/{name}endpoints using GET/POST/PUT/DELETE. - Added a
legacy_routerthat preserves/CreateUser,/ReadUser,/UpdateUser,/DeleteUserand delegates to RESTful handlers. - Updated and expanded tests to cover both RESTful and legacy endpoints.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| python/aibrix/aibrix/metadata/api/v1/users.py | Adds RESTful routes, introduces a legacy router with deprecation warnings, and adjusts delete response payload |
| python/aibrix/aibrix/metadata/app.py | Mounts both RESTful and legacy user routers |
| python/aibrix/tests/metadata/test_users_api.py | Updates tests for RESTful endpoints and adds coverage for legacy endpoints |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| logger.info(f"Deleted user: {name}") | ||
| return UserResponse(message=f"Deleted User: {name}", user=None) |
There was a problem hiding this comment.
This changes legacy /DeleteUser behavior: it now returns user=None via delegation to delete_user, whereas the previous implementation returned the deleted user in the user field. That breaks the PR’s stated ‘full backward compatibility’. Consider keeping the RESTful DELETE /users/{name} response as-is, but have legacy_delete_user return UserResponse(..., user=user) (or reconstruct the prior payload) to preserve the legacy contract.
| DeprecationWarning, | ||
| stacklevel=1, | ||
| ) | ||
| return await delete_user(request, user.name) |
There was a problem hiding this comment.
This changes legacy /DeleteUser behavior: it now returns user=None via delegation to delete_user, whereas the previous implementation returned the deleted user in the user field. That breaks the PR’s stated ‘full backward compatibility’. Consider keeping the RESTful DELETE /users/{name} response as-is, but have legacy_delete_user return UserResponse(..., user=user) (or reconstruct the prior payload) to preserve the legacy contract.
| return await delete_user(request, user.name) | |
| # Legacy behavior: return the deleted user in the response. | |
| # First, read the user (will raise 404 if it does not exist). | |
| existing_user = await get_user(request, user.name) | |
| # Then perform the deletion using the RESTful handler. | |
| delete_result = await delete_user(request, user.name) | |
| # Return a response that preserves the legacy contract: include the | |
| # deleted user in the `user` field while keeping the delete message. | |
| return UserResponse(message=delete_result.message, user=existing_user.user) |
| @router.put("/users/{name}") | ||
| async def update_user(request: Request, name: str, user: User) -> UserResponse: |
There was a problem hiding this comment.
Silently overriding user.name with the path parameter can hide client mistakes (e.g., sending mismatched names) and makes the request semantics ambiguous. Prefer either (a) validating that user.name matches name and returning a 400 on mismatch, or (b) using a different request model for updates that omits name entirely so the path is the single source of truth.
| # Use the name from URL path, override body if different | ||
| user.name = name |
There was a problem hiding this comment.
Silently overriding user.name with the path parameter can hide client mistakes (e.g., sending mismatched names) and makes the request semantics ambiguous. Prefer either (a) validating that user.name matches name and returning a 400 on mismatch, or (b) using a different request model for updates that omits name entirely so the path is the single source of truth.
| # Use the name from URL path, override body if different | |
| user.name = name | |
| # Validate that the name in the body matches the path parameter | |
| if user.name != name: | |
| logger.warning( | |
| f"Name mismatch in update_user: path name='{name}', body name='{user.name}'" | |
| ) | |
| raise HTTPException( | |
| status_code=400, | |
| detail="User name in path does not match user name in request body", | |
| ) |
| warnings.warn( | ||
| "POST /CreateUser is deprecated, use POST /users instead", | ||
| DeprecationWarning, | ||
| stacklevel=1, | ||
| ) |
There was a problem hiding this comment.
Using warnings.warn(..., DeprecationWarning) inside an API handler is easy to miss in production because DeprecationWarning is filtered out by default in many runtime configurations, and it is not visible to API consumers. If the goal is to ‘emit deprecation warnings’ to clients, consider adding an HTTP Warning/Deprecation signal (e.g., response headers) and/or logging via logger.warning. If you keep warnings.warn, stacklevel should generally be 2 so the warning points at the legacy endpoint wrapper rather than the warnings.warn line.
| def test_legacy_create_user(self, app_with_redis, mock_redis): | ||
| """Test legacy POST /CreateUser still works.""" |
There was a problem hiding this comment.
Legacy endpoint tests verify functional behavior, but they don’t assert that a deprecation signal is actually emitted (which is part of the PR description). Once the implementation is finalized (e.g., response header, logger.warning, or warnings.warn), add assertions using pytest.warns, caplog, or header checks so regressions don’t silently remove the deprecation notice.
| response = client.post("/CreateUser", json=user_data) | ||
| assert response.status_code == 200 |
There was a problem hiding this comment.
Legacy endpoint tests verify functional behavior, but they don’t assert that a deprecation signal is actually emitted (which is part of the PR description). Once the implementation is finalized (e.g., response header, logger.warning, or warnings.warn), add assertions using pytest.warns, caplog, or header checks so regressions don’t silently remove the deprecation notice.
| @pytest.fixture | ||
| def mock_redis(self): | ||
| """Mock Redis client.""" | ||
| mock = AsyncMock() | ||
| return mock | ||
|
|
||
| @pytest.fixture | ||
| def app_with_redis(self, mock_redis): | ||
| """Build app with mocked Redis.""" | ||
| from argparse import Namespace | ||
|
|
||
| args = Namespace( | ||
| enable_fastapi_docs=False, | ||
| disable_batch_api=True, | ||
| disable_file_api=True, | ||
| enable_k8s_job=False, | ||
| e2e_test=False, | ||
| ) | ||
| app = build_app(args) | ||
| app.state.redis_client = mock_redis | ||
| return app | ||
|
|
There was a problem hiding this comment.
The mock_redis and app_with_redis fixtures are duplicated across TestUsersRESTfulAPI and TestUsersLegacyAPI. Consider moving them to module-scoped fixtures (or a shared base class) to reduce repetition and keep future setup changes in one place.
| @pytest.fixture | |
| def mock_redis(self): | |
| """Mock Redis client.""" | |
| mock = AsyncMock() | |
| return mock | |
| @pytest.fixture | |
| def app_with_redis(self, mock_redis): | |
| """Build app with mocked Redis.""" | |
| from argparse import Namespace | |
| args = Namespace( | |
| enable_fastapi_docs=False, | |
| disable_batch_api=True, | |
| disable_file_api=True, | |
| enable_k8s_job=False, | |
| e2e_test=False, | |
| ) | |
| app = build_app(args) | |
| app.state.redis_client = mock_redis | |
| return app |
| @pytest.fixture | ||
| def app_with_redis(self, mock_redis): |
There was a problem hiding this comment.
The mock_redis and app_with_redis fixtures are duplicated across TestUsersRESTfulAPI and TestUsersLegacyAPI. Consider moving them to module-scoped fixtures (or a shared base class) to reduce repetition and keep future setup changes in one place.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request successfully refactors the user management API to adhere to RESTful conventions while ensuring full backward compatibility. New RESTful endpoints (/users with POST, GET, PUT, DELETE) have been introduced, and the old POST-based endpoints (/CreateUser, /ReadUser, /UpdateUser, /DeleteUser) are preserved via a legacy_router and emit deprecation warnings. The changes are well-tested, the code is clean, and documentation is updated. However, a high-severity Broken Access Control vulnerability has been identified: both the new and legacy endpoints lack authentication or authorization checks, allowing unauthorized users to manage rate limiting configurations for any user in the system. It is strongly recommended to implement an authentication and authorization layer before deploying these changes.
| @legacy_router.post("/CreateUser") | ||
| async def legacy_create_user(request: Request, user: User) -> UserResponse: | ||
| """Create a new user. Deprecated: use POST /users instead.""" | ||
| warnings.warn( | ||
| "POST /CreateUser is deprecated, use POST /users instead", | ||
| DeprecationWarning, | ||
| stacklevel=1, | ||
| ) | ||
| return await create_user(request, user) | ||
|
|
||
|
|
||
| @legacy_router.post("/ReadUser") | ||
| async def legacy_read_user(request: Request, user: User) -> UserResponse: | ||
| """Read user information. Deprecated: use GET /users/{name} instead.""" | ||
| warnings.warn( | ||
| "POST /ReadUser is deprecated, use GET /users/{name} instead", | ||
| DeprecationWarning, | ||
| stacklevel=1, | ||
| ) | ||
| return await get_user(request, user.name) | ||
|
|
||
|
|
||
| @legacy_router.post("/UpdateUser") | ||
| async def legacy_update_user(request: Request, user: User) -> UserResponse: | ||
| """Update user information. Deprecated: use PUT /users/{name} instead.""" | ||
| warnings.warn( | ||
| "POST /UpdateUser is deprecated, use PUT /users/{name} instead", | ||
| DeprecationWarning, | ||
| stacklevel=1, | ||
| ) | ||
| return await update_user(request, user.name, user) | ||
|
|
||
|
|
||
| @legacy_router.post("/DeleteUser") | ||
| async def legacy_delete_user(request: Request, user: User) -> UserResponse: | ||
| """Delete a user. Deprecated: use DELETE /users/{name} instead.""" | ||
| warnings.warn( | ||
| "POST /DeleteUser is deprecated, use DELETE /users/{name} instead", | ||
| DeprecationWarning, | ||
| stacklevel=1, | ||
| ) | ||
| return await delete_user(request, user.name) |
There was a problem hiding this comment.
The legacy endpoints are also unauthenticated and unauthorized, maintaining the security risk for backward-compatible clients.
Summary
Fixes #1640
Refactor the metadata user management API from non-standard POST-based endpoints to proper RESTful conventions while maintaining full backward compatibility.
Changes
New RESTful endpoints
/users/users/{name}/users/{name}/users/{name}Legacy endpoints (deprecated, kept for backward compatibility)
The old POST-based endpoints (
/CreateUser,/ReadUser,/UpdateUser,/DeleteUser) are preserved via a separatelegacy_routerand delegate to the new RESTful implementations. They emit deprecation warnings.Files Changed
python/aibrix/aibrix/metadata/api/v1/users.py: New RESTful router + legacy router with deprecation warningspython/aibrix/aibrix/metadata/app.py: Mount both routerspython/aibrix/tests/metadata/test_users_api.py: Tests for both RESTful and legacy endpoints