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

Refactor handler package #885

Merged
merged 56 commits into from
Nov 28, 2019
Merged

Refactor handler package #885

merged 56 commits into from
Nov 28, 2019

Conversation

vektah
Copy link
Collaborator

@vektah vektah commented Oct 1, 2019

The handler package has grown organically for a long time, its time to clean up the interfaces and get it ready for 1.0.

New handler extension API

The core of this changes the handler package to be a set of composable extensions. The extensions implement a set of optional interfaces:

  • OperationParameterMutator runs before creating a OperationContext (formerly RequestContext). allows manipulating the raw query before parsing.
  • OperationContextMutator runs after creating the OperationContext, but before executing the root resolver.
  • OperationInterceptor runs for each incoming query after parsing and validation, for basic requests the writer will be invoked once, for subscriptions it will be invoked multiple times.
  • ResponseInterceptor runs around each graphql operation response. This can be called many times for a single operation the case of subscriptions.
  • FieldInterceptor runs around each field

Anatomy of a request@2x

Users of an extension should not need to know which extension points are being used by a given extension, they are added to the server simply by calling Use(extension).

There are a few convenience methods for defining middleware inline, instead of creating an extension

  • AroundFields
  • AroundOperations
  • AroundResponses

Middleware:

  • APQ
  • Query Complexity
  • Error Presenters and Recover Func
  • Introspection Query support (I've flipped this to opt-in)
  • Query AST cache
  • Tracing API
srv.Use(extension.Introspection{})
srv.Use(extension.AutomaticPersistedQuery{
	Cache: lru.New(100),
})
srv.Use(apollotracing.Tracer{})

Transports

In addition to extensions, transports can also be registered against the server. Only one transport can serve a given request.

Transports:

  • GET
  • JSON
  • Multipart form
  • Websockets

new usage looks like this

srv := New(es)

srv.AddTransport(transport.Websocket{
	KeepAlivePingInterval: 10 * time.Second,
})
srv.AddTransport(transport.Options{})
srv.AddTransport(transport.GET{})
srv.AddTransport(transport.POST{})
srv.AddTransport(transport.MultipartForm{})

More contesistent naming

As part of cleaning up the names the RequestContext has been renamed to OperationContext, as there can be multiple created during the lifecycle of a request. A new ResponseContext has also been created and error handling has been moved here. This allows each response in a subscription to have its own errors. I'm not sure what bugs this might have been causing before...

Removal of tracing

Many of the old interfaces collapse down into just a few extension points:
Anatomy of a request@2x (1)

The tracing interface has also been removed, tracing stats are now measured in core (eg time to parse query) and made available on the operation/response contexts. Much of the old interface was designed so that users of a tracer dont need to know which extension points it was listening to, the new handler extensions have the same goal.

Backward compatibility

I've added a backwards compatibility layer that keeps most of the original interface in place. There are a few places where BC is known to be broken:

  • ResponseMiddleware: The signature used to be func(ctx context.Context, next func(ctx context.Context) []byte) []byte and is now func(ctx context.Context) *Response. We could maintain BC by marshalling to json before and after, but the change is pretty easy to make and is likely to cause less issues.
  • The Tracer interface has been removed, any tracers will need to be reimplemented against the new extension interface.

graphql/handler/server.go Outdated Show resolved Hide resolved
graphql/handler/tracer.go Outdated Show resolved Hide resolved
@vektah vektah marked this pull request as ready for review November 11, 2019 06:13
@vvakame vvakame self-requested a review November 11, 2019 15:15
@vvakame
Copy link
Collaborator

vvakame commented Nov 12, 2019

There is handler.NewDefaultServer with the reccomended defaults.

👍

I've added a Validate method instead

cool! 👍
I'm loving Validate(schema ExecutableSchema) error

Whats are you using OperationContextMutator for that has application errors?

user login checker. out (internal) application doesn't response any contents when user is not loggged-in to our company's SSO.
I want to access something likes srv.GetErrorPresenter from ctx. But I have workaround now, It's not serious matter.

func userLoginChecker() interface {
	graphql.HandlerExtension
	graphql.OperationContextMutator
} {
	errPresenter := graphQLErrorPresenter() // `srv.SetErrorPresenter(graphQLErrorPresenter())`

	return &operationContextMutatorImpl{
		name: "UserLoginChecker",
		f: func(ctx context.Context, rc *graphql.OperationContext) *gqlerror.Error {
			user := domains.CurrentUser(ctx)
			if user == nil {
				loginURL, err := aeuser.LoginURL(ctx, "/")
				if err != nil {
					return gqlerror.Errorf("%s", err.Error())
				}
				return errPresenter(ctx, domains.ErrLoginRequired.Detailf("loginURL: %s", loginURL))
			}
			return nil
		},
	}
}

I've taken a stab at allowing extensions to communicate. What do you think?

rc.Stats.GetExtension is looks good for me 👍
so, If I say something greedy, I wanna access it from APQ too. I'm logging PersistedQueryHash with

	var persistedQueryHash string
	{
		b := sha256.Sum256([]byte(rc.RawQuery))
		persistedQueryHash = hex.EncodeToString(b[:])
	}

but rc.Stats.GetExtension("APQ").(*FooBar) is more easy.


suggestions

  • OperationParameterMutator (and other HE) should be embed HandlerExtension interface
    • var _ graphql.OperationContextMutator = (*foo)(nil) is not enough. need var _ graphql.HandlerExtension = (*foo)(nil), but it's not convenience.
  • Does HandlerExtension have naming convention?
    • something like CamelCase or camelCase or...? basically, CamelCase is popular in code now.
  • graphql.GetOperationContext should not be return nil in all situations
    • problem is ... when executor#CreateOperationContext returns return nil, err, executor#DispatchError called. but apollotracing#InterceptResponse doesn't check rc is nil. nil reference occurred.
    • all of hook point (HandlerExtension) can access to OperationContext via ctx. It's easy to understand & coding I think.

@vvakame
Copy link
Collaborator

vvakame commented Nov 25, 2019

👀

@vektah
Copy link
Collaborator Author

vektah commented Nov 28, 2019

@vvakame I've injected operation context earlier now, so it should be availble during all of the handler hooks, and it now panics when trying to fetch it.

This makes it possible for APQ to push stats in (it runs very early, on the raw request params before decoding)/

@vvakame
Copy link
Collaborator

vvakame commented Nov 28, 2019

cool! 👍
I made #947 . if its merged, our internal tools will be work with this PR branch.

@vektah
Copy link
Collaborator Author

vektah commented Nov 28, 2019

Ok, time to ship this then. Lets start moving towards 0.11 (or maybe 1.0, if we get the plugin interfaces sorted too...)

@vektah vektah merged commit 412a72f into master Nov 28, 2019
@vektah vektah deleted the handler-refactor branch November 28, 2019 12:00
takanabe added a commit to takanabe/howtographql that referenced this pull request Jun 22, 2021
Refactoring the handler package deprecated some methods and this tutorial still uses those methods. Thus, this commit retires the following methods and use the new alternatives.

* https://github.com/99designs/gqlgen/blob/5ad012e3d7be1127706b9c8a3da0378df3a98ec1/handler/handler.go#L17
* https://github.com/99designs/gqlgen/blob/5ad012e3d7be1127706b9c8a3da0378df3a98ec1/handler/handler.go#L242

See 99designs/gqlgen#885 for more details about the refactor.
TasinIshmam pushed a commit to howtographql/howtographql that referenced this pull request Jun 22, 2021
* docs: Retire deprecated handler methods

Refactoring the handler package deprecated some methods and this tutorial still uses those methods. Thus, this commit retires the following methods and use the new alternatives.

* https://github.com/99designs/gqlgen/blob/5ad012e3d7be1127706b9c8a3da0378df3a98ec1/handler/handler.go#L17
* https://github.com/99designs/gqlgen/blob/5ad012e3d7be1127706b9c8a3da0378df3a98ec1/handler/handler.go#L242

See 99designs/gqlgen#885 for more details about the refactor.

* fix: use handler.NewDefaultServer

To use out-of-box transport configurations, use handler.NewDefaultServer instead of handler.New.

* docs: Retire deprecated handler methods
cgxxv pushed a commit to cgxxv/gqlgen that referenced this pull request Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants