Validate directive arguments inside SDL

[IvanGoncharov]

No description provided.

[IvanGoncharov]

@mjmahone Can you please take a look?
There are no SDL-specific tests so it couldn't be merged yet.
But I can work on tests in parallel with your review.

[mjmahone]

@IvanGoncharov sorry this is taking me longer than usual to review. This PR is giving me pause, as I'm worried with the AST validation we're too often looking through every directive to find one.

It seems like we should be moving all of the getFragment(name) functionality into the ASTValidationContext from the ValidationContext, and add a getDirective(name) function to that as well. This would let us create a cache across validation rules to provide O(1) access (once cached) to each AST node we might care about.

I think the biggest issue I have is with validation rules taking in a union of validation contexts instead of a single base validation context. For example, I would like

KnownArgumentNamesOnDirectives(context: ASTValidationContext)

instead of

KnownArgumentNamesOnDirectives(context: ValidationContext | SDLValidationContext)

To allow this, we'd need ASTValidationContext to have getDirectiveDefinition(name) or some such function.

If it happens to be a ValidationContext, it could (though may not) always get its directive definitions from the schema available, whereas if it's an SDLValidationContext it would first check the schema and then add directives from the AST. But the validation rule implementation doesn't need to know these internal details. Exposing what seem like internal-to-the-context details could make it harder to maintain these rules.

[mjmahone]

Thinking a little more, this pattern of taking in a union shouldn't be dangerous to introduce as-is: it's strictly more forwards-compatible than taking in the base class. I will take a deeper review of this tomorrow, trying to pay more attention to whether we could fix any issues forward rather than whether the implementation is perfect today.

[mjmahone]

Taking a second look at this, I actually like it mostly as-is. The main worry I had was with the updated validation rule APIs. However, I think it's better, for now, to provide ValidationContext | SDLValidationContext and do extra work because of that, which we can clean up into ASTValidationContext later, if need be. Better to hide ASTValidationContext and be using it internally anywhere we see the union of ValidationContext | SDLValidationContext than expose it and have people depending on it, and not be able to make improvements to the underlying ASTValidationContext.

So given we add some tests, I'm happy to approve this as-is.

[IvanGoncharov]

Taking a second look at this, I actually like it mostly as-is.

@mjmahone After I tested it with new performance tests I found it introduced significant performance drop due to visitInParrallel usage.
Tomorrow I will try to replace it and also to add missing test-cases.

[IvanGoncharov]

@mjmahone I added tests and fixed performance:

Can you please take a look?

[mjmahone]

This is great! Thanks for working through this @IvanGoncharov, I'm glad you were able to get it into a performant state!