Restored lost file
This commit is contained in:
255
documentation/COMMON_PARAMETERS_REFACTORING_PROPOSAL.md
Normal file
255
documentation/COMMON_PARAMETERS_REFACTORING_PROPOSAL.md
Normal file
@@ -0,0 +1,255 @@
|
|||||||
|
# Common Parameters Refactoring Proposal for Aether API V3
|
||||||
|
|
||||||
|
## 1. Problem Statement
|
||||||
|
|
||||||
|
Currently, the `common_route_params` dependency in `app/lib_general.py` bundles many disparate concerns:
|
||||||
|
- Account authentication/authorization context (`x_account_id`).
|
||||||
|
- Pagination (`limit`, `offset`).
|
||||||
|
- Status filtering (`enabled`, `hidden`).
|
||||||
|
- Pydantic serialization options (`by_alias`, `exclude_unset`).
|
||||||
|
- FastAPI internal (`response` object).
|
||||||
|
- Request delay (`delay_ms`, `X-Delay-ms`).
|
||||||
|
|
||||||
|
While functional, this monolithic dependency leads to:
|
||||||
|
- **Lack of Granularity:** Every route that uses `common_route_params` inherits all its parameters, even if it only needs a subset.
|
||||||
|
- **Reduced Clarity:** The purpose of each parameter is obscured within a single, large object.
|
||||||
|
- **Limited Flexibility:** Difficult to customize or override specific common parameters without affecting others.
|
||||||
|
- **Suboptimal OpenAPI Docs:** FastAPI generates a single, large set of parameters for `commons`, which can be less precise in the Swagger UI.
|
||||||
|
|
||||||
|
## 2. Proposed Solution: Granular Dependencies
|
||||||
|
|
||||||
|
The proposal is to break down `common_route_params` into smaller, more focused FastAPI dependency injection functions and Pydantic models. This leverages FastAPI's powerful dependency injection system to achieve clarity, flexibility, and better OpenAPI documentation.
|
||||||
|
|
||||||
|
All new models and dependency functions will reside in `app/lib_general.py`.
|
||||||
|
|
||||||
|
### A. `AccountContext` (Pydantic Model & Dependency Function)
|
||||||
|
|
||||||
|
* **Purpose:** Encapsulates the resolved account context, including the integer `account_id` and the random `account_id_random` string. It also handles the authentication/authorization logic for `x_account_id`.
|
||||||
|
* **Dependency Function:** `get_account_context()`
|
||||||
|
|
||||||
|
**Proposed `app/lib_general.py` additions:**
|
||||||
|
|
||||||
|
```python
|
||||||
|
# --- Pydantic Model for Account Context ---
|
||||||
|
class AccountContext(BaseModel):
|
||||||
|
account_id: int
|
||||||
|
account_id_random: str
|
||||||
|
# Add any other account-specific context needed here
|
||||||
|
|
||||||
|
# --- Dependency Function for Account Context ---
|
||||||
|
def get_account_context(
|
||||||
|
x_account_id: Optional[str] = Header(None, min_length=11, max_length=22),
|
||||||
|
x_no_account_id: Optional[str] = Header(None, min_length=11, max_length=22),
|
||||||
|
x_no_account_id_token: Optional[str] = Query(None, min_length=11, max_length=22), # Changed from Header to Query for easier testing
|
||||||
|
) -> AccountContext:
|
||||||
|
"""
|
||||||
|
Resolves the account context from headers/query parameters.
|
||||||
|
Raises HTTPException 403 if no valid account is found.
|
||||||
|
"""
|
||||||
|
log.setLevel(logging.DEBUG) # Adjust as needed
|
||||||
|
log.debug(locals())
|
||||||
|
|
||||||
|
resolved_account_id = None
|
||||||
|
resolved_account_id_random = None
|
||||||
|
|
||||||
|
if x_account_id:
|
||||||
|
resolved_account_id_random = x_account_id
|
||||||
|
if resolved_account_id := redis_lookup_id_random(table_name='account', record_id_random=x_account_id):
|
||||||
|
log.info(f'Found account from x-account-id: {resolved_account_id}')
|
||||||
|
else:
|
||||||
|
log.warning(f'Invalid x-account-id provided: {x_account_id}')
|
||||||
|
raise HTTPException(status_code=403, detail='Invalid X-Account-ID.')
|
||||||
|
elif x_no_account_id and len(x_no_account_id) >= 11:
|
||||||
|
# User specified no account ID but might have a special bypass token/logic
|
||||||
|
# USER INPUT REQUIRED: Clarify logic for x_no_account_id vs x_no_account_id_token
|
||||||
|
# For now, treat as no specific account and potentially allow access based on other factors
|
||||||
|
log.info(f'X-No-Account-ID header found: {x_no_account_id}. Proceeding without specific account context.')
|
||||||
|
resolved_account_id = 0 # Or some other indicator for "no specific account"
|
||||||
|
resolved_account_id_random = '--- NO ACCOUNT ---'
|
||||||
|
elif x_no_account_id_token and len(x_no_account_id_token) >= 11:
|
||||||
|
# USER INPUT REQUIRED: How should x_no_account_id_token be validated?
|
||||||
|
# Current logic in common_route_params looks up account based on this token.
|
||||||
|
resolved_account_id_random = x_no_account_id_token
|
||||||
|
if resolved_account_id := redis_lookup_id_random(table_name='account', record_id_random=x_no_account_id_token):
|
||||||
|
log.info(f'Found account from x_no_account_id_token: {resolved_account_id}')
|
||||||
|
else:
|
||||||
|
log.warning(f'Invalid x_no_account_id_token provided: {x_no_account_id_token}')
|
||||||
|
raise HTTPException(status_code=403, detail='Invalid X-No-Account-ID-Token.')
|
||||||
|
else:
|
||||||
|
log.warning('No valid account context provided via X-Account-ID or X-No-Account-ID-Token.')
|
||||||
|
raise HTTPException(status_code=403, detail='Account context required. Please provide X-Account-ID or X-No-Account-ID-Token.')
|
||||||
|
|
||||||
|
return AccountContext(account_id=resolved_account_id, account_id_random=resolved_account_id_random)
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### B. `PaginationParams` (Pydantic Model & Dependency Function)
|
||||||
|
|
||||||
|
* **Purpose:** Encapsulates pagination parameters (`limit`, `offset`).
|
||||||
|
* **Dependency Function:** `get_pagination_params()`
|
||||||
|
|
||||||
|
**Proposed `app/lib_general.py` additions:**
|
||||||
|
|
||||||
|
```python
|
||||||
|
# --- Pydantic Model for Pagination ---
|
||||||
|
class PaginationParams(BaseModel):
|
||||||
|
limit: int = 100 # Default limit
|
||||||
|
offset: int = 0
|
||||||
|
|
||||||
|
# --- Dependency Function for Pagination ---
|
||||||
|
def get_pagination_params(
|
||||||
|
limit: int = Query(100, ge=0, description="Maximum number of items to return"),
|
||||||
|
offset: int = Query(0, ge=0, description="Number of items to skip (for pagination)"),
|
||||||
|
) -> PaginationParams:
|
||||||
|
return PaginationParams(limit=limit, offset=offset)
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### C. `StatusFilterParams` (Pydantic Model & Dependency Function)
|
||||||
|
|
||||||
|
* **Purpose:** Encapsulates status filtering parameters (`enabled`, `hidden`).
|
||||||
|
* **Dependency Function:** `get_status_filter_params()`
|
||||||
|
|
||||||
|
**Proposed `app/lib_general.py` additions:**
|
||||||
|
|
||||||
|
```python
|
||||||
|
# --- Pydantic Model for Status Filtering ---
|
||||||
|
class StatusFilterParams(BaseModel):
|
||||||
|
enabled: str = 'enabled' # 'enabled', 'disabled', 'all'
|
||||||
|
hidden: str = 'not_hidden' # 'hidden', 'not_hidden', 'all'
|
||||||
|
|
||||||
|
# --- Dependency Function for Status Filtering ---
|
||||||
|
def get_status_filter_params(
|
||||||
|
enabled: str = Query('enabled', description="Filter by object enabled status ('enabled', 'disabled', 'all')"),
|
||||||
|
hidden: str = Query('not_hidden', description="Filter by object hidden status ('hidden', 'not_hidden', 'all')"),
|
||||||
|
) -> StatusFilterParams:
|
||||||
|
# USER INPUT REQUIRED: Should we validate 'enabled' and 'hidden' values here
|
||||||
|
# (e.g., raise HTTP 400 if not 'enabled', 'disabled', 'all')?
|
||||||
|
return StatusFilterParams(enabled=enabled, hidden=hidden)
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### D. `SerializationParams` (Pydantic Model & Dependency Function)
|
||||||
|
|
||||||
|
* **Purpose:** Encapsulates Pydantic serialization options (`by_alias`, `exclude_unset`).
|
||||||
|
* **Dependency Function:** `get_serialization_params()`
|
||||||
|
|
||||||
|
**Proposed `app/lib_general.py` additions:**
|
||||||
|
|
||||||
|
```python
|
||||||
|
# --- Pydantic Model for Serialization Options ---
|
||||||
|
class SerializationParams(BaseModel):
|
||||||
|
by_alias: bool = True
|
||||||
|
exclude_unset: bool = False
|
||||||
|
|
||||||
|
# --- Dependency Function for Serialization Options ---
|
||||||
|
def get_serialization_params(
|
||||||
|
by_alias: bool = Query(True, description="Whether to use field aliases for serialization"),
|
||||||
|
exclude_unset: bool = Query(False, description="Whether to exclude unset fields from the response"),
|
||||||
|
) -> SerializationParams:
|
||||||
|
return SerializationParams(by_alias=by_alias, exclude_unset=exclude_unset)
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### E. `DelayParams` (Pydantic Model & Dependency Function)
|
||||||
|
|
||||||
|
* **Purpose:** Encapsulates the calculated `sleep_time` based on header/query delay parameters.
|
||||||
|
* **Dependency Function:** `get_delay_params()`
|
||||||
|
|
||||||
|
**Proposed `app/lib_general.py` additions:**
|
||||||
|
|
||||||
|
```python
|
||||||
|
# --- Pydantic Model for Delay ---
|
||||||
|
class DelayParams(BaseModel):
|
||||||
|
sleep_time_ms: int = 0 # Raw delay value in ms
|
||||||
|
sleep_time_s: float = 0.0 # Converted to seconds for time.sleep()
|
||||||
|
|
||||||
|
# --- Dependency Function for Delay ---
|
||||||
|
def get_delay_params(
|
||||||
|
x_delay_ms: Optional[int] = Header(0, alias='X-Delay-ms', description="Delay response for X milliseconds (header)"),
|
||||||
|
delay_ms: Optional[int] = Query(0, description="Delay response for X milliseconds (query parameter)"),
|
||||||
|
) -> DelayParams:
|
||||||
|
calculated_delay_ms = max(x_delay_ms or 0, delay_ms or 0)
|
||||||
|
return DelayParams(sleep_time_ms=calculated_delay_ms, sleep_time_s=calculated_delay_ms / 1000.0)
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### F. Refactoring `common_route_params` and `common_route_params_no_account_id`
|
||||||
|
|
||||||
|
The existing `common_route_params` and `common_route_params_no_account_id` functions, along with their associated Pydantic models, would be deprecated or removed, as their functionality is now covered by these new granular dependencies.
|
||||||
|
|
||||||
|
## 3. How this affects route functions in `api_crud_v3.py`
|
||||||
|
|
||||||
|
Route functions will now explicitly declare only the dependencies they need. This significantly cleans up the function signatures and makes their requirements explicit.
|
||||||
|
|
||||||
|
**Example `get_obj_li` function signature update:**
|
||||||
|
|
||||||
|
```python
|
||||||
|
# Old:
|
||||||
|
# async def get_obj_li(
|
||||||
|
# obj_type_l1: str,
|
||||||
|
# for_obj_type: Optional[str] = None,
|
||||||
|
# for_obj_id: Optional[str] = None,
|
||||||
|
# hidden: str = 'not_hidden',
|
||||||
|
# order_by_li: Optional[str] = None,
|
||||||
|
# jp: Optional[Union[str, None]] = None,
|
||||||
|
# x_delay_ms: Optional[int] = Header(0, alias='X-Delay-ms'),
|
||||||
|
# delay_ms: Optional[int] = Query(0),
|
||||||
|
# commons: Common_Route_Params = Depends(common_route_params),
|
||||||
|
# ):
|
||||||
|
|
||||||
|
# New (using new dependencies):
|
||||||
|
from fastapi import Depends, Header, Query, HTTPException
|
||||||
|
from pydantic import BaseModel
|
||||||
|
from typing import Optional, Union # ... other typing imports (existing)
|
||||||
|
import time # (existing)
|
||||||
|
|
||||||
|
# ... (assuming new AccountContext, PaginationParams, etc. and their get_X_params functions are defined in lib_general)
|
||||||
|
|
||||||
|
@router.get('/{obj_type_l1}/', response_model=Resp_Body_Base)
|
||||||
|
async def get_obj_li(
|
||||||
|
obj_type_l1: str,
|
||||||
|
account_context: AccountContext = Depends(get_account_context),
|
||||||
|
pagination: PaginationParams = Depends(get_pagination_params),
|
||||||
|
status_filter: StatusFilterParams = Depends(get_status_filter_params),
|
||||||
|
serialization: SerializationParams = Depends(get_serialization_params),
|
||||||
|
delay_params: DelayParams = Depends(get_delay_params),
|
||||||
|
for_obj_type: Optional[str] = Query(None), # Still needed for parent filtering
|
||||||
|
for_obj_id: Optional[str] = Query(None), # Still needed for parent filtering
|
||||||
|
order_by_li: Optional[str] = None,
|
||||||
|
jp: Optional[Union[str, None]] = None,
|
||||||
|
# The 'response: Response' object will be handled by FastAPI directly, or can be passed through a dependency if really needed.
|
||||||
|
):
|
||||||
|
# Apply delay
|
||||||
|
if delay_params.sleep_time_s > 0:
|
||||||
|
time.sleep(delay_params.sleep_time_s)
|
||||||
|
|
||||||
|
# Use parameters:
|
||||||
|
# account_id = account_context.account_id
|
||||||
|
# account_id_random = account_context.account_id_random
|
||||||
|
# limit = pagination.limit
|
||||||
|
# offset = pagination.offset
|
||||||
|
# enabled_status = status_filter.enabled
|
||||||
|
# hidden_status = status_filter.hidden
|
||||||
|
# by_alias_option = serialization.by_alias
|
||||||
|
# exclude_unset_option = serialization.exclude_unset
|
||||||
|
|
||||||
|
# ... rest of your logic, now using these granular parameters ...
|
||||||
|
```
|
||||||
|
|
||||||
|
## 4. User Input Required
|
||||||
|
|
||||||
|
Please review this proposal. Specifically, I need your input on:
|
||||||
|
|
||||||
|
1. **Approval of the Overall Structure:** Do you agree with breaking down `common_route_params` into these more granular dependencies?
|
||||||
|
2. **`AccountContext` Logic:**
|
||||||
|
* How should `x_no_account_id` be handled precisely? Is it just a flag to bypass `x_account_id` validation, or does it imply a specific "anonymous" account context?
|
||||||
|
* How should `x_no_account_id_token` be validated? The current code looks up an account based on it, but is there a specific token validation mechanism beyond just lookup? Should it have a default value for unauthenticated use?
|
||||||
|
3. **`StatusFilterParams` Validation:** Should the `enabled` and `hidden` query parameters (e.g., `'enabled'`, `'disabled'`, `'all'`, `'hidden'`, `'not_hidden'`, `'all'`) be strictly validated within `get_status_filter_params` (e.g., raising `HTTPException 400` if an invalid string is provided)?
|
||||||
|
|
||||||
|
Your feedback on these points is crucial before proceeding with implementation.
|
||||||
Reference in New Issue
Block a user