Fail the query with a helpful message if the variables defined do not match the expected values

[kinow]

These changes close #3855

Requirements check-list

• I have read CONTRIBUTING.md and added my name as a Code Contributor.
• Contains logically grouped changes (else tidy your branch by rebase).
• Does not contain off-topic changes (use other PRs for other changes).
• Appropriate tests are included (unit and/or functional).
• Appropriate change log entry included.
• No documentation update required.
• No dependency changes.

[kinow]

Now if I accidentally misspell a variable in Cylc UI, the message should hint that there's something wrong with the variables.

And fixing the variables then the query runs just fine.

[kinow]

Virtual memory error when running the bash tests in the Docker container. Fix available in #3854

Will spend some more time on this later trying to add a test. Not sure if a unit test will work, might try our new test classes for the first time 👍

[kinow]

The other PR that contains the fix for the Docker builds may take a while to be merged, while we wait for GH Actions to include Python 3.9.

So I've cherry-picked the commit that should fix Docker, and pushed here 👍

[kinow]

Functional test broadcast/00-simple is failing, but I think it needs to be updated after this fix (just need to understand how it works, and confirm it indeed just needs to be updated)

[kinow]

Added a unit test, and the changelog. Confirmed locally with coverage + pytest that the unit test covered the change in the network.graphql module 👍

Existing functional tests cover that function already, though partially.

[kinow]

I fixed the functional test in this PR, and yesterday it passed in the Docker image. Strangely, it failed now, but my only updates were changelog and a new unit test. 🤔 ready for review, I think the test is just a bit brittle in Docker (will investigate why later)

[hjoliver]

LGTM, but I'll defer to @dwsutherland for the definitive review.

[dwsutherland]

Good idea @kinow , may I suggest the following instead:

        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 []
                if definition_variables:
                    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 var_names_diff:
                        msg = (f'Please check your query variables. The '
                               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': get_variable_values(
                        schema,
                        definition_variables,
                        variable_values
                    ),
                }
            elif isinstance(defn, ast.FragmentDefinition):
                self.fragment_defs[defn.name.value] = defn

Then you don't have to change the default behaviour of GraphQL (i.e. if None is desirable to fall to the schema defined default), and hence not have to change the CLI tests.

[kinow]

Good idea @kinow , may I suggest the following instead:

Then you don't have to change the default behaviour of GraphQL (i.e. if None is desirable to fall to the schema defined default), and hence not have to change the CLI tests.

Hi @dwsutherland ! Thanks for the suggestion! I will just need some time tomorrow to re-contextualize with the change, and understand your change. Then it should be a matter of just updating the PR with a new/edit commit, and check if tests pass 👍

Might be able to complete it before the executive roadshow starts tomorrow in Auckland, by the morning.

[kinow]

Fixed git conflicts, and applied @dwsutherland 's patch (thanks!). I think we now just need to wait for CI to verify.

[dwsutherland]

Good to go 👍