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 query validation #1357

Merged
merged 23 commits into from
Aug 21, 2021
Merged

Conversation

aryaniyaps
Copy link
Contributor

@aryaniyaps aryaniyaps commented Aug 13, 2021

adds a few validation rules which would be very useful in production environments. Documentation regarding validators have been added, and new test cases have also been made (which pass).

Similar validation rules are provided out of the box in other libraries like strawberry graphql and ariadne, so it would be nice for graphene users to get a similar experience.

validation rules which have been added:

  • depth limit validator
  • disable introspection

allows passing the validation rules to the validate method from graphql-core. Huge thanks to @jkimbo for his work on the depth limit validator, which was a port from https://github.com/stems/graphql-depth-limit

also fixes #907
also aims to fix CI, replacing travis CI with github actions

@aryaniyaps aryaniyaps marked this pull request as draft August 13, 2021 14:54
@aryaniyaps aryaniyaps marked this pull request as ready for review August 14, 2021 03:13
@aryaniyaps
Copy link
Contributor Author

@syrusakbary could you please review this if you have the time? It would be really helpful to use in production environments. Please let me know if any changes should be made, I'll do it! Thanks a lot!

@aryaniyaps aryaniyaps changed the title add depth limit validator add support for query validation Aug 14, 2021
@syrusakbary
Copy link
Member

I'm liking this PR! Could it be possible if we fix CI as part of the PR? (replacing Travis with Github CI should be good!)

@aryaniyaps
Copy link
Contributor Author

aryaniyaps commented Aug 19, 2021

@syrusakbary please check the new workflow I pushed in (also deleted the .travis.yml file). If any changes need to be done, please let me know! The PYPI_API_TOKEN secret needs to be set to automatically publish to PyPi, I think.

also, I think that parallel processing isn't required, because we need to execute jobs sequentially (first tests, then check coverage and finally publish). What do you think about this?

also, I created the workflow based on the .travis.yml file. I couldn't understand one thing and I am not sure about this.
Are we publishing a new version to PyPi for every PR? Or should that be done in a new workflow which fires whenever a new github version is released?

@syrusakbary
Copy link
Member

syrusakbary commented Aug 19, 2021

The PR looks great, although tests are not passing (it seems to be an issue on master).

If you could help solving those last issues then we should be good to merge!

@aryaniyaps
Copy link
Contributor Author

aryaniyaps commented Aug 20, 2021

Sure, I've managed to find out the problem. There have been a few changes in graphql core lately. The ExecutionContext class provided by graphql-core doesn't eat up exceptions now.

You can check here for reference.
https://github.com/graphql-python/graphql-core/blob/afd03a058d04d12484dd920956d4611c50bf2f86/src/graphql/execution/execute.py#L580-L656

Therefore, we could fix the failing tests by removing UnforgivingExecutionContext and related tests (we dont need it anymore)

@aryaniyaps
Copy link
Contributor Author

@AlecRosenbaum what do you think about this?

@AlecRosenbaum
Copy link
Contributor

The ExecutionContext class provided by graphql-core doesn't eat up exceptions now.

If this was true, then the test suite should pass without specifying a custom context, right?

@aryaniyaps
Copy link
Contributor Author

aryaniyaps commented Aug 20, 2021

The ExecutionContext class provided by graphql-core doesn't eat up exceptions now.

If this was true, then the test suite should pass without specifying a custom context, right?

Yep, could you please check my commits (changed files for this PR)?

Copy link
Contributor

@AlecRosenbaum AlecRosenbaum left a comment

Choose a reason for hiding this comment

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

@syrusakbary Does it really make sense to block this change on unrelated test failures?

graphene/types/tests/test_schema.py Show resolved Hide resolved
Copy link
Member

@syrusakbary syrusakbary left a comment

Choose a reason for hiding this comment

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

Looks great, merging!

@syrusakbary syrusakbary merged commit efc0353 into graphql-python:master Aug 21, 2021
@aryaniyaps
Copy link
Contributor Author

@syrusakbary I think you forgot to set PYPI_API_TOKEN or it is invalid.

@aryaniyaps
Copy link
Contributor Author

@syrusakbary should a new release be made to make this feature available?

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.

Protection against malicious queries
4 participants