-
Notifications
You must be signed in to change notification settings - Fork 347
Expose required array in custom MCP tool input schema
#3688
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
e662a6c
479a235
1bb4c99
8d34456
2bf11bb
0c38640
64cd204
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -710,6 +710,154 @@ public void InitializeMetadata_ZeroParamSP_ReturnsEmptyProperties() | |
| Assert.AreEqual(0, props.EnumerateObject().Count()); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// DB-discovered parameters with no config default/override are advertised as required. | ||
| /// </summary> | ||
| [TestMethod] | ||
| public void InitializeMetadata_IncludesRequiredArray_ForParamsWithoutDefaults() | ||
| { | ||
| // Arrange | ||
| Dictionary<string, ParameterDefinition> dbParams = new() | ||
| { | ||
| ["productId"] = new() { SystemType = typeof(int) } | ||
| }; | ||
|
|
||
| // Act | ||
| JsonElement schema = InitializeAndGetSchema(dbParams); | ||
|
|
||
| // Assert | ||
| Assert.IsTrue(schema.TryGetProperty("required", out JsonElement required), | ||
| "Schema should expose a 'required' array for parameters without defaults."); | ||
| CollectionAssert.AreEquivalent( | ||
| new[] { "productId" }, | ||
| EnumerateStrings(required), | ||
| "'productId' should be listed as required."); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Parameters that have a config default or are explicitly marked optional are excluded | ||
| /// from the required array, while parameters without defaults remain required. | ||
| /// </summary> | ||
| [TestMethod] | ||
| public void InitializeMetadata_ExcludesParamsWithDefaultsOrOptionalFlag_FromRequired() | ||
| { | ||
| // Arrange | ||
| Dictionary<string, ParameterDefinition> dbParams = new() | ||
| { | ||
| ["id"] = new() { SystemType = typeof(int) }, | ||
| ["title"] = new() { SystemType = typeof(string), HasConfigDefault = true, ConfigDefaultValue = "randomX" }, | ||
| ["category"] = new() { SystemType = typeof(string), Required = true, HasConfigDefault = true, ConfigDefaultValue = "defaultCategory" }, | ||
| ["publisher_id"] = new() { SystemType = typeof(int), Required = false } | ||
| }; | ||
|
Copilot marked this conversation as resolved.
|
||
|
|
||
| // Act | ||
| JsonElement schema = InitializeAndGetSchema(dbParams); | ||
|
|
||
| // Assert | ||
| Assert.IsTrue(schema.TryGetProperty("required", out JsonElement required)); | ||
| CollectionAssert.AreEquivalent( | ||
| new[] { "id" }, | ||
| EnumerateStrings(required), | ||
| "Only 'id' (no default, not marked optional) should be required."); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// A zero-parameter SP must not emit a 'required' array. | ||
| /// </summary> | ||
| [TestMethod] | ||
| public void InitializeMetadata_ZeroParamSP_OmitsRequiredArray() | ||
| { | ||
| // Arrange & Act | ||
| JsonElement schema = InitializeAndGetSchema(new Dictionary<string, ParameterDefinition>()); | ||
|
|
||
| // Assert | ||
| Assert.IsFalse(schema.TryGetProperty("required", out _), | ||
| "Zero-param SP should not include a 'required' array."); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// When falling back to config-based schema, parameters without a default are required. | ||
| /// </summary> | ||
| [TestMethod] | ||
| public void GetToolMetadata_ConfigFallback_MarksParamsWithoutDefaultsRequired() | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There should be a test for config defining defaults and also having the parameter specified as required. In this case, the parameter should still be present in the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @copilot check if this is covered already or add this test.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added explicit config-fallback coverage in 0c38640. The new test verifies that a parameter with |
||
| { | ||
| // Arrange - config declares one required param and one with a default | ||
| ParameterMetadata[] parameters = new[] | ||
| { | ||
| new ParameterMetadata { Name = "userId" }, | ||
| new ParameterMetadata { Name = "tenant", Default = "contoso" } | ||
| }; | ||
| Entity entity = CreateTestStoredProcedureEntity(parameters: parameters); | ||
| DynamicCustomTool tool = new("GetUser", entity); | ||
|
|
||
| // Act - no InitializeMetadata call, so the config-based schema is used | ||
| JsonElement schema = tool.GetToolMetadata().InputSchema; | ||
|
|
||
| // Assert | ||
| Assert.IsTrue(schema.TryGetProperty("required", out JsonElement required)); | ||
| CollectionAssert.AreEquivalent( | ||
| new[] { "userId" }, | ||
| EnumerateStrings(required), | ||
| "Only 'userId' (no default) should be required in the config-based schema."); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Config fallback should exclude parameters with defaults from the required array even when | ||
| /// they are explicitly marked required. | ||
| /// </summary> | ||
| [TestMethod] | ||
| public void GetToolMetadata_ConfigFallback_ExcludesRequiredParamWhenDefaultExists() | ||
| { | ||
| // Arrange | ||
| ParameterMetadata[] parameters = new[] | ||
| { | ||
| new ParameterMetadata { Name = "userId", Required = true }, | ||
| new ParameterMetadata { Name = "tenant", Required = true, Default = "contoso" } | ||
| }; | ||
| Entity entity = CreateTestStoredProcedureEntity(parameters: parameters); | ||
| DynamicCustomTool tool = new("GetUser", entity); | ||
|
|
||
| // Act | ||
| JsonElement schema = tool.GetToolMetadata().InputSchema; | ||
|
|
||
| // Assert | ||
| Assert.IsTrue(schema.TryGetProperty("required", out JsonElement required)); | ||
| CollectionAssert.AreEquivalent( | ||
| new[] { "userId" }, | ||
| EnumerateStrings(required), | ||
| "Parameters with config defaults should not be required even when marked required."); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Helper: Parses the "required" array values into a list of strings. | ||
| /// </summary> | ||
| private static List<string> EnumerateStrings(JsonElement array) | ||
| { | ||
| List<string> values = new(); | ||
| foreach (JsonElement element in array.EnumerateArray()) | ||
| { | ||
| values.Add(element.GetString()!); | ||
| } | ||
|
|
||
| return values; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Helper: Creates a DynamicCustomTool, initializes it with mocked DB metadata, and returns | ||
| /// the full input schema element. | ||
| /// </summary> | ||
| private static JsonElement InitializeAndGetSchema( | ||
| Dictionary<string, ParameterDefinition> dbParameters, | ||
| string entityName = "TestSP") | ||
| { | ||
| Entity entity = CreateTestStoredProcedureEntity(); | ||
| DynamicCustomTool tool = new(entityName, entity); | ||
| IServiceProvider sp = BuildServiceProviderForMetadata(entityName, dbParameters); | ||
|
|
||
| tool.InitializeMetadata(sp); | ||
| return tool.GetToolMetadata().InputSchema; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Helper: Creates a DynamicCustomTool, initializes it with mocked DB metadata, and returns | ||
| /// the "properties" element from the resulting input schema. | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.