Use config parameter descriptions in GraphQL stored procedure args - #3733
Use config parameter descriptions in GraphQL stored procedure args#3733anushakolan wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Updates GraphQL stored-procedure argument description generation so schema docs prefer runtime config parameter descriptions, improving introspection/docs quality for API consumers (per #3500).
Changes:
- Updated
GraphQLStoredProcedureBuilderto resolve argument descriptions with precedence: config description → DB/definition description → default fallback text. - Added unit tests validating config-description precedence and DB-description fallback for stored-procedure args.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Service.GraphQLBuilder/GraphQLStoredProcedureBuilder.cs | Applies new description precedence logic when building stored-procedure GraphQL argument definitions. |
| src/Service.Tests/GraphQLBuilder/Sql/StoredProcedureBuilderTests.cs | Adds regression tests for config-vs-DB description precedence on stored-procedure arguments. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| { | ||
| Parameters = new() | ||
| { | ||
| { parameterName, new() { SystemType = typeof(string), Description = dbDescription } } |
There was a problem hiding this comment.
can we really get description of a parameter from database metadata?
the definition here maybe showing the property Description, but are you sure it can get populated by querying the database? Did you test that?
Aniruddh25
left a comment
There was a problem hiding this comment.
Approved, but left some questions.
| } | ||
|
|
||
| [TestMethod] | ||
| public void StoredProcedure_ParameterDescription_UsesConfigDescription() |
There was a problem hiding this comment.
This test doesn't exercise the production path that appears responsible for the underlying issue. During normal initialization, both SQL metadata-provider implementations copy ParameterMetadata.Description onto ParameterDefinition.Description before GraphQL schema generation. Therefore, the old builder should also return the config description after normal metadata initialization.
Here the test bypasses that merge and manually creates a state where paramMetadata.Description == configDescription while definition.Description == dbDescription. That proves the new builder-level precedence, but it does not reproduce the reported production bug.
Could we add a regression test that starts from runtime config, initializes the metadata provider, builds the GraphQL schema, and asserts the argument description through introspection? Ideally, that test should fail on the PR's base commit. If it already passes on the base, we should identify and cover the specific path.
| parameterTypeNode = new NonNullTypeNode((INullableTypeNode)parameterTypeNode); | ||
| } | ||
|
|
||
| string parameterDescription = !string.IsNullOrWhiteSpace(paramMetadata?.Description) |
There was a problem hiding this comment.
This direct config lookup creates first-wins behavior for descriptions because FirstOrDefault() selects the first matching parameter. Metadata hydration has different behavior: it iterates the complete parameter list and repeatedly assigns to the same ParameterDefinition, so the last duplicate wins.
The parameter-array schema currently does not enforce uniqueness by name. For a config containing two title entries with different descriptions, GraphQL would now expose the first description while OpenAPI/MCP can expose the last merged description.
Could we either reject duplicate parameter names during semantic config validation or use the normalized ParameterDefinition consistently here? Rejecting duplicates would be preferable because it also removes ambiguity for required and default.
| } | ||
|
|
||
| [TestMethod] | ||
| public void StoredProcedure_ParameterDescription_FallsBackToDefaultText() |
There was a problem hiding this comment.
nit:
Could we add cases where the config description is "" or " " and the definition description is populated, plus a case where both values are empty/whitespace? That would verify both whitespace fallback branches and protect the behavior that this PR is introducing.
Why make this change?
What is this change?
source.parameters[].descriptionfrom runtime config (when present)parameters for <stored-procedure> stored-procedure)How was this tested?
Focused unit test run:
Sample Request(s)