-
Notifications
You must be signed in to change notification settings - Fork 541
feat: refactor user management API to RESTful conventions #1980
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -12,6 +12,7 @@ | |||||||||||||||||||||||
| # See the License for the specific language governing permissions and | ||||||||||||||||||||||||
| # limitations under the License. | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| import warnings | ||||||||||||||||||||||||
| from typing import Optional | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| import redis.asyncio as redis | ||||||||||||||||||||||||
|
|
@@ -23,6 +24,9 @@ | |||||||||||||||||||||||
| logger = init_logger(__name__) | ||||||||||||||||||||||||
| router = APIRouter() | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| # Legacy router for backward compatibility with old POST-based endpoints | ||||||||||||||||||||||||
| legacy_router = APIRouter() | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| class User(BaseModel): | ||||||||||||||||||||||||
| """User model with rate limiting configuration. | ||||||||||||||||||||||||
|
|
@@ -77,7 +81,10 @@ async def _get_redis_client(request: Request) -> redis.Redis: | |||||||||||||||||||||||
| return request.app.state.redis_client | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| @router.post("/CreateUser") | ||||||||||||||||||||||||
| # ==================== RESTful API Endpoints ==================== | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| @router.post("/users") | ||||||||||||||||||||||||
| async def create_user(request: Request, user: User) -> UserResponse: | ||||||||||||||||||||||||
| """Create a new user with rate limits. | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
|
@@ -106,13 +113,13 @@ async def create_user(request: Request, user: User) -> UserResponse: | |||||||||||||||||||||||
| return UserResponse(message=f"Created User: {user.name}", user=user) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| @router.post("/ReadUser") | ||||||||||||||||||||||||
| async def read_user(request: Request, user: User) -> UserResponse: | ||||||||||||||||||||||||
| """Read user information. | ||||||||||||||||||||||||
| @router.get("/users/{name}") | ||||||||||||||||||||||||
| async def get_user(request: Request, name: str) -> UserResponse: | ||||||||||||||||||||||||
| """Read user information by name. | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||
| request: FastAPI request | ||||||||||||||||||||||||
| user: User with name to look up | ||||||||||||||||||||||||
| name: User name to look up | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Returns: | ||||||||||||||||||||||||
| UserResponse with user data | ||||||||||||||||||||||||
|
|
@@ -121,11 +128,11 @@ async def read_user(request: Request, user: User) -> UserResponse: | |||||||||||||||||||||||
| HTTPException: 404 if user not found | ||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||
| redis_client = await _get_redis_client(request) | ||||||||||||||||||||||||
| key = _gen_key(user.name) | ||||||||||||||||||||||||
| key = _gen_key(name) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| data = await redis_client.get(key) | ||||||||||||||||||||||||
| if not data: | ||||||||||||||||||||||||
| logger.warning(f"User not found: {user.name}") | ||||||||||||||||||||||||
| logger.warning(f"User not found: {name}") | ||||||||||||||||||||||||
| raise HTTPException(status_code=404, detail="user does not exist") | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| # Parse JSON data | ||||||||||||||||||||||||
|
|
@@ -135,12 +142,13 @@ async def read_user(request: Request, user: User) -> UserResponse: | |||||||||||||||||||||||
| return UserResponse(message=f"User: {stored_user.name}", user=stored_user) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| @router.post("/UpdateUser") | ||||||||||||||||||||||||
| async def update_user(request: Request, user: User) -> UserResponse: | ||||||||||||||||||||||||
| @router.put("/users/{name}") | ||||||||||||||||||||||||
| async def update_user(request: Request, name: str, user: User) -> UserResponse: | ||||||||||||||||||||||||
| """Update user information. | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||
| request: FastAPI request | ||||||||||||||||||||||||
| name: User name in the URL path | ||||||||||||||||||||||||
| user: Updated user data | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Returns: | ||||||||||||||||||||||||
|
|
@@ -150,28 +158,31 @@ async def update_user(request: Request, user: User) -> UserResponse: | |||||||||||||||||||||||
| HTTPException: 404 if user not found | ||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||
| redis_client = await _get_redis_client(request) | ||||||||||||||||||||||||
| key = _gen_key(user.name) | ||||||||||||||||||||||||
| key = _gen_key(name) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| # Check if user exists | ||||||||||||||||||||||||
| exists = await redis_client.exists(key) | ||||||||||||||||||||||||
| if not exists: | ||||||||||||||||||||||||
| logger.warning(f"Cannot update non-existent user: {user.name}") | ||||||||||||||||||||||||
| raise HTTPException(status_code=404, detail=f"User: {user.name} does not exist") | ||||||||||||||||||||||||
| logger.warning(f"Cannot update non-existent user: {name}") | ||||||||||||||||||||||||
| raise HTTPException( | ||||||||||||||||||||||||
| status_code=404, detail=f"User: {name} does not exist" | ||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| # Update user | ||||||||||||||||||||||||
| # Use the name from URL path, override body if different | ||||||||||||||||||||||||
| user.name = name | ||||||||||||||||||||||||
|
Comment on lines
+171
to
+172
|
||||||||||||||||||||||||
| # 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", | |
| ) |
Copilot
AI
Mar 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Mar 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Mar 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Silently overriding
user.namewith the path parameter can hide client mistakes (e.g., sending mismatched names) and makes the request semantics ambiguous. Prefer either (a) validating thatuser.namematchesnameand returning a 400 on mismatch, or (b) using a different request model for updates that omitsnameentirely so the path is the single source of truth.