From d1e6ac389d8cd7540b5cbbf93f60eaf71c1b401d Mon Sep 17 00:00:00 2001 From: Daniel Reynolds <55194784+danielreynolds1@users.noreply.github.com> Date: Fri, 20 Dec 2024 09:41:43 +0000 Subject: [PATCH] [Fusion] Added pre-merge validation rule `ExternalArgumentDefaultMismatch` (#7844) --- .../Logging/LogEntryCodes.cs | 1 + .../Logging/LogEntryHelper.cs | 14 ++ .../ExternalArgumentDefaultMismatchRule.cs | 53 +++++ .../CompositionResources.Designer.cs | 80 +++----- .../Properties/CompositionResources.resx | 3 + .../Fusion.Composition/SourceSchemaMerger.cs | 1 + ...xternalArgumentDefaultMismatchRuleTests.cs | 182 ++++++++++++++++++ 7 files changed, 278 insertions(+), 56 deletions(-) create mode 100644 src/HotChocolate/Fusion-vnext/src/Fusion.Composition/PreMergeValidation/Rules/ExternalArgumentDefaultMismatchRule.cs create mode 100644 src/HotChocolate/Fusion-vnext/test/Fusion.Composition.Tests/PreMergeValidation/Rules/ExternalArgumentDefaultMismatchRuleTests.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 7639b36c3e3..a9fd60572b6 100644 --- a/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/Logging/LogEntryCodes.cs +++ b/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/Logging/LogEntryCodes.cs @@ -3,6 +3,7 @@ namespace HotChocolate.Fusion.Logging; public static class LogEntryCodes { public const string DisallowedInaccessible = "DISALLOWED_INACCESSIBLE"; + public const string ExternalArgumentDefaultMismatch = "EXTERNAL_ARGUMENT_DEFAULT_MISMATCH"; 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 053bd5b0510..ae4ded0c857 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,20 @@ public static LogEntry DisallowedInaccessibleDirectiveArgument( new SchemaCoordinate(directiveName, argumentName: argument.Name, ofDirective: true), schema: schema); + public static LogEntry ExternalArgumentDefaultMismatch( + string argumentName, + string fieldName, + string typeName) + { + var coordinate = new SchemaCoordinate(typeName, fieldName, argumentName); + + return new LogEntry( + string.Format(LogEntryHelper_ExternalArgumentDefaultMismatch, coordinate), + LogEntryCodes.ExternalArgumentDefaultMismatch, + LogSeverity.Error, + coordinate); + } + public static LogEntry ExternalMissingOnBase(string fieldName, string typeName) => new( string.Format( diff --git a/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/PreMergeValidation/Rules/ExternalArgumentDefaultMismatchRule.cs b/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/PreMergeValidation/Rules/ExternalArgumentDefaultMismatchRule.cs new file mode 100644 index 00000000000..9b84ebc3069 --- /dev/null +++ b/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/PreMergeValidation/Rules/ExternalArgumentDefaultMismatchRule.cs @@ -0,0 +1,53 @@ +using System.Collections.Immutable; +using HotChocolate.Fusion.Events; +using HotChocolate.Language; +using static HotChocolate.Fusion.Logging.LogEntryHelper; + +namespace HotChocolate.Fusion.PreMergeValidation.Rules; + +/// +/// This rule ensures that arguments on fields marked as @external have default values +/// compatible with the corresponding arguments on fields from other source schemas where the field +/// is defined (non-@external). +/// +/// +/// Specification +/// +internal sealed class ExternalArgumentDefaultMismatchRule : IEventHandler +{ + public void Handle(OutputFieldGroupEvent @event, CompositionContext context) + { + var (fieldName, fieldGroup, typeName) = @event; + + var externalFields = fieldGroup + .Where(i => ValidationHelper.IsExternal(i.Field)) + .ToImmutableArray(); + + if (externalFields.Length == 0) + { + return; + } + + var argumentNames = fieldGroup + .SelectMany(i => i.Field.Arguments, (_, arg) => arg.Name) + .ToImmutableHashSet(); + + foreach (var argumentName in argumentNames) + { + var arguments = fieldGroup + .SelectMany(i => i.Field.Arguments.Where(a => a.Name == argumentName)) + .ToImmutableArray(); + + var defaultValue = arguments[0].DefaultValue; + + foreach (var argument in arguments) + { + if (!SyntaxComparer.BySyntax.Equals(argument.DefaultValue, defaultValue)) + { + context.Log.Write( + ExternalArgumentDefaultMismatch(argumentName, 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 75dd11a241e..3da2729fa65 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 @@ -11,46 +11,32 @@ namespace HotChocolate.Fusion.Properties { using System; - /// - /// A strongly-typed resource class, for looking up localized strings, etc. - /// - // This class was auto-generated by the StronglyTypedResourceBuilder - // class via a tool like ResGen or Visual Studio. - // To add or remove a member, edit your .ResX file then rerun ResGen - // with the /str option, or rebuild your VS project. - [global::System.CodeDom.Compiler.GeneratedCodeAttribute("System.Resources.Tools.StronglyTypedResourceBuilder", "4.0.0.0")] - [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] - [global::System.Runtime.CompilerServices.CompilerGeneratedAttribute()] + [System.CodeDom.Compiler.GeneratedCodeAttribute("System.Resources.Tools.StronglyTypedResourceBuilder", "4.0.0.0")] + [System.Diagnostics.DebuggerNonUserCodeAttribute()] + [System.Runtime.CompilerServices.CompilerGeneratedAttribute()] internal class CompositionResources { - private static global::System.Resources.ResourceManager resourceMan; + private static System.Resources.ResourceManager resourceMan; - private static global::System.Globalization.CultureInfo resourceCulture; + private static System.Globalization.CultureInfo resourceCulture; - [global::System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("Microsoft.Performance", "CA1811:AvoidUncalledPrivateCode")] + [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("Microsoft.Performance", "CA1811:AvoidUncalledPrivateCode")] internal CompositionResources() { } - /// - /// Returns the cached ResourceManager instance used by this class. - /// - [global::System.ComponentModel.EditorBrowsableAttribute(global::System.ComponentModel.EditorBrowsableState.Advanced)] - internal static global::System.Resources.ResourceManager ResourceManager { + [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Advanced)] + internal static System.Resources.ResourceManager ResourceManager { get { - if (object.ReferenceEquals(resourceMan, null)) { - global::System.Resources.ResourceManager temp = new global::System.Resources.ResourceManager("HotChocolate.Fusion.Properties.CompositionResources", typeof(CompositionResources).Assembly); + if (object.Equals(null, resourceMan)) { + System.Resources.ResourceManager temp = new System.Resources.ResourceManager("HotChocolate.Fusion.Properties.CompositionResources", typeof(CompositionResources).Assembly); resourceMan = temp; } return resourceMan; } } - /// - /// Overrides the current thread's CurrentUICulture property for all - /// resource lookups using this strongly typed resource class. - /// - [global::System.ComponentModel.EditorBrowsableAttribute(global::System.ComponentModel.EditorBrowsableState.Advanced)] - internal static global::System.Globalization.CultureInfo Culture { + [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Advanced)] + internal static System.Globalization.CultureInfo Culture { get { return resourceCulture; } @@ -59,72 +45,54 @@ internal CompositionResources() { } } - /// - /// Looks up a localized string similar to Pre-merge validation failed. View the composition log for details.. - /// internal static string ErrorHelper_PreMergeValidationFailed { get { return ResourceManager.GetString("ErrorHelper_PreMergeValidationFailed", resourceCulture); } } - /// - /// Looks up a localized string similar to The built-in scalar type '{0}' is not accessible.. - /// internal static string LogEntryHelper_DisallowedInaccessibleBuiltInScalar { get { return ResourceManager.GetString("LogEntryHelper_DisallowedInaccessibleBuiltInScalar", resourceCulture); } } - /// - /// Looks up a localized string similar to The argument '{0}' on built-in directive type '{1}' is not accessible.. - /// - internal static string LogEntryHelper_DisallowedInaccessibleDirectiveArgument { + internal static string LogEntryHelper_DisallowedInaccessibleIntrospectionType { get { - return ResourceManager.GetString("LogEntryHelper_DisallowedInaccessibleDirectiveArgument", resourceCulture); + return ResourceManager.GetString("LogEntryHelper_DisallowedInaccessibleIntrospectionType", resourceCulture); + } + } + + internal static string LogEntryHelper_DisallowedInaccessibleIntrospectionField { + get { + return ResourceManager.GetString("LogEntryHelper_DisallowedInaccessibleIntrospectionField", resourceCulture); } } - /// - /// Looks up a localized string similar to The introspection argument '{0}' with schema coordinate '{1}' is not accessible.. - /// internal static string LogEntryHelper_DisallowedInaccessibleIntrospectionArgument { get { return ResourceManager.GetString("LogEntryHelper_DisallowedInaccessibleIntrospectionArgument", resourceCulture); } } - /// - /// Looks up a localized string similar to The introspection field '{0}' on type '{1}' is not accessible.. - /// - internal static string LogEntryHelper_DisallowedInaccessibleIntrospectionField { + internal static string LogEntryHelper_DisallowedInaccessibleDirectiveArgument { get { - return ResourceManager.GetString("LogEntryHelper_DisallowedInaccessibleIntrospectionField", resourceCulture); + return ResourceManager.GetString("LogEntryHelper_DisallowedInaccessibleDirectiveArgument", resourceCulture); } } - /// - /// Looks up a localized string similar to The introspection type '{0}' is not accessible.. - /// - internal static string LogEntryHelper_DisallowedInaccessibleIntrospectionType { + internal static string LogEntryHelper_ExternalArgumentDefaultMismatch { get { - return ResourceManager.GetString("LogEntryHelper_DisallowedInaccessibleIntrospectionType", resourceCulture); + return ResourceManager.GetString("LogEntryHelper_ExternalArgumentDefaultMismatch", resourceCulture); } } - /// - /// 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.. - /// internal static string LogEntryHelper_OutputFieldTypesNotMergeable { get { return ResourceManager.GetString("LogEntryHelper_OutputFieldTypesNotMergeable", resourceCulture); 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 ebf5d41ed99..e9249fef475 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. + + The argument with schema coordinate '{0}' has inconsistent default values. + Field '{0}' on type '{1}' is only declared as external. diff --git a/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/SourceSchemaMerger.cs b/src/HotChocolate/Fusion-vnext/src/Fusion.Composition/SourceSchemaMerger.cs index fabbe6a8cce..ef997e408b7 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 ExternalArgumentDefaultMismatchRule(), new ExternalMissingOnBaseRule(), new OutputFieldTypesMergeableRule() ]; diff --git a/src/HotChocolate/Fusion-vnext/test/Fusion.Composition.Tests/PreMergeValidation/Rules/ExternalArgumentDefaultMismatchRuleTests.cs b/src/HotChocolate/Fusion-vnext/test/Fusion.Composition.Tests/PreMergeValidation/Rules/ExternalArgumentDefaultMismatchRuleTests.cs new file mode 100644 index 00000000000..397ef2cf576 --- /dev/null +++ b/src/HotChocolate/Fusion-vnext/test/Fusion.Composition.Tests/PreMergeValidation/Rules/ExternalArgumentDefaultMismatchRuleTests.cs @@ -0,0 +1,182 @@ +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 ExternalArgumentDefaultMismatchRuleTests +{ + [Theory] + [MemberData(nameof(ValidExamplesData))] + public void Examples_Valid(string[] sdl) + { + // arrange + var log = new CompositionLog(); + var context = new CompositionContext([.. sdl.Select(SchemaParser.Parse)], log); + var preMergeValidator = new PreMergeValidator([new ExternalArgumentDefaultMismatchRule()]); + + // act + var result = preMergeValidator.Validate(context); + + // assert + Assert.True(result.IsSuccess); + Assert.True(log.IsEmpty); + } + + [Theory] + [MemberData(nameof(InvalidExamplesData))] + public void Examples_Invalid(string[] sdl) + { + // arrange + var log = new CompositionLog(); + var context = new CompositionContext([.. sdl.Select(SchemaParser.Parse)], log); + var preMergeValidator = new PreMergeValidator([new ExternalArgumentDefaultMismatchRule()]); + + // act + var result = preMergeValidator.Validate(context); + + // assert + Assert.True(result.IsFailure); + Assert.Single(log); + Assert.Equal("EXTERNAL_ARGUMENT_DEFAULT_MISMATCH", log.First().Code); + Assert.Equal(LogSeverity.Error, log.First().Severity); + } + + public static TheoryData ValidExamplesData() + { + return new TheoryData + { + // Here, the `name` field on Product is defined in one source schema and marked as + // @external in another. The argument `language` has the same default value in both + // source schemas, satisfying the rule. + { + [ + """ + type Product { + name(language: String = "en"): String + } + """, + """ + type Product { + name(language: String = "en"): String @external + } + """ + ] + }, + // Here, the `name` field on Product is defined with multiple arguments. Both arguments + // have the same default value in the source schemas, satisfying the rule. + { + [ + """ + type Product { + name(language: String = "en", localization: String = "sr"): String + } + """, + """ + type Product { + name(localization: String = "sr", language: String = "en"): String @external + } + """, + """ + type Product { + name(language: String = "en", localization: String = "sr"): String @external + } + """ + ] + } + }; + } + + public static TheoryData InvalidExamplesData() + { + return new TheoryData + { + // Here, the `name` field on Product is defined in one source schema and marked as + // @external in another. The argument `language` has different default values in the + // two source schemas, violating the rule. + { + [ + """ + type Product { + name(language: String = "en"): String + } + """, + """ + type Product { + name(language: String = "de"): String @external + } + """ + ] + }, + // In the following example, the `name` field on Product is defined in one source schema + // and marked as @external in another. The argument `language` has a default value in + // the source schema where the field is defined, but it does not have a default value in + // the source schema where the field is marked as @external, violating the rule. + { + [ + """ + type Product { + name(language: String = "en"): String + } + """, + """ + type Product { + name(language: String): String @external + } + """ + ] + }, + // Here, the `name` field on Product is defined without a default value in the + // non-external source schema, violating the rule. + { + [ + """ + type Product { + name(language: String): String + } + """, + """ + type Product { + name(language: String = "en"): String @external + } + """ + ] + }, + // Here, the `name` field on Product is defined with multiple arguments. One argument + // has a matching default value, whilst the other does not, violating the rule. + { + [ + """ + type Product { + name(language: String = "en", localization: String = "sr"): String + } + """, + """ + type Product { + name(language: String = "en", localization: String = "sa"): String @external + } + """ + ] + }, + // Here, the `name` field on Product is defined with multiple arguments. One argument + // has a matching default value, whilst the other omits the value completely, + // violating the rule. + { + [ + """ + type Product { + name(language: String = "en", localization: String = "sr"): String + } + """, + """ + type Product { + name(language: String = "en", localization: String): String @external + } + """ + ] + } + }; + } +}