Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Directives execution order #1398

Closed
prusov opened this issue Nov 23, 2020 · 2 comments
Closed

Directives execution order #1398

prusov opened this issue Nov 23, 2020 · 2 comments

Comments

@prusov
Copy link

prusov commented Nov 23, 2020

What happened?

Directives execution order is not logical.
Directives for input are executed before directives for mutation/query itself

In my case, this behavior leads to that input validation occurs in any case, even if the user does not have a role to execute mutation.

What did you expect?

The order of calling directives should be from "general" to "specific". That is, first for the query / mutation, and then for its arguments.

See schema for example.
expected: hasRole, canEdit
actial: canEdit, hasRole

Minimal graphql.schema and models to reproduce

directive @hasRole(role: Role!) on FIELD_DEFINITION
directive @canEdit on FIELD_DEFINITION

input EditProjectData {
    id: Int! @canEdit
    data: String!
}

type Mutation {
	editProject(input: EditProjectData): Bool @hasRole(role: USER)
}

versions

  • gqlgen version?
    v0.11.3-dev

  • go version?
    go version go1.13.4 windows/amd64

  • dep or go modules?
    go modules

@kim-hetrafi
Copy link

kim-hetrafi commented Feb 10, 2021

Additionally, is the right to left order intended? Can find anything on it in the official gql spec.

type Query {
  me: User! @isAuthenticated() @isLevel(role: USER)
}

will execute isLevel before isAuthenticated

generated.go:

func (ec *executionContext) _Query_me(ctx context.Context, field graphql.CollectedField) (ret graphql.Marshaler) {
	defer func() {
		if r := recover(); r != nil {
			ec.Error(ctx, ec.Recover(ctx, r))
			ret = graphql.Null
		}
	}()
	fc := &graphql.FieldContext{
		Object:     "Query",
		Field:      field,
		Args:       nil,
		IsMethod:   true,
		IsResolver: true,
	}

	ctx = graphql.WithFieldContext(ctx, fc)
	resTmp, err := ec.ResolverMiddleware(ctx, func(rctx context.Context) (interface{}, error) {
		directive0 := func(rctx context.Context) (interface{}, error) {
			ctx = rctx // use context from middleware stack in children
			return ec.resolvers.Query().Me(rctx)
		}
		directive1 := func(ctx context.Context) (interface{}, error) {
			if ec.directives.IsAuthenticated == nil {
				return nil, errors.New("directive isAuthenticated is not implemented")
			}
			return ec.directives.IsAuthenticated(ctx, nil, directive0)
		}
		directive2 := func(ctx context.Context) (interface{}, error) {
			role, err := ec.unmarshalNRole(ctx, "USER")
			if err != nil {
				return nil, err
			}
			if ec.directives.IsLevel == nil {
				return nil, errors.New("directive isLevel is not implemented")
			}
			return ec.directives.IsLevel(ctx, nil, directive1, role)
		}

		tmp, err := directive2(rctx)
		if err != nil {
			return nil, graphql.ErrorOnPath(ctx, err)
		}
		if tmp == nil {
			return nil, nil
		}
		if data, ok := tmp.(*model.User); ok {
			return data, nil
		}
		return nil, fmt.Errorf(`unexpected type %T from directive, should be *model.User`, tmp)
	})
	if err != nil {
		ec.Error(ctx, err)
		return graphql.Null
	}
	if resTmp == nil {
		if !graphql.HasFieldError(ctx, fc) {
			ec.Errorf(ctx, "must not be null")
		}
		return graphql.Null
	}
	res := resTmp.(*model.User)
	fc.Result = res
	return ec.marshalNUser(ctx, field.Selections, res)
}

@prusov
Copy link
Author

prusov commented Apr 28, 2021

Directive order is significant, but the specification does not specify how.

There should be a clarification in the new spec

RFC: graphql/graphql-spec#470

ChilliCream/graphql-platform#1199

@frederikhors frederikhors converted this issue into discussion #1936 Feb 4, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants