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

Exception thrown in AuthorizeDirectiveType due to no default constructor in AuthorizeDirective #440

Closed
michaelmccord opened this issue Dec 31, 2018 · 3 comments
Assignees
Labels
🐛 bug Something isn't working 🎉 enhancement New feature or request
Milestone

Comments

@michaelmccord
Copy link
Contributor

Describe the bug
Due to the behavior of Directive.ToObject, AuthorizeDirectiveType throws an exception in AuthorizeAsync on this line:

AuthorizeDirective directive = context.Directive
                .ToObject<AuthorizeDirective>();

The exception is thrown due to the use of Activator.CreateInstance where AuthorizeDirective has no default constructor and no constructor arguments are provided to Activator.CreateInstance. This circumstance seems to be the result of choices made around the "implicit" arguments model which seem to imply (at least currently...assuming I understand correctly) that a directive must have a default constructor.

To Reproduce
Steps to reproduce the behavior:

  1. Add the @authorize directive to a schema
  2. Register the directive
  3. Query the field or type which is protected by the directive
  4. Note that an exception is thrown and processing continues as normal (the fact that processing continues as normal seems to be an issue as well, I would have expected that an error be returned for this too, separate bug?)

Expected behavior

  1. When an exception is thrown in the GraphQL middleware pipeline, I would expect to see an error response noting this.
  2. The use of the @authorize directive should not throw an exception in the GraphQL pipeline

Desktop (please complete the following information):

  • OS: Windows 10
  • Version:10.0.17134.0

Additional context
N/A

@michaelmccord
Copy link
Contributor Author

Pull Request: #441 created to resolve this issue.

@michaelstaib michaelstaib added the 🎉 enhancement New feature or request label Dec 31, 2018
@michaelstaib michaelstaib added this to the 0.7.0 milestone Dec 31, 2018
@michaelstaib
Copy link
Member

Note that an exception is thrown and processing continues as normal (the fact that processing continues as normal seems to be an issue as well, I would have expected that an error be returned for this too, separate bug?)

I will write a test for this one.

@michaelstaib
Copy link
Member

This one is now fixed with #485 and will be included in 0.7.0-preview.31.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working 🎉 enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants