Skip to content

Fix MCP orderby validation to return actionable argument errors - #3816

Open
aaronburtle wants to merge 2 commits into
mainfrom
dev/aaronburtle/MCP-incompatible-shape
Open

aaronburtle wants to merge 2 commits into
mainfrom
dev/aaronburtle/MCP-incompatible-shape

Conversation

@aaronburtle

Copy link
Copy Markdown
Contributor

Why make this change?

Closes #3810

What is this change?

  • Addresses the opaque UnexpectedError responses reported in #3810.
  • Validates orderby JSON types before accessing their values:
    • read_records requires an array of non-empty strings and validates every array member before metadata resolution.
    • aggregate_records requires a direction string, while preserving its existing null/blank handling.
  • Invalid shapes now return InvalidArguments with the parameter name, expected format, and an example.
  • Preserves the existing ordering contracts:
    • Read operations sort by one or more entity fields.
    • Grouped aggregates sort by the computed aggregate value, defaulting to descending order.
    • Scalar aggregate ordering remains ignored without grouping, preserving the behavior introduced by #3294.

How was this tested?

  • Integration Tests
  • Unit Tests

27 new error-validation regression cases failed against the original implementation and passed after the fix.

Sample Request(s)

Invalid scalar ordering for read_records

{
  "jsonrpc": "2.0",
  "id": 1,
  "method": "tools/call",
  "params": {
    "name": "read_records",
    "arguments": {
      "entity": "Book",
      "orderby": "id desc"
    }
  }
}

Previously returned UnexpectedError. Now returns InvalidArguments explaining that orderby must be an array of non-empty strings.

The supported form remains "orderby": ["id desc"].

Invalid array ordering for aggregate_records

{
  "jsonrpc": "2.0",
  "id": 2,
  "method": "tools/call",
  "params": {
    "name": "aggregate_records",
    "arguments": {
      "entity": "Book",
      "function": "count",
      "groupby": ["title"],
      "orderby": ["desc"]
    }
  }
}

Previously returned UnexpectedError. Now returns InvalidArguments explaining that orderby must be a string containing asc or desc.

Copilot AI left a comment

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.

🟢 Approval recommended

Validation changes are covered by unit and HTTP-level regression tests, with no unresolved review issues.

Pull request overview

Improves MCP orderby validation by returning actionable InvalidArguments errors for malformed inputs.

Changes:

  • Validates read and aggregate ordering types.
  • Preserves existing ordering behavior and defaults.
  • Adds unit and HTTP regression coverage.
File summaries
File Description
src/Service.Tests/Mcp/McpServerConfigurationTests.cs Adds HTTP-level MCP validation tests.
src/Service.Tests/Mcp/BuiltInDmlToolValidationTests.cs Tests read ordering shapes and messages.
src/Service.Tests/Mcp/AggregateRecordsToolTests.cs Tests aggregate ordering validation and compatibility.
src/Azure.DataApiBuilder.Mcp/BuiltInTools/ReadRecordsTool.cs Validates and materializes orderby arrays.
src/Azure.DataApiBuilder.Mcp/BuiltInTools/AggregateRecordsTool.cs Validates aggregate orderby JSON types.
Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

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.

[Bug]: MCP orderby takes incompatible shapes in read_records and aggregate_records, and the mismatch returns an opaque UnexpectedError

4 participants