Add Validating Directives #583

dackroyd · 2023-02-18T14:22:19Z

Adding support for directives which are evaluated prior to executing any resolvers. This allows validation to be performed on the request and prevent it from executing any significant work by rejecting the request early.

The most obvious case for this is authorization: based on the requested fields, we can tell whether the request is valid given the current user, and reject the entire request. If that were applied at resolution time, the request would have partially resolved, only to return errors for the specific fields which are not authorized.

fixes: #570

internal/exec/resolvable/resolvable.go

pavelnikolov · 2023-02-18T15:15:56Z

graphql_test.go

+			Variables:      map[string]interface{}{"episode": "NEWHOPE"},
+			ExpectedResult: "null",
+			ExpectedErrors: []*gqlerrors.QueryError{
+				{Message: `rebel scum cannot request imperial units`, Locations: []gqlerrors.Location{{Line: 58, Column: 3}}, Path: []interface{}{"hero", "height"}},


Maybe

Suggested change

{Message: `rebel scum cannot request imperial units`, Locations: []gqlerrors.Location{{Line: 58, Column: 3}}, Path: []interface{}{"hero", "height"}},

{Message: `rebes cannot request imperial units`, Locations: []gqlerrors.Location{{Line: 58, Column: 3}}, Path: []interface{}{"hero", "height"}},

pavelnikolov · 2023-02-18T15:16:20Z

graphql_test.go

+		Unit string
+	})
+	if ok && v.Unit == "FOOT" {
+		return fmt.Errorf("rebel scum cannot request imperial units")


pavelnikolov · 2023-02-18T15:19:48Z

internal/exec/resolvable/resolvable.go

@@ -47,26 +47,32 @@ type Field struct {
 	HasContext        bool
 	HasError          bool
 	ArgsPacker        *packer.StructPacker
-	DirectiveVisitors []directives.ResolverInterceptor
+	DirectiveVisitors *FieldDirectives


Technically speaking the type here FieldDirectives is a bit misleading because these are being applied to FIELD_DEFINITION. And I would like to add field directives too, which are applied on FIELD and are packed at query exec time.

Will have to think on the naming here. Intent is that they are the directives applied to the schema field. In the resolvable package, the schema field is referred to as Field

internal/exec/resolvable/resolvable.go

pavelnikolov · 2023-02-18T15:25:52Z

internal/exec/resolvable/resolvable.go

-		// For now this is the only valid directive function
-		if _, ok := v.(directives.ResolverInterceptor); !ok {
+		switch v.(type) {
+		default:


default last, please

internal/exec/resolvable/resolvable.go

pavelnikolov · 2023-02-18T17:10:36Z

internal/exec/validate.go

+			errs[i] = &errors.QueryError{
+				Err:        err,
+				Message:    err.Error(),
+				Locations:  []errors.Location{f.field.Loc}, // TODO: is this the correct location?


I should’ve added more context in the comment here: I’m unclear whether the location should be where the field is defined in the schema definition, or where in the requested query the field is

We are validating the query.

This is the location in the schema being applied here, rather than the query. I'm not sure that we have access to the other

internal/exec/validate.go

pavelnikolov · 2023-02-20T23:09:33Z

I resolved conflicts in this PR.

pavelnikolov · 2023-02-21T13:07:49Z

graphql_test.go

+					social.Schema,
+					"role: Role!",
+					`role: Role! @hasRole(role: "ADMIN")`,
+				),


(nitpick) You could have used schema extension here : )

I'll add examples how to do that soon.

internal/exec/resolvable/resolvable.go

pavelnikolov · 2023-02-21T13:23:52Z

internal/exec/validate.go

+		switch sel := sel.(type) {
+		case *selected.SchemaField:
+			field, ok := fieldByAlias[sel.Alias]
+			if !ok { // validation already checked for conflict (TODO)


What does // validation already checked for conflict (TODO) mean in this context?

Copy-pasted from the execX equivalent, I believe would apply here as well, but not sure what the intended long-term resolution is

I guess we are checking that the alias doesn't exist as a field. This is fine I guess. I'll add a unit test to confirm that. Not related to your PR, though.

internal/exec/validate.go

pavelnikolov · 2023-02-23T09:34:10Z

After thinking a little bit more, directives should not be part of the resolver. Instead, they should indeed be passed as a schema option. The reason for that is the possibility to parse a schema without actually passing any resolver. This would allow us to inspect the schema and to call [Schema.AST] on it. In future we might have some directives on schema for example or on one of the types which would give some schema parsing instructions. Therefore, directives should not be tied to the resolver in order to make them future-proof.
I also created a separate issue for point 2. above, so this PR is ready to merge, IMO.

Adding support for directives which are evaluated prior to executing any resolvers. This allows validation to be performed on the request and prevent it from executing any significant work by rejecting the request early. The most obvious case for this is authorization: based on the requested fields, we can tell whether the request is valid given the current user, and reject the entire request. If that were applied at resolution time, the request would have partially resolved, only to return errors for the specific fields which are not authorized.

pavelnikolov · 2023-02-23T11:40:44Z

internal/exec/exec.go

+			r.Errs = errs
+			out.Write([]byte("null"))
+			return
+		}


We can add a global short circuit here. If the schema doesn't have any validation directives, then this entire block can be skipped.

Adding support for directives which are evaluated prior to executing any resolvers. This allows validation to be performed on the request and prevent it from executing any significant work by rejecting the request early. The most obvious case for this is authorization: based on the requested fields, we can tell whether the request is valid given the current user, and reject the entire request. If that were applied at resolution time, the request would have partially resolved, only to return errors for the specific fields which are not authorized.

pavelnikolov reviewed Feb 18, 2023

View reviewed changes

dackroyd force-pushed the add-validating-directives branch from d3994f5 to f0be47b Compare February 19, 2023 23:34

pavelnikolov marked this pull request as ready for review February 20, 2023 23:09

pavelnikolov reviewed Feb 21, 2023

View reviewed changes

internal/exec/resolvable/resolvable.go Show resolved Hide resolved

pavelnikolov reviewed Feb 21, 2023

View reviewed changes

internal/exec/resolvable/resolvable.go Show resolved Hide resolved

pavelnikolov reviewed Feb 21, 2023

View reviewed changes

internal/exec/validate.go Show resolved Hide resolved

pavelnikolov reviewed Feb 21, 2023

View reviewed changes

internal/exec/validate.go Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

pavelnikolov added this to the v1.6.0 milestone Feb 23, 2023

pavelnikolov added the enhancement label Feb 23, 2023

dackroyd force-pushed the add-validating-directives branch from b6cd960 to 9ae7df1 Compare February 23, 2023 10:44

pavelnikolov reviewed Feb 23, 2023

View reviewed changes

pavelnikolov merged commit d126bba into graph-gophers:master Feb 23, 2023

dackroyd deleted the add-validating-directives branch February 25, 2023 10:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Validating Directives #583

Add Validating Directives #583

dackroyd commented Feb 18, 2023 •

edited by pavelnikolov

Loading

pavelnikolov Feb 18, 2023

pavelnikolov Feb 18, 2023

pavelnikolov Feb 18, 2023

dackroyd Feb 18, 2023

pavelnikolov Feb 18, 2023

pavelnikolov Feb 18, 2023

dackroyd Feb 18, 2023

pavelnikolov Feb 20, 2023

dackroyd Feb 23, 2023

pavelnikolov commented Feb 20, 2023

pavelnikolov Feb 21, 2023

pavelnikolov Feb 21, 2023

dackroyd Feb 23, 2023

pavelnikolov Feb 23, 2023

This comment was marked as resolved.

pavelnikolov commented Feb 23, 2023

pavelnikolov Feb 23, 2023

	{Message: `rebel scum cannot request imperial units`, Locations: []gqlerrors.Location{{Line: 58, Column: 3}}, Path: []interface{}{"hero", "height"}},
	{Message: `rebes cannot request imperial units`, Locations: []gqlerrors.Location{{Line: 58, Column: 3}}, Path: []interface{}{"hero", "height"}},

Add Validating Directives #583

Add Validating Directives #583

Conversation

dackroyd commented Feb 18, 2023 • edited by pavelnikolov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavelnikolov commented Feb 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

pavelnikolov commented Feb 23, 2023

Choose a reason for hiding this comment

dackroyd commented Feb 18, 2023 •

edited by pavelnikolov

Loading