refactor: separate validation, response and business logic #1301
Conversation
There was a problem hiding this comment.
Linter job fails:
Check failure on line 100 in backend/pkg/api/handlers/input_validation.go
printf: non-constant format string in call to
github.com/gobitfly/beaconchain/pkg/api/handlers.newBadRequestErr
Check failure on line 156 in backend/pkg/api/handlers/input_validation.go
unnecessary leading newline (whitespace)
LuccaBitfly
force-pushed
the
BEDS-94/abstract-handle-test
branch
10 times, most recently
from
ee1c378 to
35649cc
Compare
LuccaBitfly
force-pushed
the
BEDS-94/abstract-handle-test
branch
from
35649cc to
def1b01
Compare
…oardGroupSummary` logics
LuccaBitfly
force-pushed
the
BEDS-94/abstract-handle-test
branch
from
def1b01 to
f58c2e6
Compare
LuccaBitfly
force-pushed
the
BEDS-94/abstract-handle-test
branch
2 times, most recently
from
714ef42 to
500906e
Compare
Deploying beaconchain with
|
| Latest commit: |
500906e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3d82629b.beaconchain.pages.dev |
| Branch Preview URL: | https://beds-94-abstract-handle-test.beaconchain.pages.dev |
| type Checker[T any] interface { | ||
| Check(params map[string]string, payload io.ReadCloser) (T, error) | ||
| } |
There was a problem hiding this comment.
Imo it's worth re-thinking naming for the interface and method name here, Check(er) is a bit vague. It's only supposed to be used in the context of input validation, right? Maybe InputValidator and Validate() or so
| if contentType := r.Header.Get("Content-Type"); !reJsonContentType.MatchString(contentType) { | ||
| v.add("request body", "'Content-Type' header must be 'application/json'") | ||
| } |
There was a problem hiding this comment.
Couldn't this check be kept somehow? Maybe move it to middleware, like if there is a body present it should be application/json
|
|
||
| bodyBytes, err := io.ReadAll(r.Body) | ||
| r.Body = io.NopCloser(bytes.NewReader(bodyBytes)) // unconsume body for error logging | ||
| func (v *validationError) checkBody(data interface{}, requestBody io.ReadCloser) error { // <- should be checkBody after refactoring |
There was a problem hiding this comment.
| func (v *validationError) checkBody(data interface{}, requestBody io.ReadCloser) error { // <- should be checkBody after refactoring | |
| func (v *validationError) checkBody(data interface{}, requestBody io.ReadCloser) error { |
| @@ -560,3 +561,28 @@ func (v *validationError) checkTimestamps(r *http.Request, chartLimits ChartTime | |||
| return afterTs, beforeTs | |||
| } | |||
| } | |||
|
|
|||
| func (v *validationError) checkDashboardId(id string) interface{} { | |||
There was a problem hiding this comment.
This looks essentially like the same as https://github.com/gobitfly/beaconchain/blob/staging/backend/pkg/api/handlers/handler_service.go#L85-L109 to me, what's the reason for this? Would be cool if it was possible to merge them
| "github.com/gobitfly/beaconchain/pkg/api/types" | ||
| ) | ||
|
|
||
| // PublicPostValidatorDashboardGroups godoc |
There was a problem hiding this comment.
check: I think doc generation doesn't work anymore since the method name changed
| } | ||
|
|
||
| func (h *HandlerService) PostValidatorDashboardGroups(ctx context.Context, input inputPostValidatorDashboardGroups) (*types.ApiDataResponse[types.VDBPostCreateGroupData], error) { | ||
| dataAccessor := h.getDataAccessor(ctx) |
There was a problem hiding this comment.
This step will exist in virtually every business logic func, right? Could you think of a clean way to also inject this via handler_service.Handle(), I think that would be cool
| func stringAsBody(s string) io.ReadCloser { | ||
| return io.NopCloser(strings.NewReader(s)) | ||
| } |
There was a problem hiding this comment.
I guess this will be used in other unit test files, too. Maybe such helpers should be placed in a new/existing common file (prob same for handlerTestSetup())
| return ctx, NewHandlerService(da, da, nil, false) | ||
| } | ||
|
|
||
| func TestPostValidatorDashboardGroups(t *testing.T) { |
There was a problem hiding this comment.
Looking at the split of tests into input validation and business logic: Maybe both could be grouped under one test method which is responsible for the entire api call, and then either use subsubtests or just do input first and business logic after.
What do you think? Collapsing entire units at once could be nice dev qol
Adds a pattern for splitting the HTTP logic from the business logic in the API handler funcs. Adds a few example test cases. Changes the test workflow to include these tests.