Edit Content Part, Add New Field, Kills GraphQL

[kef7]

Adding "OpenInNewWindow" Bool field to LinkMenuItemPart from Orchard Core Admin breaks GraphQL. (I believe the content part definition for LinkMenuItemPart came from The Agency theme.)

• Accessing /api/graphql gives 400 Bad Request

• GraphQL admin area shows "No Schema" message; can't execute queries

• IIS error in Windows Event Log:
  Category: Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware
  EventId: 1
  RequestId: 80000d14-0400-f900-b63f-84710c7967bb
  RequestPath: /api/graphql
  SpanId: |c69cdb0c-44c6865f99a2049c.
  TraceId: c69cdb0c-44c6865f99a2049c
  ParentId:

An unhandled exception has occurred while executing the request.

Exception:
System.ArgumentOutOfRangeException: A field with the name: linkMenuItem is already registered for GraphType: LinkMenuItem (Parameter 'Name')
at GraphQL.Types.ComplexGraphType`1.AddField(FieldType fieldType)
at OrchardCore.ContentManagement.GraphQL.Queries.Types.DynamicContentTypeBuilder.Build(FieldType contentQuery, ContentTypeDefinition contentTypeDefinition, ContentItemType contentItemType) in C:\projects\orchardcore\src\OrchardCore\OrchardCore.ContentManagement.GraphQL\Queries\Types\DynamicContentTypeBuilder.cs:line 65
at OrchardCore.ContentManagement.GraphQL.Queries.ContentTypeQuery.BuildAsync(ISchema schema) in C:\projects\orchardcore\src\OrchardCore\OrchardCore.ContentManagement.GraphQL\Queries\ContentTypeQuery.cs:line 63
at OrchardCore.Apis.GraphQL.Services.SchemaService.GetSchemaAsync() in C:\projects\orchardcore\src\OrchardCore.Modules\OrchardCore.Apis.GraphQL\Services\SchemaService.cs:line 56
at OrchardCore.Apis.GraphQL.GraphQLMiddleware.ExecuteAsync(HttpContext context, ISchemaFactory schemaService) in C:\projects\orchardcore\src\OrchardCore.Modules\OrchardCore.Apis.GraphQL\GraphQLMiddleware.cs:line 79
at OrchardCore.Apis.GraphQL.GraphQLMiddleware.Invoke(HttpContext context, IAuthorizationService authorizationService, IAuthenticationService authenticationService, ISchemaFactory schemaService) in C:\projects\orchardcore\src\OrchardCore.Modules\OrchardCore.Apis.GraphQL\GraphQLMiddleware.cs:line 70
at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
at OrchardCore.Diagnostics.DiagnosticsStartupFilter.<>c__DisplayClass3_0.<b__1>d.MoveNext() in C:\projects\orchardcore\src\OrchardCore.Modules\OrchardCore.Diagnostics\DiagnosticsStartupFilter.cs:line 47
--- End of stack trace from previous location where exception was thrown ---
at Microsoft.AspNetCore.Diagnostics.StatusCodePagesMiddleware.Invoke(HttpContext context)
at Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware.g__Awaited|6_0(ExceptionHandlerMiddleware middleware, HttpContext context, Task task)

Fixed by removing the added ContentPartFieldDefinitonRecord from the LinkMenuItemPart item under ContentPartDefinitionRecords in file ContentDefinition.json. It's possible there are in fact two definitions for LinkMenuItemPart and they are conflicting; not sure how to check this accurately.

[jtkech]

Okay, i can repro, i will dig into it asap

[jtkech]

So, it doesn't happen if you add a field to a content type, but it happens for any field that you add dynamically to any part.

I think it is because of DynamicContentTypeBuilder that allows to take into account a part that you create dynamically through the admin but also when you add a field to an existing part.

I'm working on it

[jtkech]

@carlwoodhouse or another GraphQL expert

So, if we add a field to an existing part (e.g a TextField to the HtmlBodyPart) that is used in a content type, we get the above issue saying that the GraphQL element is already registered. This because in DynamicContentTypeBuilder the related element is already registered by TypedContentTypeBuilder.

So, in this case i thought about forcing the collapsing of fields, so just to show what i needed to prevent it from failing, in DynamicContentTypeBuilder i added for testing the following check

            var test = contentItemType.GetField(partName.ToFieldName());

            if (_contentOptions.ShouldCollapse(part) || test != null)
            {
                 ... 

I also noticed that in both builders we check

            // Check if another builder has already added a field for this part.
            if (contentItemType.HasField(partName)) continue; // Never true

Just for confirmation because it is never true, should we use partName.ToFieldName() as used for registering? Notice that if i use partName.ToFieldName() it doesn't fail because the HtmlBody is not registered twice but then the TextField that has been added dynamically is not resolved.

[carlwoodhouse]

heh, for sure that check is wrong ... but probably what we should be doing is checking if it has the field already, if it does then just add the dynamic fields to the existing field.

[carlwoodhouse]

if you don't have time to work on it @jtkech i can take it if you like

[jtkech]

@carlwoodhouse

Yes thanks, it was just to show my findings but you are better placed to fix it in the right way

[Fermain]

@carlwoodhouse Have you made any headway with this?

[carlwoodhouse]

I looked into it a bit but I need to pick it back up. There’s an easy fix (as suggested here; the check for did I already build this part in another builder is checking the wrong case of the field name; so it re-adds it; this is technically what breaks it ..) so the simplest fix is to check the right case. But this isn’t actually ideal; as you’d never be able to mix types and dynamic parts; you’d only ever get the typed definition .. so we actually need to merge them. I’ll try take a proper look on Friday when I have some time. Carl.

…

On Tue, 19 May 2020 at 19:24, Fermain ***@***.***> wrote: @carlwoodhouse <https://github.com/carlwoodhouse> Have you made any headway with this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5536 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADRUEBJIKPXTDJNUQBD4ALRSLFFJANCNFSM4KUFLEEA> .

[hyzx86]

[hyzx86]

I have created a pull request , #6477

[agriffard]

Has it been fixed by #6536 ?

[carlwoodhouse]

Not as far as I can see Antoine

…

On Thu, 13 Aug 2020 at 11:00, Antoine Griffard ***@***.***> wrote: Has it been fixed by #6536 <#6536> ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5536 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADRUEFDN62XMNPMNHE4WSDSAO2SXANCNFSM4KUFLEEA> .

[agriffard]

@carlwoodhouse Can you see a way to fix the issue?

[carlwoodhouse]

Will try and pick it up / been manic at work; the issue is when a dynamic field name clashes with a static one; it’s supposed to ignore the static one but the case is wrong; that’s not a great fix though ... really we need to add them with unique names or so

…

On Thu, 28 Jan 2021 at 21:48, Antoine Griffard ***@***.***> wrote: @carlwoodhouse <https://github.com/carlwoodhouse> Can you see a way to fix the issue? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5536 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADRUEEIN7CI6D3IQ4WQKTDS4HLRJANCNFSM4KUFLEEA> .