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

Don't expose the disposed ServiceProvider instance #2847

Conversation

ganhammar
Copy link
Contributor

The IServiceProvider instance in the ResolveEventStreamContext has been disposed at the point where it gets exposed to the subscription resolver and any data loaded fields included in the GraphQL request. With this PR it is set to null instead of the disposed instance. See discussion here.

I don't know if you would like to see tests for this change?

@ganhammar
Copy link
Contributor Author

@Shane32 I think this is the change you suggested?

I don't know if the issue should be moved from the server project and be linked with this PR?

@sungam3r
Copy link
Member

sungam3r commented Jan 7, 2022

Rel: graphql-dotnet/server#706

Co-authored-by: Ivan Maximov <sungam3r@yandex.ru>
@Shane32
Copy link
Member

Shane32 commented Jan 7, 2022

@ganhammar yes thanks. It looks like the branch needs to be merged with the current 'master' branch so that we can let it run the tests and merge it.

@ganhammar
Copy link
Contributor Author

@ganhammar yes thanks. It looks like the branch needs to be merged with the current 'master' branch so that we can let it run the tests and merge it.

Yeah, sorry, I missed that I already had forked the repo a while back, so didn't pull latest changes. Fixed now!

@codecov-commenter
Copy link

Codecov Report

Merging #2847 (652fdc2) into master (157074f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2847   +/-   ##
=======================================
  Coverage   83.78%   83.78%           
=======================================
  Files         364      364           
  Lines       14978    14979    +1     
  Branches     2369     2369           
=======================================
+ Hits        12549    12550    +1     
  Misses       1798     1798           
  Partials      631      631           
Impacted Files Coverage Δ
...QL.SystemReactive/SubscriptionExecutionStrategy.cs 88.19% <100.00%> (+0.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 157074f...652fdc2. Read the comment docs.

@Shane32
Copy link
Member

Shane32 commented Jan 7, 2022

@sungam3r look good?

@sungam3r sungam3r added the bugfix Pull request that fixes a bug label Jan 7, 2022
@sungam3r sungam3r added this to the 4.8 milestone Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Pull request that fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants