owncloud / owncloud/ocis

service handlers should use more concrete signatures

Open
#5,209 1 comment 0 reactions 0 assignees View on GitHub
Type:Bug Type:Technical-Debt
Dominant language
Go
Stars
2.1k
Forks
274
Avg merge
2d 1h
Merged PRs (30d)
103

Description

Currently, our http services use always use a `(w http.ResponseWriter, r *http.Request)` signature. While that pattern is used for http handlers it is not used for grpc handlers which use specific request and response objects, delegating the request parsing and writing to middlewares.

AFAICT `ocis-hello` shows the correct pattern to use:
https://github.com/owncloud/ocis-hello/blob/edc8ef1d4b79dba29321cc70a3f6a20373d30f65/pkg/service/v0/service.go#L19-L21

Actually it should receive a Context as the first parameter, but anyway, the HTTP service is then created using chi by parsing the json and then calling the handler:
https://github.com/owncloud/ocis-hello/blob/edc8ef1d4b79dba29321cc70a3f6a20373d30f65/pkg/server/http/server.go#L92

The command first creates the handler and then wraps it with tracing, logging and metrics middlewares which can now operate directly on the request and response parameters.

https://github.com/owncloud/ocis-hello/blob/edc8ef1d4b79dba29321cc70a3f6a20373d30f65/pkg/command/server.go#L90-L107

In the chi handler for graph we can parse the odata request using `godata.ParseRequest(...)` and pass that to the Service interface so it can sort, filter, select and expand the object as needed.
An example signature would be:
```go
// the old service interface
func (g Graph) GetMe(w http.ResponseWriter, r *http.Request) {}

// the new signature should be concerned with concrete types
func (g Graph) GetMe(ctx context.Context, id string) (libregraph.User, error) {}

// unfortunately, we need to pass along
// - which filelds to select and expand, GRPC uses field/update mask, see https://aip.bybutter.com/161
// - pagination, GRPC uses page_size and page_token, see https://aip.bybutter.com/158) and
// - filtering, GRPC uses an [EBNF](https://aip.bybutter.com/assets/misc/ebnf-filtering.txt) based filter, see https://aip.bybutter.com/160
func (g Graph) GetMe(ctx context.Context, id string, fieldMask string) (libregraph.User, error) {}

// for listing this becomes clearer:
func (g Graph) ListUsers(ctx context.Context, filter, fieldMask, page_token string, pageSize int) (libregraph.User, error) {}
```

Then the service interface from
```go
type Service interface {
ServeHTTP(http.ResponseWriter, *http.Request)
GetMe(http.ResponseWriter, *http.Request)
GetUsers(http.ResponseWriter, *http.Request)
GetUser(http.ResponseWriter, *http.Request)
PostUser(http.ResponseWriter, *http.Request)
DeleteUser(http.ResponseWriter, *http.Request)
PatchUser(http.ResponseWriter, *http.Request)
ChangeOwnPassword(http.ResponseWriter, *http.Request)

GetGroups(http.ResponseWriter, *http.Request)
GetGroup(http.ResponseWriter, *http.Request)
PostGroup(http.ResponseWriter, *http.Request)
PatchGroup(http.ResponseWriter, *http.Request)
DeleteGroup(http.ResponseWriter, *http.Request)
GetGroupMembers(http.ResponseWriter, *http.Request)
PostGroupMember(http.ResponseWriter, *http.Request)
DeleteGroupMember(http.ResponseWriter, *http.Request)

GetDrives(w http.ResponseWriter, r *http.Request)
GetSingleDrive(w http.ResponseWriter, r *http.Request)
GetAllDrives(w http.ResponseWriter, r *http.Request)
GetRootDriveChildren(w http.ResponseWriter, r *http.Request)
CreateDrive(w http.ResponseWriter, r *http.Request)
UpdateDrive(w http.ResponseWriter, r *http.Request)
DeleteDrive(w http.ResponseWriter, r *http.Request)
}
```

should become more meaningful...

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.