Skip to content

fix: Return empty list instead of None from _get_required_fields for schemas without properties#6377

Open
ItsMacto wants to merge 3 commits into
google:mainfrom
ItsMacto:fix-get-required-fields-empty-schema
Open

fix: Return empty list instead of None from _get_required_fields for schemas without properties#6377
ItsMacto wants to merge 3 commits into
google:mainfrom
ItsMacto:fix-get-required-fields-empty-schema

Conversation

@ItsMacto

Copy link
Copy Markdown
Contributor

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

1. Link to an existing issue (if applicable):

2. Or, if no issue exists, describe the change:

Problem:
_get_required_fields in tools/_function_parameter_parse_util.py is annotated -> list[str] but has a bare return when the schema has no properties, so it returns None. Any caller that relies on the declared contract (e.g. iterating the result, as reported in #5920) gets TypeError: 'NoneType' object is not iterable. A maintainer confirmed the bare return as the culprit in the issue thread and endorsed return [].

Solution:
Return [] instead, honoring the declared return type. One-line change.

One honest scoping note: the two current callers in _automatic_function_calling_util.py guard the call behind a truthy properties dict, so today the empty branch is only reachable by calling the util directly (a function whose sole parameter is tool_context produces parameters=None rather than required=None). The fix corrects the contract violation at its source and hardens the util for present and future callers; the regression test pins the contract at the util level, and an added declaration-path test documents the tool_context-only behavior.

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

Three tests added to tests/unittests/tools/test_build_function_declaration.py:

  • _get_required_fields returns [] (not None) for a schema without properties — fails on the unfixed code (assert None == []), passes with the fix.
  • _get_required_fields returns the correct non-empty list for a schema with properties (guards against over-correction).
  • A tool_context-only function's declaration has parameters is None (documents the actual public-path behavior).
# before fix:
FAILED test_get_required_fields_no_properties_returns_empty_list - assert None == []

# with fix:
$ pytest tests/unittests/tools/test_build_function_declaration.py -q
41 passed

pyink --check and isort --check-only are clean on both touched files.

Manual End-to-End (E2E) Tests:

Not applicable beyond the unit level — the change is a pure return-value correction in a private utility with no runtime surface of its own; the declaration-path test covers the integrated behavior.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • I have manually tested my changes end-to-end.
  • Any dependent changes have been merged and published in downstream modules.

Additional context

None.

@ItsMacto

Copy link
Copy Markdown
Contributor Author

/gemini review

@adk-bot adk-bot added the tools [Component] This issue is related to tools label Jul 11, 2026
@adk-bot

adk-bot commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator

Response from ADK Triaging Agent

Hello @ItsMacto, thank you for creating this PR!

This PR is extremely well-structured and fully complies with our contribution guidelines:

I have labeled this PR with the tools label because it addresses tool function parameter parsing (tools/_function_parameter_parse_util.py). I have also assigned @xuanyang15 as the shepherd for this component.

Thank you for contributing!

@ItsMacto

ItsMacto commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Note on the Mypy check failures: the flagged "new" errors are in src/google/adk/models/lite_llm.py, which this PR does not touch. The baseline-vs-PR error diff appears to be unstable for that file (it also only fails on 3.10–3.13, passing on 3.14). All unit tests, the linter, and A2A checks pass. Happy to rebase/retrigger if that helps.

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

Labels

tools [Component] This issue is related to tools

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG: _get_required_fields returns None instead of [] for empty properties, causing TypeError

4 participants