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

[Fusion] Added pre-merge validation rule "LookupShouldHaveNullableReturnTypeRule" #7889

Merged
merged 5 commits into from
Jan 2, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ public static class LogEntryCodes
public const string KeyFieldsSelectInvalidType = "KEY_FIELDS_SELECT_INVALID_TYPE";
public const string KeyInvalidFields = "KEY_INVALID_FIELDS";
public const string KeyInvalidSyntax = "KEY_INVALID_SYNTAX";
public const string LookupShouldHaveNullableReturnType = "LOOKUP_SHOULD_HAVE_NULLABLE_RETURN_TYPE";
public const string OutputFieldTypesNotMergeable = "OUTPUT_FIELD_TYPES_NOT_MERGEABLE";
public const string ProvidesDirectiveInFieldsArg = "PROVIDES_DIRECTIVE_IN_FIELDS_ARG";
public const string ProvidesFieldsHasArgs = "PROVIDES_FIELDS_HAS_ARGS";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,25 @@ public static LogEntry KeyInvalidSyntax(
schema);
}

public static LogEntry LookupShouldHaveNullableReturnType(
OutputFieldDefinition field,
INamedTypeDefinition type,
SchemaDefinition schema)
{
var coordinate = new SchemaCoordinate(type.Name, field.Name);

return new LogEntry(
string.Format(
LogEntryHelper_LookupShouldHaveNullableReturnType,
coordinate,
schema.Name),
LogEntryCodes.LookupShouldHaveNullableReturnType,
LogSeverity.Warning,
coordinate,
field,
schema);
}

public static LogEntry OutputFieldTypesNotMergeable(
OutputFieldDefinition field,
string typeName,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
using HotChocolate.Fusion.Events;
using HotChocolate.Skimmed;
using static HotChocolate.Fusion.Logging.LogEntryHelper;

namespace HotChocolate.Fusion.PreMergeValidation.Rules;

/// <summary>
/// Fields annotated with the <c>@lookup</c> directive are intended to retrieve a single entity
/// based on provided arguments. To properly handle cases where the requested entity does not exist,
/// such fields should have a nullable return type. This allows the field to return null when an
/// entity matching the provided criteria is not found, following the standard GraphQL practices for
/// representing missing data.
/// </summary>
/// <seealso href="https://graphql.github.io/composite-schemas-spec/draft/#sec--lookup-Should-Have-Nullable-Return-Type">
/// Specification
/// </seealso>
internal sealed class LookupShouldHaveNullableReturnTypeRule : IEventHandler<OutputFieldEvent>
{
public void Handle(OutputFieldEvent @event, CompositionContext context)
{
var (field, type, schema) = @event;

if (ValidationHelper.IsLookup(field) && field.Type is NonNullTypeDefinition)
{
context.Log.Write(LookupShouldHaveNullableReturnType(field, type, schema));
}
}
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@
<data name="LogEntryHelper_KeyInvalidSyntax" xml:space="preserve">
<value>A @key directive on type '{0}' in schema '{1}' contains invalid syntax in the 'fields' argument.</value>
</data>
<data name="LogEntryHelper_LookupShouldHaveNullableReturnType" xml:space="preserve">
<value>The lookup field '{0}' in schema '{1}' should return a nullable type.</value>
</data>
<data name="LogEntryHelper_OutputFieldTypesNotMergeable" xml:space="preserve">
<value>Field '{0}' has a different type shape in schema '{1}' than it does in schema '{2}'.</value>
</data>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ private CompositionResult<SchemaDefinition> MergeSchemaDefinitions(CompositionCo
new KeyFieldsSelectInvalidTypeRule(),
new KeyInvalidFieldsRule(),
new KeyInvalidSyntaxRule(),
new LookupShouldHaveNullableReturnTypeRule(),
new OutputFieldTypesMergeableRule(),
new ProvidesDirectiveInFieldsArgumentRule(),
new ProvidesFieldsHasArgumentsRule(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ public static bool IsExternal(IDirectivesProvider type)
return type.Directives.ContainsName(WellKnownDirectiveNames.External);
}

public static bool IsLookup(IDirectivesProvider type)
{
return type.Directives.ContainsName(WellKnownDirectiveNames.Lookup);
}

/// <summary>
/// Returns <c>true</c> if the specified <paramref name="field"/> has a <c>@provides</c>
/// directive that references the specified <paramref name="fieldName"/>.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ internal static class WellKnownDirectiveNames
public const string External = "external";
public const string Inaccessible = "inaccessible";
public const string Key = "key";
public const string Lookup = "lookup";
public const string Provides = "provides";
public const string Require = "require";
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
using HotChocolate.Fusion.Logging;
using HotChocolate.Fusion.PreMergeValidation;
using HotChocolate.Fusion.PreMergeValidation.Rules;

namespace HotChocolate.Composition.PreMergeValidation.Rules;

public sealed class LookupShouldHaveNullableReturnTypeRuleTests : CompositionTestBase
{
private readonly PreMergeValidator _preMergeValidator =
new([new LookupShouldHaveNullableReturnTypeRule()]);

[Theory]
[MemberData(nameof(ValidExamplesData))]
public void Examples_Valid(string[] sdl)
{
// arrange
var context = CreateCompositionContext(sdl);

// act
var result = _preMergeValidator.Validate(context);

// assert
Assert.True(result.IsSuccess);
Assert.True(context.Log.IsEmpty);
}

[Theory]
[MemberData(nameof(InvalidExamplesData))]
public void Examples_Invalid(string[] sdl, string[] errorMessages)
{
// arrange
var context = CreateCompositionContext(sdl);

// act
var result = _preMergeValidator.Validate(context);

// assert
Assert.True(result.IsSuccess);
Assert.Equal(errorMessages, context.Log.Select(e => e.Message).ToArray());
Assert.True(context.Log.All(e => e.Code == "LOOKUP_SHOULD_HAVE_NULLABLE_RETURN_TYPE"));
Assert.True(context.Log.All(e => e.Severity == LogSeverity.Warning));
}

public static TheoryData<string[]> ValidExamplesData()
{
return new TheoryData<string[]>
{
// In this example, "userById" returns a nullable "User" type, aligning with the
// recommendation.
{
[
"""
type Query {
userById(id: ID!): User @lookup
}

type User {
id: ID!
name: String
}
"""
]
}
};
}

public static TheoryData<string[], string[]> InvalidExamplesData()
{
return new TheoryData<string[], string[]>
{
// Here, "userById" returns a non-nullable "User!", which does not align with the
// recommendation that a @lookup field should have a nullable return type.
{
[
"""
type Query {
userById(id: ID!): User! @lookup
}

type User {
id: ID!
name: String
}
"""
],
[
"The lookup field 'Query.userById' in schema 'A' should return a nullable type."
]
}
};
}
}
Loading