Add the ability to instrument a graphql request #8
Conversation
edersonmf
requested review from
GiancarloJung,
pedrocunial,
pdominguite and
pedro-gomes-sumup
chain.go
Outdated
| ResponseParser interface { | ||
| parseResponse() (*GraphResponse, Error) | ||
| } | ||
|
|
||
| Action interface { | ||
| call() ResponseParser | ||
| } | ||
|
|
||
| Validated interface { | ||
| createRequest() Action | ||
| } | ||
|
|
||
| Init interface { | ||
| validate() Validated | ||
| } |
There was a problem hiding this comment.
Do we need all these different interfaces for defining private methods from a private struct? Defining public interfaces with private methods seems a bit off to me.
Maybe keeping this as different interfaces, but having the methods public and their implementations separated could be a better approach? This way we could allow the users of our lib to only substitute a part of the chain (e.g. they could annotate one of the steps with a custom error handler, counter, etc) instead of requiring them to re-implement the whole lib.
There was a problem hiding this comment.
That's a good point and I was wondering that as well.
Let me submit another version of it. Let me know your thoughts.
Thanks!
A Chain of Responsibility pattern is employed to allow callers adding custom instrumentation targeting graphql request and response objects. A couple good use cases for this are request logging, caching, and certain kinds of auth (e.g. bearer token renewal). In order to proceed of this kind of support a small refactor was required. So, there are somee changes that impact directly the previous structure of this module.
edersonmf
force-pushed
the
ederson/refactor
branch
from
b3439d7 to
4a7b996
Compare
edersonmf
force-pushed
the
ederson/refactor
branch
from
5ae59f9 to
d93b381
Compare
| c.logf("<< %s", buf.String()) | ||
| resp := c.operation.ResponseBodyAs() | ||
| var gr *GraphResponse | ||
| if c.operation.IsMutation() { |
There was a problem hiding this comment.
I created the Mutation struct to make the result.Successful field control if the request was of the Request (query) or Mutation type and based on the type of the request this flow decided what flow was gonna be used. If you think is easier to control this by a boolean flag we could also remove some of that logic and merge the Operation and Request interfaces and make the naming simpler, wdyt?
| @@ -5,147 +5,154 @@ import ( | |||
| "net/http" | |||
| ) | |||
|
|
|||
| // Request is a GraphQL request. | |||
| // queryOperation is a GraphQL defaultRequest. | |||
There was a problem hiding this comment.
| // queryOperation is a GraphQL defaultRequest. |
✂️
| ) | ||
| // Deprecated: in favor of NewGraphOperation | ||
| func NewRequest(q string) Operation { |
There was a problem hiding this comment.
| ) | |
| // Deprecated: in favor of NewGraphOperation | |
| func NewRequest(q string) Operation { | |
| ) | |
| // Deprecated: in favor of NewGraphOperation | |
| func NewRequest(q string) Operation { |
| Mutation struct { | ||
| Req *Req | ||
| // queryOperation is the regular graphQL query operation | ||
| queryOperation struct { |
There was a problem hiding this comment.
A request could be either a query or a mutation if struct can be both my suggestion is to name it defaultOperation, the field queryName could also be just name. wdyt?
There was a problem hiding this comment.
The refactor looks great, and I like the chain principle, could you also provide some examples on how to use its benefits on some tests? I also got a little confused between the names on the request.go file between the interfaces and the structs, but I left a comment about the operation that could make this simpler =]
| // client.log = func(s string) { log.Println(s) } | ||
| log func(s string) |
There was a problem hiding this comment.
Could this have a setter function? I'm not sure we could do this since this is a private field 🤔
A Chain of Responsibility pattern is employed to allow
callers adding custom instrumentation targeting graphql
request and response objects.
A couple good use cases for this are request logging,
caching, and certain kinds of auth (e.g. bearer token renewal).
In order to proceed of this kind of support a small refactor
was required. So, there are somee changes that impact directly
the previous structure of this module.