Skip to content

DynamicCustomTool: DB metadata based parameter support#3601

Open
souvikghosh04 wants to merge 12 commits into
mainfrom
Usr/sogh/cust-db-param-support
Open

DynamicCustomTool: DB metadata based parameter support#3601
souvikghosh04 wants to merge 12 commits into
mainfrom
Usr/sogh/cust-db-param-support

Conversation

@souvikghosh04
Copy link
Copy Markdown
Contributor

@souvikghosh04 souvikghosh04 commented May 19, 2026

Why make this change?

DynamicCustomTool (per-entity custom MCP tools) used entityConfig.Source.Parameters / ParameterMetadata for parameter validation and defaults. This meant:

  • Parameters discovered only via DB metadata (not listed in config) were rejected.
  • Defaults were applied all-or-nothing instead of per-missing-parameter.

What is this change?

  • Validate against DB metadata: After authorization, safe-cast dbObject to DatabaseStoredProcedure and validate user-supplied params against spDefinition.Parameters (source of truth populated by FillSchemaForStoredProcedureAsync).
  • Per-param config defaults: Iterate spDefinition.Parameters; for each missing param with HasConfigDefault == true, inject ConfigDefaultValue. User-supplied params always take precedence.
  • Removed old logic: Replaced entityConfig.Source.Parameters / ParameterMetadata loop.

How was this tested?

  • Unit Tests
Test Scenario
AcceptsDbDiscoveredParam Param in DB metadata but not config - accepted
RejectsInvalidParamName (x2) Param not in DB metadata - error
AppliesConfigDefaults Missing params filled from HasConfigDefault/ConfigDefaultValue
UserParamsOverride User-supplied values take precedence over config defaults
DoesNotInjectWithoutDefault Param without HasConfigDefault is not injected
ZeroParamSP SP with no parameters succeeds with no input
  • Integration Tests
Test Scenario
Success (x4 DataRows) GetBook(id), InsertBook(null), InsertBook(override), GetBooks(zero-param)
GetBookById_ReturnsRecord Validates returned data matches expected
Rejection (x2 DataRows) Invalid param names rejected with clear error
  • Manual Testing
Category Result
Valid params / no results Pass
Type mismatch - error Pass
Zero-param SPs Pass
Config defaults / partial params Pass
Multi-param update Pass
SQL injection (parameterized) Pass
Boundary values (-1, 0, MAX_INT) Pass
Empty/long/unicode strings Pass
Read-after-write, FK errors, cross-entity Pass

Sample Request(s)

NA

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds DB-metadata-based parameter validation and per-parameter default application for MCP stored-procedure tools, addressing scenarios where parameters are discovered only via DB metadata and defaults should apply only to missing params.

Changes:

  • Validate user-supplied parameters against StoredProcedureDefinition.Parameters (DB metadata) in ExecuteEntityTool and DynamicCustomTool.
  • Apply config defaults per missing parameter using ParameterDefinition.HasConfigDefault/ConfigDefaultValue.
  • Add/extend unit + MSSQL integration tests and enable MCP custom tools in MSSQL config generator commands.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
src/Azure.DataApiBuilder.Mcp/Core/DynamicCustomTool.cs Switches validation/defaulting to DB metadata definitions.
src/Azure.DataApiBuilder.Mcp/BuiltInTools/ExecuteEntityTool.cs Same validation/defaulting changes for the built-in execute tool.
src/Service.Tests/Mcp/ExecuteEntityToolTests.cs New unit tests covering validation/default application + gating.
src/Service.Tests/Mcp/ExecuteEntityToolMsSqlIntegrationTests.cs New end-to-end MSSQL integration tests for execute tool behavior.
src/Service.Tests/Mcp/DynamicCustomToolTests.cs Adds execution-time unit tests for DynamicCustomTool behavior.
src/Service.Tests/Mcp/DynamicCustomToolMsSqlIntegrationTests.cs New MSSQL integration tests for DynamicCustomTool behavior.
config-generators/mssql-commands.txt Enables --mcp.custom-tool true for stored-procedure entities used by tests.

Comment thread src/Service.Tests/Mcp/ExecuteEntityToolTests.cs Outdated
Comment thread src/Service.Tests/Mcp/ExecuteEntityToolTests.cs
Comment thread src/Service.Tests/Mcp/ExecuteEntityToolMsSqlIntegrationTests.cs Outdated
Comment thread src/Service.Tests/Mcp/ExecuteEntityToolMsSqlIntegrationTests.cs Outdated
Comment thread src/Service.Tests/Mcp/DynamicCustomToolMsSqlIntegrationTests.cs Outdated
Comment thread src/Service.Tests/Mcp/DynamicCustomToolMsSqlIntegrationTests.cs Outdated
Comment thread src/Azure.DataApiBuilder.Mcp/BuiltInTools/ExecuteEntityTool.cs Outdated
Comment thread src/Service.Tests/Mcp/DynamicCustomToolMsSqlIntegrationTests.cs
Comment thread src/Azure.DataApiBuilder.Mcp/BuiltInTools/ExecuteEntityTool.cs Outdated
Comment thread src/Service.Tests/Mcp/DynamicCustomToolTests.cs
/// is accepted even if it has no config-side entry.
/// </summary>
[TestMethod]
public async Task ExecuteEntity_AcceptsDbDiscoveredParam_NotInConfig()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why do we have ExecuteEntityTool tests in this PR?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This branch was created on top of the ExecuteEntityTool fix (PR #3600) since DynamicCustomTool follows the same pattern. Because #3600 hasn't merged to main yet, GitHub shows the ExecuteEntityTool files in this diff as well.

Once #3600 merges, I'll rebase this branch onto main and those files will drop out of the diff automatically, avoiding a force-push or rewrite.

Copy link
Copy Markdown
Collaborator

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

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

Approving with suggestions on fixing the tests, and few questions.

@anushakolan
Copy link
Copy Markdown
Contributor

The snapshot file needs to be updated; I am approving this PR to prevent any blockers from my side.

@souvikghosh04
Copy link
Copy Markdown
Contributor Author

waiting for #3600 to merge as this was branched out from that with related fixes.

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

Projects

Status: Review In Progress

Development

Successfully merging this pull request may close these issues.

Parameter behaviour in MCP tool: DynamicCustomTool

5 participants