Directives: Allow Argument Validation #609
Conversation
| @@ -31,11 +31,15 @@ type HasRoleDirective struct { | |||
| Role string | |||
| } | |||
|
|
|||
| func (h *HasRoleDirective) AllowLocation(l string) bool { | |||
| return l == "FIELD_DEFINITION" | |||
There was a problem hiding this comment.
Suspect that locations need to become part of the API, maybe constants in the ast package?
type Location string
const (
LocationFieldDefinition Location = "FIELD_DEFINITION"
LocationArgumentDefinition Location = "ARGUMENT_DEFINITION"
… etc
)
There was a problem hiding this comment.
These definitely need to become part of ast package. Users should not repeat strings like "FIELD_DEFINITION" in their code.
| @@ -26,5 +27,5 @@ type ResolverInterceptor interface { | |||
|
|
|||
| // Validator directive which executes before anything is resolved, allowing the request to be rejected. | |||
| type Validator interface { | |||
There was a problem hiding this comment.
Thinking that the type (& function) may need a rename before a release is made. I believe there are 3 different times where parts of the schema could be “visited” (are there more?):
- When the schema is parsed (during parsing? After successful?)
- While handling a request before executing any resolvers ie validation currently, part of query complexity scoring soon
- While executing resolvers for a request ie as resolver middleware, currently applied in the interceptor types
Unsure how each would ideally be named for clarity, and for consistency with other GraphQL implementations
There was a problem hiding this comment.
Yes, you are right. I think that the middleware makes sense to be called interceptors as they are definitely not visitors. A visitor implies visiting all fields in the schema or the current executable document. So I would call the existing directives interceptors and I would allow for a pluggable visitor plugin which has access to the field directives. These directives, however, would be just decorative and would not have any callback themselves. The author of the visitor should decide whether and how to use them when it makes sense.
Proof of concept for implementing directive validation for field arguments. Validation for many cases applies better on the arguments than it does against the field itself.
Updating validators to use the same interface for field and arg defs, allowing the implementation to define where it can be used. The ast definition is supplied to the call which allows the implementation to do the appropriate thing for each (supported) type, and retrieve details from the ast like argument names, which are useful for errors etc
WithNullableArgumentDirective required updates to work with the changed directive interfaces
dackroyd
force-pushed
the
directive-validators-for-args
branch
from
8057ef9
to
d4af0bd
Compare
|
Nearly a year on, and life has been busy. Perhaps we can pick this back up soon @pavelnikolov ? |
|
I'll get back on this either this or next week. |
| func (h *HasRoleDirective) ImplementsDirective() string { | ||
| return "hasRole" | ||
| } | ||
|
|
||
| func (h *HasRoleDirective) Validate(ctx context.Context, _ interface{}) error { | ||
| func (h *HasRoleDirective) Validate(ctx context.Context, _, _ interface{}) error { |
There was a problem hiding this comment.
Just thinking out loud - should this method be called Validate? Can these do something than validation? I know that as per the PR title and your intentions is to add validation, however, I'm trying just to think if this can be more general. Still don't have a good use case, though 🤔
| @@ -19,11 +19,15 @@ type HasRoleDirective struct { | |||
| Role string | |||
| } | |||
|
|
|||
| func (h *HasRoleDirective) AllowLocation(l string) bool { | |||
| return l == "FIELD_DEFINITION" | |||
There was a problem hiding this comment.
As per above, this needs to be a constant, however, we need to probably export only the field definitions that are supported by directives at the moment.
| return "customDirective" | ||
| } | ||
|
|
||
| func (v *customArgDirectiveVisitor) Resolve(ctx context.Context, args interface{}, next directives.Resolver) (output interface{}, err error) { |
There was a problem hiding this comment.
Let's drop the Visitor suffix from this type.
| @@ -86,6 +87,34 @@ func (f *Field) Resolve(ctx context.Context, resolver reflect.Value, args interf | |||
| return wrapResolver(ctx, args) | |||
| } | |||
|
|
|||
| func (f *Field) ValidateArgs(ctx context.Context, args map[string]interface{}) []error { | |||
| d := f.Visitors | |||
There was a problem hiding this comment.
Still unsure about naming (thinking about Visitors), especially given the fact that the only method we call on these is Validate. Therefore, Validators makes more sense in this context.
| type FieldVisitors struct { | ||
| Interceptors []directives.ResolverInterceptor | ||
| Validators []directives.Validator | ||
| ArgValidators map[string][]directives.Validator | ||
| Interceptors []directives.ResolverInterceptor | ||
| Validators []directives.Validator |
There was a problem hiding this comment.
I should document these 3 use cases with appropriate example tests.
- ArgValidators is easy to understand
- Validators is for things that are not argument specific, e.g.
hasRole(applied to fields). - I can't see an example of resolver interceptor in this PR, unless I'm missing something. Did you mean a field directive to be applied to a (non-trivial) resolver? Can you give me an example?
|
I've opted to close this, as we've discussed taking directives in a different direction. I've started on #655 to strip out the current directives implementation, with the intention to replace it with something simpler and more flexible |
Proof of concept for implementing directive validation for field arguments. Validation for many cases applies better on the arguments than it does against the field itself.