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.

Sign up for GitHub

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

Jump to bottom

Fixes several minor issues when building the GraphQL schema #16151

Merged
merged 23 commits into from
Jun 7, 2024
Merged

Fixes several minor issues when building the GraphQL schema #16151

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
dac997c
Fixes several minor issues when building the GraphQL schema.
gvkries May 24, 2024
90792ba
Merge branch 'main' into gvkries/graphql-fixes
gvkries May 28, 2024
325bf2c
Merge branch 'main' into gvkries/graphql-fixes
hishamco May 30, 2024
fe2f394
Correctly add fields to existing GraphQL types for a content part.
gvkries May 30, 2024
b0a2b05
Merge branch 'main' into gvkries/graphql-fixes
gvkries May 30, 2024
b5d5b8b
Merge branch 'main' into gvkries/graphql-fixes
gvkries May 31, 2024
46e47c7
Fix merge error.
gvkries May 31, 2024
b5e8f2f
Added a note about possible breaking changes to the GraphQL schema.
gvkries May 31, 2024
610bec0
Remove empty lines.
gvkries May 31, 2024
121875c
Added extension method for finding fields with ignore-case.
gvkries May 31, 2024
92410f6
Merge branch 'main' into gvkries/graphql-fixes
gvkries Jun 2, 2024
e8fd17d
Remove redundant comment.
gvkries Jun 2, 2024
9d826ac
Merge branch 'main' into gvkries/graphql-fixes
gvkries Jun 4, 2024
df29362
Remove comments.
gvkries Jun 4, 2024
2f40a25
Added documentation to HasFieldIgnoreCase().
gvkries Jun 5, 2024
f80337d
Update src/docs/releases/2.0.0.md
gvkries Jun 5, 2024
c11d3fb
Merge branch 'main' into gvkries/graphql-fixes
gvkries Jun 5, 2024
a9d5349
Update src/OrchardCore/OrchardCore.ContentManagement.GraphQL/Queries/…
gvkries Jun 6, 2024
edae2ee
Merge branch 'main' into gvkries/graphql-fixes
gvkries Jun 6, 2024
749d9e5
Combine if statements.
gvkries Jun 6, 2024
873db76
Merge branch 'main' into gvkries/graphql-fixes
hishamco Jun 6, 2024
c45c3d5
Merge branch 'main' into gvkries/graphql-fixes
hishamco Jun 7, 2024
780407f
Update src/docs/releases/2.0.0.md
hishamco Jun 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion ...GraphQL/Extensions/FieldTypeExtensions.cs → ...aphQL/Extensions/GraphQLTypeExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,20 @@
using System;
using System.Linq;
using GraphQL.Types;

namespace OrchardCore.ContentManagement.GraphQL;

public static class FieldTypeExtensions
public static class GraphQLTypeExtensions
hishamco marked this conversation as resolved.
Show resolved Hide resolved
{
public static FieldType WithPartCollapsedMetaData(this FieldType fieldType, bool collapsed = true)
=> fieldType.WithMetaData("PartCollapsed", collapsed);

public static FieldType WithPartNameMetaData(this FieldType fieldType, string partName)
=> fieldType.WithMetaData("PartName", partName);

public static bool HasFieldIgnoreCase(this IComplexGraphType graphType, string fieldName)
=> graphType.Fields.Any(field => field.Name.Equals(fieldName, StringComparison.OrdinalIgnoreCase));

private static FieldType WithMetaData(this FieldType fieldType, string name, object value)
{
// TODO: Understand if locking is the best solution to https://github.com/OrchardCMS/OrchardCore/issues/15308
Expand Down
81 changes: 54 additions & 27 deletions ...hardCore/OrchardCore.ContentManagement.GraphQL/Queries/Types/DynamicContentTypeBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System;
using System.Collections.Generic;
using System.Linq;
using GraphQL.Types;
Expand Down Expand Up @@ -75,6 +76,11 @@ public void Build(ISchema schema, FieldType contentQuery, ContentTypeDefinition
continue;
}

if (contentItemType.HasFieldIgnoreCase(fieldType.Name))
{
continue;
}

contentItemType.AddField(fieldType);
break;
}
Expand All @@ -84,45 +90,66 @@ public void Build(ISchema schema, FieldType contentQuery, ContentTypeDefinition
else
{
// Check if another builder has already added a field for this part.
var existingField = contentItemType.GetField(partName.ToFieldName());
if (existingField != null)
var partFieldName = partName.ToFieldName();
gvkries marked this conversation as resolved.
Show resolved Hide resolved
var partFieldType = contentItemType.GetField(partFieldName);

if (partFieldType != null)
{
// Add content field types.
foreach (var field in part.PartDefinition.Fields)
// Add dynamic content field types to the static part type.
var partContentItemType = schema.AdditionalTypeInstances
.OfType<IObjectGraphType>()
.Where(type => type.GetType() == partFieldType.Type)
.FirstOrDefault();

if (partContentItemType != null)
{
foreach (var fieldProvider in contentFieldProviders)
foreach (var field in part.PartDefinition.Fields)
{
var contentFieldType = fieldProvider.GetField(schema, field, part.Name);

if (contentFieldType != null && !contentItemType.HasField(contentFieldType.Name))
foreach (var fieldProvider in contentFieldProviders)
{
contentItemType.AddField(contentFieldType);
break;
var contentFieldType = fieldProvider.GetField(schema, field, part.Name);

if (contentFieldType != null)
{
if (_contentOptions.ShouldSkip(contentFieldType.Type, contentFieldType.Name))
{
continue;
}

if (partContentItemType.HasFieldIgnoreCase(contentFieldType.Name))
{
continue;
}
gvkries marked this conversation as resolved.
Show resolved Hide resolved

partContentItemType.AddField(contentFieldType);
break;
}
}
}
}
continue;
}

if (_dynamicPartFields.TryGetValue(partName, out var fieldType))
{
contentItemType.AddField(fieldType);
}
else
{
var field = contentItemType
.Field<DynamicPartGraphType>(partName.ToFieldName())
.Description(S["Represents a {0}.", part.PartDefinition.Name])
.Resolve(context =>
{
var nameToResolve = partName;
var typeToResolve = context.FieldDefinition.ResolvedType.GetType().BaseType.GetGenericArguments().First();
if (_dynamicPartFields.TryGetValue(partName, out var fieldType))
{
contentItemType.AddField(fieldType);
}
else
{
var field = contentItemType
.Field<DynamicPartGraphType>(partFieldName)
.Description(S["Represents a {0}.", part.PartDefinition.Name])
.Resolve(context =>
{
var nameToResolve = partName;
var typeToResolve = context.FieldDefinition.ResolvedType.GetType().BaseType.GetGenericArguments().First();

return context.Source.Get(typeToResolve, nameToResolve);
});
return context.Source.Get(typeToResolve, nameToResolve);
});

field.Type(new DynamicPartGraphType(part));
_dynamicPartFields[partName] = field.FieldType;
field.Type(new DynamicPartGraphType(part));
_dynamicPartFields[partName] = field.FieldType;
}
}
}
}
Expand Down
15 changes: 11 additions & 4 deletions ...rchardCore/OrchardCore.ContentManagement.GraphQL/Queries/Types/TypedContentTypeBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System;
using System.Linq;
using GraphQL;
using GraphQL.Resolvers;
Expand Down Expand Up @@ -40,9 +41,10 @@ public void Build(ISchema schema, FieldType contentQuery, ContentTypeDefinition
}

var partName = part.Name;
var partFieldName = partName.ToFieldName();

// Check if another builder has already added a field for this part.
if (contentItemType.HasField(partName))
if (contentItemType.HasField(partFieldName))
{
continue;
}
Expand All @@ -63,6 +65,11 @@ public void Build(ISchema schema, FieldType contentQuery, ContentTypeDefinition
continue;
}

if (contentItemType.HasFieldIgnoreCase(field.Name))
{
continue;
}

var partType = typeActivator.GetTypeActivator(part.PartDefinition.Name).Type;
var rolledUpField = new FieldType
{
Expand Down Expand Up @@ -94,11 +101,11 @@ public void Build(ISchema schema, FieldType contentQuery, ContentTypeDefinition
{
var field = new FieldType
{
Name = partName.ToFieldName(),
Name = partFieldName,
Type = queryGraphType.GetType(),
Description = queryGraphType.Description,
};
contentItemType.Field(partName.ToFieldName(), queryGraphType.GetType())
contentItemType.Field(partFieldName, queryGraphType.GetType())
.Description(queryGraphType.Description)
.Resolve(context =>
{
Expand Down Expand Up @@ -135,7 +142,7 @@ public void Build(ISchema schema, FieldType contentQuery, ContentTypeDefinition
whereInput.AddField(new FieldType
{
Type = inputGraphTypeResolved.GetType(),
Name = partName.ToFieldName(),
Name = partFieldName,
Description = inputGraphTypeResolved.Description
}.WithPartNameMetaData(partName));
}
Expand Down
5 changes: 5 additions & 0 deletions src/docs/releases/2.0.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,11 @@ Here are the updated signatures:

These adjustments ensure compatibility and adherence to the latest conventions within the `SectionDisplayDriver` class.

### GraphQL Module

The GraphQL schema may change, because fields are now always added to the correct part. Previously, additional fields may have been added to the parent content item type directly.
Copy link
Member

Choose a reason for hiding this comment

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

additional fields may have been added to the parent content item type directly

Not sure what it means, but having content type fields attached to the dynamic part (the implicit one matching the type name) was by design. Does it mean that an Article type will have to access an ArticlePart property in graphql to get the fields?

Copy link
Contributor Author

@gvkries gvkries Jun 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This only affects fields that have been added to a statically typed part. E.g. if you add fields to the HtmlBodyPart they were added to the content type directly, which will have failed often because of duplicated field names. The dynamic part itself has not been changed and also collapsing fields to the parent content item type is left unchanged as well. So the schema will only change in rare cases, not commonly.
This is fixing bug #14592.

gvkries marked this conversation as resolved.
Show resolved Hide resolved
You may have to adjust your GraphQL queries in that case.
hishamco marked this conversation as resolved.
Show resolved Hide resolved

## Change Logs

### Azure AI Search Module
Expand Down