Cherry pick #3600 #3601 #3619#3629
Closed
aaronburtle wants to merge 3 commits into
Closed
Conversation
## Why make this change? - Closes #3588 `ExecuteEntityTool` validated parameters against config-side `Source.Parameters` only, rejecting DB-discovered parameters that had no config entry. Defaults were applied in an all-or-nothing fashion (only when the user supplied zero parameters). ## What is this change? - Parameter validation now uses `StoredProcedureDefinition.Parameters` (the merged DB metadata) as source of truth, accepting any parameter the DB declares. - Default application iterates `ParameterDefinition.HasConfigDefault`/`ConfigDefaultValue` per-missing-parameter, so partial user input gets remaining defaults filled in. This is an intentional behavioral change from the previous all-or-nothing guard. - Safe-cast `dbObject` to `DatabaseStoredProcedure` with graceful error instead of unconditional cast. ## How was this tested? - [x] Unit Tests | Test | Scenario | |------|----------| | `AcceptsDbDiscoveredParam` | Param in DB metadata but not config - accepted | | `RejectsInvalidParamName` (x2) | Param not in DB metadata - error | | `AcceptsMultipleValidParams` | Multiple valid params pass validation | | `RejectsRequest_WhenAnyParamInvalid` | Mix of valid + invalid params - rejected | | `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 | | `RejectsNonStoredProcedureEntity` | Non-SP entity types return error | | `ReturnsError_WhenEntityNotFound` | Non-existent entity returns EntityNotFound | - [x] Integration Tests | Test | Scenario | |------|----------| | Success (x4 DataRows) | GetBook(id), InsertBook(null), InsertBook(override), GetBooks(zero-param) | | `GetBookById_ReturnsRecord` | Validates returned data matches expected | | `InsertBookWithDefaults_ExecutesSuccessfully` | Config defaults produce valid insert | | Rejection (x2 DataRows) | Invalid param names rejected with clear error | - [x] Manual Testing | Category | Result | |----------|--------| | Valid DB-discovered params | Pass | | Invalid/unknown/extra params rejected | Pass | | Config defaults / partial params / override | Pass | | Non-SP entity type gating | Pass | | Successful SP execution (zero/single/multi-param) | Pass | | Non-existent entity | Pass | | SQL injection (parameterized) | Pass | | Type mismatch | Pass | | Permission denied | Pass | | No results for valid param | Pass | ## Sample Request(s) NA (cherry picked from commit 5d6faf9)
## Why make this change? - Closes #3589 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? - [x] 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 | - [x] 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 | - [x] 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 --------- Co-authored-by: Anusha Kolan <anushakolan10@gmail.com> (cherry picked from commit a085d86)
#3619) ## Why make this change? - Closes #3615 Custom MCP tools used a permissive multi-type array for all parameters in the input schema. MCP clients had no type information to guide users or validate inputs before execution. ## What is this change? - **InitializeMetadata**: Resolves StoredProcedureDefinition.Parameters from the metadata provider and builds a typed JSON Schema (integer/number/boolean/string) for each DB-discovered parameter. - **Fallback**: Falls back to config-based permissive schema when DB metadata is unavailable. - **Both modes**: Applied in HTTP mode (McpToolRegistryInitializer) and stdio mode (McpStdioHelper) via shared `McpToolRegistry.InitializeAndRegisterTools` helper. - **Default descriptions**: Surfaces `(default: value)` in parameter descriptions when HasConfigDefault is set. ## How was this tested? - [x] Unit Tests | Test | Scenario | |------|----------| | UsesDbMetadata_WhenInitialized | Schema uses typed params after InitializeMetadata | | FallsBackToConfig_WhenDbMetadataUnavailable | Permissive array when no metadata factory | | UsesConfigSchema_WhenNotInitialized | Config schema used without InitializeMetadata call | | IncludesAllDbDiscoveredParams | Params not in config but in DB appear in schema | | MapsSystemTypesToJsonSchemaTypes (x12) | int/long/short/byte to integer, float/double/decimal to number, bool to boolean, string/Guid/DateTime to string | | UsesPermissiveType_WhenSystemTypeIsNull | Null SystemType falls back to multi-type array | | IncludesDefaultInDescription | (default: value) in description | | ZeroParamSP_ReturnsEmptyProperties | Empty properties for zero-param SP | - [x] Integration Tests | Test | Scenario | |------|----------| | InitializeMetadata_SchemaReflectsDbParameterTypes (x3) | int->integer, varchar->string mapping against live DB | | InitializeMetadata_ZeroParamSP_HasEmptyProperties | Zero-param SP has empty properties from DB metadata | | InitializeMetadata_DescriptionIncludesConfigDefaults (x2) | Config defaults (randomX, 1234) in descriptions | - [x] Manual Testing | Tool | Before | After | |------|--------|-------| | get_book(id) | type: [...] | type: integer | | insert_book(title, publisher_id) | permissive | string, integer + defaults in desc | | update_book_title(id, title) | permissive | integer, string + defaults in desc | | get_books (zero-param) | empty | empty | | End-to-end execution get_book(id=1) | N/A | returns book record | ## Sample Request(s) tools/list response (get_book schema after fix): ```json { "name": "get_book", "inputSchema": { "type": "object", "properties": { "id": { "type": "integer", "description": "Parameter id" } } } } ``` (cherry picked from commit b84d8d6)
aaronburtle
requested review from
Alekhya-Polavarapu,
Aniruddh25,
JerryNixon,
RubenCerna2079,
anushakolan,
rusamant,
sourabh1007,
souvikghosh04,
stuartpa and
vadeveka
as code owners
github-project-automation
Bot
moved this from Review In Progress
to Done
in Data API builder
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why make this change?
Closes #3628
What is this change?
This cherry picks PRs:
#3600
#3601
#3619
How was this tested?
Built locally, run against normal test suite.