Upgrade GraphQL to 8.2 #16736
Upgrade GraphQL to 8.2 #16736
Conversation
|
@hyzx86 you upgraded GraphQL package last time. You may be more familiar with this. Can you please check this out? |
|
IMHO we need to stop bumping the packages ourselves like you said before until the Dependabot is fixed |
The Dependabot is fixed. But it can't do breaking changes like this one. These will need to always be done manually. |
|
I thought it was straightforward Anyhow, to try to fix the broken unit tests. @hyzx86 your help appreciated |
|
This problem seems to be related to dynamically generated filtering conditions. Sorry, I'm busy with other projects recently. It's at a critical stage, and I may not have time to deal with this problem for some time. |
|
Thanks @hyzx86 @MikeAlhayek can you handle this? Or we might need help from @carlwoodhouse or @mdameer who worked recently on some GraphQL PRs |
…chardCore into ma/upgeade-graphql
|
This is going to be a breaking change and should not be part of 2.x. We should wait until 3 before doing this as we have to set |
|
So, let us defer this PR then |
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
|
@sebastienros this is ready |
|
The build is still fail |
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
There was a problem hiding this comment.
Nice work, thanks @MikeAlhayek. I've added some comments, but I've not fully tested this yet.
| // Clean Query, Mutation and Subscription if they have no fields | ||
| // to prevent GraphQL configuration errors. | ||
|
|
||
| if (schema.Query.Fields.Count == 0) | ||
| if (schema.Query?.Fields is null || schema.Query.Fields.Count == 0) |
There was a problem hiding this comment.
Query, Mutation and Subscription can't be null here, they are all set above.
| Description = fieldDescriptor.Description, | ||
| Type = fieldDescriptor.FieldType, | ||
| ResolvedType = fieldDescriptor.ResolvedType, |
There was a problem hiding this comment.
If I understand the changes here correctly, we can get rid of the assignment Type = FieldType. The type has been replaced by ResolvedType because the fields are added during schema initialization. Is that assumption correct?
| public Func<ContentElement, object> FieldAccessor { get; set; } | ||
| public string Index { get; set; } | ||
| public Type IndexType { get; set; } | ||
| } |
There was a problem hiding this comment.
I would prefer to leave this class here as a nested type, because it is an implementation detail of the ContentFieldsProvider only.
| private readonly GraphQLContentOptions _graphQLContentOptions; | ||
|
|
||
| internal IStringLocalizer S; |
There was a problem hiding this comment.
Can't these be private, or does the PoExtractor depend on those fields being internal?
| @@ -117,6 +117,7 @@ private static FieldType BuildSchemaBasedFieldType(Query query, JsonNode querySc | |||
| Name = nameLower, | |||
| Description = description, | |||
| Type = typeof(StringGraphType), | |||
| ResolvedType = new StringGraphType(), | |||
There was a problem hiding this comment.
This should not be necessary in an ISchemaBuilder. Using the resolved type is only mandatory if we are adding fields later on. See https://graphql-dotnet.github.io/docs/migrations/migration8/#30-code-classlanguage-textgraphtypeinitializecode-method-is-now-called-after-initialization-is-complete
This applies to the other occurrences as well.
|
|
||
| private readonly Dictionary<Type, IGraphType> graphTypes = new(); | ||
|
|
||
| private IGraphType CreateGraphType(Type type) |
There was a problem hiding this comment.
Please double check if we absolutely need to instantiate all of the types here as well. It actually depends on when we are adding those fields.
There was a problem hiding this comment.
Yes. This is part of the transition from v7 to v8. There are Schema validation that happens, and because we create the dynamic field in the Initialize method instead of the constructor, we get errors like this
The field 'Amount' of an Input Object type 'PricePartWhereInput' must have non-null 'ResolvedType' property for all types in the chain.
You may want to try to not set the ResolvedType and see the error.
| @@ -34,9 +34,6 @@ public override void Initialize(ISchema schema) | |||
| } | |||
| } | |||
|
|
|||
| // Part is not required here anymore, do not keep it alive. | |||
| _part = null; | |||
There was a problem hiding this comment.
There is no need to keep this referenced. The schema is a singleton, which effectively keeps that reference alive forever.
There was a problem hiding this comment.
Where is it being registered as singleton? The DynamicPartGraphType is instantiated from DynamicContentTypeBuilder. I don't think it lives for a long time.
There was a problem hiding this comment.
The graph types are kept alive in the schema, IIRC.
There was a problem hiding this comment.
Not sure. SchemaService is a singleton and hold ISchema instances. But part is not something we keep in the Schema. I could be wrong, but please double check before we add it back.
Fixes #16826.