ChilliCream / graphql-platform

Welcome to the home of the Hot Chocolate GraphQL server for .NET, the Strawberry Shake GraphQL client for .NET and Banana Cake Pop the awesome Monaco based GraphQL IDE.
https://chillicream.com
MIT License
5.23k stars 744 forks source link

Incorrect @cost when auto-injecting global state with AddParameterExpressionBuilder #7399

Open cmeeren opened 2 months ago

cmeeren commented 2 months ago

Product

Hot Chocolate

Version

14.0.0-rc.1

Link to minimal reproduction

See zip below

Steps to reproduce

Repro solution: HotChocolateBugRepro.zip

Code for quick reference:

var builder = WebApplication.CreateBuilder(args);
builder.Services.AddGraphQLServer().AddQueryType<Query>()
    .AddParameterExpressionBuilder(ctx => ctx.GetGlobalState<MyGlobalState>("MyGlobalState"));
var app = builder.Build();
app.MapGraphQL();
app.Run();

public record MyGlobalState();

public record MyThing(string Id);

public class Query
{
    public MyThing Test(MyGlobalState state) => new("Foo");
}

What is expected?

The following SDL:

schema {
  query: Query
}

type MyThing {
  id: String
}

type Query {
  test: MyThing
}

What is actually happening?

The following SDL:

schema {
  query: Query
}

type MyThing {
  id: String
}

type Query {
  test: MyThing @cost(weight: "10")
}

"The purpose of the `cost` directive is to define a `weight` for GraphQL types, fields, and arguments. Static analysis can use these weights when calculating the overall cost of a query or response."
directive @cost("The `weight` argument defines what value to add to the overall cost for every appearance, or possible appearance, of a type, field, argument, etc." weight: String!) on SCALAR | OBJECT | FIELD_DEFINITION | ARGUMENT_DEFINITION | ENUM | INPUT_FIELD_DEFINITION

Relevant log output

No response

Additional context

This only happens when using AddParameterExpressionBuilder as shown above. If using [GlobalState("MyGlobalSate")] before the parameter, the cost is correct.

michaelstaib commented 2 months ago

In this case its kind of correct. Since we cannot fully inspect what the impact is of custom expressions the field will be executed on the standard path which is async. This has not really something to do with cost. The cost interceptor just inspects if the resolver is pure or not. In this case its not.

You can work around that by either implementing a full parameter expression builder that sets the context to pure ... in this case it really is a pure. Or you could combine it with a configuration builder which sets a cost.

michaelstaib commented 2 months ago

We could however add another configuration method that allows mapping states ... like

builder.Services.AddGraphQLServer().AddQueryType<Query>().MapGlobalState<Fooo>(key);

which we know is pure and then weight it as pure.

michaelstaib commented 2 months ago

Thanks for putting these issues together ... this helps us alot!

cmeeren commented 2 months ago

Happy to help!

I haven't yet been able to look in detail at the implications of your replies, but the fact that the cost disappears just by adding GlobalStateAttribute seems to be an inconsistency regardless.

If more context helps, in our case the global state is the fully loaded principal (a custom User object) that is pre-loaded for all requests and used in most resolves (at least all root level ones) for authorization.

cmeeren commented 2 months ago

More context, in case I'm using the wrong tool: The only thing I'm trying to achieve with AddParameterExpressionBuilder is to inject the custom principal in any resolver I want with as little verbosity as possible. The method I use allows me to omit GlobalStateAttribute. I think I found it in the official docs somewhere. On mobile now, so can't easily find it, but probably related to docs on global state.