From 005da8bff7b91c7669919122536492bcf7a5282e Mon Sep 17 00:00:00 2001 From: "Bruno P. Kinoshita" Date: Tue, 6 Oct 2020 15:39:29 +1300 Subject: [PATCH 1/4] Fail the query with a helpful message if the variables defined do not match the expected values --- cylc/flow/network/graphql.py | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/cylc/flow/network/graphql.py b/cylc/flow/network/graphql.py index 0012d16dc8c..f0f7d721e28 100644 --- a/cylc/flow/network/graphql.py +++ b/cylc/flow/network/graphql.py @@ -150,14 +150,28 @@ def __init__(self, schema, document_ast, variable_values): for defn in document_ast.definitions: if isinstance(defn, ast.OperationDefinition): root_type = get_operation_root_type(schema, defn) + definition_variables = defn.variable_definitions or [] + variables = {} + if definition_variables: + variables = get_variable_values( + schema, + definition_variables, + variable_values + ) + # check if we are missing some of the definition variables + if not variables or \ + len(variables) != len(definition_variables): + variable_names = [ + v.variable.name.value for v in definition_variables + ] + msg = (f'Please check your query variables. The ' + f'following variables are expected: ' + f'[{", ".join(variable_names)}]') + raise ValueError(msg) self.operation_defs[getattr(defn.name, 'value', root_type)] = { 'definition': defn, 'parent_type': root_type, - 'variables': get_variable_values( - schema, - defn.variable_definitions or [], - variable_values - ), + 'variables': variables, } elif isinstance(defn, ast.FragmentDefinition): self.fragment_defs[defn.name.value] = defn From 7fd40cf3ee26d733b1c6c8264be4031fa958f318 Mon Sep 17 00:00:00 2001 From: "Bruno P. Kinoshita" Date: Wed, 14 Oct 2020 10:44:44 +1300 Subject: [PATCH 2/4] Add unit test for GraphQL query variables parsing and error handling --- tests/unit/network/test_graphql.py | 119 +++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+) create mode 100644 tests/unit/network/test_graphql.py diff --git a/tests/unit/network/test_graphql.py b/tests/unit/network/test_graphql.py new file mode 100644 index 00000000000..5091c3a4a27 --- /dev/null +++ b/tests/unit/network/test_graphql.py @@ -0,0 +1,119 @@ +# THIS FILE IS PART OF THE CYLC SUITE ENGINE. +# Copyright (C) NIWA & British Crown (Met Office) & Contributors. +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + +from typing import Optional + +import pytest +from graphql import parse + +from cylc.flow.network.graphql import AstDocArguments +from cylc.flow.network.schema import schema + + +@pytest.mark.parametrize( + 'query,' + 'variables,' + 'expected_variables,' + 'expected_error', + [ + # a simple query with the correct variables + ( + ''' + query ($workflowID: ID) { + workflows (ids: [$workflowID]) { + id + } + } + ''', + { + 'workflowID': 'cylc|workflow' + }, + { + 'workflowID': 'cylc|workflow' + }, + None + ), + # a query with a fragment and with the correct variables + ( + ''' + query ($workflowID: ID) { + ...WorkflowData + } + fragment WorkflowData on workflows { + workflows (ids: [$workflowID]) { + id + } + } + ''', + { + 'workflowID': 'cylc|workflow' + }, + { + 'workflowID': 'cylc|workflow' + }, + None + ), + # a query with the right variable definition, but missing + # variable in the provided values + ( + ''' + query ($workflowID: ID) { + workflows (ids: [$workflowID]) { + id + } + } + ''', + { + 'workflowId': 'cylc|workflow' + }, + None, + ValueError + ) + ] +) +def test_query_variables( + query: str, + variables: dict, + expected_variables: Optional[dict], + expected_error: Optional[Exception] +): + """Test that query variables are parsed correctly. + + Args: + query (str): a valid GraphQL query (using our schema) + variables (dict): map with variable values for the query + expected_variables (dict): expected parsed variables + expected_error (Exception): expected error, if any + """ + def test(): + """Inner function to avoid duplication in if/else""" + document = parse(query) + document_arguments = AstDocArguments( + schema=schema, + document_ast=document, + variable_values=variables + ) + parsed_variables = next( + iter( + document_arguments.operation_defs.values() + ) + )['variables'] + assert expected_variables == parsed_variables + if expected_error is not None: + with pytest.raises(expected_error): + test() + else: + test() From 8c4083912247a7220931db753ef1da25d5d3066c Mon Sep 17 00:00:00 2001 From: "Bruno P. Kinoshita" Date: Wed, 14 Oct 2020 10:45:57 +1300 Subject: [PATCH 3/4] Add changelog --- CHANGES.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index cb586902688..b762a7fa889 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -48,6 +48,10 @@ compatibility, the `cylc run` command will automatically symlink an existing ### Enhancements +[#3856](https://github.com/cylc/cylc-flow/pull/3856) - fail the GraphQL query +with a helpful message if the variables defined do not match the expected +values. + [#3853](https://github.com/cylc/cylc-flow/pull/3853) - Update protobuf and pyzmq. From e1a23f522ab7a3d3e62cc613ff38d9f6e9f923d5 Mon Sep 17 00:00:00 2001 From: "Bruno P. Kinoshita" Date: Mon, 2 Nov 2020 09:23:14 +1300 Subject: [PATCH 4/4] Apply David Sutherland's suggested patch in GH review --- cylc/flow/network/graphql.py | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/cylc/flow/network/graphql.py b/cylc/flow/network/graphql.py index f0f7d721e28..f656dc8f4f2 100644 --- a/cylc/flow/network/graphql.py +++ b/cylc/flow/network/graphql.py @@ -151,27 +151,30 @@ def __init__(self, schema, document_ast, variable_values): if isinstance(defn, ast.OperationDefinition): root_type = get_operation_root_type(schema, defn) definition_variables = defn.variable_definitions or [] - variables = {} if definition_variables: - variables = get_variable_values( - schema, - definition_variables, - variable_values - ) + def_var_names = { + v.variable.name.value + for v in definition_variables + } + var_names_diff = def_var_names.difference({ + k + for k in variable_values + if k in def_var_names + }) # check if we are missing some of the definition variables - if not variables or \ - len(variables) != len(definition_variables): - variable_names = [ - v.variable.name.value for v in definition_variables - ] + if var_names_diff: msg = (f'Please check your query variables. The ' - f'following variables are expected: ' - f'[{", ".join(variable_names)}]') + f'following variables are missing: ' + f'[{", ".join(var_names_diff)}]') raise ValueError(msg) self.operation_defs[getattr(defn.name, 'value', root_type)] = { 'definition': defn, 'parent_type': root_type, - 'variables': variables, + 'variables': get_variable_values( + schema, + definition_variables, + variable_values + ), } elif isinstance(defn, ast.FragmentDefinition): self.fragment_defs[defn.name.value] = defn