-
Notifications
You must be signed in to change notification settings - Fork 336
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
[Refactor] Unify endpoints response / REST API v2 #764
Comments
@enrichman Thx for the versioned prefix suggestion! |
|
(Notes from dev meeting) V2 REST API:
|
I would like to work on the issue :) My idea is to create 2 APIRouter and for each endpoint function create 2 functions (one for each version). The differences is that in the v2 endpoint function wraps the result into a JSendResponse. For example: class JSendResponse(BaseModel):
status:JSendStatusEnum
data: Union[Dict, None] = None
message:Union[str, None] = None # Default router
router_v1 = APIRouter()
# Router v2
router_v2 = APIRouter()
def home() -> Dict:
with open("pyproject.toml", "rb") as f:
project_toml = tomli.load(f)["project"]
return {
"status": "We're all mad here, dear!",
"version": project_toml['version']
}
# server status
@router_v1.get("/")
async def home_v1() -> Dict:
"""Server status"""
return home()
@router_v2.get("/", response_model=JSendResponse, response_model_exclude_none=True)
async def home_v2():
"""Server status"""
return {
"status" : JSendStatusEnum.success,
"data" : home()
} And then in main.py add the endpoints for v2. This implementation keeps the behaviour of the current endpoints but requires the refactoring of all the endpoint functions. What do you think? I am open to suggestions if you think there is a better way to implement this. |
@dave90 agree on the refactoring strategy, I don't know about the JSON send because from the research I'm doing it is not widespread and only adds overhad on directly inspecting the status code of the http response. Also many are suggesting to have Pydantic types for all endpoints, I don't see the point of encapsulating them inside I assigned this issue to you, start from something easy and do small PRs ;) Thanks!!! |
Agrre with @pieroit, Data should be a Union of Classes (LLMSettings,EmbedderSettings etc... )! |
My points are:
|
|
|
At the moment all the endpoints response are not uniform ( sometimes we give jsonSchema, sometimes a json with some field etc) and can create problem when the user uses the REST API instead of Language Specific clients, like the Typescript client.
I suggest the use of JSend as standard JSON response for all endpoints and change all HTTPExceptions responses, FastAPI provide the override of these exceptions with a simple function.
Fast API is a typed Backend framework, so I suggest also the use of an object call
JSendResponse
that wraps the JSend standard.ALL of this must be under this versioned prefix
/api/v2
so we can delay the breaking changes!P.S
JSend is usually the simplest and have many variations (only have status and data where wraps a string or an object, status can be the HTTP status code instead of a string etc…) !
The text was updated successfully, but these errors were encountered: