Skip to content

Conversation

@jumacabo
Copy link
Contributor

What does this PR do?

Return useful error message for AIs if we fail to deserialize custom data types, the error message contains instructions for getting the table schema, cast them to out-of-the-box data types and retry the operation.

Captura de pantalla 2025-10-31 132257

GitHub issue number?

#279

Pre-merge Checklist

  • Required for All PRs
    • Read contribution guidelines
    • PR title clearly describes the change
    • Commit history is clean with descriptive messages (cleanup guide)
    • Added comprehensive tests for new/modified functionality
    • Updated servers/Azure.Mcp.Server/CHANGELOG.md and/or servers/Fabric.Mcp.Server/CHANGELOG.md for product changes (features, bug fixes, UI/UX, updated dependencies)
  • For MCP tool changes:
    • One tool per PR: This PR adds or modifies only one MCP tool for faster review cycles
    • Updated servers/Azure.Mcp.Server/README.md and/or servers/Fabric.Mcp.Server/README.md documentation
    • Validate README.md changes using script at eng/scripts/Process-PackageReadMe.ps1. See Package README
    • Updated command list in /servers/Azure.Mcp.Server/docs/azmcp-commands.md and/or /docs/fabric-commands.md
    • Run .\eng\scripts\Update-AzCommandsMetadata.ps1 to update tool metadata in azmcp-commands.md (required for CI)
    • For new or modified tool descriptions, ran ToolDescriptionEvaluator and obtained a score of 0.4 or more and a top 3 ranking for all related test prompts
    • For new tools associated with Azure services or publicly available tools/APIs/products, add URL to documentation in the PR description
  • Extra steps for Azure MCP Server tool changes:
    • Updated test prompts in /servers/Azure.Mcp.Server/docs/e2eTestPrompts.md
    • 👉 For Community (non-Microsoft team member) PRs:
      • Security review: Reviewed code for security vulnerabilities, malicious code, or suspicious activities before running tests (crypto mining, spam, data exfiltration, etc.)
      • Manual tests run: added comment /azp run mcp - pullrequest - live to run Live Test Pipeline

@jumacabo jumacabo self-assigned this Oct 31, 2025
@jumacabo jumacabo added the bug Something isn't working label Oct 31, 2025
@jumacabo jumacabo added the tools-Postgres PostgreSQL label Oct 31, 2025
@jumacabo jumacabo marked this pull request as ready for review October 31, 2025 12:31
@jumacabo jumacabo requested a review from kk-src as a code owner October 31, 2025 12:31
Copilot AI review requested due to automatic review settings October 31, 2025 12:31
@jumacabo jumacabo requested review from a team and xiangyan99 as code owners October 31, 2025 12:31
Copy link
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

This PR adds error handling for unsupported complex PostgreSQL data types by catching InvalidCastException during query result processing. When the Npgsql data reader encounters non-standard data types (such as extension or user-defined types) that cannot be converted to strings, the code now throws a CommandValidationException with detailed guidance for the user on how to resolve the issue.

  • Added try-catch block around reader[i]?.ToString() to handle InvalidCastException when reading query results
  • Throws CommandValidationException with actionable error message including column name and remediation steps
  • Added necessary imports: System.Net and Azure.Mcp.Core.Exceptions

@joshfree joshfree added this to the 2025-11 milestone Oct 31, 2025
@joshfree joshfree moved this from Untriaged to In Progress in Azure MCP Server Oct 31, 2025
Copy link
Contributor

@kk-src kk-src left a comment

Choose a reason for hiding this comment

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

Can you please add test for this with the PR.

@jumacabo
Copy link
Contributor Author

jumacabo commented Nov 7, 2025

Can you please add test for this with the PR.

Hi @kk-src , this simple request had more work than what I have imagined, turns out that the PostgresService class was not prepared for mocking (it creates instances of services directly inside, also some of them cannot be mocked because they are sealed classes, etc.) and all of it's corresponding unit tests (in PostgresServiceParameterizedQueryTests) are not doing what they say they are doing, in fact, throwing an exception at the beginning of their corresponding PostgresService methods don't make them fail.

I have refactored the code so we can use mocked services for Entra and Npgsql, also created a FakeDbDataReader to simulate getting data from Postgres and also created a new set of unit tests that do validate the output of ExecuteQueryAsync (see PostgresServiceTests)

If you agree with the changes please approve the PR, in any case when you have a chance would like to talk to you about the pre-existing unit tests and what we should do to fix them.

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

Labels

bug Something isn't working tools-Postgres PostgreSQL

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

4 participants