Updated AuthorizationValidationRule to skip authorization checks when…#30
Updated AuthorizationValidationRule to skip authorization checks when…#30chris-nissen wants to merge 7 commits intographql-dotnet:masterfrom
Conversation
|
If there's a more elegant way to get the argument values from the validation context, let me know. |
joemcbride
left a comment
joemcbride
left a comment
There was a problem hiding this comment.
Mind adding some tests for each directive? Probably one each of where it should exclude the field that has a policy and it passes. And another where it should include the field and it hits the validation?
|
|
||
| var operation = !string.IsNullOrWhiteSpace(context.OperationName) | ||
| ? context.Document.Operations.WithName(context.OperationName) | ||
| : context.Document.Operations.FirstOrDefault(); |
There was a problem hiding this comment.
The Operation and Variables will be the same for both calls, so could pull that up so it isn't calculated twice.
There was a problem hiding this comment.
Extracted the operations to a variable. I wasn't sure what you meant by the Variables here.
There was a problem hiding this comment.
ExecutionHelper.GetVariableValues is called in the GetDirectiveValue method. That only needs to be calculated once.
|
I'm not sure if this is enough for tests; I tried doing them so that the directive conditions came from inputs set up on the test config, but when the rule was pulling the argument values they weren't there, so I'm not sure if there's something more I'd need to do in the tests... |
|
I wonder if I should also be accounting for these directives in the ObjectField handler? |
|
@joemcbride, sorry, I don't know, should I be notifying you I made changes, somehow? I understand you might just not have had time to get back to this. |
|
Yes should probably be handling this on the |
|
As for the |
… the field is not included or skipped due to directives.
3 participants
… the field is not included or skipped due to directives.