Skip to content

Fix custom parameter binding in source-generated resolvers - #10039

Closed
glen-84 wants to merge 2 commits into
mainfrom
gai/sourcegen-custom-parameter-binding
Closed

glen-84 wants to merge 2 commits into
mainfrom
gai/sourcegen-custom-parameter-binding

Conversation

@glen-84

@glen-84 glen-84 commented Jul 2, 2026

Copy link
Copy Markdown
Member

Summary

  • Custom parameter expression builders registered via AddParameterExpressionBuilder are now honored on the source-generated resolver binding path, matching the reflection path. CustomParameterExpressionBuilder participates in the source-gen ParameterBindingResolver (it now implements IParameterBindingFactory), like the built-in builders.
  • Previously such a parameter was misclassified as a GraphQL input argument, so a source-generated (partial) [QueryType]/[MutationType]/[ObjectType] resolver taking a context-supplied parameter could fail schema build when that CLR type was not a valid input type.
  • When a builder is registered with a ParameterInfo predicate (which the source generator cannot evaluate), schema build now fails with a clear, actionable error instead of silently misclassifying the parameter.
  • Related to Source Generator does not set MethodInfo or ParameterInfo #9908 (both stem from the source generator not surfacing parameter metadata).

Test plan

  • Unit tests for ParameterBindingResolver: a custom builder is classified as Custom (not Argument), its binding executes the registered delegate, and a ParameterInfo-predicate builder throws for a matching parameter.
  • End-to-end source-generator tests: a partial [MutationType] with a custom parameter builds and executes with the parameter injected and absent from the schema as an argument; an interface/base parameter matched by a predicate builder fails loudly.

Copilot AI review requested due to automatic review settings July 2, 2026 17:13
@glen-84

glen-84 commented Jul 2, 2026

Copy link
Copy Markdown
Member Author

Design options considered

Root cause: the source-generated resolver binding path (ParameterBindingResolver) narrows custom parameter builders to IParameterBindingFactory. CustomParameterExpressionBuilder (created by AddParameterExpressionBuilder) implements only IParameterExpressionBuilder, so it was dropped — both GetBindingInfo (kind) and GetBinding (runtime value) fell back to the default argument binding. The reflection path keeps the full builder list, which is why non-partial resolvers worked and HC15 (reflection-only) was unaffected.

Option A — make CustomParameterExpressionBuilder a dual-interface builder (chosen). Implement IParameterBindingFactory on it, mirroring every built-in builder that already implements both interfaces. Minimal and framework-native; two internal Data builders (PagingArguments, EF ContextFactory) had already hand-rolled this exact workaround, which confirmed the shape.

Option B — widen ParameterBindingResolver to adapt any IParameterExpressionBuilder (rejected). More literally "align with reflection", but infeasible: the base interface is ParameterInfo/Expression-based, and the source-gen runtime has neither a ParameterInfo (its ParameterDescriptor carries only name/type/nullability/attributes) nor an expression-compile step. Only CustomParameterExpressionBuilder can be bridged (it holds a compilable Expression<Func<IResolverContext, TArg>>), so the fix belongs on that type.

Option C — surface ParameterInfo in the source generator (deferred). The strategic fix for the shared root cause (see #9908), via emitted synthetic metadata or runtime reflection. Larger, and out of scope here. If it lands, the predicate limitation below disappears.

Sub-decision — custom Func<ParameterInfo, bool> predicates. The source generator cannot evaluate an arbitrary ParameterInfo predicate. Rather than silently approximate it with a type check, the build now fails loud with an actionable error (register with the type-based overload). The relevance guard uses parameter.Type.IsAssignableFrom(typeof(TArg)), so a predicate matching an interface/base parameter is caught too. Default (type-based) AddParameterExpressionBuilder<T>(...) registrations are fully supported.

Drafted by Claude (Anthropic AI assistant).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR aligns source-generated resolver parameter binding with the reflection-based path by ensuring custom parameter expression builders registered via AddParameterExpressionBuilder participate in the source-gen ParameterBindingResolver. It also adds explicit failure behavior when a custom builder relies on a ParameterInfo predicate that the source generator cannot evaluate.

Changes:

  • Update CustomParameterExpressionBuilder to implement IParameterBindingFactory so custom builders are considered during source-generated parameter binding.
  • Add a guard in ParameterBindingResolver to throw a clear SchemaException when a ParameterInfo-predicate custom builder would otherwise be silently misclassified.
  • Add unit + end-to-end source generator tests to validate correct classification/injection and the new failure mode.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/HotChocolate/Data/src/EntityFramework/ContextFactoryParameterExpressionBuilder.cs Updates overrides to match new virtual ParameterDescriptor binding API in custom builder base.
src/HotChocolate/Data/src/Data/PagingArgumentsParameterExpressionBuilder.cs Same as above, ensures paging argument binding participates correctly on source-gen path.
src/HotChocolate/Core/src/Types/Resolvers/ParameterBindingResolver.cs Adds EnsureParameterInfoNotRequired to fail loudly when a ParameterInfo predicate is required.
src/HotChocolate/Core/src/Types/Internal/CustomParameterExpressionBuilder.cs Implements IParameterBindingFactory and adds descriptor-based handling + runtime binding creation for source-gen.
src/HotChocolate/Core/test/Types.Tests/Resolvers/ParameterBindingResolverTests.cs Unit coverage for classification, execution, and predicate-based failure behavior.
src/HotChocolate/Core/test/Types.Analyzers.Tests/SourceGeneratorReproTests.cs End-to-end coverage for source-gen injection (not argument) and loud failure for predicate builder cases.

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

Comment thread src/HotChocolate/Core/src/Types/Internal/CustomParameterExpressionBuilder.cs Outdated
@github-actions

github-actions Bot commented Jul 2, 2026 •

Copy link
Copy Markdown
Contributor

Patch coverage

95.3% of changed lines covered (41/43)

File Covered Changed Patch %
…/EntityFramework/ContextFactoryParameterExpressionBuilder.cs 0 1 0.0% 🔴
…/Core/src/Types/Internal/CustomParameterExpressionBuilder.cs 21 22 95.5% 🟡
…/Core/src/Types/Resolvers/ParameterBindingResolver.cs 20 20 100.0% 🟢
Uncovered changed lines (JSON)
{
  "sha": "c06d35238ca3b7017082ecf9d42641d7d911cce6",
  "files": [
    { "path": "src/HotChocolate/Data/src/EntityFramework/ContextFactoryParameterExpressionBuilder.cs", "ranges": [[23, 23]] },
    { "path": "src/HotChocolate/Core/src/Types/Internal/CustomParameterExpressionBuilder.cs", "ranges": [[71, 71]] }
  ]
}

Project coverage: 52.5% (217149/413306 lines)

@glen-84

glen-84 commented Jul 3, 2026

Copy link
Copy Markdown
Member Author

Superseded by #10041, which fixes the same root cause with a cleaner, less invasive change: a single file, with explicit interface implementations on the generic CustomParameterExpressionBuilder<TArg>, so it avoids touching ParameterBindingResolver, PagingArguments, and ContextFactory. Closing in favor of it.

The only behavior this PR added that #10041 does not: a custom Func<ParameterInfo, bool> predicate builder fails loud with a clear error under the source generator (instead of silently falling through to a GraphQL argument). Leaving that here as a possible follow-up if guarding that case is considered worthwhile.

Drafted by Claude (Anthropic AI assistant).

@glen-84 glen-84 closed this Jul 3, 2026
@glen-84
glen-84 deleted the gai/sourcegen-custom-parameter-binding branch July 3, 2026 08:34
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.

2 participants