-
Notifications
You must be signed in to change notification settings - Fork 691
[feat] open api eval other #288
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?
Conversation
(LogID: 2025092312090101009108820949935E9) Co-Authored-By: Coda <[email protected]>
(LogID: 2025092312090101009108820949935E9) Co-Authored-By: Coda <[email protected]>
(LogID: 2025092312090101009108820949935E9) Co-Authored-By: Coda <[email protected]>
…ationSet, GetEvaluationSet, ListEvaluationSets (LogID: 202509281948170100910941219983125) Co-Authored-By: Coda <[email protected]>
… convertor (LogID: 202509281948170100910941219983125) Co-Authored-By: Coda <[email protected]>
…ic to separate methods (LogID: 202509281948170100910941219983125) Co-Authored-By: Coda <[email protected]>
…rate methods (LogID: 202509281948170100910941219983125) Co-Authored-By: Coda <[email protected]>
…unified method parameter (LogID: 202509281948170100910941219983125) Co-Authored-By: Coda <[email protected]>
…error tracking (LogID: 202509281948170100910941219983125) Co-Authored-By: Coda <[email protected]>
…DTO/xxxDTO2DO convention (LogID: 202509281948170100910941219983125) Co-Authored-By: Coda <[email protected]>
…rchitecture (LogID: 20250928231227010091094121028E1E4) Co-Authored-By: Coda <[email protected]>
Change-Id: I17c1551cdb927585c03e7f7fb1131588643ce8a7
CozeLoop
left a comment
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.
Performed server-side Go code review focused on concurrency safety, correctness, and API functionality. See inline comments for Must-Fix and Should-Fix items.
|
invalid |
(LogID: 202511051154490100910941219725949) Co-Authored-By: Coda <[email protected]>
(LogID: 202511051154490100910941219725949) Co-Authored-By: Coda <[email protected]>
CozeLoop
left a comment
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.
Summary: This PR introduces open API endpoints and application/domain changes, but several handlers return empty payloads instead of invoking the generated Kitex client, breaking functional correctness and contracts. Additionally, the dataset RPC adapter ignores the new exact version filter, risking wrong version selection; and the metrics initialization uses sync.Once in a way that can permanently block initialization if meter==nil on the first call. Please address the Must-Fix handler delegations, wire the version filter correctly, and adjust metrics initialization semantics. Route annotations should also be aligned with the actual router paths.
| var err error | ||
| var req openapi0.SubmitExperimentOApiRequest | ||
| err = c.BindAndValidate(&req) | ||
| if err != nil { | ||
| c.String(consts.StatusBadRequest, err.Error()) | ||
| return | ||
| } | ||
|
|
||
| resp := new(openapi0.SubmitExperimentOApiResponse) | ||
|
|
||
| c.JSON(consts.StatusOK, resp) |
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.
[P1] Handler bypasses service invocation and returns empty payload
This endpoint validates input but never calls the underlying EvalOpenAPIService client. It responds with an empty JSON object, which breaks functional correctness and observability (no metrics, no domain logic). For consistency with other handlers in this file that use invokeAndRender, please delegate to the generated Kitex client:
| var err error | |
| var req openapi0.SubmitExperimentOApiRequest | |
| err = c.BindAndValidate(&req) | |
| if err != nil { | |
| c.String(consts.StatusBadRequest, err.Error()) | |
| return | |
| } | |
| resp := new(openapi0.SubmitExperimentOApiResponse) | |
| c.JSON(consts.StatusOK, resp) | |
| invokeAndRender(ctx, c, localEvalOpenAPIClient.SubmitExperimentOApi) |
This ensures request binding, RPC execution, error mapping, and standard response handling align with the existing pattern.
| var err error | ||
| var req openapi0.GetExperimentsOApiRequest | ||
| err = c.BindAndValidate(&req) | ||
| if err != nil { | ||
| c.String(consts.StatusBadRequest, err.Error()) | ||
| return | ||
| } | ||
|
|
||
| resp := new(openapi0.GetExperimentsOApiResponse) | ||
|
|
||
| c.JSON(consts.StatusOK, resp) |
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.
[P1] Handler returns empty JSON instead of calling service
GetExperimentsOApi binds and validates the request but does not invoke the RPC service, returning an empty response. This breaks the endpoint contract and makes monitoring misleading. Align with existing handlers by delegating to the Kitex client:
| var err error | |
| var req openapi0.GetExperimentsOApiRequest | |
| err = c.BindAndValidate(&req) | |
| if err != nil { | |
| c.String(consts.StatusBadRequest, err.Error()) | |
| return | |
| } | |
| resp := new(openapi0.GetExperimentsOApiResponse) | |
| c.JSON(consts.StatusOK, resp) | |
| invokeAndRender(ctx, c, localEvalOpenAPIClient.GetExperimentsOApi) |
| var err error | ||
| var req openapi0.ListExperimentResultOApiRequest | ||
| err = c.BindAndValidate(&req) | ||
| if err != nil { | ||
| c.String(consts.StatusBadRequest, err.Error()) | ||
| return | ||
| } | ||
|
|
||
| resp := new(openapi0.ListExperimentResultOApiResponse) | ||
|
|
||
| c.JSON(consts.StatusOK, resp) |
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.
[P1] Handler does not execute business logic
ListExperimentResultOApi validates the request and returns an empty payload. It should call the EvaluationOpenAPIService.ListExperimentResultOApi method to fetch results and metrics. Please use the standard delegation pattern:
| var err error | |
| var req openapi0.ListExperimentResultOApiRequest | |
| err = c.BindAndValidate(&req) | |
| if err != nil { | |
| c.String(consts.StatusBadRequest, err.Error()) | |
| return | |
| } | |
| resp := new(openapi0.ListExperimentResultOApiResponse) | |
| c.JSON(consts.StatusOK, resp) | |
| invokeAndRender(ctx, c, localEvalOpenAPIClient.ListExperimentResultOApi) |
| var err error | ||
| var req openapi0.GetExperimentAggrResultOApiRequest | ||
| err = c.BindAndValidate(&req) | ||
| if err != nil { | ||
| c.String(consts.StatusBadRequest, err.Error()) | ||
| return | ||
| } | ||
|
|
||
| resp := new(openapi0.GetExperimentAggrResultOApiResponse) | ||
|
|
||
| c.JSON(consts.StatusOK, resp) |
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.
[P1] Aggregation endpoint returns empty response, missing service call
GetExperimentAggrResultOApi should invoke the RPC client to retrieve aggregator results. Returning an empty object will mislead clients and break contracts. Please delegate to the Kitex client:
| var err error | |
| var req openapi0.GetExperimentAggrResultOApiRequest | |
| err = c.BindAndValidate(&req) | |
| if err != nil { | |
| c.String(consts.StatusBadRequest, err.Error()) | |
| return | |
| } | |
| resp := new(openapi0.GetExperimentAggrResultOApiResponse) | |
| c.JSON(consts.StatusOK, resp) | |
| invokeAndRender(ctx, c, localEvalOpenAPIClient.GetExperimentAggrResultOApi) |
| var err error | ||
| var req openapi0.UpdateEvaluationSetOApiRequest | ||
| err = c.BindAndValidate(&req) | ||
| if err != nil { | ||
| c.String(consts.StatusBadRequest, err.Error()) | ||
| return | ||
| } | ||
|
|
||
| resp := new(openapi0.UpdateEvaluationSetOApiResponse) | ||
|
|
||
| c.JSON(consts.StatusOK, resp) |
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.
[P1] Update handler does not call service
UpdateEvaluationSetOApi currently binds input and returns an empty response without invoking domain logic. This should delegate to the generated client to update the set and return a proper response:
| var err error | |
| var req openapi0.UpdateEvaluationSetOApiRequest | |
| err = c.BindAndValidate(&req) | |
| if err != nil { | |
| c.String(consts.StatusBadRequest, err.Error()) | |
| return | |
| } | |
| resp := new(openapi0.UpdateEvaluationSetOApiResponse) | |
| c.JSON(consts.StatusOK, resp) | |
| invokeAndRender(ctx, c, localEvalOpenAPIClient.UpdateEvaluationSetOApi) |
Change-Id: I669812745d8dd1db07558ea27ca58d405ac67faf
(LogID: 202511051154490100910941219725949) Co-Authored-By: Coda <[email protected]>
(LogID: 20251106114148010091094121586687D) Co-Authored-By: Coda <[email protected]>
What type of PR is this?
Check the PR title.
(Optional) Translate the PR title into Chinese.
(Optional) More detailed description for this PR(en: English/zh: Chinese).
en:
zh(optional):
(Optional) Which issue(s) this PR fixes: