Improve string validation error #414
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 3 commits
July 19, 2019 14:02
…sho-technologies/graphql-compiler into improve-string-validation-user-error
pmantica1
changed the title
pmantica1
commented
Jul 22, 2019
graphql_compiler/compiler/helpers.py
Outdated
|
|
||
|
|
||
| def validate_output_name(value): | ||
| def _validate_safe_argument_or_output_name(name, name_description): | ||
| """Ensure that the provided argument or output name does not have illegal characters.""" |
There was a problem hiding this comment.
I added a name description in order for the user to be able to quickly tell whether it was a tagged parameter, runtime argument or output name that failed validation. I think that it's important that this information is communicated. However, I am unsure of how to best reflect this change in the code. Here is one way to do it, but I am open to suggestions.
pmantica1
changed the title
obi1kenobi
requested changes
Jul 22, 2019
graphql_compiler/compiler/helpers.py
Outdated
| if not set(value).issubset(VARIABLE_ALLOWED_CHARS): | ||
| raise GraphQLCompilationError(u'Encountered illegal characters in {}: {}. It is only ' | ||
| u'allowed to have upper and lower case letters, ' | ||
| u'digits and underscores. ' |
There was a problem hiding this comment.
Trailing space in the message. Also, let's refactor this to say which specific disallowed characters were present:
disallowed_chars = frozenset(value) - VARIABLE_ALLOWED_CHARS
if disallowed_chars:
...
graphql_compiler/compiler/helpers.py
Outdated
| raise GraphQLCompilationError(u'Encountered illegal characters in string: {}'.format(value)) | ||
| def validate_tagged_argument_name(name): | ||
| """Ensure that provided string is valid for use as a tagged argument name""" | ||
| validate_safe_string(name, 'tagged argument name') |
There was a problem hiding this comment.
do not pass kwargs positionally
| validate_safe_string(base_location_name) | ||
| validate_safe_string(edge_direction) | ||
| validate_safe_string(edge_name) | ||
| validate_safe_or_special_string(mark_name) |
There was a problem hiding this comment.
Many of these shouldn't be safe_or_special, they should just be safe. The mark name can never be @rid.
I realize that this is something the old code allowed, but now that we are making this in-depth change, leaving that in would just add to the confusion since it seems like an intentional decision to allow these special names.
added 6 commits
July 22, 2019 15:22
…sho-technologies/graphql-compiler into improve-string-validation-user-error
obi1kenobi
previously approved these changes
Jul 22, 2019
…sho-technologies/graphql-compiler into improve-string-validation-user-error
obi1kenobi
approved these changes
Jul 22, 2019
vmaksimovski
pushed a commit
that referenced
this pull request
Jul 25, 2019
* Partial implementation of the Cypher lowering and query emission code. * Refactor the common IR lowering steps into their own subpackage. * Make location rewriting always use the query metadata table. * Implement conversion of IR blocks to CypherQuery object. * Finish up IR lowering. Implement entry points into Cypher compilation. * Add Cypher expression formatting, and some fixes for Gremlin lowering. * Various small bugfixes discovered through addition of Cypher test code. * Merge master and fix minor bugs. * Implement proper optional + filter semantics. * Enable a few Cypher tests. * Fix recursive Cypher, and add more test cases. * Remove merge artifact. * Record the GraphQL type of LocalField expressions. Required to make progress on Cypher backend, since a lowering step there converts LocalField expressions into ContextField expressions. Prior to this commit, ContextField expressions include the GraphQL type of the field, whereas LocalField expressions do not, making that lowering step impossible to implement. * Properly lower LocalField into ContextField expressions, preserving type info. * Add support for filtering within optional traversals. * Delint. * Add tests For not_in_collection and not_contains filters as well as contains and in_collection tests that were missing. * Delint * Change Cypher version * Add filter operators documentation README documentation added for filter operators not_in_collection and not_contains. * Fix formatting * Add "vertex" after "Animal" * Clarify what filter returns * Emitting Cypher and some compilation tests (#374) * Partial implementation of the Cypher lowering and query emission code. * Refactor the common IR lowering steps into their own subpackage. * Make location rewriting always use the query metadata table. * Implement conversion of IR blocks to CypherQuery object. * Finish up IR lowering. Implement entry points into Cypher compilation. * Add Cypher expression formatting, and some fixes for Gremlin lowering. * Various small bugfixes discovered through addition of Cypher test code. * Merge master and fix minor bugs. * Implement proper optional + filter semantics. * Enable a few Cypher tests. * Fix recursive Cypher, and add more test cases. * Remove merge artifact. * Record the GraphQL type of LocalField expressions. Required to make progress on Cypher backend, since a lowering step there converts LocalField expressions into ContextField expressions. Prior to this commit, ContextField expressions include the GraphQL type of the field, whereas LocalField expressions do not, making that lowering step impossible to implement. * Properly lower LocalField into ContextField expressions, preserving type info. * Add support for filtering within optional traversals. * Delint. * Fix SchemaGraph connection documentation * Rename variables Small change suggested at #369 (review) but better to split that off the integration test PR. * Add schema merging without tests * Nit * Nit * Fix GraphQLScalarType object generation from schema text (#388) * Add test for date parameter bug * Add serialization and parsing for custom scalar types inferred from a schema string * Add missing birthday fields in OrientDB integration data * Fix linter issues * Undo README.md changes * Remove GraphQLString as a custom scalar type * Remove disabling of pylint messages * Various name changes * Add name validity check * Renamed schema (#390) * Add schema renaming without tests * Small name change * Fix lint errors * Change to using function, not constructor RenamedSchema is now a lightweight namedtuple that can be made by the function rename_schema RenameTypesVisitor has shrunk in size, without repeated method definitions frozensets are used in various places * Add copyright lines * Address comments * Address comments * Address comments * Fix lint errors * Pack validity checking into helper; some name changes * Use ast instead of string * Address comments, various name changes * Change output dict to only contain modified name pairs * Fix lint issues * Nit * Add name validity check * Fix lint errors and import issues * Nit * Missed micro nit * Micro nit grammar * Add underscore to pattern * Remove validity/conflict checking from field renaming; add error docs * Fix lint error * Nit * Small comments update * Fix lint error * Add tests for rename_schema * Nit * More tests; relative imports * Address some comments * Rename tests; add init file to schema_transformation * Implement Cypher support for Neo4j and Redisgraph (#381) * Compiler changes for integration tests * Nits * Undo Cypher version change Makes more sense to make this change in a separate branch, so I'm undoing it here. * nits * Implement _safe_cypher_* methods * delint * Fix python version string encoding comment * Manually interpolate parameters for Redisgraph only Neo4j's client can automatically do its own interpolation so it's better than doing it ourselves. Unfortunately Redisgraph doesn't support parameters at all, so we have to interpolate them ourselves. * Revert "delint" This reverts commit eddb5e2. This doesn't have anything to do with compiler changes to support Cypher for Neo4j and Redisgraph-- it was just the linter getting overexcited. * lint * Address comments * Remove Cypher version from queries Neo4j does accept Cypher version in queries but it doesn't match the OpenCypher docs, which specifies Cypher 9. Instead, it only accepts 2.3, 3.4, and 3.5 according to this Github issue: neo4j/neo4j#12239 RedisGraph doesn't support the version number syntax at all, according to this Github issue: RedisGraph/RedisGraph#552 * Fix _safe_cypher_string comment * Nits * Nit * Nit * Split renaming and validation * Revert to deepcopy * Split tests into validation and renaming * All validity checking moved into one func; some name and docstring changes * Remove unnecessary name validity checks; small comment changes * Add reminder to install dependencies after adding package Previously the instructions just said to run the script and I didn't realize this didn't work until I'd run `pipenv sync --dev`. * Fix typo * Change name of MergedSchema attribute * Remove deepcopy * Nit (#402) Breaking up PR #369 (integration tests) into more logical chunks. This is a url formatting nit. * Add Docker containers to docker-compose.yml (#403) Breaking up PR #369 (integration tests) into more logical chunks. * Create integration data for Cypher backends (#405) The data here is meant to match the data in the file create_orientdb_integration.sql Breaking up PR #369 (integration tests) into more logical chunks. * Update Pipfile for Cypher backends (#404) * Update Pipfile for Cypher backends Add relevant libraries for integration tests later. Breaking up PR #369 (integration tests) into more logical chunks. * Move Cypher backend libraries to dev-packages only These are only used for integration tests, which will come later. * Fix Pipfile versioning * Create integration test clients Breaking up PR #369 (integration tests) into more logical chunks. * Add comment explaning RedisGraph vs. Neo4j temporal type support (#407) * Cypher support graph generation (#408) * Implement integration test graph setup Takes in the .cypher files and creates the graph needed for the integration tests. Breaking up PR #369 (integration tests) into more logical chunks. * Add missing docstrings * Create integration test fixtures (#410) * Create integration test fixtures Currently waiting on clients and data_tool to get merged into master. Breaking up PR #369 (integration tests) into more logical chunks. * Remove unnecessary keyword from retry decorator * Add missing docstrings * Change database name into global variable * Nit * Delint * Refactor merge_schemas (#406) * Address comments on merge_schemas * Address comments * Remove util change meant for query renaming * Update comments and docstrings * Address comments * Further detail error message * Import COMPILED_NAME_PATTERN and remove _graphql_type_name_pattern (#418) * Import COMPILED_NAME_PATTERN and remove _graphql_type_name_pattern * Remove unused import * Create the integration tests themselves for Cypher backends (#411) * Create the integration tests themselves for Cypher backends Breaking up PR #369 (integration tests) into more logical chunks. * Add comment explaining excluded test * Combine backend decorators into one function * Indentation nit * Fix silly mistake for backend choice * Move docstring explanation into its own comment * Amend custom GraphQLScalarType objects in parsed schemas (#398) * Amend custom GraphQLScalarType in parsed schemas * Nit * Fix linter issues * Undo unintented changes * Address comments * Nit * Nit * Improve invalid argument error (#416) * Improve invalid argument error * Nit * Nit * Nit * Fix typo * Add newline (#423) * Improve schema graph documentation (#413) * Improve SchemaGraph docummentation * Nit * Nits * Nit * Nit * Remove added line * Nit * Fix typo * Address comments * Nit * Address comments * Improve string validation error (#414) * Improve validation error message. * Nit * Nit * Improve argument or output name validation error * Improve safe string validation error * Refacto validate safe string into validate_safe_or_special_string * Nit * Mention which are the illegal characters in string validation method * Replace validate_safe_or_special_string with validate_safe_string in many places * Stop special casing space error message. * Replace validate_safe_or_special_string with validate_safe_string in many places * Fix failing tests by loosening validation * Made the validation for cypher expression names more tight * Release v1.11.0. (#396) * Add version v1.11.0 to changelog * Nit * Fix changelog * Add missing parts to the changelog * Remove mention of indexes and mention GraphQL schema generation * Update changelog with pr links * Add missing filter operations * Add amend_custom_scalar_types to changelog * Release v1.11.0. * Update CHANGELOG.md * Add cost estimation and validation error message notes to the changelog. * Add tests for merge_schemas (#400) * Add tests for merge_schemas * Add test for checking original ASTs unmodified * Fix declaration of ordered dicts * Explain output name id map in doc string * Undo changes from other branch (add back deepcopy) * Remove deepcopy again * Various name changes, tests for identifier * Temporarily disable neo4j tests (#426) * Cypher support emit output tests (#379) * Test Cypher emit output * delint * Revert "delint" This reverts commit 35d211c. Undo a lot of changes automatically done by a linter. * Delint properly * Remove "CYPHER 3.5" from tests * Undo unnecessary lint See here: https://github.com/kensho-technologies/graphql-compiler/pull/379/files#r304957815 * Use test schema in Cypher emit output tests * Nit * Add missing comment * Remove extra newlines * Update MATCH emit output tests (#421) * Add GlobalOperationsStart block to each ir_blocks list Addresses comment from PR #379 #379 (comment) * Fix imports * Adapt MATCH emit output tests to use test schema * Fix incorrect emit output test test_simple_traverse_filter_output() is supposed to comapre a child vertex's name with the parent's name, and just comparing to a variable named `desired_name` defeats the point of this test. EmitGremlinTests seems to do it right. * Nit * remove newline * Update gremlin emit output tests (#422) * Find and replace to match test schema Foo -> Animal Bar -> BornAt etc * Add global blocks * Add equivalent GraphQL strings for reference * Add test_datetime_variable_representation() test for Gremlin This test existed for MATCH but not for Gremlin * Replace received_match with received_gremlin * Lint * Delint * remove newline * Delint * Improve error message for contains and not_contains operations (#425) * Improve error message for contains and not_contains operations * Add missing dot at the end * Fix typo * Add back neo4j client (#428)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR aims to invalid string validation errors, specifically those relating to output and argument names, since those are the most likely ones to be encountered by the user.