Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support schema extensions for subsctriptions - Alternative #2825

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

kristjanvalur
Copy link
Contributor

@kristjanvalur kristjanvalur commented Jun 8, 2023

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, This is a separate PR from #2784, because it contains an alternative, less intrusive, way to do this.
It avoids changing the signature of schema.subscribe which now, like graphql-core, continues to return either
an ExcecutionResult or an AsyncGenerator.

Description

the Schema.subscribe(), when awaited returns a Union[ExeuctionResult, AsyncGenerator[ExecutionResult,None]].
This is similar to before, but previously it would return a raw GraphQLExecutionResult object, or an AsyncIterator over said type, directly from graphql-core.

Internally, an AsyncGenerator is created which yields an extra initial result, which is either a ExecutionResult in case
the inner subscribe method failed and no iterator is forthcoming, or a None if subscription is about to proceed. An
outer method examines this initially yielded value and either returns that to the caller, or the already started AsyncGenerator, which the caller then continues to iterate from, maintaining all context which that iterator had already established.

Tests are added to tests for result extensions.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Issues Fixed or Closed by This PR

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 we

@codecov
Copy link

codecov bot commented Jun 8, 2023

Codecov Report

Merging #2825 (14a83ba) into main (6d86d1c) will increase coverage by 0.00%.
The diff coverage is 98.04%.

Additional details and impacted files
@@           Coverage Diff            @@
##             main    #2825    +/-   ##
========================================
  Coverage   96.55%   96.55%            
========================================
  Files         468      468            
  Lines       29201    29417   +216     
  Branches     3592     3616    +24     
========================================
+ Hits        28194    28403   +209     
- Misses        827      831     +4     
- Partials      180      183     +3     

@kristjanvalur kristjanvalur force-pushed the kristjan/subscription-extenstions-2 branch 2 times, most recently from 39b9aa8 to eb8c340 Compare June 8, 2023 11:37
@kristjanvalur kristjanvalur marked this pull request as ready for review June 8, 2023 11:47
@botberry
Copy link
Member

botberry commented Jun 8, 2023

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


Subscriptions now support Schema Extensions.


Here's the preview release card for twitter:

Here's the tweet text:

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

Get it here 👉 https://github.com/strawberry-graphql/strawberry/releases/tag/(next)

@codspeed-hq
Copy link

codspeed-hq bot commented Jul 18, 2023

CodSpeed Performance Report

Merging #2825 will degrade performances by 79.84%

Comparing mainframeindustries:kristjan/subscription-extenstions-2 (14a83ba) with main (6d86d1c)

Summary

❌ 1 regressions
✅ 10 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main mainframeindustries:kristjan/subscription-extenstions-2 Change
test_subscription 232.7 ms 1,154.1 ms -79.84%

@patrick91
Copy link
Member

/pre-release

@botberry
Copy link
Member

Pre-release

👋

Releasing commit [cb3949a] to PyPi as pre-release! 📦

@botberry
Copy link
Member

Pre-release

👋

Pre-release 0.196.0.dev.1689676980 [cb3949a] has been released on PyPi! 🚀
You can try it by doing:

poetry add strawberry-graphql==0.196.0.dev.1689676980

Update graphql_ws handler

fix tests now that field verification is done by Strawberry

Return "extensions" as part of ExecutionResult if present.

Add some error/validation test cases

Add release.md

Fix tests, remove extra "extensions" payload we don't want to inspect

Add extensive tests for extension hook execution while running subscriptions

update docs for `resolve` extension hook
@kristjanvalur kristjanvalur force-pushed the kristjan/subscription-extenstions-2 branch from cb3949a to 14a83ba Compare August 25, 2023 13:43
@kristjanvalur
Copy link
Contributor Author

Performance regression is expected when all of the schema extension boilerplate code is added on top of a trivial method.
I suspect that if we add a performance test which tests iterating over 1000 entries of a subscription, that will perform much more favourably

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants