Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions src/Azure.DataApiBuilder.Mcp/Core/DynamicCustomTool.cs
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,7 @@ private JsonElement BuildInputSchema()
}

Dictionary<string, object> properties = new();
List<string> requiredParameters = new();
foreach ((string paramName, ParameterDefinition paramDef) in spDefinition.Parameters)
{
Dictionary<string, object> paramSchema = new()
Expand All @@ -370,6 +371,13 @@ private JsonElement BuildInputSchema()
};

properties[paramName] = paramSchema;

// A DB metadata parameter is required unless config marks it optional or supplies
// a default the engine applies when the caller omits it.
if (IsParameterRequired(paramDef.Required, paramDef.HasConfigDefault))
{
requiredParameters.Add(paramName);
}
}

Dictionary<string, object> schema = new()
Expand All @@ -378,6 +386,11 @@ private JsonElement BuildInputSchema()
["properties"] = properties
};

if (requiredParameters.Count > 0)
{
schema["required"] = requiredParameters;
}

return JsonSerializer.SerializeToElement(schema);
}

Expand All @@ -396,6 +409,7 @@ private JsonElement BuildInputSchemaFromConfig()
if (_entity.Source.Parameters != null && _entity.Source.Parameters.Any())
{
Dictionary<string, object> properties = (Dictionary<string, object>)schema["properties"];
List<string> requiredParameters = new();

foreach (ParameterMetadata param in _entity.Source.Parameters)
{
Expand All @@ -404,12 +418,40 @@ private JsonElement BuildInputSchemaFromConfig()
["type"] = new[] { "string", "number", "boolean", "null" },
["description"] = param.Description ?? $"Parameter {param.Name}"
};

// A parameter is required unless config marks it optional or supplies a default.
if (IsParameterRequired(param.Required, param.Default is not null))
{
requiredParameters.Add(param.Name);
}
}

if (requiredParameters.Count > 0)
{
schema["required"] = requiredParameters;
}
}

return JsonSerializer.SerializeToElement(schema);
}

/// <summary>
/// Determines whether a stored procedure parameter should be advertised as required in the
/// tool input schema. A parameter is required when configuration does not mark it optional
/// and does not provide a default value the engine can apply when the caller omits it.
/// </summary>
private static bool IsParameterRequired(bool? configuredRequired, bool hasDefault)
{
// If the engine can supply a default when the caller omits the parameter,
// it should not be advertised as required.
if (hasDefault)
{
return false;
}

return configuredRequired ?? true;
}
Comment thread
Copilot marked this conversation as resolved.

/// <summary>
/// Maps a .NET System.Type to the appropriate JSON Schema type string.
/// </summary>
Expand Down
77 changes: 77 additions & 0 deletions src/Service.Tests/Mcp/DynamicCustomToolMsSqlIntegrationTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,83 @@ public void InitializeMetadata_DescriptionIncludesConfigDefaults(string entityNa
$"'{paramName}' description should mention config default '{expectedDefault}'.");
}

/// <summary>
/// Validates that a DB-discovered parameter without a config default is advertised as required.
/// </summary>
[TestMethod]
public void InitializeMetadata_RequiredArray_IncludesParamWithoutDefault()
{
IServiceProvider serviceProvider = BuildQueryServiceProvider();
RuntimeConfigProvider configProvider = serviceProvider.GetRequiredService<RuntimeConfigProvider>();
Entity entity = configProvider.GetConfig().Entities["GetBook"];

DynamicCustomTool tool = new("GetBook", entity);
tool.InitializeMetadata(serviceProvider);

JsonElement schema = tool.GetToolMetadata().InputSchema;

Assert.IsTrue(schema.TryGetProperty("required", out JsonElement required),
"GetBook schema should expose a 'required' array.");

List<string> requiredParams = new();
foreach (JsonElement element in required.EnumerateArray())
{
requiredParams.Add(element.GetString()!);
}

CollectionAssert.Contains(requiredParams, "id",
"'id' has no config default and should be required.");
}

/// <summary>
/// Validates that parameters with config defaults (marked optional) are excluded from required.
/// </summary>
[TestMethod]
public void InitializeMetadata_RequiredArray_ExcludesParamsWithConfigDefaults()
{
IServiceProvider serviceProvider = BuildQueryServiceProvider();
RuntimeConfigProvider configProvider = serviceProvider.GetRequiredService<RuntimeConfigProvider>();
Entity entity = configProvider.GetConfig().Entities["InsertBook"];

DynamicCustomTool tool = new("InsertBook", entity);
tool.InitializeMetadata(serviceProvider);

JsonElement schema = tool.GetToolMetadata().InputSchema;

List<string> requiredParams = new();
if (schema.TryGetProperty("required", out JsonElement required))
{
foreach (JsonElement element in required.EnumerateArray())
{
requiredParams.Add(element.GetString()!);
}
}

CollectionAssert.DoesNotContain(requiredParams, "title",
"'title' has a config default and must not be required.");
CollectionAssert.DoesNotContain(requiredParams, "publisher_id",
"'publisher_id' has a config default and must not be required.");
}

/// <summary>
/// Validates that a zero-parameter SP does not emit a 'required' array.
/// </summary>
[TestMethod]
public void InitializeMetadata_ZeroParamSP_OmitsRequiredArray()
{
IServiceProvider serviceProvider = BuildQueryServiceProvider();
RuntimeConfigProvider configProvider = serviceProvider.GetRequiredService<RuntimeConfigProvider>();
Entity entity = configProvider.GetConfig().Entities["GetBooks"];

DynamicCustomTool tool = new("GetBooks", entity);
tool.InitializeMetadata(serviceProvider);

JsonElement schema = tool.GetToolMetadata().InputSchema;

Assert.IsFalse(schema.TryGetProperty("required", out _),
"Zero-param SP should not include a 'required' array.");
}

#endregion

/// <summary>
Expand Down
148 changes: 148 additions & 0 deletions src/Service.Tests/Mcp/DynamicCustomToolTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
};
Comment thread
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()

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.

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 required array.

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.

@copilot check if this is covered already or add this test.

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.

Added explicit config-fallback coverage in 0c38640. The new test verifies that a parameter with Required = true and a config default is excluded from inputSchema.required, matching the current default-application behavior.

{
// 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.
Expand Down