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

Allow skipping ValidationWrapper for introspection queries #1837

Merged
merged 7 commits into from
Aug 17, 2023

Conversation

kyri-petrou
Copy link
Collaborator

@kyri-petrou kyri-petrou commented Aug 15, 2023

Introspection queries are relatively large + deep, so in order for introspection to be allowed we need for these wrappers to allow 250+ fields and 12 levels depth. With this PR, we can disable these checks for introspection queries.

One other way that this could be implemented is to be able to set a separate limit for introspections, e.g.,:

def maxFields(maxFields: Int, maxIntrospectionFields: Option[Int] = None): ValidationWrapper[Any] = ???

Thoughts?

EDIT: This PR now adds the skipForIntrospection method which returns a new ValidationWrapper that is skipped for introspection queries.

@frekw
Copy link
Collaborator

frekw commented Aug 15, 2023

In my opinion we should always skip it for introspection. Not much point of having it in that case, right?

@kyri-petrou
Copy link
Collaborator Author

In my opinion we should always skip it for introspection. Not much point of having it in that case, right?

Ideally I think we should do this, I agree. My main concern though is that our isIntrospection check is not particularly strict, so we probably want to be able to give the option to limit introspection queries as well.

This is the main reason I'm also partially inclined for the method to accept a maxIntrospectionFields: Option[Int] argument instead.

@ghostdogpr
Copy link
Owner

ghostdogpr commented Aug 15, 2023

How about something more generic, like def skipForIntrospection(validationWrapper: ValidationWrapper): ValidationWrapper, that would check if it's introspection and run or not the inner wrapper. It feels more re-usable and prevent adding a boolean everywhere.
Maybe we can add some syntax to make it nicer.

@kyri-petrou
Copy link
Collaborator Author

@ghostdogpr what do you think about making it a final method on ValidationWrapper? Or do you prefer having it as a method / syntax in Wrappers which we can import instead?

@ghostdogpr
Copy link
Owner

Method sounds good!

@kyri-petrou kyri-petrou changed the title Allow skipping maxDepth and maxFields validations for introspection Allow skipping ValidationWrapper for introspection queries Aug 16, 2023
@kyri-petrou kyri-petrou requested a review from ghostdogpr August 16, 2023 12:22
@kyri-petrou
Copy link
Collaborator Author

@ghostdogpr I'll add a unit test just in case before merging in

Comment on lines +64 to +73
final def skipForIntrospection: ValidationWrapper[R] = new ValidationWrapper[R] {
override val priority: Int = self.priority

override def wrap[R1 <: R](
f: Document => ZIO[R1, ValidationError, ExecutionRequest]
): Document => ZIO[R1, ValidationError, ExecutionRequest] =
(doc: Document) =>
if (doc.isIntrospection) f(doc)
else self.wrap(f)(doc)
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ghostdogpr turns out we can't simply override apply and |+|. Do we think there might be cases where someone implemented custom logic in those methods?

For apply, I could get this to work, but looks rather weird so I left it out - let me know if you think we should add it:

override def apply[R1 <: R](that: GraphQL[R1]): GraphQL[R1] = super.apply(self.apply(that))

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably okay

@kyri-petrou kyri-petrou merged commit 4eeb87a into series/2.x Aug 17, 2023
@kyri-petrou kyri-petrou deleted the skip-validations-for-introspection branch August 17, 2023 02:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants