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

Fix DynamicContentTypeBuilder field double registration #12424

Conversation

tropcicstefan
Copy link
Contributor

@tropcicstefan tropcicstefan commented Sep 15, 2022

Fixes #12423

@tropcicstefan tropcicstefan marked this pull request as draft September 15, 2022 14:03
@tropcicstefan tropcicstefan reopened this Sep 15, 2022
Copy link
Member

@jtkech jtkech left a comment

Choose a reason for hiding this comment

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

LGTM

@jtkech jtkech marked this pull request as ready for review September 15, 2022 19:39
@tropcicstefan
Copy link
Contributor Author

After more debugging it turns out that current check isn't ok, because HasField is method that checks strings with StringComparison.Ordinal (GraphQL core method, allows for fields to have fields with same names but different casing) but Orchard uses OrchardFieldNameConverter in Schema definition which converts any field name in camelcase. So on refresh of Schema in runtime, there could occur same exception, of field with two definitions

So I'd argue to use ToCamelCase on FieldType.Name in both IContentFieldProvider, similar as we convert every part's name with ToFieldName to define some consistency of camelCasing field names in graphql

@jtkech
Copy link
Member

jtkech commented Sep 19, 2022

Okay so need someone that better know GraphQl than me ;)

@carlwoodhouse ?

@tropcicstefan
Copy link
Contributor Author

Any feedback on this? I know @carlwoodhouse made graphql modules and it's his child 😀 but I cannot notice he is short on time for orchard lately. Is there chance for someone else to take ownership over graphql because upgrading graphql also took a lot of time?

@hishamco
Copy link
Member

/cc @hyzx86

@hyzx86
Copy link
Contributor

hyzx86 commented Jan 18, 2023

@tropcicstefan
How are your types and parts defined?
And your Implementation of IContentFieldProvider?

@tropcicstefan
Copy link
Contributor Author

tropcicstefan commented Jan 18, 2023

    public class ArticlePart : ContentPart
    {
        public TextField SuperTitle { get; set; }
        ...
    }

 public class ArticleObjectType : ObjectGraphType<ArticlePart>
    {
        public ArticleObjectType(IStringLocalizer<ArticleObjectType> S)
        {
            Name = nameof(ArticlePart);
            Description = S["Alternative part definition"];

            Field<StringGraphType, string>()
                .Name(nameof(ArticlePart).ToCamelCase())
                .Description("default field")
                .Resolve(x =>
                {
                    return nameof(ArticlePart);
                });

            Field<StringGraphType, string>()
                .Name(nameof(ArticlePart.SuperTitle).ToCamelCase())
                .Description("short description")
                .Resolve(x =>
                {
                    return x.Source.SuperTitle.Text;
                });
           ...
         }
 }

I have something like this with multiple fields. And no custom IContentFieldProvider.

Object has been built with TypedContentTypeBuilder and DynamicContentTypeBuilder was prevented with the 'default field' with the name of articlePart until #8669 where this line was deleted if (contentItemType.HasField(partName)) continue;

But issue is if OrchardFieldNameConverter uses default camel case then field providers should too

@hyzx86
Copy link
Contributor

hyzx86 commented Jan 18, 2023

Have you tried implementing ISchemaBuilder?
It can modify registered types

Just like this :

I rewrote the Lucene query schema to return statistics
It will be executed after building the schema for the Lucene query at OC

using EasyOC.GraphQL.Queries.Types;
using GraphQL;
using GraphQL.Types;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
using Newtonsoft.Json;
using Newtonsoft.Json.Linq;
using OrchardCore.Apis.GraphQL;
using OrchardCore.Apis.GraphQL.Resolvers;
using OrchardCore.ContentManagement;
using OrchardCore.ContentManagement.GraphQL.Queries;
using OrchardCore.Search.Lucene;
using OrchardCore.Queries;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using LuceneQueryResults = OrchardCore.Search.Lucene.LuceneQueryResults;

namespace EasyOC.GraphQL.Queries
{
    public class EOCLuceneQueryFieldTypeProvider : ISchemaBuilder
    {
        private readonly IHttpContextAccessor _httpContextAccessor;
        private readonly ILogger<EOCLuceneQueryFieldTypeProvider> _logger;

        public EOCLuceneQueryFieldTypeProvider(IHttpContextAccessor httpContextAccessor,
            ILogger<EOCLuceneQueryFieldTypeProvider> logger)
        {
            _httpContextAccessor = httpContextAccessor;
            _logger = logger;
        }

        public Task<string> GetIdentifierAsync()
        {
            var queryManager = _httpContextAccessor.HttpContext.RequestServices.GetService<IQueryManager>();
            return queryManager.GetIdentifierAsync();
        }

        public async Task BuildAsync(ISchema schema)
        {

            var queryManager = _httpContextAccessor.HttpContext.RequestServices.GetService<IQueryManager>();

            var queries = await queryManager.ListQueriesAsync();

            foreach (var query in queries.OfType<LuceneQuery>())
            {
                if (string.IsNullOrWhiteSpace(query.Schema))
                    continue;

                var name = query.Name;

                try
                {
                    var querySchema = JObject.Parse(query.Schema);
                    if (!querySchema.ContainsKey("type"))
                    {
                        _logger.LogError("The Query '{Name}' schema is invalid, the 'type' property was not found.",
                            name);
                        continue;
                    }

                    var type = querySchema["type"].ToString();


                    if (querySchema.ContainsKey("hasTotal") && querySchema["hasTotal"].ToString()
                            .Equals("true", StringComparison.OrdinalIgnoreCase))
                    {
                        var fieldTypeName = querySchema["fieldTypeName"]?.ToString() ?? query.Name;
                        FieldType fieldType = schema.Query.GetField(fieldTypeName);
                        if (fieldType == null)
                        {
                            continue;
                        }

                        if (query.ReturnContentItems &&
                            type.StartsWith("ContentItem/", StringComparison.OrdinalIgnoreCase))
                        {
                            var contentType = type.Remove(0, 12);
                            BuildTotalContentTypeFieldType(fieldType, schema, contentType, query, fieldTypeName);
                        }
                        else
                        {
                            BuildTotalSchemaBasedFieldType(fieldType, query, querySchema, fieldTypeName);
                        }
                    }
                }
                catch (Exception e)
                {
                    _logger.LogError(e, "The Query '{Name}' has an invalid schema.", name);
                }
            }
        }

        private FieldType BuildTotalSchemaBasedFieldType(FieldType fieldType, LuceneQuery query, JToken querySchema,
            string fieldTypeName)
        {
            var properties = querySchema["properties"];
            if (properties == null)
            {
                return null;
            }

            var totalType = new ObjectGraphType<TotalQueryResults>() { Name = fieldTypeName };

            var typetype = new ObjectGraphType<JObject> { Name = fieldTypeName };
            var listType = new ListGraphType(typetype);

            totalType.Field(listType.GetType(), "items",
                resolve: context =>
                {
                    return context.Source?.Items;
                });
            var total = totalType.Field<IntGraphType>("total",
                resolve: context =>
                {
                    return context.Source?.Total;
                });


            foreach (JProperty child in properties.Children())
            {
                var name = child.Name;
                var nameLower = name.Replace('.', '_');
                var type = child.Value["type"].ToString();
                var description = child.Value["description"]?.ToString();

                if (type == "string")
                {
                    var field = typetype.Field(
                        typeof(StringGraphType),
                        nameLower,
                        description: description,
                        resolve: context =>
                        {
                            var source = context.Source;
                            return source[context.FieldDefinition.Metadata["Name"].ToString()].ToObject<string>();
                        });
                    field.Metadata.Add("Name", name);
                }
                else if (type == "integer")
                {
                    var field = typetype.Field(
                        typeof(IntGraphType),
                        nameLower,
                        description: description,
                        resolve: context =>
                        {
                            var source = context.Source;
                            return source[context.FieldDefinition.Metadata["Name"].ToString()].ToObject<int>();
                        });
                    field.Metadata.Add("Name", name);
                }
            }

            fieldType.Arguments = new QueryArguments(
                new QueryArgument<StringGraphType> { Name = "parameters" }
            );
            fieldType.Name = fieldTypeName;
            fieldType.Description = "Represents the " + query.Source + " Query : " + query.Name;
            fieldType.ResolvedType = totalType;
            fieldType.Resolver = new LockedAsyncFieldResolver<object, object>(async context =>
            {
                var queryManager = context.RequestServices.GetService<IQueryManager>();
                var iquery = await queryManager.GetQueryAsync(query.Name);

                var parameters = context.GetArgument<string>("parameters");

                var queryParameters = parameters != null
                    ? JsonConvert.DeserializeObject<Dictionary<string, object>>(parameters)
                    : new Dictionary<string, object>();

                var result = await queryManager.ExecuteQueryAsync(iquery, queryParameters) as LuceneQueryResults;
                return result;
            });
            fieldType.Type = totalType.GetType();

            return fieldType;
        }


        private FieldType BuildTotalContentTypeFieldType(FieldType fieldType, ISchema schema, string contentType,
            LuceneQuery query, string fieldTypeName)
        {
            var typetype = schema.Query.Fields.OfType<ContentItemsFieldType>()
                .FirstOrDefault(x => x.Name == contentType);
            if (typetype == null)
            {
                return null;
            }

            var totalType = new ObjectGraphType<TotalQueryResults> { Name = fieldTypeName };

            var items = totalType.Field(typetype.Type, "items",
                resolve: context =>
                {
                    return context.Source?.Items ?? Array.Empty<ContentItem>();
                });
            items.ResolvedType = typetype.ResolvedType;
            totalType.Field<IntGraphType>("total",
                resolve: context =>
                {
                    return context.Source?.Total ?? 0;
                });

            fieldType.Arguments = new QueryArguments(
                new QueryArgument<StringGraphType> { Name = "parameters" });

            fieldType.Name = fieldTypeName;
            fieldType.Description = "Represents the " + query.Source + " Query : " + query.Name;
            fieldType.ResolvedType = totalType;
            fieldType.Resolver = new LockedAsyncFieldResolver<object, object>(async context =>
            {
                var queryManager = context.RequestServices.GetService<IQueryManager>();
                var iquery = await queryManager.GetQueryAsync(query.Name);

                var parameters = context.GetArgument<string>("parameters");

                var queryParameters = parameters != null
                    ? JsonConvert.DeserializeObject<Dictionary<string, object>>(parameters)
                    : new Dictionary<string, object>();
                var result = await queryManager.ExecuteQueryAsync(iquery, queryParameters);

                return new TotalQueryResults
                {
                    Total = (result as LuceneQueryResults)?.Count, Items = result?.Items ?? Array.Empty<ContentItem>()
                };
            });
            fieldType.Type = totalType.GetType();

            return fieldType;
        }
    }
}

@hyzx86
Copy link
Contributor

hyzx86 commented Jan 18, 2023

You can try to query your custom field this way and then update it.

  var typetype = schema.Query.Fields.OfType<ContentItemsFieldType>()
                .FirstOrDefault(x => x.Name == contentType);

@tropcicstefan
Copy link
Contributor Author

If this PR is a no go I think it's cleaner to go with custom IContentTypeBuilder to have some out of the box features as default ContentItem filters, permissions and possibility to add typed index filters.
Thx

@hyzx86
Copy link
Contributor

hyzx86 commented Jan 19, 2023

@tropcicstefan

I don't think there's anything wrong with this PR, after all, line 90 below does the same thing, right

foreach (var fieldProvider in contentFieldProviders)
{
var fieldType = fieldProvider.GetField(field);
if (fieldType != null)
{
if (_contentOptions.ShouldSkip(fieldType.Type, fieldType.Name))
{
continue;
}
contentItemType.AddField(fieldType);
break;
}
}
}
}
else
{
// Check if another builder has already added a field for this part.
var existingField = contentItemType.GetField(partName.ToFieldName());
if (existingField != null)
{
// Add content field types.
foreach (var field in part.PartDefinition.Fields)
{
foreach (var fieldProvider in contentFieldProviders)
{
var contentFieldType = fieldProvider.GetField(field);
if (contentFieldType != null && !contentItemType.HasField(contentFieldType.Name))
{
contentItemType.AddField(contentFieldType);
break;

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.

A field with the name: XX is already registered for GraphType: YY
4 participants