Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Query complexity calculation does not take @include directive into account #6251

Closed
1 task done
sgabler opened this issue Jun 7, 2023 · 5 comments
Closed
1 task done

Comments

@sgabler
Copy link
Contributor

sgabler commented Jun 7, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Product

Hot Chocolate

Describe the bug

I am executing the following query against my HC12 (v12.16.0) API:

query TEST {
  currentUser {
    user {
      id
      email @include(if: true)
    }
  }
}

This returns the id and email fields correctly and results in a query complexity of 8.

If I run the query with @include set to false, then I will still get a complexity of 8, but would expect 7, since one field less was resolved.

To me this sounds like a bug, since the complexity of the query should take into account the @include or @skip directives, but please correct me if I'm wrong.

This is relevant for us, since we are using the calculated complexity for billing the customers.

Would it be possible to fix this for versions 12 and 13? Also happy to help if your resources are stretched too thin at the moment.

Thank you

Steps to reproduce

  1. Use any query and add the @include directive as shown above
  2. Set a breakpoint in the code where the query complexity is calculated
  3. Compare the complexity when switching @include to true or false

Relevant log output

No response

Additional Context?

No response

Version

12.16.0

@michaelstaib
Copy link
Member

This is known ... we will fix it with 14 as it is a breaking change,

@sgabler
Copy link
Contributor Author

sgabler commented Jun 8, 2023

I tried to built a workaround by overriding the Calculation field. It works if the directive contains a boolean, e.g. @include(if: true). I can get the directive and read it's argument value, which is true or false.

But it doesn't work if the directive contains a variable, e.g. @include(if: $includeThisField), because the variable is not exposed in the ComplexityContext struct. The field _valueCollection is private.

Here is my code attempt:

                    o.Complexity.Calculation = context =>
                    {
                        // Check if the "@include" directive is present for the current field
                        var includeDirective = context.Selection.Directives.SingleOrDefault(d => d.Name.Value == "include");

                        if (includeDirective != null)
                        {
                            // Get the "if" argument of the "@include" directive
                            var ifArgument = includeDirective.Arguments.SingleOrDefault(a => a.Name.Value == "if");

                            // If the "if" argument exists and its value is `false`, set the complexity to zero
                            if (ifArgument is { Value: BooleanValueNode { Value: false } })
                            {
                                return 0;
                            }

                            if (ifArgument is { Value: VariableNode vn })
                            {
                                // not possible to access the variable here
                            }
                        }

                        // omitted the default calculation 

I guess there is nothing that can be done here except for forking the repo and exposing the _valueCollection? @michaelstaib

@sgabler
Copy link
Contributor Author

sgabler commented Jun 14, 2023

Discussed with Michael and I created two PRs:

They are already merged. Once the nuget package for v12 and v13 is published, developers will have a workaround for this problem.

@sgabler
Copy link
Contributor Author

sgabler commented Jun 28, 2023

Now that v12.19.2 was released (thanks again @michaelstaib), I was able to implement this workaround to get a correct complexity calculation in v12:

        private static int ComplexityCalculation(ComplexityContext context)
        {
            // This dictionary contains a mapping between the directive and the boolean value for which the the calculation should be set to 0
            var directives = new Dictionary<string, bool>
            {
                { "include", false },
                { "skip", true }
            };

            foreach (var directiveEntry in directives)
            {
                // Look for a directive in the current selection that matches the current key (directive name).
                var directive = context.Selection.Directives.SingleOrDefault(d => d.Name.Value == directiveEntry.Key);

                if (directive == null)
                {
                    continue;
                }

                // Look for an argument named "if" in the found directive.
                var ifArgument = directive.Arguments.SingleOrDefault(a => a.Name.Value == "if");

                // Return 0
                // - if the "if" argument exists and its value matches the one defined in the dictionary for the current directive,
                // - or if the "if" argument is a variable that exists and its value matches the one defined in the dictionary
                if ((ifArgument is { Value: BooleanValueNode { Value: var directValue } } && directValue == directiveEntry.Value) ||
                    (ifArgument is { Value: VariableNode vn } && context.Variables.TryGetVariable(vn.Value, out bool variable) &&
                     variable == directiveEntry.Value))
                {
                    return 0;
                }
            }

            #region Original HC12 code (see ComplexityAnalyzerSettings.cs)

            if (context.Multipliers.Count == 0)
            {
                return context.Complexity + context.ChildComplexity;
            }

            var cost = context.Complexity + context.ChildComplexity;
            var needsDefaultMultiplier = true;

            foreach (MultiplierPathString multiplier in context.Multipliers)
            {
                if (context.TryGetArgumentValue(multiplier, out int value))
                {
                    cost *= value;
                    needsDefaultMultiplier = false;
                }
            }

            if (needsDefaultMultiplier && context.DefaultMultiplier.HasValue)
            {
                cost *= context.DefaultMultiplier.Value;
            }

            return cost;

            #endregion
        }

And then later you need to register this function like this:

o.Complexity.Calculation = ComplexityCalculation;

@michaelstaib
Copy link
Member

We have implemented the new IBM Cost spec for 14. For now only the static analysis which ignores skip and include.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants