fix: Return empty list instead of None from _get_required_fields for schemas without properties#6377
Open
ItsMacto wants to merge 1 commit into
Open
Conversation
…schemas without properties Fixes google#5920
Contributor
Author
|
/gemini review |
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 ( Thank you for contributing! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_fieldsintools/_function_parameter_parse_util.pyis annotated-> list[str]but has a barereturnwhen the schema has no properties, so it returnsNone. Any caller that relies on the declared contract (e.g. iterating the result, as reported in #5920) getsTypeError: 'NoneType' object is not iterable. A maintainer confirmed the bare return as the culprit in the issue thread and endorsedreturn [].Solution:
Return
[]instead, honoring the declared return type. One-line change.One honest scoping note: the two current callers in
_automatic_function_calling_util.pyguard 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 istool_contextproducesparameters=Nonerather thanrequired=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 thetool_context-only behavior.Testing Plan
Unit Tests:
Three tests added to
tests/unittests/tools/test_build_function_declaration.py:_get_required_fieldsreturns[](notNone) for a schema without properties — fails on the unfixed code (assert None == []), passes with the fix._get_required_fieldsreturns the correct non-empty list for a schema with properties (guards against over-correction).tool_context-only function's declaration hasparameters is None(documents the actual public-path behavior).pyink --checkandisort --check-onlyare 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
Additional context
None.