-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add NoSchemaIntrospectionCustomRule #2600
Add NoSchemaIntrospectionCustomRule #2600
Conversation
a626231
to
78ce9cf
Compare
@danielrearden Thanks, looks very tidy and match the style of other rules 👍 I give it more though and technically
Also, I think in this case being able to customize error messages would be very helpful. But I'm not sure if I'm overcomplicating it or not so I need more time to think. |
@danielrearden Actually scratch it this rule is very short so config for the more generic rule would be more longer that rule itself. |
@IvanGoncharov That makes sense. Would |
@danielrearden Generally, yes. But in this case, we forbid something that is totally valid and you ban it because of your own preference/requirements not even because it represents best practice. |
78ce9cf
to
7784367
Compare
The use of introspection types with user-defined fields is unconventional but I don't believe it's prohibited by the spec. Since the rule just tests the field's type, it's possible it would also throw in the event a non-standard introspection field like that was requested. I think that behavior lines up with the intent behind the rule. |
src/validation/rules/optional/ProhibitIntrospectionQueriesRule.js
Outdated
Show resolved
Hide resolved
src/validation/rules/optional/ProhibitIntrospectionQueriesRule.js
Outdated
Show resolved
Hide resolved
src/validation/rules/optional/ProhibitIntrospectionQueriesRule.js
Outdated
Show resolved
Hide resolved
e95cc70
to
f291cf3
Compare
@IvanGoncharov Ok, that should be all of it 😓 |
src/validation/__tests__/ProhibitIntrospectionQueriesRule-test.js
Outdated
Show resolved
Hide resolved
src/validation/__tests__/ProhibitIntrospectionQueriesRule-test.js
Outdated
Show resolved
Hide resolved
src/validation/__tests__/ProhibitIntrospectionQueriesRule-test.js
Outdated
Show resolved
Hide resolved
src/validation/__tests__/ProhibitIntrospectionQueriesRule-test.js
Outdated
Show resolved
Hide resolved
src/validation/rules/optional/ProhibitIntrospectionQueriesRule.js
Outdated
Show resolved
Hide resolved
src/validation/rules/optional/ProhibitIntrospectionQueriesRule.js
Outdated
Show resolved
Hide resolved
src/validation/__tests__/ProhibitIntrospectionQueriesRule-test.js
Outdated
Show resolved
Hide resolved
src/validation/__tests__/ProhibitIntrospectionQueriesRule-test.js
Outdated
Show resolved
Hide resolved
f291cf3
to
9bdde6f
Compare
@danielrearden Forget about this one. P.S. It's totally okay to ping in comments after you addressed all review comments. |
9bdde6f
to
b1fbab0
Compare
}, | ||
}, | ||
}), | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danielrearden I merged #2608 so now buildSchema
should work fine can you please use it instead.
b1fbab0
to
13086bb
Compare
13086bb
to
d48a015
Compare
This rule is adapted from graphql/graphql-js#2600 and should be replaced once using graphql >=15.2.0. Co-authored by: dzucconi <mail@damonzucconi.com>
This rule is adapted from graphql/graphql-js#2600 and should be replaced once using graphql >=15.2.0. Co-authored by: dzucconi <mail@damonzucconi.com>
#2597