Support Schema Extensions in subscriptions #2784

kristjanvalur · 2023-05-27T13:31:25Z

Subscriptions are now executed like queries and mutations, and extension hooks are called.
Result extension hooks are called for each result yielded by the subscription

NOTE: #2825 is an updated version of this, with a less intrusive change to the schema API.

Description

the Schema.subscribe() is turned into an AsyncGenerator yielding ExecutionResult objects.
In case the validation of the requests results in a single result instead of a subscription, e.g. when the inner
graphql-core.subscribe() does not return an iterator, a special exception is thrown, SubscribeSingleResult which' value
attribute contains the single ExecutionResult.

This pattern, always being a AsyncGenerator which aborts under certain circumstances, makes for a cleaner interface
and allows for much simpler use of context managers. Following the pattern of graphql-core which either returns
an iterator or an object, makes it unnecessarily tricky to implement with the Extensions context managers.

The payload part of the returned messages in both protocols is augmented with an extensions member if added by the corresponding hooks.

Tests are added to tests for result extensions.

A side effect of this change is that initial validation now happens on a worker task. This feature was a part of a separate PR in progress, so corresponding tests are also added to verify that functionality.

Types of Changes

Issues Fixed or Closed by This PR

Extenstions support for subscriptions. #2097

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

codecov · 2023-05-27T13:32:35Z

Codecov Report

Attention: Patch coverage is 96.23188% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 94.45%. Comparing base (17f05a7) to head (ef6688d).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2784      +/-   ##
==========================================
- Coverage   96.49%   94.45%   -2.05%     
==========================================
  Files         510      505       -5     
  Lines       32808    31866     -942     
  Branches     5443     3659    -1784     
==========================================
- Hits        31658    30098    -1560     
- Misses        917     1477     +560     
- Partials      233      291      +58

botberry · 2023-05-27T13:33:55Z

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:

Subscriptions now support Schema Extensions.

Here's the tweet text:

🆕 Release (next) is out! Thanks to @kristjanvalur for the PR 👏

Get it here 👉 https://strawberry.rocks/release/(next)

kristjanvalur · 2023-05-31T10:15:35Z

Current unit test flakiness is addressed by #2785

kristjanvalur · 2023-05-31T22:20:14Z

Now, this is an internally breaking change in that the results from schema.subscribe() have changed. but is this a documented interface?

We could change the interface but it would impact the possibilities of what kind of context it is possible to maintain during the evaluation of a subscription.

patrick91 · 2023-06-01T13:25:24Z

Now, this is an internally breaking change in that the results from schema.subscribe() have changed. but is this a documented interface?

We could change the interface but it would impact the possibilities of what kind of context it is possible to maintain during the evaluation of a subscription.

I think breaking it is fine, as long as we write it down :)

kristjanvalur · 2023-06-01T13:27:17Z

Right, so please advise, if you like the approach I'm suggesting in this PR, then should I add the breakage to the RELEASE.md, similar to what was done with the recent channels PR?

patrick91 · 2023-06-01T13:31:37Z

Right, so please advise, if you like the approach I'm suggesting in this PR, then should I add the breakage to the RELEASE.md, similar to what was done with the recent channels PR?

we have a folder for breaking changes now: https://github.com/strawberry-graphql/strawberry/tree/main/docs/breaking-changes

I'll update the version number before merging 😊

nrbnlulu

Hey, I have worked on this PR long time ago though it got outdated... added some notes, if you want I can help.

nrbnlulu · 2023-06-05T04:01:30Z

strawberry/schema/execute.py

+    async with extensions_runner.operation():
+        # Note: In graphql-core the schema would be validated here but in
+        # Strawberry we are validating it at initialisation time instead
+        assert execution_context.query is not None


Should raise here MissingQueryError

Should it? I removed that code because it didn't get hit by coverage testing. It is my understanding that that can only happen if a query parameter is missing from a "query string", and we don't have these for subscriptions. Under what conditions could that possibly happen?

hmm I'm not sure I think @jthorniley added this.

nrbnlulu · 2023-06-05T04:04:28Z

strawberry/schema/execute.py

+    during parsing or validation.
+    Because we need to maintain execution context, we cannot return an
+    async generator, we must _be_ an async generator.  So we yield a
+    (bool, ExecutionResult) tuple, where the bool indicates whether the result is an


This tuple hack is not very pythonic IMO other than the fact that it is a breaking change... You might wanna check what I did at https://github.com/strawberry-graphql/strawberry/pull/2810/files#diff-88aa6fd17e4c6feac6e7152ebd3f2b8f972544c444a071b550e4d23061b97a3fR215 where if there is an error I return ExecutionResultError which is basically the same as normal ExecutionResult

That is a good idea. Yes, tuples are problematic and thought this might be a sticking point. I'll create a special exception class instead, much nicer.

Well, either an exception, or a special result class.. I think the exception might be cleaner, since one expects a subscription and the failure to get one is an exception of sorts. I'll see which one is nicer.

nrbnlulu · 2023-06-05T04:08:46Z

strawberry/schema/execute.py

+        # Strawberry we are validating it at initialisation time instead
+        assert execution_context.query is not None
+
+        async with extensions_runner.parsing():


This is duplicated from async execution, probably can be reused... see https://github.com/nrbnlulu/strawberry/blob/support_extensions_on_subscriptions/strawberry/schema/execute.py#L192

I'll see if I can do that after I change the tuple semantics.

I did refactor the common validation tests

nrbnlulu · 2023-06-05T04:11:34Z

strawberry/schema/execute.py

+                yield False, ExecutionResult(data=None, errors=execution_context.errors)
+                return  # pragma: no cover
+
+        async def process_result(result: GraphQLExecutionResult):


Why is that defined here, on every execution?

Its a closure. Avoids writing a compicated external function and passing arguments, as well as scoping functionality.
Closures aren't actually defined when run, merely instantiated with bindings. I guess it is down to style, I very much favor closures for their conciseness and locality. Happy to change it if you like.

I think it's a weak point for performance (though I agree it is much more readable), I would be happy if we would merge #2810 here or vice-versa. There I added a base class for async execution and it is done for subscription and async execution via https://github.com/nrbnlulu/strawberry/blob/support_extensions_on_subscriptions/strawberry/schema/execute.py#LL225C15-L225C39

I wasn't aware of #2810, I'll look at that.

I just want to add, as a general comment, that I think you will find that performance is not affected.
First of all, instantiate a closure a drop in the ocean of all the stuff that goes on to actually start executing the subscription.
Secondly, all of the execution time of a subscription actually happens parsing and processing the multiple results. Setting up of an subscription is not performance critical.

Finally, you suggested using contextlib.suppress, and that involves not only instantiating a class on every query, but also invoking two methods on it. There is quite a bit of machinery involved in a a contextlib.contextmanager.

I understand when people are concerned about performance, but one always needs to look at that in context.

I understand when people are concerned about performance, but one always needs to look at that in context.

Thanks. ig it is just something that bugs me... unless it is in a decorator I won't do it...
Sorry if i'm being too strict.

Sure, its moved out of the function in the latest commit.

nrbnlulu · 2023-06-05T04:14:02Z

strawberry/schema/execute.py

+                return
+
+            aiterator = result.__aiter__()
+            try:


why not contextlib.suppress?

Suppress what exactly? AttributeError? Because that exception might come from anywhere. This is surgically testing for the existence of the aclose method.
The need for this will go away with release 3.3.0 in graphql-core, where the subscribe() will return an async-generator.

I don't understand.

Because that exception might come from anywhere

What's wrong with

with contextlib.supress(BaseException): async for result in aiterator: yield True, await process_result(result) if hasattr(aiterator, "aclose"): await aiterator.aclose()

AFAIK this is the same as what you are doing...

No, you don't want to suppress BaseException. You never want to do that. CancelledError is one reason. And we would be suppressing any kind of Exception happening during iteration, including errors we want to pass to the callers, internal errors, whatnot.

No, try-finally exists precisely to do this kind of thing. In fact, that is how contextlib.aclosing() is implemented and it would be appropriate here, except that a) it requires python 3.8 and b), it assumes that aclose() is present.

with graphql-core 3.3, there will be an aclose() method present, and so aclosing() can be used. We can implement it manually if using 3.7

kristjanvalur · 2023-06-05T16:19:26Z

ruff is such a moving target. v 270 gives me no errors.
You probably want to add ruff as dev-dependency and pin it to a version

nrbnlulu · 2023-06-05T16:36:15Z

tests/views/schema.py

+            await asyncio.sleep(kwargs["sleep"])
+        return not self.fail
+
+
 class MyExtension(SchemaExtension):


Can this use

strawberry/tests/schema/extensions/test_extensions.py

Line 245 in b798287

class MyExtension(ExampleExtension):

?

No, well, those are defined inside test fixtures. It would be preferable to do it the other way round, since we have these extensions in the schema.

We would have to rewrite all of the http/websocket test bits to use schema extensions as fixtures.

We need the extension defined in the tests/views/schema.py, because that is the schema which is used for all the websocket unit tests, and we want to test the websockets/subscriptions against all the different integrations.

It would be preferable to do it the other way round

sure.

kristjanvalur · 2023-06-06T14:16:19Z

By the way, I offer a different kind of resolving the "either return an ExecutionResult or an Iterator" thing, to more
closely match what graphql-core does. Basically, it involves an outer helper around an inner generator.
The code looks like this:

import asyncio
import contextlib
from typing import AsyncGenerator, Optional, Union, cast

incontext = False


@contextlib.asynccontextmanager
async def mycontext() -> AsyncGenerator[None, None]:
    global incontext
    incontext = True
    try:
        yield
    finally:
        incontext = False


async def _subscribe(dofail: bool) -> AsyncGenerator[Optional[str], None]:
    async with mycontext():

        # yield an initial value, which will be the single result value,
        # or None if we're going to yield more values
        if dofail:
            yield "failure"
            return  # typically not reached
        else:
            yield None

        # yield more values
        yield "success"
        yield "success2"


async def subscribe(dofail: bool) -> Union[str, AsyncGenerator[str, None]]:
    # check the first value, and if it's not None, return it,
    # aborting the generator
    gen = _subscribe(dofail)
    first = await gen.__anext__()
    if first is not None:
        await gen.aclose()
        return first
    else:
        return cast(AsyncGenerator[str, None], gen)


async def main() -> None:
    result = []
    for dofail in [True, False]:
        assert not incontext
        v = await subscribe(dofail)
        if isinstance(v, str):
            result.append(v)
            assert not incontext
        else:
            assert incontext
            async for val in v:
                result.append(val)
                assert incontext
            assert not incontext
    assert result == ["failure", "success", "success2"]


if __name__ == "__main__":
    asyncio.run(main())

Using this pattern, we could avoid the SubscribeSingleResult exception, but we would then, again, have to check the
type of schema.subscribe() result to determine if it is a ExecutionResult or an iterator. The started generator is returned, maintaining the state of all context managers.

This essentially changes the interface back to what it was, which is probably preferable, it will reduce the size of the patch.

The drawback is that now
validation will happen before a Task is created, so I will also need to revert that unittest, but that's fine, I have a different PR to fix the validation and preparation of websocket operations anyway.

Both work. Which one is peferred by the maintainers? I'm happy to convert.

kristjanvalur · 2023-06-08T11:18:17Z

🛑 Important, before merging this, please consider #2825, which is a less intrusive change.

patrick91 · 2023-07-25T09:11:20Z

@kristjanvalur sorry for my lazyness, could you show me the two different APIs here? (or even another issue)

kristjanvalur · 2023-07-25T09:25:13Z

here we go. 2784 style:

result_source = self.schema.subscribe(...)
try:
    async for result_source:
        await self.send_result(operation, result, False)
except SubscribeSingleResult as single_result:
    await self.send_result(operation, single_result.value, True)
finally:
    await result_source.aclose()

2825 style (and the current style in code, inherited from graphql-core)

result_source = self.schema.subscribe(...)
if isinstance(result_source, ExecutionResult):
    await self.send_result(operation, result_source, True)
    return
try:
    async for result in result_source:
        await self.send_result(operation, result, False)
finally:
    await result_source.aclose()

Basically, switching on the result value type, or treating a single result (usually validation error) as an "exceptional" case.

The api must distinguish between the two cases (cannot return the error as a single result iterator) because the application will respond differently. An error during iteration is not fatal, an initial error is.

XChikuX · 2024-05-03T00:13:54Z

@patrick91 Can we merge this?

kristjanvalur · 2024-05-03T10:01:10Z

let me see if I can bring this up to date

…while executing.

…iptions

…ing a subscription

kristjanvalur · 2024-05-03T14:51:43Z

Still some minor fixes needed.
Please consider also if you think pr #2825 is a better api, see discussion above.

kristjanvalur · 2024-05-03T16:48:09Z

There have recently been made some changes to extension error handling.
It is unclear to me how to translate that to this PR.
Note that errors during evaluation of the request need to be raised as SingleValueException with an ExecutionResult.
After that, ExecutionResults are not fatal.
But unexpected exceptions, such as should indeed be fatal during iteration.

botberry added bot:has-release-file labels May 27, 2023

kristjanvalur force-pushed the kristjan/subscription-extensions branch 4 times, most recently from a4b4cc5 to 87fbff0 Compare May 31, 2023 07:51

kristjanvalur force-pushed the kristjan/subscription-extensions branch 3 times, most recently from 4923aa6 to 5a81ff3 Compare May 31, 2023 22:15

nrbnlulu suggested changes Jun 5, 2023

View reviewed changes

kristjanvalur mentioned this pull request Jun 5, 2023

Use our own iterator to iterate and transform subscription results #2724

Open

11 tasks

kristjanvalur force-pushed the kristjan/subscription-extensions branch 4 times, most recently from 78191f1 to cce672d Compare June 5, 2023 16:08

nrbnlulu reviewed Jun 5, 2023

View reviewed changes

nrbnlulu approved these changes Jun 8, 2023

View reviewed changes

kristjanvalur mentioned this pull request Jun 8, 2023

Support schema extensions for subsctriptions - Alternative #2825

Open

11 tasks

kristjanvalur mentioned this pull request Jun 20, 2023

Add handler method to process websocket connections #2617

Open

11 tasks

botberry removed the bot:has-release-file label Jun 22, 2023

kristjanvalur force-pushed the kristjan/subscription-extensions branch from abfd60e to ad6ca49 Compare August 25, 2023 13:40

kristjanvalur added 19 commits May 3, 2024 13:11

schema.subscribe is now an async generator. Apply extension contexts …

b466ecf

…while executing.

Update graphql_ws handler

9d5d16f

Remove unused code

46c9aa1

fix tests now that field verification is done by Strawberry

e0cd1fc

Return "extensions" as part of ExecutionResult if present.

22855f4

Add tests for blocking operation validation

7213377

Add some error/validation test cases

c8cec42

Add release.md

6dedf22

Fix schema tests for new subscribe() signature

4998305

Add extensive tests for extension hook execution while running subscr…

41d053e

…iptions

make extension test more robust, wait for old connections to drain.

8a039af

update docs for resolve extension hook

b622dac

fix channels tests to work with sync channels

9a8a633

schema.subscribe() now raises a SubscribeSingleResult when not return…

7aa2009

…ing a subscription

Move closure function out into a separate function

c5eec36

ruff

edd6d2e

tests for process_errors() (graphql_ws)

64c33ca

update benchmark test

a033624

Update newly added tests

699a910

kristjanvalur force-pushed the kristjan/subscription-extensions branch from f7a3873 to 699a910 Compare May 3, 2024 13:14

Fix typing

4fdbac4

fix new tests

5130fba

formatting

ef6688d

Support Schema Extensions in subscriptions #2784

Are you sure you want to change the base?

Support Schema Extensions in subscriptions #2784

Conversation

kristjanvalur commented May 27, 2023 • edited Loading

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

codecov bot commented May 27, 2023 • edited Loading

Codecov Report

botberry commented May 27, 2023 • edited Loading

kristjanvalur commented May 31, 2023

kristjanvalur commented May 31, 2023

patrick91 commented Jun 1, 2023

kristjanvalur commented Jun 1, 2023

patrick91 commented Jun 1, 2023

nrbnlulu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrbnlulu Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrbnlulu Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristjanvalur Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

nrbnlulu Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

kristjanvalur Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

kristjanvalur commented Jun 5, 2023 • edited Loading

nrbnlulu Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrbnlulu Jun 6, 2023 • edited Loading

Choose a reason for hiding this comment

kristjanvalur commented Jun 6, 2023 • edited Loading

kristjanvalur commented Jun 8, 2023 • edited Loading

patrick91 commented Jul 25, 2023

kristjanvalur commented Jul 25, 2023 • edited Loading

XChikuX commented May 3, 2024

kristjanvalur commented May 3, 2024

kristjanvalur commented May 3, 2024

kristjanvalur commented May 3, 2024

kristjanvalur commented May 27, 2023 •

edited

Loading

codecov bot commented May 27, 2023 •

edited

Loading

botberry commented May 27, 2023 •

edited

Loading

nrbnlulu Jun 5, 2023 •

edited

Loading

nrbnlulu Jun 5, 2023 •

edited

Loading

kristjanvalur Jun 5, 2023 •

edited

Loading

nrbnlulu Jun 5, 2023 •

edited

Loading

kristjanvalur Jun 5, 2023 •

edited

Loading

kristjanvalur commented Jun 5, 2023 •

edited

Loading

nrbnlulu Jun 5, 2023 •

edited

Loading

nrbnlulu Jun 6, 2023 •

edited

Loading

kristjanvalur commented Jun 6, 2023 •

edited

Loading

kristjanvalur commented Jun 8, 2023 •

edited

Loading

kristjanvalur commented Jul 25, 2023 •

edited

Loading