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

Add support for validation rules #1475

Merged
merged 7 commits into from Dec 20, 2023
Merged

Add support for validation rules #1475

merged 7 commits into from Dec 20, 2023

Conversation

kiendang
Copy link
Collaborator

@kiendang kiendang commented Nov 1, 2023

Close #1472
Close #1342

@kiendang kiendang mentioned this pull request Nov 1, 2023
Copy link
Collaborator

@sjdemartini sjdemartini left a comment

Choose a reason for hiding this comment

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

This approach LGTM! Maybe worth a quick mention in some docs/ file about how to specify validation_rules? And a quick test seems worthwhile before merging.

Thanks for adding this! 🎉

@kiendang
Copy link
Collaborator Author

@firaskafri should we release another patch version (v3.1.6) before merging this? I think this deserves a minor bump to v3.2.0 but there were a couple of small bug fixing PRs merged since v3.1.5 so I think it's a good idea to have another patch version for v3.1.

Copy link
Collaborator

@sjdemartini sjdemartini left a comment

Choose a reason for hiding this comment

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

💯

Awesome stuff! Really appreciate the clear docs examples and tests!

Agreed with your suggestion that we should release another patch version (v3.1.6) before merging this and releasing this as a minor bump to 3.2.0

def test_validation_disallow_introspection(client, url):
response = client.post(url_string(url, query="{__schema {queryType {name}}}"))

assert response.status_code == 400
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about we also check that the json representation of the response does not contain the __schema in its data (or data is null/empty, however it appears in practice)? That way we ensure it's not merely returning a 400 and the error message, but is also omitting the introspection data from the response as intended. (Same goes for the other 400 error tests below)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point!

@kiendang kiendang added this to the v3.6.0 milestone Dec 3, 2023
Copy link

@pmcarlos pmcarlos 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 good to me, testing it locally with another approach I used to have

@kiendang
Copy link
Collaborator Author

This looks good to me, testing it locally with another approach I used to have

@pmcarlos Would you be able to share your current method? This PR is to be merged eventually but there's someone in Graphene Discord asking if there's another way to disable introspection.

@kiendang kiendang modified the milestone: v3.2.0 Dec 14, 2023
@lee-pai-long
Copy link

Hi guys, any idea when this PR will be merged ?

@kiendang
Copy link
Collaborator Author

kiendang commented Dec 20, 2023

Hi guys, any idea when this PR will be merged ?

This would be part of the 3.2.0 release. The plan is to release another patch version 3.1.6 with the current main branch, then we'll merge this PR and do a 3.2.0 later. @firaskafri is there any roadblock on making a 3.1.6 release?

@firaskafri
Copy link
Collaborator

Hi guys, any idea when this PR will be merged ?

This would be part of the 3.2.0 release. The plan is to release another patch version 3.1.6 with the current main branch, then we'll merge this PR and do a 3.2.0 later. @firaskafri is there any roadblock on making a 3.1.6 release?

Hello @kiendang not at all.
I just released 3.1.6 and merging this to main now.

@firaskafri firaskafri merged commit feb7252 into main Dec 20, 2023
12 checks passed
@firaskafri firaskafri deleted the validation-rules branch December 20, 2023 09:48
@kiendang
Copy link
Collaborator Author

Hi guys, any idea when this PR will be merged ?

This would be part of the 3.2.0 release. The plan is to release another patch version 3.1.6 with the current main branch, then we'll merge this PR and do a 3.2.0 later. @firaskafri is there any roadblock on making a 3.1.6 release?

Hello @kiendang not at all. I just released 3.1.6 and merging this to main now.

@firaskafri Awesome. Thanks! We're good to go for 3.2.0 as well.

@lee-pai-long
Copy link

lee-pai-long commented Dec 22, 2023

Hi @kiendang any example on how to implement both rules ? I tried to follow the graphene documentation:

from graphene import Schema
from .auto import schema_operations_builder
from graphql import validate, parse
from graphene.validation import depth_limit_validator, DisableIntrospection

schema = Schema(query=ALL_QUERIES, mutation=ALL_MUTATIONS)

validation_errors = validate(
    schema=schema.graphql_schema,
    document_ast=parse('THE QUERY'),
    rules=(
        depth_limit_validator(max_depth=10),
        DisableIntrospection
    )
)

But I get an error ModuleNotFoundError: No module named 'graphene.validation'

@kiendang
Copy link
Collaborator Author

kiendang commented Dec 23, 2023

@lee-pai-long hmm that's weird. It comes with Graphene. We have a test for it in this repo and it works fine. You can try taking a look at your installed packages? Alternatively the introspection disabling rule is also implemented in graphql-core. You can do

from graphql.validation.rules.custom.no_schema_introspection import NoSchemaIntrospectionCustomRule

NoSchemaIntrospectionCustomRule is the same as graphene.validation.DisableIntrospection. However depth_limit_validator is only available in Graphene though.

@lee-pai-long
Copy link

Hi @kiendang thanks for you help, actually my error was that I didn't have the right version of graphene, after upgrading it and the other dependencies I can import the validation module.

superlevure pushed a commit to loft-orbital/graphene-django that referenced this pull request Jan 30, 2024
* Add support for validation rules

* Enable customizing validate max_errors through settings

* Add tests for validation rules

* Add examples for validation rules

* Allow setting validation_rules in class def

* Add tests for validation_rules inherited from parent class

* Make tests for validation rules stricter
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.

Query Limiting Depth How to disable introspection via validation rules?
5 participants