GraphQL 4 #9087

agriffard · 2021-04-07T12:55:19Z

Giving another try to see if we can migrate to GraphQL 4.

Many changes in versions 3 and 4:
https://graphql-dotnet.github.io/docs/migrations/migration3/
https://graphql-dotnet.github.io/docs/migrations/migration4/

update from @carlwoodhouse

I fixed a bunch of the tests and issues, still some stuff outstanding. Not sure of the state of the lucene stuff either as im not as familiar with that integration but suggest we fix the other issues before that

Dynamic fields are broken
Custom validators for permissions need re-implementing to new spec
Custom validators for max results need re-implementing to new spec - fixed in Update graphql.net to 4.6.1 #10782
Check metadata thread safety changes for permissions/part attributes meta
Remove asynclock resolver and replace with built in scoped resolver / check this works with parallel execution strategy
Check dataloader changes are actually deferring load
Check GraphQLFieldNameAttribute changes; how we've resolved it atm doesnt seem right to me ...
Not correctly setting response code on auth issues - fixed in Update graphql.net to 4.6.1 #10782
buffered stream issue from (Update graphql.net to 4.6.1 #10782)

carlwoodhouse · 2021-04-07T16:58:53Z

Will take a look this week mate

…

On Wed, 7 Apr 2021 at 13:55, Antoine Griffard ***@***.***> wrote: @agriffard <https://github.com/agriffard> requested your review on: #9087 <#9087> GraphQL 4 as a code owner. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#9087 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADRUEGW37WXXTZ4UTHSJPLTHRI4NANCNFSM42QXTWYA> .

carlwoodhouse

@agriffard these are more notes then anything; will checkout your branch and try and resolve the remaining issues + anything ele; main convern is that im not sure the dataloaders will be working as intended atm

carlwoodhouse · 2021-04-08T22:10:44Z

src/OrchardCore.Modules/OrchardCore.Lists/GraphQL/ListQueryObjectType.cs

@@ -36,9 +37,9 @@ public ListQueryObjectType(IStringLocalizer<ListQueryObjectType> S)

                    var dataLoader = accessor.Context.GetOrAddCollectionBatchLoader<string, ContentItem>("ContainedPublishedContentItems", x => LoadPublishedContentItemsForListAsync(x, session));

-                    return (await dataLoader.LoadAsync(g.Source.ContentItem.ContentItemId))
+                    return ((await dataLoader.LoadAsync(g.Source.ContentItem.ContentItemId).GetResultAsync())


i need to read up on the dataloader changes; but this doesnt seem right ... seems like it would return the results immediately rather then batching them into a dataloader

yes this seems odd

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

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

… usercontext

carlwoodhouse · 2021-04-09T00:17:36Z

Now i fixed some stuff theres another bunch of compilation errors ;) will do some more over the weekend @agriffard

carlwoodhouse · 2021-04-09T08:26:33Z

theres changes around how the execution strat works for dataloaders too we will need to change; can probably remove the asynclockresolver we implemented now we can use a serial start with dataloaders and some other di changes to enforce the right scopes

carlwoodhouse · 2021-04-09T11:06:07Z

think we also need to handle https://graphql-dotnet.github.io/docs/migrations/migration4/#metadata-is-not-thread-safe

agriffard · 2021-04-09T17:28:04Z

Sorry @carlwoodhouse , it was to fix ShouldFilterByCollapsedWhereInputForCollapsedParts that makes an exception:

System.InvalidCastException : Unable to cast object of type 'GraphQL.Types.StringGraphType' to type 'GraphQL.Types.ListGraphType'.

carlwoodhouse · 2021-04-09T17:30:11Z

Yeah I have a fix for this one I think; the type is passed wrong in the test :) looking back one of us (I think me) set it wrong when redoing the test to work with resolved type instead of return type

…

On Fri, 9 Apr 2021 at 18:28, Antoine Griffard ***@***.***> wrote: Sorry @carlwoodhouse <https://github.com/carlwoodhouse> , it was to fix ShouldFilterByCollapsedWhereInputForCollapsedParts that makes an exception: System.InvalidCastException : Unable to cast object of type 'GraphQL.Types.StringGraphType' to type 'GraphQL.Types.ListGraphType'. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9087 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADRUEBRYMHFV4PMGSJS5HDTH42LRANCNFSM42QXTWYA> .

agriffard · 2022-01-14T16:58:32Z

Can you fix the conflicts @agriffard ? thanks

Solved.

MikeKry · 2022-02-10T15:46:42Z

@carlwoodhouse

is there any description of graphql n+1 issue so I could try to look at dataloaders myself?

I would love to have this merged as current version is not compatible with gatsby.

sebastienros · 2022-02-10T18:59:54Z

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

            {
-                throw new ArgumentNullException(nameof(message));
+                await documentWriter.WriteAsync(stream, value);


Adding a comment here since the one before got removed

MikeKry · 2022-03-09T17:26:26Z

There might be another thing to solve (both on old and new implementation)

Sometimes when you create new field that is also in graphql - you need to reset CMS (either by reload tenant or some other action) to make it work correctly. Also it seems that is works in graphql viewer in CMS instantly but when accessed through API it is cached or something (but CMS reload helps, does not need to be changed on client site).

Otherwise no errors till now.

agriffard · 2022-04-04T08:36:13Z

https://www.nuget.org/packages/GraphQL/5.0.0

agriffard · 2022-04-04T08:36:36Z

https://graphql-dotnet.github.io/docs/migrations/migration5/

MikeKry · 2022-04-05T13:41:59Z

so,... shall I update it to GraphQL 5 ?
Or will dataloaders be solved first before that?

MikeKry · 2022-04-07T16:31:46Z

I have addressed issue with buffered stream - if its ok it can be merged into this branch.

Tomorrow I plan to look at dataloaders, possibly updateto GQL 5 if it is wanted.

Co-authored-by: Michal Kužela <michal.kuzela@puxdesign.cz>

agriffard · 2022-04-28T18:15:59Z

Also started 1 year ago.

agriffard · 2022-09-01T18:17:03Z

ping @sebastienros

MikeKry · 2022-09-03T13:55:39Z

It seems RequiresPermissionValidationRuleTests is broken due to changes in newtonsoft/systemtextjson, but otherwise we are running projects on this branch for a long time without problems.

I did not manage to find the problem with dataloaders though.

Skrypt · 2022-09-08T23:43:51Z

Will we have someone working to get this up to GraphQL.NET 7.0.2 ?

MikeKry · 2022-11-16T09:31:53Z

Would give sense, there are interesting query complexity calculation changes that could positively influence headless apps.

GraphQL 4

738f448

agriffard requested review from carlwoodhouse, Jetski5822 and Skrypt as code owners April 7, 2021 12:55

carlwoodhouse reviewed Apr 8, 2021

View reviewed changes

carlwoodhouse added 2 commits April 9, 2021 01:12

started on dataloaders, removed no longer required serviceprovider on…

f42ac11

… usercontext

updated some more requestservices references

f2fdd21

carlwoodhouse and others added 2 commits April 9, 2021 01:28

few type fixes

a4c54b7

More fixes

4d80c30

carlwoodhouse and others added 4 commits April 9, 2021 10:11

fix newtonsoft conflicting reference

58330aa

More fixes

b86b539

improving middleware

80c983b

merge

b41bdb3

carlwoodhouse added GraphQL notready labels Apr 9, 2021

This comment has been minimized.

Sign in to view

carlwoodhouse and others added 7 commits April 9, 2021 14:41

actually fixed one test :P

782f516

fixed some more tests

5851eb1

.

ee07c35

fixed some of the blog integration tests

1501f38

fix some tests

79d6ce2

fixed more tests

c97640f

ResolvedType

257dc55

fix unit tests

78d0b52

Merge branch 'main' into ag/graphql4

58b48dd

sebastienros reviewed Feb 10, 2022

View reviewed changes

MikeKry mentioned this pull request Feb 19, 2022

GraphQL & Gatsby 3, 4 - not working #10781

Closed

MikeKry mentioned this pull request Mar 20, 2022

GraphQL session MARS/DataReader is open problem #9330

Closed

Merge branch 'main' into ag/graphql4

f9e6b91

MikeKry mentioned this pull request Apr 7, 2022

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

Merged

MikeKry and others added 2 commits April 28, 2022 11:08

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

098106b

Co-authored-by: Michal Kužela <michal.kuzela@puxdesign.cz>

Merge branch 'main' into ag/graphql4

d96848a

sebastienros added 4 commits September 1, 2022 11:23

Merge branch 'main' into ag/graphql4

7a5eddc

Update RequiresPermissionValidationRuleTests.cs

2ddcc51

Update RequiresPermissionValidationRuleTests.cs

78cf8d6

Reference NewtonSoft.Json

89556f6

sebastienros added 2 commits September 6, 2022 11:22

Fix build

b8eb3ba

Bump version to 4.8.0

8d9bc0a

sebastienros merged commit 6a69e25 into main Sep 8, 2022

sebastienros deleted the ag/graphql4 branch September 8, 2022 18:01

Skrypt removed the notready label Sep 10, 2022

Piedone mentioned this pull request Jan 16, 2024

Fix DataLoader concurrent issues #9769

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GraphQL 4 #9087

GraphQL 4 #9087

agriffard commented Apr 7, 2021 •

edited by carlwoodhouse

Loading

carlwoodhouse commented Apr 7, 2021 via email

carlwoodhouse left a comment

carlwoodhouse Apr 8, 2021

deanmarcussen Jan 14, 2022

carlwoodhouse commented Apr 9, 2021

carlwoodhouse commented Apr 9, 2021

This comment has been minimized.

carlwoodhouse commented Apr 9, 2021

agriffard commented Apr 9, 2021

carlwoodhouse commented Apr 9, 2021 via email

agriffard commented Jan 14, 2022

MikeKry commented Feb 10, 2022

sebastienros Feb 10, 2022

MikeKry commented Mar 9, 2022 •

edited

Loading

agriffard commented Apr 4, 2022

agriffard commented Apr 4, 2022

MikeKry commented Apr 5, 2022

MikeKry commented Apr 7, 2022

agriffard commented Apr 28, 2022

agriffard commented Sep 1, 2022

MikeKry commented Sep 3, 2022

Skrypt commented Sep 8, 2022

MikeKry commented Nov 16, 2022

GraphQL 4 #9087

GraphQL 4 #9087

Conversation

agriffard commented Apr 7, 2021 • edited by carlwoodhouse Loading

carlwoodhouse commented Apr 7, 2021 via email

carlwoodhouse left a comment

Choose a reason for hiding this comment

carlwoodhouse Apr 8, 2021

Choose a reason for hiding this comment

deanmarcussen Jan 14, 2022

Choose a reason for hiding this comment

carlwoodhouse commented Apr 9, 2021

carlwoodhouse commented Apr 9, 2021

This comment has been minimized.

carlwoodhouse commented Apr 9, 2021

agriffard commented Apr 9, 2021

carlwoodhouse commented Apr 9, 2021 via email

agriffard commented Jan 14, 2022

MikeKry commented Feb 10, 2022

sebastienros Feb 10, 2022

Choose a reason for hiding this comment

MikeKry commented Mar 9, 2022 • edited Loading

agriffard commented Apr 4, 2022

agriffard commented Apr 4, 2022

MikeKry commented Apr 5, 2022

MikeKry commented Apr 7, 2022

agriffard commented Apr 28, 2022

agriffard commented Sep 1, 2022

MikeKry commented Sep 3, 2022

Skrypt commented Sep 8, 2022

MikeKry commented Nov 16, 2022

agriffard commented Apr 7, 2021 •

edited by carlwoodhouse

Loading

MikeKry commented Mar 9, 2022 •

edited

Loading