search

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.28k stars 748 forks source link

DefaultValue attribute on an input object doesn't get applied in v13 like it did in v12. #6897

Open DaveRMaltby opened 9 months ago

DaveRMaltby commented 9 months ago

Product

Hot Chocolate

Version

13.8.1

Link to minimal reproduction

https://github.com/Servant-Software-LLC/HotChocolate_DefaultValue_not_applied

Steps to reproduce

  1. Build the solution.
  2. There are 2 unit tests. Both use the same exact code. One runs with 12.7.0 packages and the other with 13.8.1 packages. The v13 unit test fails.

What is expected?

The output of the GraphQL call to this mutation is providing the DefaultValue that was to be applied to the MyInputObject.ValuesToRetrieveInBatch field. In v12, this value was applied to the property and so I expected it to likewise be applied in v13.

What is actually happening?

The Optional of the ValuesToRetrieveInBatch property has the value of 0.

Relevant log output

No response

Additional context

Maybe mutation calls directly to IExecutionResult bypass some intermediate steps in the pipeline that have been added in v13, whereas in v12 the default values were applied when calling IExecutionResult?

DaveRMaltby commented 9 months ago

I've tracked down the issue. Although in v13 the default value is still in the definition for this field that uses the DefaultValue attribute, turns out that the Optional<T>.From() method has changed such that it ignores the default value. image REF: https://github.com/ChilliCream/graphql-platform/blob/main-version-13/src/HotChocolate/Core/src/Abstractions/Optional.cs#L165

Next, I'll look at the blame and try to figure out why the From() method was changed. Maybe it is called with multiple purposes, such that in at least one case, a new Optional<T>() (without the DefaultValue) is required.

DaveRMaltby commented 9 months ago

If an overload of From() method is needed, then for reference, the From is setup (in a System.Linq.Expressions.Expression) to be called at https://github.com/ChilliCream/graphql-platform/blob/main-version-13/src/HotChocolate/Core/src/Types/Utilities/Serialization/InputObjectCompiler.cs#L318

DaveRMaltby commented 9 months ago

The Blame points to #4965 which points to #4677. I'm in the works of creating a simple PR with unit test to fix this.

DaveRMaltby commented 9 months ago

I've created a PR against the main-version-13 branch. Let me know if I need to do the same for the v14 (Main) branch. Thanks!

michaelstaib commented 9 months ago

We do not allow PRs against version 13 branches ... all changes have to go against main. We can cherrypick it to 13 once it went through the main branch.