Skip to content
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

Add support for responses to call_service WS cmd #98610

Merged
merged 8 commits into from
Nov 10, 2023

Conversation

raman325
Copy link
Contributor

@raman325 raman325 commented Aug 18, 2023

Proposed change

The call_service websocket API command did not previously support returning a response. With this change it does. A new return_response parameter has been added to the WS command inputs that allows the caller to get a response from a service.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@home-assistant
Copy link

Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration (websocket_api) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of websocket_api can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign websocket_api Removes the current integration label and assignees on the pull request, add the integration domain after the command.

@allenporter
Copy link
Contributor

allenporter commented Aug 18, 2023

While easier for the caller to not have to supply additional input arguments, the previous call was to add an explicit parameter to request response values for performance (e.g. to avoid unnecessarily computing response values, or making a cloud call, etc, where they might not be needed). I think its worth considering either for consistency or performance how much worse this API is with an extra input argument for response values. Not adding it is a harder to reverse decision, but adding it is easier to reverse if we want to make it always in the future.

(Having the argument as a pass through means this call also doesn't have to check whether or not it is supported, as it can use the existing check)

@raman325
Copy link
Contributor Author

I'm open to adding it as a parameter for consistency, and you are right, a lot of the logic changes are simpler as a result.

@MartinHjelmare
Copy link
Member

Looks like this is a near duplicate of #95056.

I think we can continue here, but if we merge here we should close that one.

@raman325
Copy link
Contributor Author

I think my only question here is about catching the ValueErrors that get thrown when return_response does not match the service signature. I realize that is a broad catch and the error I am returning is not supported but that may not be accurate for other services, only for the response exceptions. Do we need to change the ValueErrors for the response exceptions to something else? then we will have to find all of the places where these ValueErrors are caught, which are hopefully not many

@raman325
Copy link
Contributor Author

doesn't look like we handle the ValueError's in the core, so maybe it would just be tests that need to be updated, and perhaps the frontend?

@allenporter
Copy link
Contributor

doesn't look like we handle the ValueError's in the core, so maybe it would just be tests that need to be updated, and perhaps the frontend?

We're kind of in an inconsistent state right now -- this discussion has a good summary: #78889 (comment)

@raman325
Copy link
Contributor Author

raman325 commented Sep 16, 2023

doesn't look like we handle the ValueError's in the core, so maybe it would just be tests that need to be updated, and perhaps the frontend?

We're kind of in an inconsistent state right now -- this discussion has a good summary: #78889 (comment)

Yeah so I guess the question is how do we give WS clients a response that lets them know that the option that they provided in their call was not valid? That's essentially what the outcome is, but if catch this exception, it will catch some of those other scenarios that were being discussed in that PR.

@raman325
Copy link
Contributor Author

Thinking about this more, even though we return the unknown error code, which is probably not great, we also share the reason, so the caller can operate off of that. I'm going to remove the catch here

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Thanks!

@MartinHjelmare MartinHjelmare merged commit 618b666 into home-assistant:dev Nov 10, 2023
50 checks passed
@raman325 raman325 deleted the websocket branch November 10, 2023 22:05
dgomes pushed a commit to dgomes/home-assistant that referenced this pull request Nov 11, 2023
)

* Add support for responses to call_service WS cmd

* Revert ServiceNotFound removal and add a parameter for return_response

* fix type

* fix tests

* remove exception handling that was added

* Revert unnecessary modifications

* Use kwargs
@github-actions github-actions bot locked and limited conversation to collaborators Nov 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants