Skip to content
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

service handlers should use more concrete signatures #5209

Open
butonic opened this issue Dec 8, 2022 · 1 comment
Open

service handlers should use more concrete signatures #5209

butonic opened this issue Dec 8, 2022 · 1 comment

Comments

@butonic
Copy link
Member

butonic commented Dec 8, 2022

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:

// 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

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...

@butonic
Copy link
Member Author

butonic commented Jan 17, 2023

maybe we can refactor the handlers one by one ... one challenge is that we would have to parse the query parameters in the http service but still hand them over to the business implementation. we would be implementing a protobuf like generated api and include filters and pagination. 🤔 but that would have to be able to express the odata filters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant