From 83b704447bfa1f8d2ef9ec08b218e82962fb143f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=E2=96=88=E2=96=88=E2=96=88=E2=96=88=E2=96=88?= Date: Mon, 18 Nov 2024 21:47:00 -0500 Subject: [PATCH] feat|fix: adding attribute to skip null properties Adding the [SerializeIfNotNullAttribute] which can be applied to properties serialized by the QuerySerializer to indicate they should never be serialized when null. As a proof of concept, the RuleParameterInput used in the creation of rulesets for the repository was failing due to null properties being seralized into the final output. This code now functions as expected. I did not make a global change to no longer serialized all null values as I anticipate that there are "side effects" we depend on where there are expected null serialzations that will break if such a change is made. This is an "opt-in" feature that does not disturb the greater echosystem that may depend on these "side effects". octokit/octokit.graphql.net#320 --- .../Core/Serializers/QuerySerializer.cs | 35 +++++-- .../SerializeIfNotNullAttribute.cs | 9 ++ .../QueryBuilderTests.cs | 95 +++++++++++++++++++ .../Model/RuleParametersInput.cs | 13 ++- 4 files changed, 141 insertions(+), 11 deletions(-) create mode 100644 src/Octokit.GraphQL.Core/Core/Serializers/SerializeIfNotNullAttribute.cs diff --git a/src/Octokit.GraphQL.Core/Core/Serializers/QuerySerializer.cs b/src/Octokit.GraphQL.Core/Core/Serializers/QuerySerializer.cs index 1f840327..5526ff8e 100644 --- a/src/Octokit.GraphQL.Core/Core/Serializers/QuerySerializer.cs +++ b/src/Octokit.GraphQL.Core/Core/Serializers/QuerySerializer.cs @@ -12,7 +12,13 @@ namespace Octokit.GraphQL.Core.Serializers { public class QuerySerializer { - private static readonly ConcurrentDictionary[]> typeCache = new ConcurrentDictionary[]>(); + /// + /// A cache of previously reflected types where: + /// Item1: The name of the type + /// Item2: The method info object to retrieve the value of the property. + /// Item3: A boolean indicating whether the value should always be serialized (true) or only serialized when not null (false) + /// + private static readonly ConcurrentDictionary[]> typeCache = new ConcurrentDictionary[]>(); private readonly int indentation; private readonly string comma = ","; @@ -232,12 +238,12 @@ private void SerializeValue(StringBuilder builder, object value) { var objectType = value.GetType(); - Tuple[] properties; + Tuple[] properties; if (!typeCache.TryGetValue(objectType, out properties)) { properties = objectType.GetRuntimeProperties() .Where(info => info.GetMethod.IsPublic) - .Select(info => new Tuple(info.Name.LowerFirstCharacter(), info.GetMethod)) + .Select(info => new Tuple(info.Name.LowerFirstCharacter(), info.GetMethod, null == info.GetCustomAttribute())) .ToArray(); typeCache.TryAdd(objectType, properties); @@ -247,12 +253,23 @@ private void SerializeValue(StringBuilder builder, object value) //Cache Hit } + var hasOpenBrace = false; for (var index = 0; index < properties.Length; index++) { var property = properties[index]; + var valueObj = property.Item2.Invoke(value, null); - if (index == 0) + // Property 3 indicates if we should write the property when the value of the property is null. + // True: always write the value + // False: only write the value if it isn't null + if (property.Item3 == false && null == valueObj) { + continue; + } + + if (!hasOpenBrace) + { + hasOpenBrace = true; OpenBrace(builder); } else @@ -261,12 +278,12 @@ private void SerializeValue(StringBuilder builder, object value) } builder.Append(property.Item1.LowerFirstCharacter()).Append(colon); - SerializeValue(builder, property.Item2.Invoke(value, null)); + SerializeValue(builder, valueObj); + } - if (index + 1 == properties.Length) - { - CloseBrace(builder); - } + if (hasOpenBrace) + { + CloseBrace(builder); } } } diff --git a/src/Octokit.GraphQL.Core/Core/Serializers/SerializeIfNotNullAttribute.cs b/src/Octokit.GraphQL.Core/Core/Serializers/SerializeIfNotNullAttribute.cs new file mode 100644 index 00000000..510b8dd2 --- /dev/null +++ b/src/Octokit.GraphQL.Core/Core/Serializers/SerializeIfNotNullAttribute.cs @@ -0,0 +1,9 @@ +using System; + +namespace Octokit.GraphQL.Core.Serializers +{ + [AttributeUsage(AttributeTargets.Property)] + public class SerializeIfNotNull : Attribute + { + } +} diff --git a/src/Octokit.GraphQL.UnitTests/QueryBuilderTests.cs b/src/Octokit.GraphQL.UnitTests/QueryBuilderTests.cs index 81128f38..63a0ec3e 100644 --- a/src/Octokit.GraphQL.UnitTests/QueryBuilderTests.cs +++ b/src/Octokit.GraphQL.UnitTests/QueryBuilderTests.cs @@ -394,5 +394,100 @@ ... on RepositoryOwner { } + + /// + /// Tests that the mutation skips serializing null values in the + /// class. When the null values in this class are sent to GitHub the call + /// fails. + /// + [Fact] + public void Repository_CreateRepositoryRuleset() + { + var expected = @"mutation { + createRepositoryRuleset(input: { + clientMutationId: null,sourceId: ""hello world"",name: ""main"",target: BRANCH,rules: [ { + id: null,type: DELETION,parameters: null + }, { + id: null,type: PULL_REQUEST,parameters: { + pullRequest: { + dismissStaleReviewsOnPush: true,requireCodeOwnerReview: true,requireLastPushApproval: false,requiredApprovingReviewCount: 0,requiredReviewThreadResolution: false + } + } + }, { + id: null,type: REQUIRED_STATUS_CHECKS,parameters: { + requiredStatusChecks: { + requiredStatusChecks: [ { + context: ""ng test"",integrationId: null + }, { + context: ""ng lint"",integrationId: null + }],strictRequiredStatusChecksPolicy: true + } + } + }],conditions: { + refName: { + exclude: [],include: [""~DEFAULT_BRANCH""] + },repositoryName: null,repositoryId: null,repositoryProperty: null + },enforcement: ACTIVE,bypassActors: null + }) { + ruleset { + id + } + } +}"; + + var expression = new Mutation() + .CreateRepositoryRuleset(new Arg(new CreateRepositoryRulesetInput + { + SourceId = new ID("hello world"), + Name = "main", + Enforcement = RuleEnforcement.Active, + Target = RepositoryRulesetTarget.Branch, + Rules = new[] + { + new RepositoryRuleInput { + Type = RepositoryRuleType.Deletion + }, + new RepositoryRuleInput { + Type = RepositoryRuleType.PullRequest, + Parameters = new RuleParametersInput { + PullRequest = new PullRequestParametersInput { + DismissStaleReviewsOnPush = true, + RequireCodeOwnerReview = true + } + } + }, + new RepositoryRuleInput { + Type = RepositoryRuleType.RequiredStatusChecks, + Parameters = new RuleParametersInput { + RequiredStatusChecks = new RequiredStatusChecksParametersInput { + RequiredStatusChecks = new[] + { + new StatusCheckConfigurationInput { + Context = "ng test" + }, + new StatusCheckConfigurationInput { + Context = "ng lint" + } + }, + StrictRequiredStatusChecksPolicy = true + } + } + } + }, + Conditions = new RepositoryRuleConditionsInput + { + RefName = new RefNameConditionTargetInput + { + Include = new[] { "~DEFAULT_BRANCH" }, + Exclude = new string[] { } + } + } + })) + .Select(x => x.Ruleset.Id); + + var query = expression.Compile(); + + Assert.Equal(expected, query.ToString(2), ignoreLineEndingDifferences: true); + } } } diff --git a/src/Octokit.GraphQL/Model/RuleParametersInput.cs b/src/Octokit.GraphQL/Model/RuleParametersInput.cs index 99e8a80f..279b215b 100644 --- a/src/Octokit.GraphQL/Model/RuleParametersInput.cs +++ b/src/Octokit.GraphQL/Model/RuleParametersInput.cs @@ -1,7 +1,6 @@ namespace Octokit.GraphQL.Model { - using System; - using System.Collections.Generic; + using Core.Serializers; /// /// Specifies the parameters for a `RepositoryRule` object. Only one of the fields should be specified. @@ -11,51 +10,61 @@ public class RuleParametersInput /// /// Parameters used for the `update` rule type /// + [SerializeIfNotNull] public UpdateParametersInput Update { get; set; } /// /// Parameters used for the `required_deployments` rule type /// + [SerializeIfNotNull] public RequiredDeploymentsParametersInput RequiredDeployments { get; set; } /// /// Parameters used for the `pull_request` rule type /// + [SerializeIfNotNull] public PullRequestParametersInput PullRequest { get; set; } /// /// Parameters used for the `required_status_checks` rule type /// + [SerializeIfNotNull] public RequiredStatusChecksParametersInput RequiredStatusChecks { get; set; } /// /// Parameters used for the `commit_message_pattern` rule type /// + [SerializeIfNotNull] public CommitMessagePatternParametersInput CommitMessagePattern { get; set; } /// /// Parameters used for the `commit_author_email_pattern` rule type /// + [SerializeIfNotNull] public CommitAuthorEmailPatternParametersInput CommitAuthorEmailPattern { get; set; } /// /// Parameters used for the `committer_email_pattern` rule type /// + [SerializeIfNotNull] public CommitterEmailPatternParametersInput CommitterEmailPattern { get; set; } /// /// Parameters used for the `branch_name_pattern` rule type /// + [SerializeIfNotNull] public BranchNamePatternParametersInput BranchNamePattern { get; set; } /// /// Parameters used for the `tag_name_pattern` rule type /// + [SerializeIfNotNull] public TagNamePatternParametersInput TagNamePattern { get; set; } /// /// Parameters used for the `workflows` rule type /// + [SerializeIfNotNull] public WorkflowsParametersInput Workflows { get; set; } } } \ No newline at end of file