Skip to content

Omit requireOneSlicingArgument if not set in listSize directive#7418

Draft
glen-84 wants to merge 9 commits intomainfrom
gai/issue-7398
Draft

Omit requireOneSlicingArgument if not set in listSize directive#7418
glen-84 wants to merge 9 commits intomainfrom
gai/issue-7398

Conversation

@glen-84
Copy link
Member

@glen-84 glen-84 commented Sep 2, 2024

Summary of the changes (Less than 80 chars)

  • Omitted requireOneSlicingArgument if not set in listSize directive.

Closes #7398


@codecov
Copy link

codecov bot commented Sep 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.13%. Comparing base (e3c132c) to head (94a3c41).
Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7418   +/-   ##
=======================================
  Coverage   74.13%   74.13%           
=======================================
  Files        2674     2674           
  Lines      140657   140661    +4     
  Branches    16358    16358           
=======================================
+ Hits       104282   104286    +4     
- Misses      30780    30783    +3     
+ Partials     5595     5592    -3     
Flag Coverage Δ
unittests 74.13% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@glen-84 glen-84 marked this pull request as draft September 4, 2024 08:32
@michaelstaib michaelstaib self-assigned this Sep 24, 2024
@michaelstaib michaelstaib self-requested a review September 24, 2024 09:56
@michaelstaib michaelstaib marked this pull request as ready for review September 29, 2024 20:41
@glen-84 glen-84 added this to the HC-15.0.0 milestone Oct 14, 2024
Copilot AI review requested due to automatic review settings March 8, 2026 10:54
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adjusts how @listSize is represented so that requireOneSlicingArgument is not emitted into SDL unless it was explicitly set, addressing #7398.

Changes:

  • Track whether RequireOneSlicingArgument was explicitly set on ListSizeAttribute and forward a nullable value into the directive.
  • Simplify ListSizeDirective to store RequireOneSlicingArgument as provided (nullable) so formatting can omit it.
  • Add a snapshot-based test verifying SDL output for omitted/explicit requireOneSlicingArgument.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/HotChocolate/CostAnalysis/test/CostAnalysis.Tests/AttributeTests.cs Adds coverage asserting SDL printing behavior for @listSize with and without requireOneSlicingArgument.
src/HotChocolate/CostAnalysis/src/CostAnalysis/Types/ListSizeDirective.cs Changes directive construction to preserve “unset” (null) for RequireOneSlicingArgument.
src/HotChocolate/CostAnalysis/src/CostAnalysis/Types/ListSizeAttribute.cs Adds nullable backing field so the attribute can distinguish “unset” vs explicit values and omit the SDL argument accordingly.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 30 to 35
AssumedSize = assumedSize;
SlicingArguments = slicingArguments ?? [];
SizedFields = sizedFields ?? [];
SlicingArgumentDefaultValue = slicingArgumentDefaultValue;

// https://ibm.github.io/graphql-specs/cost-spec.html#sec-requireOneSlicingArgument
// Per default, requireOneSlicingArgument is enabled,
// and has to be explicitly disabled if not desired for a field.
RequireOneSlicingArgument = SlicingArguments is { Length: > 0 } && (requireOneSlicingArgument ?? true);
RequireOneSlicingArgument = requireOneSlicingArgument;
}
Copy link

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing ListSizeDirective to assign RequireOneSlicingArgument = requireOneSlicingArgument removes the previous defaulting behavior (enabled by default when SlicingArguments are provided). Since ValidateRequireOneSlicingArgument treats null as false, fields that specify slicingArguments but omit requireOneSlicingArgument will no longer enforce the directive type’s default (DefaultValue(true)), changing runtime cost-analysis semantics. Consider restoring the effective default (e.g., treat null as true when slicing arguments are present) while still omitting the argument from SDL when it wasn’t explicitly set.

Copilot uses AI. Check for mistakes.
SlicingArguments?.ToImmutableArray(),
SizedFields?.ToImmutableArray(),
RequireOneSlicingArgument));
_requireOneSlicingArgument));
Copy link

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ListSizeAttribute.OnConfigure now forwards _requireOneSlicingArgument (nullable) instead of the RequireOneSlicingArgument property (defaulting to true). This means leaving the attribute property unset will produce a directive value of null even when SlicingArguments are provided, which (given current validation logic) effectively disables requireOneSlicingArgument by default. If the intended default remains true (as indicated by ListSizeDirectiveType), consider applying that default during analysis/validation when the backing field is null.

Suggested change
_requireOneSlicingArgument));
RequireOneSlicingArgument));

Copilot uses AI. Check for mistakes.
// assert
listSizeDirective1Sdl.MatchInlineSnapshot("@listSize(assumedSize: 10)");
listSizeDirective2Sdl.MatchInlineSnapshot("@listSize(requireOneSlicingArgument: true)");
listSizeDirective3Sdl.MatchInlineSnapshot("@listSize(requireOneSlicingArgument: false)");
Copy link

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new snapshot test covers SDL omission when only AssumedSize is set, but it doesn’t cover the case where SlicingArguments are set and RequireOneSlicingArgument is left unset. Adding an assertion for that combination would help ensure the SDL omission change doesn’t unintentionally change the effective default behavior during cost analysis (per the directive argument default).

Suggested change
listSizeDirective3Sdl.MatchInlineSnapshot("@listSize(requireOneSlicingArgument: false)");
listSizeDirective3Sdl.MatchInlineSnapshot("@listSize(requireOneSlicingArgument: false)");
// and the omitted argument still respects the directive default at runtime
var listSizeDirective1 = query.Fields["examplesAssumedSizeOnly"]
.Directives
.Single(d => d.Type.Name == "listSize")
.ToValue<ListSizeDirective>();
Assert.True(listSizeDirective1.RequireOneSlicingArgument);

Copilot uses AI. Check for mistakes.
@glen-84 glen-84 marked this pull request as draft March 8, 2026 11:03
@glen-84 glen-84 changed the title Omitted requireOneSlicingArgument if not set in listSize directive Omit requireOneSlicingArgument if not set in listSize directive Mar 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Omit requireOneSlicingArgument from @listSize if false

3 participants