Skip to content

Allow configuration of the ofType introspection depth #4317

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 4 commits into
base: 16.x.x
Choose a base branch
from

Conversation

Nols1000
Copy link

@Nols1000 Nols1000 commented Dec 31, 2024

This allows for a better configuration in case the server restricts the maximum query depth.


Added by @benjie:

…ery depth

This allows for a better configuration in case the server restricts the maximum query depth.
@Nols1000 Nols1000 requested a review from a team as a code owner December 31, 2024 14:19
Copy link

linux-foundation-easycla bot commented Dec 31, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@@ -52,6 +61,7 @@ export function getIntrospectionQuery(options?: IntrospectionOptions): string {
schemaDescription: false,
inputValueDeprecation: false,
oneOf: false,
typeDepth: 9,
Copy link
Member

Choose a reason for hiding this comment

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

so this default is a breaking change. If we want this to land in v16 then we should do Infinity and in v17 we can restrict the limits.

Copy link
Author

@Nols1000 Nols1000 Jan 2, 2025

Choose a reason for hiding this comment

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

Why is that a breaking change? The behaviour is exactly the same as before. Infinity would lead to an infinite recursive loop.

Copy link
Member

Choose a reason for hiding this comment

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

@saihaj I think you may have misunderstood which depth this relates to - it's the ofType { ofType { ofType { ... } } } depth of the generated query. We couldn't set that to infinity.

benjie
benjie previously requested changes Jan 3, 2025
Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

Let's keep this nicely formatted :)

@benjie benjie changed the title Update getIntrospectionQuery.ts to allow configuration of the type query depth Update getIntrospectionQuery.ts to allow configuration of the type introspection depth Jan 3, 2025
@benjie benjie changed the title Update getIntrospectionQuery.ts to allow configuration of the type introspection depth Allow configuration of the ofType introspection depth Jan 3, 2025
Co-authored-by: Benjie <benjie@jemjie.com>
@Nols1000
Copy link
Author

Nols1000 commented Jan 3, 2025

@benjie Thanks for the suggestions. I've commited them my branch. I think for the scope of this PR that is the right move.

I'd also like to take this as an opportunity to discuss formatting the introspection query. I think it would be simpler to just keep the query in one line, without indentions. This could clean up the code and save a few byte over the wire. It would of course reduce the readability. Maybe we can document the query better, so that would become less of an issue.

@benjie
Copy link
Member

benjie commented Jan 5, 2025

You can always minify it by parsing it and using a GraphQL minifier. I don't think our source code should concern itself with that too much - we should make it easy to read and edit. What sort of comments do you have in mind?

@ThePlenkov
Copy link

@Nols1000 can you run please npm run lint -- --fix in your branch to get rid of a lint error?

@ThePlenkov
Copy link

@Nols1000 also please cover this line with a test case:
image

You can see it here https://app.codecov.io/gh/graphql/graphql-js/pull/4317

Thanks!

I decided not to submit identical MR, so now also interested in introducing this change

Copy link
Member

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

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

This looks like a good feature to have, great work, left some comments but pending those changes this should be good to go

Co-authored-by: Jovi De Croock <decroockjovi@gmail.com>
@Nols1000
Copy link
Author

@benjie At least for me it's not 100% clear how the query works. I would have expected more documentation e.g. here https://graphql.org/learn/introspection/. For me a breakdown of this introspection query would have been a great learning resource.

@JoviDeCroock JoviDeCroock added the PR: feature 🚀 requires increase of "minor" version number label Jan 16, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
PR: feature 🚀 requires increase of "minor" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reduce introspection complexity programmatically
5 participants