From 7df44ba0d917670f127abe17cb4f56124de0e719 Mon Sep 17 00:00:00 2001 From: Daniel Reynolds <55194784+danielreynolds1@users.noreply.github.com> Date: Thu, 2 Jan 2025 12:28:17 +0000 Subject: [PATCH] [Fusion] Added pre-merge validation rule "LookupMustNotReturnListRule" (#7890) --- .../Logging/LogEntryCodes.cs | 1 + .../Logging/LogEntryHelper.cs | 19 +++ .../Rules/LookupMustNotReturnListRule.cs | 27 +++++ .../CompositionResources.Designer.cs | 9 ++ .../Properties/CompositionResources.resx | 3 + .../Fusion.Composition/SourceSchemaMerger.cs | 1 + .../Rules/LookupMustNotReturnListRuleTests.cs | 109 ++++++++++++++++++ 7 files changed, 169 insertions(+) create mode 100644 src/HotChocolate/Fusion-vnext/src/Fusion.Composition/PreMergeValidation/Rules/LookupMustNotReturnListRule.cs create mode 100644 src/HotChocolate/Fusion-vnext/test/Fusion.Composition.Tests/PreMergeValidation/Rules/LookupMustNotReturnListRuleTests.cs diff --git a/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/Logging/LogEntryCodes.cs b/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/Logging/LogEntryCodes.cs index d718bd55970..41131149f44 100644 --- a/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/Logging/LogEntryCodes.cs +++ b/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/Logging/LogEntryCodes.cs @@ -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 LookupMustNotReturnList = "LOOKUP_MUST_NOT_RETURN_LIST"; 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"; diff --git a/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/Logging/LogEntryHelper.cs b/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/Logging/LogEntryHelper.cs index 0bc8eed19f1..d3ab4feeed9 100644 --- a/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/Logging/LogEntryHelper.cs +++ b/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/Logging/LogEntryHelper.cs @@ -257,6 +257,25 @@ public static LogEntry KeyInvalidSyntax( schema); } + public static LogEntry LookupMustNotReturnList( + OutputFieldDefinition field, + INamedTypeDefinition type, + SchemaDefinition schema) + { + var coordinate = new SchemaCoordinate(type.Name, field.Name); + + return new LogEntry( + string.Format( + LogEntryHelper_LookupMustNotReturnList, + coordinate, + schema.Name), + LogEntryCodes.LookupMustNotReturnList, + LogSeverity.Error, + coordinate, + field, + schema); + } + public static LogEntry LookupShouldHaveNullableReturnType( OutputFieldDefinition field, INamedTypeDefinition type, diff --git a/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/PreMergeValidation/Rules/LookupMustNotReturnListRule.cs b/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/PreMergeValidation/Rules/LookupMustNotReturnListRule.cs new file mode 100644 index 00000000000..67bb6f79563 --- /dev/null +++ b/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/PreMergeValidation/Rules/LookupMustNotReturnListRule.cs @@ -0,0 +1,27 @@ +using HotChocolate.Fusion.Events; +using HotChocolate.Skimmed; +using static HotChocolate.Fusion.Logging.LogEntryHelper; + +namespace HotChocolate.Fusion.PreMergeValidation.Rules; + +/// +/// Fields annotated with the @lookup directive are intended to retrieve a single entity +/// based on provided arguments. To avoid ambiguity in entity resolution, such fields must return a +/// single object and not a list. This validation rule enforces that any field annotated with +/// @lookup must have a return type that is NOT a list. +/// +/// +/// Specification +/// +internal sealed class LookupMustNotReturnListRule : IEventHandler +{ + public void Handle(OutputFieldEvent @event, CompositionContext context) + { + var (field, type, schema) = @event; + + if (ValidationHelper.IsLookup(field) && field.Type.NullableType() is ListTypeDefinition) + { + context.Log.Write(LookupMustNotReturnList(field, type, schema)); + } + } +} diff --git a/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/Properties/CompositionResources.Designer.cs b/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/Properties/CompositionResources.Designer.cs index bc038610c89..6957e88c8b0 100644 --- a/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/Properties/CompositionResources.Designer.cs +++ b/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/Properties/CompositionResources.Designer.cs @@ -194,6 +194,15 @@ internal static string LogEntryHelper_KeyInvalidSyntax { } } + /// + /// Looks up a localized string similar to The lookup field '{0}' in schema '{1}' must not return a list.. + /// + internal static string LogEntryHelper_LookupMustNotReturnList { + get { + return ResourceManager.GetString("LogEntryHelper_LookupMustNotReturnList", resourceCulture); + } + } + /// /// Looks up a localized string similar to The lookup field '{0}' in schema '{1}' should return a nullable type.. /// diff --git a/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/Properties/CompositionResources.resx b/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/Properties/CompositionResources.resx index fe4221a64d2..0cf256d5b5e 100644 --- a/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/Properties/CompositionResources.resx +++ b/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/Properties/CompositionResources.resx @@ -63,6 +63,9 @@ A @key directive on type '{0}' in schema '{1}' contains invalid syntax in the 'fields' argument. + + The lookup field '{0}' in schema '{1}' must not return a list. + The lookup field '{0}' in schema '{1}' should return a nullable type. diff --git a/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/SourceSchemaMerger.cs b/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/SourceSchemaMerger.cs index 457afde74e8..6d40047f6ef 100644 --- a/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/SourceSchemaMerger.cs +++ b/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/SourceSchemaMerger.cs @@ -56,6 +56,7 @@ private CompositionResult MergeSchemaDefinitions(CompositionCo new KeyFieldsSelectInvalidTypeRule(), new KeyInvalidFieldsRule(), new KeyInvalidSyntaxRule(), + new LookupMustNotReturnListRule(), new LookupShouldHaveNullableReturnTypeRule(), new OutputFieldTypesMergeableRule(), new ProvidesDirectiveInFieldsArgumentRule(), diff --git a/src/HotChocolate/Fusion-vnext/test/Fusion.Composition.Tests/PreMergeValidation/Rules/LookupMustNotReturnListRuleTests.cs b/src/HotChocolate/Fusion-vnext/test/Fusion.Composition.Tests/PreMergeValidation/Rules/LookupMustNotReturnListRuleTests.cs new file mode 100644 index 00000000000..b29d33071b6 --- /dev/null +++ b/src/HotChocolate/Fusion-vnext/test/Fusion.Composition.Tests/PreMergeValidation/Rules/LookupMustNotReturnListRuleTests.cs @@ -0,0 +1,109 @@ +using HotChocolate.Fusion.Logging; +using HotChocolate.Fusion.PreMergeValidation; +using HotChocolate.Fusion.PreMergeValidation.Rules; + +namespace HotChocolate.Composition.PreMergeValidation.Rules; + +public sealed class LookupMustNotReturnListRuleTests : CompositionTestBase +{ + private readonly PreMergeValidator _preMergeValidator = + new([new LookupMustNotReturnListRule()]); + + [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.IsFailure); + Assert.Equal(errorMessages, context.Log.Select(e => e.Message).ToArray()); + Assert.True(context.Log.All(e => e.Code == "LOOKUP_MUST_NOT_RETURN_LIST")); + Assert.True(context.Log.All(e => e.Severity == LogSeverity.Error)); + } + + public static TheoryData ValidExamplesData() + { + return new TheoryData + { + // In this example, "userById" returns a "User" object, satisfying the requirement. + { + [ + """ + type Query { + userById(id: ID!): User @lookup + } + + type User { + id: ID! + name: String + } + """ + ] + } + }; + } + + public static TheoryData InvalidExamplesData() + { + return new TheoryData + { + // Here, "usersByIds" returns a list of "User" objects, which violates the requirement + // that a @lookup field must return a single object. + { + [ + """ + type Query { + usersByIds(ids: [ID!]!): [User!] @lookup + } + + type User { + id: ID! + name: String + } + """ + ], + [ + "The lookup field 'Query.usersByIds' in schema 'A' must not return a list." + ] + }, + // Non-null list. + { + [ + """ + type Query { + usersByIds(ids: [ID!]!): [User!]! @lookup + } + + type User { + id: ID! + name: String + } + """ + ], + [ + "The lookup field 'Query.usersByIds' in schema 'A' must not return a list." + ] + } + }; + } +}