Skip to content

Fix the null value assignment in GetValueOrDefault()#457

Merged
ericshape merged 5 commits intomicrosoft:mainfrom
ericshape:fix_get_value_default_bug
Sep 16, 2025
Merged

Fix the null value assignment in GetValueOrDefault()#457
ericshape merged 5 commits intomicrosoft:mainfrom
ericshape:fix_get_value_default_bug

Conversation

@ericshape
Copy link
Copy Markdown
Member

@ericshape ericshape commented Sep 16, 2025

What does this PR do?

Fix the null value assignment in GetValueOrDefault()
context: #437 (comment)

when the optional value passed in var options = BindOptions(parseResult); it will directly assign long? to 0, int? to 0, and bool? to false. however, they are null optional values.

i.e. when I run 'azmcp sql db create -subscription sub --resource-group rg --server server1 --database testdb'

the GetValueOrDefault will auto assign optional values:

long? --max-size-bytes to 0,
int? sku-capacity to 0,
and bool? --zone-redundan to false

this bug fix is blocking #434

GitHub issue number?

#437

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
    • Updated command list in /docs/azmcp-commands.md and/or /docs/fabric-commands.md
    • 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
  • Extra steps for Azure MCP Server tool changes:
    • Updated test prompts in /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

@ericshape ericshape marked this pull request as ready for review September 16, 2025 18:29
@ericshape ericshape requested a review from a team as a code owner September 16, 2025 18:29
Copy link
Copy Markdown
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 fixes a bug in the GetValueOrDefault() method where optional nullable values were incorrectly being assigned default values (0 for int/long, false for bool) instead of null when not explicitly provided by the user.

  • Improves the HasOptionResult() method to properly handle bool options as switches or with explicit values
  • Updates GetValueOrDefault() to check if an option was explicitly provided before returning values
  • Adds comprehensive test coverage for the fixed behavior across different nullable types

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
CommandResultExtensions.cs Enhanced option presence detection and fixed null value assignment logic
CommandResultExtensionsTests.cs Added comprehensive test coverage for nullable options with explicit and missing values

Comment thread core/Azure.Mcp.Core/src/Extensions/CommandResultExtensions.cs
Comment thread core/Azure.Mcp.Core/src/Extensions/CommandResultExtensions.cs Outdated
@joshfree joshfree moved this from Untriaged to In Progress in Azure MCP Server Sep 16, 2025
@ericshape
Copy link
Copy Markdown
Member Author

once the review is done, I will add fix description into Changelog md file.
Changelog file is easy to have conflicts. I will add before merge

@jongio
Copy link
Copy Markdown
Contributor

jongio commented Sep 16, 2025

/azp run mcp - pullrequest - live

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@ericshape
Copy link
Copy Markdown
Member Author

/azp run mcp - pullrequest - live

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@ericshape ericshape enabled auto-merge (squash) September 16, 2025 23:47
@ericshape ericshape merged commit 5b5bf8b into microsoft:main Sep 16, 2025
26 checks passed
@github-project-automation github-project-automation Bot moved this from In Progress to Done in Azure MCP Server Sep 16, 2025
@ericshape ericshape deleted the fix_get_value_default_bug branch September 17, 2025 23:11
colbytimm pushed a commit to colbytimm/microsoft-mcp that referenced this pull request Sep 27, 2025
* Fix the null value assignment in GetValueOrDefault

* fix format issue

* fix Missing --limit uses the default value of 10 from DefaultValueFactory

* fix the failed empty string unit test case in storage unit test

* address comments about nullable type and default value is null
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

GetValueOrDefault optional logic would assign default value if the pass-in optional parameter is empty

4 participants