From 7cb0019b2acdf496facc53c613a6fc10deed4eb4 Mon Sep 17 00:00:00 2001 From: Glen Date: Tue, 17 Dec 2024 23:54:07 +0200 Subject: [PATCH] [Fusion] Added pre-merge validation rule `ExternalMissingOnBaseRule` (#7842) --- .../Extensions/ImmutableArrayExtensions.cs | 21 ++++ .../Logging/LogEntryCodes.cs | 1 + .../Logging/LogEntryHelper.cs | 10 ++ .../Rules/ExternalMissingOnBaseRule.cs | 30 +++++ .../CompositionResources.Designer.cs | 9 ++ .../Properties/CompositionResources.resx | 3 + .../Fusion.Composition/SourceSchemaMerger.cs | 1 + .../Rules/ExternalMissingOnBaseRuleTests.cs | 117 ++++++++++++++++++ 8 files changed, 192 insertions(+) create mode 100644 src/HotChocolate/Fusion-vnext/src/Fusion.Composition/Extensions/ImmutableArrayExtensions.cs create mode 100644 src/HotChocolate/Fusion-vnext/src/Fusion.Composition/PreMergeValidation/Rules/ExternalMissingOnBaseRule.cs create mode 100644 src/HotChocolate/Fusion-vnext/test/Fusion.Composition.Tests/PreMergeValidation/Rules/ExternalMissingOnBaseRuleTests.cs diff --git a/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/Extensions/ImmutableArrayExtensions.cs b/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/Extensions/ImmutableArrayExtensions.cs new file mode 100644 index 00000000000..7545765a032 --- /dev/null +++ b/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/Extensions/ImmutableArrayExtensions.cs @@ -0,0 +1,21 @@ +using System.Collections.Immutable; + +namespace HotChocolate.Fusion.Extensions; + +internal static class ImmutableArrayExtensions +{ + public static int Count(this ImmutableArray array, Predicate predicate) + { + var count = 0; + + foreach (var item in array) + { + if (predicate(item)) + { + count++; + } + } + + return count; + } +} 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 298f766cf33..7639b36c3e3 100644 --- a/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/Logging/LogEntryCodes.cs +++ b/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/Logging/LogEntryCodes.cs @@ -3,5 +3,6 @@ namespace HotChocolate.Fusion.Logging; public static class LogEntryCodes { public const string DisallowedInaccessible = "DISALLOWED_INACCESSIBLE"; + public const string ExternalMissingOnBase = "EXTERNAL_MISSING_ON_BASE"; public const string OutputFieldTypesNotMergeable = "OUTPUT_FIELD_TYPES_NOT_MERGEABLE"; } 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 15bda6cc934..053bd5b0510 100644 --- a/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/Logging/LogEntryHelper.cs +++ b/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/Logging/LogEntryHelper.cs @@ -76,6 +76,16 @@ public static LogEntry DisallowedInaccessibleDirectiveArgument( new SchemaCoordinate(directiveName, argumentName: argument.Name, ofDirective: true), schema: schema); + public static LogEntry ExternalMissingOnBase(string fieldName, string typeName) + => new( + string.Format( + LogEntryHelper_ExternalMissingOnBase, + fieldName, + typeName), + LogEntryCodes.ExternalMissingOnBase, + LogSeverity.Error, + new SchemaCoordinate(typeName, fieldName)); + public static LogEntry OutputFieldTypesNotMergeable(string fieldName, string typeName) => new( string.Format( diff --git a/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/PreMergeValidation/Rules/ExternalMissingOnBaseRule.cs b/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/PreMergeValidation/Rules/ExternalMissingOnBaseRule.cs new file mode 100644 index 00000000000..97db2fa4961 --- /dev/null +++ b/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/PreMergeValidation/Rules/ExternalMissingOnBaseRule.cs @@ -0,0 +1,30 @@ +using HotChocolate.Fusion.Events; +using HotChocolate.Fusion.Extensions; +using static HotChocolate.Fusion.Logging.LogEntryHelper; + +namespace HotChocolate.Fusion.PreMergeValidation.Rules; + +/// +/// This rule ensures that any field marked as @external in a source schema is actually +/// defined (non-@external) in at least one other source schema. The @external +/// directive is used to indicate that the field is not usually resolved by the source schema it is +/// declared in, implying it should be resolvable by at least one other source schema. +/// +/// +/// Specification +/// +internal sealed class ExternalMissingOnBaseRule : IEventHandler +{ + public void Handle(OutputFieldGroupEvent @event, CompositionContext context) + { + var (fieldName, fieldGroup, typeName) = @event; + + var externalFieldCount = fieldGroup.Count(i => ValidationHelper.IsExternal(i.Field)); + var nonExternalFieldCount = fieldGroup.Length - externalFieldCount; + + if (externalFieldCount != 0 && nonExternalFieldCount == 0) + { + context.Log.Write(ExternalMissingOnBase(fieldName, typeName)); + } + } +} 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 8a2c9d46b34..75dd11a241e 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 @@ -113,6 +113,15 @@ internal static string LogEntryHelper_DisallowedInaccessibleIntrospectionType { } } + /// + /// Looks up a localized string similar to Field '{0}' on type '{1}' is only declared as external.. + /// + internal static string LogEntryHelper_ExternalMissingOnBase { + get { + return ResourceManager.GetString("LogEntryHelper_ExternalMissingOnBase", resourceCulture); + } + } + /// /// Looks up a localized string similar to Field '{0}' on type '{1}' is not mergeable.. /// 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 78fde65b49b..ebf5d41ed99 100644 --- a/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/Properties/CompositionResources.resx +++ b/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/Properties/CompositionResources.resx @@ -36,6 +36,9 @@ The argument '{0}' on built-in directive type '{1}' is not accessible. + + Field '{0}' on type '{1}' is only declared as external. + Field '{0}' on type '{1}' is not mergeable. diff --git a/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/SourceSchemaMerger.cs b/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/SourceSchemaMerger.cs index 61e5b4658f7..fabbe6a8cce 100644 --- a/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/SourceSchemaMerger.cs +++ b/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/SourceSchemaMerger.cs @@ -47,6 +47,7 @@ private CompositionResult MergeSchemaDefinitions(CompositionCo private static readonly List _preMergeValidationRules = [ new DisallowedInaccessibleElementsRule(), + new ExternalMissingOnBaseRule(), new OutputFieldTypesMergeableRule() ]; } diff --git a/src/HotChocolate/Fusion-vnext/test/Fusion.Composition.Tests/PreMergeValidation/Rules/ExternalMissingOnBaseRuleTests.cs b/src/HotChocolate/Fusion-vnext/test/Fusion.Composition.Tests/PreMergeValidation/Rules/ExternalMissingOnBaseRuleTests.cs new file mode 100644 index 00000000000..4179bb35a71 --- /dev/null +++ b/src/HotChocolate/Fusion-vnext/test/Fusion.Composition.Tests/PreMergeValidation/Rules/ExternalMissingOnBaseRuleTests.cs @@ -0,0 +1,117 @@ +using HotChocolate.Fusion; +using HotChocolate.Fusion.Logging; +using HotChocolate.Fusion.PreMergeValidation; +using HotChocolate.Fusion.PreMergeValidation.Rules; +using HotChocolate.Skimmed.Serialization; + +namespace HotChocolate.Composition.PreMergeValidation.Rules; + +public sealed class ExternalMissingOnBaseRuleTests +{ + [Test] + [MethodDataSource(nameof(ValidExamplesData))] + public async Task Examples_Valid(string[] sdl) + { + // arrange + var log = new CompositionLog(); + var context = new CompositionContext([.. sdl.Select(SchemaParser.Parse)], log); + var preMergeValidator = new PreMergeValidator([new ExternalMissingOnBaseRule()]); + + // act + var result = preMergeValidator.Validate(context); + + // assert + await Assert.That(result.IsSuccess).IsTrue(); + await Assert.That(log.IsEmpty).IsTrue(); + } + + [Test] + [MethodDataSource(nameof(InvalidExamplesData))] + public async Task Examples_Invalid(string[] sdl) + { + // arrange + var log = new CompositionLog(); + var context = new CompositionContext([.. sdl.Select(SchemaParser.Parse)], log); + var preMergeValidator = new PreMergeValidator([new ExternalMissingOnBaseRule()]); + + // act + var result = preMergeValidator.Validate(context); + + // assert + await Assert.That(result.IsFailure).IsTrue(); + await Assert.That(log.Count()).IsEqualTo(1); + await Assert.That(log.First().Code).IsEqualTo("EXTERNAL_MISSING_ON_BASE"); + await Assert.That(log.First().Severity).IsEqualTo(LogSeverity.Error); + } + + public static IEnumerable> ValidExamplesData() + { + return + [ + // Here, the `name` field on Product is defined in source schema A and marked as + // @external in source schema B, which is valid because there is a base definition in + // source schema A. + () => + [ + """ + # Source schema A + type Product { + id: ID + name: String + } + """, + """ + # Source schema B + type Product { + id: ID + name: String @external + } + """ + ] + ]; + } + + public static IEnumerable> InvalidExamplesData() + { + return + [ + // In this example, the `name` field on Product is marked as @external in source schema + // B but has no non-@external declaration in any other source schema, violating the + // rule. + () => + [ + """ + # Source schema A + type Product { + id: ID + } + """, + """ + # Source schema B + type Product { + id: ID + name: String @external + } + """ + ], + // The `name` field is external in both source schemas. + () => + [ + """ + # Source schema A + type Product { + id: ID + name: String @external + } + """, + """ + # Source schema B + type Product { + id: ID + name: String @external + } + """ + ] + ]; + } +}