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

Graphql 5.3 #232

Merged
merged 4 commits into from
Jul 4, 2022
Merged

Graphql 5.3 #232

merged 4 commits into from
Jul 4, 2022

Conversation

pleandre
Copy link
Contributor

@pleandre pleandre commented Jun 24, 2022

Upgrading to Graph QL 5.3 to use with the last version of Graph QL Server and dotnet 6.

I still have an issue in the Graph QL Subscription Sample Project if someone can help me have a look.
I couldn't find the issue (it runs but there is an exception on runtime when you update a field and have a subscription), I would need to dig dipper in Graph QL dotnet internals and all the reflection but I didn't have time.

image

Other things seems to work

@pestrada-trimble
Copy link

This would also help resolve the NewtonsoftJson vulnerability for versions < 13.0.1

Currently this package references GraphQL.NewtonsoftJson/4.1.0 which references NewtonsoftJson/12.0.3

@sungam3r
Copy link
Member

sungam3r commented Jul 1, 2022

I advise to switch to STJ whenever possible.

Comment on lines 25 to 28
public override object Resolve(IResolveFieldContext context)
{
return context.Source;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is critical for subscription support.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @Shane32

Copy link
Member

@Shane32 Shane32 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These three changes should fix subscription support.

@pleandre pleandre requested a review from Shane32 July 4, 2022 03:59
@pleandre
Copy link
Contributor Author

pleandre commented Jul 4, 2022

It works with @Shane32 changes, would it be possible to approve this PR if there are no more comments ?
I agree with @sungam3r to change to System Text Json, but it requires to rewrite RequestDeserializer.cs which takes some time. Maybe in another PR.

Co-authored-by: Shane Krueger <[email protected]>
Copy link
Member

@Shane32 Shane32 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't use this project, but I don't see any problems with merging this. I did read through each change. @sungam3r Do you want to review?


// ReSharper disable InconsistentNaming

namespace Tests.Adapters.Engine.Types
{
#region .NET Class
public class JSON : ValueNode<IDictionary<string, object>>
public class JSON : GraphQLValue
Copy link
Member

@Shane32 Shane32 Jul 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI this test is completely wrong (not the fault of this PR). Deserializing a GraphQL request cannot possibly deserialize to a JSON node. Rather, ParseLiteral should be parsing a structured tree of objects and values into an Inputs object or something, or interpreting a JSON string value stored as a string literal and deserializing it as an Inputs object. Further, the ParseValue method should probably just take the object and return it, as the GraphQL serializer should be deserializing variables into a structured map. It won't ever be a GraphQLValue node of any kind. And ToAST should convert the structured map into a tree of nodes.

@tlil
Copy link
Collaborator

tlil commented Jul 4, 2022

LGTM – will let @sungam3r take a look as well

Copy link
Member

@sungam3r sungam3r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't use this project too. Merge as you see fit.

@tlil tlil merged commit be60ed2 into graphql-dotnet:master Jul 4, 2022
@pleandre pleandre deleted the graphql-5.3 branch July 5, 2022 06:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants