-
-
Notifications
You must be signed in to change notification settings - Fork 38.2k
Google Assistant SDK: support audio response playback #85989
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,7 +22,10 @@ | |
| from .const import CONTENT_AUTH_EXPIRY_TIME, MediaClass, MediaType | ||
|
|
||
| # Paths that we don't need to sign | ||
| PATHS_WITHOUT_AUTH = ("/api/tts_proxy/",) | ||
| PATHS_WITHOUT_AUTH = ( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While there is not a super high security bar here, uuid1 paths seem below the bar as my impression is those are on the easier side to predict. Is there a technical motivation for opting out of signed paths?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just followed what tts_proxy does. See comment below where this constant is used: elif parsed.path.startswith(PATHS_WITHOUT_AUTH):
# We don't sign this path if it doesn't need auth. Although signing itself can't
# hurt, some devices are unable to handle long URLs and the auth signature might
# push it over.
passI just did some tests on my setup and I don't have any such issues on any of my media players. So I removed the change in this file and also set If anyone complains that playback doesn't work because of long URLs we can consider adding this back. |
||
| "/api/google_assistant_sdk/audio/", | ||
| "/api/tts_proxy/", | ||
| ) | ||
|
|
||
|
|
||
| @callback | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,9 +13,18 @@ | |
|
|
||
| from .conftest import ComponentSetup, ExpectedCredentials | ||
|
|
||
| from tests.common import async_mock_service | ||
| from tests.test_util.aiohttp import AiohttpClientMocker | ||
|
|
||
|
|
||
| async def fetch_api_url(hass_client, url): | ||
| """Fetch an API URL and return HTTP status and contents.""" | ||
| client = await hass_client() | ||
| response = await client.get(url) | ||
| contents = await response.read() | ||
| return response.status, contents | ||
|
|
||
|
|
||
| async def test_setup_success( | ||
| hass: HomeAssistant, setup_integration: ComponentSetup | ||
| ) -> None: | ||
|
|
@@ -129,7 +138,7 @@ async def test_send_text_command( | |
| blocking=True, | ||
| ) | ||
| mock_text_assistant.assert_called_once_with( | ||
| ExpectedCredentials(), expected_language_code | ||
| ExpectedCredentials(), expected_language_code, audio_out=False | ||
| ) | ||
| mock_text_assistant.assert_has_calls([call().__enter__().assist(command)]) | ||
|
|
||
|
|
@@ -180,6 +189,78 @@ async def test_send_text_command_expired_token_refresh_failure( | |
| assert any(entry.async_get_active_flows(hass, {"reauth"})) == requires_reauth | ||
|
|
||
|
|
||
| async def test_send_text_command_media_player( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The expiration also seems like an important thing to test. Typically this is done with
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| hass: HomeAssistant, setup_integration: ComponentSetup, hass_client | ||
| ) -> None: | ||
| """Test send_text_command with media_player.""" | ||
| await setup_integration() | ||
|
|
||
| play_media_calls = async_mock_service(hass, "media_player", "play_media") | ||
|
|
||
| command = "tell me a joke" | ||
| media_player = "media_player.office_speaker" | ||
| audio_response1 = b"joke1 audio response bytes" | ||
| audio_response2 = b"joke2 audio response bytes" | ||
| with patch( | ||
| "homeassistant.components.google_assistant_sdk.helpers.TextAssistant.assist", | ||
| side_effect=[ | ||
| ("joke1 text", None, audio_response1), | ||
| ("joke2 text", None, audio_response2), | ||
| ], | ||
| ) as mock_assist_call: | ||
| # Run the same command twice, getting different audio response each time. | ||
| await hass.services.async_call( | ||
| DOMAIN, | ||
| "send_text_command", | ||
| { | ||
| "command": command, | ||
| "media_player": media_player, | ||
| }, | ||
| blocking=True, | ||
| ) | ||
| await hass.services.async_call( | ||
| DOMAIN, | ||
| "send_text_command", | ||
| { | ||
| "command": command, | ||
| "media_player": media_player, | ||
| }, | ||
| blocking=True, | ||
| ) | ||
|
|
||
| mock_assist_call.assert_has_calls([call(command), call(command)]) | ||
| assert len(play_media_calls) == 2 | ||
| for play_media_call in play_media_calls: | ||
| assert play_media_call.data["entity_id"] == [media_player] | ||
| assert play_media_call.data["media_content_id"].startswith( | ||
| "/api/google_assistant_sdk/audio/" | ||
| ) | ||
|
|
||
| audio_url1 = play_media_calls[0].data["media_content_id"] | ||
| audio_url2 = play_media_calls[1].data["media_content_id"] | ||
| assert audio_url1 != audio_url2 | ||
|
|
||
| # Assert that both audio responses can be served multiple times. | ||
| status, response = await fetch_api_url(hass_client, audio_url1) | ||
| assert status == http.HTTPStatus.OK | ||
| assert response == audio_response1 | ||
| status, response = await fetch_api_url(hass_client, audio_url1) | ||
| assert status == http.HTTPStatus.OK | ||
| assert response == audio_response1 | ||
| status, response = await fetch_api_url(hass_client, audio_url2) | ||
| assert status == http.HTTPStatus.OK | ||
| assert response == audio_response2 | ||
| status, response = await fetch_api_url(hass_client, audio_url2) | ||
| assert status == http.HTTPStatus.OK | ||
| assert response == audio_response2 | ||
|
|
||
| # Assert a nonexistent URL returns 404 | ||
| status, _ = await fetch_api_url( | ||
| hass_client, "/api/google_assistant_sdk/audio/nonexistent" | ||
| ) | ||
| assert status == http.HTTPStatus.NOT_FOUND | ||
|
|
||
|
|
||
| async def test_conversation_agent( | ||
| hass: HomeAssistant, | ||
| setup_integration: ComponentSetup, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more commonly seen pattern is to use something like an audio storage manager, rather than passing around references to the View directly. Other code can stick stuff in the manger and the view can interact with that.
(You could also consider having that intermediate manager thing be single object that holds the session, though not sure)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.