Skip to content

Conversation

vincent0426
Copy link

Motivation and Context

Closes #1402

pydantic treats Optional type as required if there is no default value

import json
from pydantic import BaseModel, Field
from typing import Optional


class TestModel(BaseModel):
    param_1: str | None = Field(description="Optional without default")
    param_2: Optional[str] = Field(description="Optional without default")

schema = TestModel.model_json_schema()
print("Pydantic schema:")
print(json.dumps(schema, indent=2))

output

Pydantic schema:
{
  "properties": {
    "param_1": {
      "anyOf": [
        {
          "type": "string"
        },
        {
          "type": "null"
        }
      ],
      "description": "Optional without default",
      "title": "Param 1"
    },
    "param_2": {
      "anyOf": [
        {
          "type": "string"
        },
        {
          "type": "null"
        }
      ],
      "description": "Optional without default",
      "title": "Param 2"
    }
  },
  "required": [
    "param_1",
    "param_2"
  ],
  "title": "TestModel",
  "type": "object"
}                

How Has This Been Tested?

Unit tested on 5 scenarios

  • required
  • optional with type | none
  • optional with Optional[type]

Breaking Changes

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

@vincent0426 vincent0426 requested review from a team and dsp-ant September 26, 2025 04:32
@felixweinberger
Copy link
Contributor

PR attempting to resolve the same issue: #1408

Copy link
Contributor

@felixweinberger felixweinberger left a comment

Choose a reason for hiding this comment

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

Between this and #1408 if it turns out we actually need this change and not a fix to our Pydantic usage, I'd probably prefer this one for greater simplicity.

Will figure out whether this is the right approach though - there may be ways we can better use Pydantic tools rather than work around them here.

@felixweinberger felixweinberger added bug Something isn't working needs more eyes Needs alignment among maintainers whether this is something we want to add needs confirmation Needs confirmation that the PR is actually required or needed. labels Sep 29, 2025
@Kludex
Copy link
Member

Kludex commented Sep 29, 2025

I don't think we should add a default value if the type accepts None.

@vincent0426
Copy link
Author

vincent0426 commented Oct 1, 2025

I think pydantic treats Optional[str] as required, do we not want to treat it as Optional if specify

https://github.com/pydantic/pydantic/blob/main/docs/migration.md#snippet_4:~:text=f2%3A%20str%20%3D%20%27abc%27-,Required%2C%20can%20be%20None,-f3%3A%20Optional%5Bstr

@Kludex
Copy link
Member

Kludex commented Oct 1, 2025

I think pydantic treats Optional[str] as required

Yes, which is correct.


Optional keyword doesn't mean optional, it means nullable in Python.

@Kludex Kludex closed this Oct 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working needs confirmation Needs confirmation that the PR is actually required or needed. needs more eyes Needs alignment among maintainers whether this is something we want to add

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SDK does not handle optional parameters in tools properly

3 participants