graphql-core v3 upgrade

[dwsutherland]

closes #333
closes #388
closes #698

Is a sibling to:
cylc/cylc-flow#6478
cylc/cylc-ui#2091
(cylc-flow/cylc-uiserver PR needed to be merged at the same time)

Tests will fail until cylc-flow end is in.

Dependencies on:

graphene-tornado==2.6.*
graphql-ws==0.4.4

have been removed and/or rewritten as a Cylc port of active projects.

There are some short comings in graphene==3.4.3 with the use of middleware and our bespoke execution context (for null-stripping fields) in subscriptions. So I had to adopt and augment a graphql-core package:
https://github.com/graphql-python/graphql-core/blob/v3.2.6/src/graphql/execution/subscribe.py
to our own:
cylc/uiserver/graphql/subscribe.py

The HEAD of graphql-core has this remedied, but both that and the a corresponding graphene will need to be released.
Until then cylc-flow has graphene fixed at 3.4.3, as we'll need to update our code with any future release of graphene/graphql-core (hopefully dropping this workaround).

All aspects appear to be functional:

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).
• Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
• Tests are included (or explain why tests are not needed).
• Changelog entry included if this is a change that can affect users

[oliver-sanders]

🚀

[oliver-sanders]

gdammit.

Looks like this works under graphql-core v2 but not v3?!

The message sounds like it is complaining about the use of a list as a default value, however, we do the exact same thing with broadcasts just a couple of lines above?!

    broadcasts = GenericScalar(
        ids=graphene.List(
            ID,
            description=sstrip('''
                Node IDs, cycle point and/or-just family/task namespace.

                E.g: `["1234/foo", "1234/FAM", "*/FAM"]`
            '''),
            default_value=[]  # OK?
        ),
        resolver=resolve_broadcasts,
        description='Any active workflow broadcasts.'
    )
    pruned = Boolean()  # TODO: what is this? write description
    n_edge_distance = Int(
        description=sstrip('''
            The maximum graph distance (n) from an active node
            of the data-store graph window.
        '''),
    )
    log_records = graphene.List(
        LogRecord,
        description=sstrip('''
            Scheduler log messages of level WARNING and above.

            Warning: This is an incrementally updated field. Each update
            contains a list of **new** log messages, not a list of all
            messages. It is down to the client to store / preserve previous
            records.
        '''),
        default_value=[],  # Not OK?!
    )

[oliver-sanders]

This seems to do the trick: cylc/cylc-flow#6803

[oliver-sanders]

Integration failure fixed, poking tests...

[dwsutherland]

The message sounds like it is complaining about the use of a list as a default value, however, we do the exact same thing with broadcasts just a couple of lines above?!

I guess default values on arguments are treated different to those on the fields?
Funny thing is, the error is complaining about an immutable type:

ValueError: mutable default <class 'list'> for field log_records is not allowed: use default_factory

and default_factory isn't a graphene option

../../.envs/uis-gql/lib/python3.10/site-packages/graphene/types/argument.py:112: in to_arguments
    raise ValueError(f'Unknown argument "{default_name}".')
E   ValueError: Unknown argument "default_factory".

but if you make it a immutable type:

        default_value=(),

It doesn't complain 😆 (also default_value=None works) .. Maybe a bug? idk
(I had to remove similar default_value=[] elsewhere BTW)

However, I think the field default is implicit? (if there is nothing but you ask for the field it will return [] not null)
So removing it is probably the right thing to do..

[dwsutherland]

I've been trying to get the subscription test working, so far I have got to using:

@pytest.fixture
def gql_sub(jp_ws_fetch):
    """Perform a GraphQL request.
    E.G:
    conn = await gql_sub(
        *('cylc', 'subscriptions'),
        query='''
            subscription {
                workflows {
                    id
                    status
                }
            }
        ''',
    )
    while True:
        msg = await conn.read_message()
        # . . .
    """
    async def _fetch(*endpoint, query=None, headers=None):
        headers = headers or {}
        #headers = {
        #    **headers,
        #    'Content-Type': 'application/json'
        #}
        return await jp_ws_fetch(
            *endpoint,
            headers=headers,
            params={'query': query},
            subprotocols=['graphql-ws'],
        )
    return _fetch

async def test_subscription(gql_sub, dummy_workflow):
    """Test sending the most basic GraphQL subscription."""
    # configure two dummy workflows so we have something to look at
    await dummy_workflow('foo')
    await dummy_workflow('bar')
    # perform the request
    conn = await gql_sub(
        *('cylc', 'subscriptions'),
        query='''
            subscription {
                workflows {
                    id
                    status
                }
            }
        ''',
    )
    print(conn)
    while True:
        msg = await conn.read_message()
        print(msg)
        break
    assert False

However, I must be doing something wrong.. as it hangs on the conn = await gql_sub ... Ref to:
https://www.tornadoweb.org/en/stable/_modules/tornado/websocket.html#websocket_connect
and
https://github.com/jupyter-server/pytest-jupyter/blob/main/pytest_jupyter/jupyter_server.py

maybe I have to do something like:

with await gql_sub(...) as conn:
    ...

Will have another crack at it next week...

[oliver-sanders]

If this is a new test, maybe remove it from this PR and prepare a new PR to add the tests?

[MetRonnie]

Not a new problem in this PR, but I have figured out why we don't get any visibility of subscription errors anywhere. It's because this function doesn't do anything if the operation ID is not registered in the connection context, which it isn't for a graphql subscription error.

But why doesn't it do anything? As far as I can tell, neither self.build_message nor connection_context.send make use of the iterator registered with the operation ID, so why not just do?

Suggested change

	if op_id is None or connection_context.has_operation(op_id):
	message = self.build_message(op_id, op_type, payload)
	return await connection_context.send(message)
	message = self.build_message(op_id, op_type, payload)
	return await connection_context.send(message)

To test out what I'm talking about, try running the GUI with a typo in the Workflows.vue subscription, or by doing a GraphiQL subscription like

subscription App {
  deltas {
    id
    borked
  }
}

[dwsutherland]

Ok, yes, I get your meaning.. I think this may be inherited code, otherwise I would have thought op_id would need to be associated with a subscription channel.

[dwsutherland]

If this is a new test, maybe remove it from this PR and prepare a new PR to add the tests?

Yes, will do.

[dwsutherland]

Added a back compat patch for:
#698

(justification: #698 (comment))

[MetRonnie]

🎉

[oliver-sanders]

🎉

[hjoliver]

🚀