Update graphql.net to 4.6.1 #10782

MikeKry · 2021-11-30T15:04:28Z

Gatsby in version 3 and 4 is not working with current version of GraphQL in OrchardCore, so I have tried to update GraphQL.NET package.

I am currently testing it, so it does not break anything, right now it looks ok, but it definitely needs some more checking. I will be testing it further on existing projects.

It was quite a lot of work, so I decided to create a PR right away, so we can potentially merge it.

related to #10781
related also to #10760

deanmarcussen · 2021-11-30T16:03:58Z

can you merge dev on this, so that the tests run.

also for infos here is the current pr to do this upgrade, with the outstanding work #9087 which maybe useful to see what has already been done, and is still needed to do

carlwoodhouse · 2021-11-30T16:09:43Z

can you merge dev on this, so that the tests run.

also for infos here is the current pr to do this upgrade, with the outstanding work #9087 which maybe useful to see what has already been done, and is still needed to do

yeah sadly actual work keeps getting the better of me so i haven't had chance to work on it much in ages. Theres def stuff in the other PR we should use though; as i changed a lot to work to how graphql.net 4 expects in terms of DI, Permissions, etc and it should be more performant (as it removes some locks etc).

It would be good if we could maybe bring these 2 pr's together tho? take any of this which fixers outstanding issues in the other one ? @MikeKry

MikeKry · 2021-11-30T16:25:44Z

Ups.. I do not know why I could not find this PR when I searched for any update tasks/tickets..

So far my PR is working, I have found some problems when including into existing project and fixed them. I can take a look at other PR and use some of it. @carlwoodhouse do you have anything specific in mind that you know should be changed?

IDK if it would not be better to merge just an update so it would work with new versions of gatsby and then create tickets to update authorization etc. to new standards?

MikeKry · 2021-11-30T16:31:29Z

i think this pr also resolves - Dynamic fields are broken & Custom validators for max results need re-implementing to new spec & Check GraphQLFieldNameAttribute from original PR

carlwoodhouse · 2021-11-30T16:34:53Z

yeah this is why its worth doing a comparision of the two (maybe target this at that branch as its on orchards github) and seeing where we are.

My main concern is a lot changed around document execution, dataloaders and some other threadsafety stuff ... but its been a while since i looked at it ...; so while this may compile and work .... (you've done an awesome job btw) - we need to make sure we dont break all the weird race conditions we had to fix in the first place (a lot of which are fixed in the newer versions)

MikeKry · 2021-11-30T16:47:33Z

@carlwoodhouse Thanks, I will take a look at it and compare what could be changed. I have done this using migration guides from 2->3 and 3->4 and used suggested solutions, so I hope it will be all OK, but it will definitely need someone who will check for know errors, as I test it only for errors that I know.

@deanmarcussen Is it enough to merge dev into my branch and push? or should i create a PR against dev? So far I am fixing some minor errors in tests but soon it will be fixed.

carlwoodhouse · 2021-11-30T16:51:16Z

@carlwoodhouse Thanks, I will take a look at it and compare what could be changed. I have done this using migration guides from 2->3 and 3->4 and used suggested solutions, so I hope it will be all OK, but it will definitely need someone who will check for know errors, as I test it only for errors that I know.

@deanmarcussen Is it enough to merge dev into my branch and push? So far I am fixing some minor errors in tests but soon it will be fixed.

I mean a quick look through a lot is really similar; so you've done a great job (its no small thing which is why time has eluded me to finish the other PR heh); just think its worth bringing the two together so we get any optimizations etc from the other PR - and the unfinished stuff from here too. You seem to have done the DI wire-up somewhat different to me which i'd need to look at too.

carlwoodhouse · 2021-11-30T16:52:54Z

these days we have to merge main not dev i think @deanmarcussen :P

carlwoodhouse · 2021-11-30T17:21:16Z

your text field changes fix the dynamic content issue on the other PR :)

deanmarcussen · 2021-11-30T17:37:43Z

yes merge main, not dev :)

fix tests, remove services from graphqlcontext, fix where and orderby

… update-grahpqlnet-4.6.1

MikeKry · 2021-11-30T18:33:17Z

@deanmarcussen @carlwoodhouse

Ok, I have merged items that made sense for me to merge from other PR, tests ran successfully locally, and I merged main into this branch, so I hope it could be ok.

carlwoodhouse · 2021-11-30T18:42:17Z

If you’re ok with it; it might be an idea to target this pr at the other branch ag/graphql4 rather then main so we can see if there is any other notable differences. (That branch is in this repo rather then a fork so you should be able to merge it down to your fork) Then we can use that branch for further testing. I agree we should aim to get that mergable asap and then optimise after; but I want to do some testing around data loaders and concurrency before we do.

…

On Tue, 30 Nov 2021 at 18:33, MikeKry ***@***.***> wrote: @deanmarcussen <https://github.com/deanmarcussen> @carlwoodhouse <https://github.com/carlwoodhouse> Ok, I have merged items that made sense for me to merge from other PR, tests ran successfully locally, and I merged main into this branch, so I hope it could be ok. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10782 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADRUEF3RI7VEF5MEKSIS5DUOUKHRANCNFSM5JCBLZKA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

Skrypt · 2021-11-30T19:15:57Z

As long as it's not ready it should stay in a branch. Move it to the Orchard Core repository if you want but don't merge it in the main branch if it's creating regressions that need to be fixed first. So, move it to a graphql.net-4.6.1 branch in here. Then you may be able to compare the 2 branches locally.

carlwoodhouse · 2021-11-30T19:18:38Z

That’s what i said i think :p the ag:graphql4 branch is here ;)

…

On Tue, 30 Nov 2021 at 19:16, Jasmin Savard ***@***.***> wrote: As long as it's not ready it should stay in a branch. Move it to the Orchard Core repository if you want but don't merge it in the main branch if it's creating regressions that need to be fixed first. So, move it to a graphql-4.6.1 branch in here. Then you may be able to compare the 2 branches locally. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10782 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADRUEHDGDJZ7VHTW3OOK53UOUPHRANCNFSM5JCBLZKA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

Skrypt · 2021-11-30T19:19:35Z

Well, as long as it's not merged on top of the main branch yet. 😉

MikeKry · 2021-11-30T20:13:18Z

@carlwoodhouse

I am okay with that as long as it will help with getting done ASAP :)
Do whatever you like with this PR or instruct me what should I do if you do not have rights to edit PR.

I would be thankful if it would get merged this week if possible, because it blocks for me two projects that are dependend on graphql.. and I am ready to assist with this if needed.

carlwoodhouse

generally looks good, added a few comments and queries

src/OrchardCore.Modules/OrchardCore.Apis.GraphQL/GraphQLMiddleware.cs

carlwoodhouse · 2021-12-01T13:01:09Z

src/OrchardCore.Modules/OrchardCore.Apis.GraphQL/GraphQLMiddleware.cs

+
+            // changed in V4
+            var encodedBytes = _utf8Encoding.GetBytes(await _writer.WriteToStringAsync(result));
+            await context.Response.Body.WriteAsync(encodedBytes, 0, encodedBytes.Length); // documentWriter causes problems when querying _schema


can you elaborate?

documentWriter.WriteAsync throws error when grahpql tries to get schema
"System.InvalidOperationException: Synchronous operations are disallowed. Call WriteAsync or set AllowSynchronousIO to true instead."

I did not find the cause and using workaround with AllowSynchronousIO does not seem ok to me.

this does ring a bell ... is this an issue in the ag/graphql4 branch currently then ? i have no idea why i changed it .... yours looks closer to the original tbh

Needs more thoughts, will buffer all responses, and the big ones could be an issue in terms of perf. Maybe at least use an ArrayPool?

src/OrchardCore.Modules/OrchardCore.Apis.GraphQL/Startup.cs

src/OrchardCore/OrchardCore.ContentManagement.GraphQL/Extensions/DataLoaderExtensions.cs

...chardCore/OrchardCore.Apis.GraphQL.Abstractions/OrchardCore.Apis.GraphQL.Abstractions.csproj

src/OrchardCore/OrchardCore.ContentManagement.GraphQL/Extensions/FieldTypeExtensions.cs

src/OrchardCore/OrchardCore.ContentManagement.GraphQL/Queries/ContentItemsFieldType.cs

src/docs/requirements.txt

MikeKry · 2021-12-02T13:21:05Z

@carlwoodhouse

I have found some problems with update, I will try to resolve them and then push along with resolved change request

Fix where condition with nested objects, fix where conditions with AddScalarFilterFields

MikeKry · 2021-12-02T14:15:25Z

@carlwoodhouse

Pushed with fixes of nested where conditions and scalar filters => maybe there could be better solution, but this one ensures same functionality as we had before. There could be also unit test for nested where conditions.

I will continue testing, maybe some more errors will comes up.

carlwoodhouse · 2021-12-02T14:17:36Z

Yeah I think there is def some missing unit tests .. feel free to add any that cover scenarios you find issues with

…

On Thu, 2 Dec 2021 at 14:15, MikeKry ***@***.***> wrote: @carlwoodhouse <https://github.com/carlwoodhouse> Pushed with fixes of nested where conditions and scalar filters => maybe there could be better solution, but this one ensures same functionality as we had before. There could be also unit test for nested where conditions. I will continue testing, maybe some more errors will comes up. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10782 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADRUEFUXTFA2CFLIOG3TPLUO55QRANCNFSM5JCBLZKA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

carlwoodhouse · 2021-12-02T14:27:42Z

...ardCore.Modules/OrchardCore.Apis.GraphQL/ValidationRules/RequiresPermissionValidationRule.cs

@@ -26,14 +26,13 @@ public RequiresPermissionValidationRule(IAuthorizationService authorizationServi

        public async Task<INodeVisitor> ValidateAsync(ValidationContext validationContext)
        {
-            var operationType = OperationType.Query;
+            // shouldn't we access UserContext from validationcontext inside MatchingNodeVisitor actions?


probably :)

MikeKry · 2021-12-13T10:21:00Z

@carlwoodhouse

Can I ask, is there anything waiting for me or can we move this forward?
We are using it on project for some time and no other bugs appeared.

carlwoodhouse · 2021-12-13T11:50:45Z

@carlwoodhouse

Can I ask, is there anything waiting for me or can we move this forward? We are using it on project for some time and no other bugs appeared.

mostly thoughts on @sebastienros comment about the buffering of the response for the changes around the write and the negative performance implications ..

MikeKry · 2021-12-13T14:09:16Z

@carlwoodhouse

Do you want me to look at it or you will do it yourself?

carlwoodhouse · 2021-12-13T14:10:59Z

If you could that would be great :) happy to feedback on it though.

…

On Mon, 13 Dec 2021 at 14:09, MikeKry ***@***.***> wrote: @carlwoodhouse <https://github.com/carlwoodhouse> Do you want me to look at it or you will do it yourself? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10782 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADRUEFZHNUUTRA227GVOSDUQX5BRANCNFSM5JCBLZKA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

MikeKry · 2021-12-14T10:19:14Z

@carlwoodhouse @deanmarcussen

The problem with WriteAsync is in Serialize method in newtonsoft.json.
It is using synchronous Serialize internal method (https://github.com/JamesNK/Newtonsoft.Json/blob/master/Src/Newtonsoft.Json/JsonSerializer.cs#L1146)

which causes problems when writing directly into response.

I see two variants in here:
1/ overwrite this method - seems too complex
2/ serialize it into memory stream and then copy to response like this

MikeKry · 2021-12-15T14:23:16Z

I have pushed the change so you can all look at it. I think it is indeed better than before - WriteToString calls internally WriteAsync, so we have 1 less method call and we do not hold another variable in memory

src/OrchardCore.Modules/OrchardCore.Apis.GraphQL/GraphQLMiddleware.cs

sebastienros · 2021-12-15T15:15:42Z

src/OrchardCore.Modules/OrchardCore.Apis.GraphQL/GraphQLMiddleware.cs

+
+        private async Task WriteAsync<T>(Stream stream2, T value, IDocumentWriter documentWriter, CancellationToken cancellationToken = default)
+        {
+            // needs to be always async, otherwise __schema request is not working, direct write into response does not work as serialize is using sync method inside


Do you mean that somewhere in our grapqhl code with have a call to a Serialize method (NewtonSoft?) that is not async and this is triggered in IDocumentWriter.WriteaAsync.
If buffering is required, maybe use a smaller reused buffer instead of a single one as big as the payload, and pool it too? Another option might be to use BufferedStream between documentWriter and context.Response.Body.

…ware.cs Co-authored-by: Sébastien Ros <[email protected]>

…OrchardCore into update-grahpqlnet-4.6.1

sebastienros · 2021-12-16T18:42:06Z

src/OrchardCore.Modules/OrchardCore.Apis.GraphQL/GraphQLMiddleware.cs

-            await documentWriter.WriteAsync(stream, value);
-            stream.Seek(0, SeekOrigin.Begin);
-            await stream.CopyToAsync(stream2, cancellationToken);
+            using (MemoryStream stream = new MemoryStream())


Might use BufferedStream so not all the payload is serialized in memory.

sebastienros · 2021-12-16T18:42:41Z

Looks promising. What's left to do, and is it replacing the other PR that is meant to migrate to 4.x ?

carlwoodhouse · 2021-12-16T18:44:51Z

this one is targeted at that branch the other pr is in mate currently (that branch is merged into this)

…

On Thu, 16 Dec 2021 at 18:42, Sébastien Ros ***@***.***> wrote: Looks promising. What's left to do, and is it replacing the other PR that is meant to migrate to 4.x ? — Reply to this email directly, view it on GitHub <#10782 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADRUEEFYTXLTD25WKK6DFDURIXKZANCNFSM5JCBLZKA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

carlwoodhouse · 2021-12-16T18:58:01Z

In terms of what’s left .. could really do with testing the dataloaders are still deferring execution .. I think there’s many optimisations we can make around the dl’s as we can probably remove the lockedresolver now you can use dataloaders with serial execution … but those things can wait for a later pr.

MikeKry · 2022-01-03T13:46:09Z

I did not encounter any other problems after using it locally for month.
If you want to use buffered stream, could you do it @carlwoodhouse or @deanmarcussen ? I just dont see how to use it in there.

deanmarcussen · 2022-01-05T09:59:15Z

@carlwoodhouse

I'm confused, what's the story with this, it's targetting the other branch Antoine and you were working on ?

Can we merge it into there, so we can see review the two as one, and fix the stream buffering issue / look at what else is to do?

carlwoodhouse · 2022-01-05T13:06:55Z

yeah merge it to that branch and we can fix whats outstanding there

…

On Wed, 5 Jan 2022 at 09:59, Dean Marcussen ***@***.***> wrote: @carlwoodhouse <https://github.com/carlwoodhouse> I'm confused, what's the story with this, it's targetting the other branch Antoine and you were working on ? Can we merge it into there, so we can see review the two as one, and fix the stream buffering issue / look at what else is to do? — Reply to this email directly, view it on GitHub <#10782 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADRUECQAOPJUIYIZZLYZVDUUQI77ANCNFSM5JCBLZKA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

MikeKry · 2022-01-10T07:37:51Z

and who can merge it? I can't..

carlwoodhouse · 2022-01-10T11:36:01Z

merged to ##9087

PUXMKU added 2 commits November 30, 2021 08:56

update graphql.net from 2.4.0 to 4.6.1

1bb2098

Update gql to 4.6.1

db0571f

MikeKry requested review from carlwoodhouse, Jetski5822 and Skrypt as code owners November 30, 2021 15:04

PUXMKU added 2 commits November 30, 2021 16:38

Add service provider to execution options

908a97f

Update field resolvers for textfield and multitextfield, add test fixes

d1316e4

revert uintentional change of .NET framework

dc4f3fc

carlwoodhouse mentioned this pull request Nov 30, 2021

GraphQL 4 #9087

Merged

9 tasks

agriffard and others added 3 commits November 30, 2021 19:24

mkdocs-material 8.0.2

cb9af75

Updates based on other PR

6d77452

fix tests, remove services from graphqlcontext, fix where and orderby

Merge branch 'main' of https://github.com/OrchardCMS/OrchardCore into…

83fe01a

… update-grahpqlnet-4.6.1

carlwoodhouse changed the base branch from main to ag/graphql4 November 30, 2021 20:22

carlwoodhouse requested a review from agriffard as a code owner November 30, 2021 20:22

fix MaxNumberOfResultsValidationRule

6434fd6

carlwoodhouse requested changes Dec 1, 2021

View reviewed changes

Fixes according to requested changes.

15c07da

Fix where condition with nested objects, fix where conditions with AddScalarFilterFields

carlwoodhouse reviewed Dec 2, 2021

View reviewed changes

Remove WriteToStringAsync method use, save some memory

d8deeff

sebastienros reviewed Dec 15, 2021

View reviewed changes

src/OrchardCore.Modules/OrchardCore.Apis.GraphQL/GraphQLMiddleware.cs Outdated Show resolved Hide resolved

sebastienros reviewed Dec 15, 2021

View reviewed changes

MikeKry and others added 3 commits December 16, 2021 13:28

Update src/OrchardCore.Modules/OrchardCore.Apis.GraphQL/GraphQLMiddle…

f7d210a

…ware.cs Co-authored-by: Sébastien Ros <[email protected]>

using on memory stream

9c3429e

Merge branch 'update-grahpqlnet-4.6.1' of https://github.com/MikeKry/…

6dbbe25

…OrchardCore into update-grahpqlnet-4.6.1

sebastienros reviewed Dec 16, 2021

View reviewed changes

carlwoodhouse merged commit cc3eb91 into OrchardCMS:ag/graphql4 Jan 10, 2022

MikeKry mentioned this pull request Apr 7, 2022

GraphQL4 - resolve async write using System.Text.Json #11499

Merged

Update graphql.net to 4.6.1 #10782

Update graphql.net to 4.6.1 #10782

Conversation

MikeKry commented Nov 30, 2021 • edited Loading

deanmarcussen commented Nov 30, 2021

carlwoodhouse commented Nov 30, 2021

MikeKry commented Nov 30, 2021

MikeKry commented Nov 30, 2021

carlwoodhouse commented Nov 30, 2021

MikeKry commented Nov 30, 2021 • edited Loading

carlwoodhouse commented Nov 30, 2021

carlwoodhouse commented Nov 30, 2021

carlwoodhouse commented Nov 30, 2021

deanmarcussen commented Nov 30, 2021

MikeKry commented Nov 30, 2021

carlwoodhouse commented Nov 30, 2021 via email • edited Loading

Skrypt commented Nov 30, 2021 • edited Loading

carlwoodhouse commented Nov 30, 2021 via email

Skrypt commented Nov 30, 2021

MikeKry commented Nov 30, 2021

carlwoodhouse left a comment

Choose a reason for hiding this comment

carlwoodhouse Dec 1, 2021

Choose a reason for hiding this comment

MikeKry Dec 2, 2021

Choose a reason for hiding this comment

carlwoodhouse Dec 2, 2021

Choose a reason for hiding this comment

sebastienros Dec 2, 2021

Choose a reason for hiding this comment

MikeKry commented Dec 2, 2021

MikeKry commented Dec 2, 2021

carlwoodhouse commented Dec 2, 2021 via email

carlwoodhouse Dec 2, 2021

Choose a reason for hiding this comment

MikeKry commented Dec 13, 2021

carlwoodhouse commented Dec 13, 2021

MikeKry commented Dec 13, 2021

carlwoodhouse commented Dec 13, 2021 via email

MikeKry commented Dec 14, 2021

MikeKry commented Dec 15, 2021

sebastienros Dec 15, 2021

Choose a reason for hiding this comment

sebastienros Dec 16, 2021

Choose a reason for hiding this comment

sebastienros commented Dec 16, 2021

carlwoodhouse commented Dec 16, 2021 via email

carlwoodhouse commented Dec 16, 2021 via email

MikeKry commented Jan 3, 2022

deanmarcussen commented Jan 5, 2022

carlwoodhouse commented Jan 5, 2022 via email

MikeKry commented Jan 10, 2022

carlwoodhouse commented Jan 10, 2022

MikeKry commented Nov 30, 2021 •

edited

Loading

MikeKry commented Nov 30, 2021 •

edited

Loading

carlwoodhouse commented Nov 30, 2021 via email •

edited

Loading

Skrypt commented Nov 30, 2021 •

edited

Loading