Make pushserver more generic #1531
Conversation
This generalizes the push server to allow using it for other uses besides event handling, e.g., to create device simulators. The token for client connections is currently hard-coded to be full of 0s.
rytilahti
force-pushed
the
feat/generalize_server
branch
from
ad4aa1f to
75f5244
Compare
Codecov Report
@@ Coverage Diff @@
## master #1531 +/- ##
==========================================
+ Coverage 82.33% 82.62% +0.28%
==========================================
Files 145 146 +1
Lines 14248 14359 +111
Branches 1617 3489 +1872
==========================================
+ Hits 11731 11864 +133
+ Misses 2287 2262 -25
- Partials 230 233 +3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Looks good to me.
Sorry for the late review, I was quite bussy.
I really like we now have some tests for the serverprotocol, thank you very much!
I have some small feedback, maybe you could adress that in a follow up PR?
I will try to test this new code with my devices tonight and let you know!
| The response can be either a callable or a dictionary to send back as response. | ||
| """ | ||
| self._methods[name] = response | ||
|
|
||
| def register_miio_device(self, device: Device, callback: PushServerCallback): |
There was a problem hiding this comment.
I would add a check here:
if self._device_ip is None:
warning
return
To make sure this is a proper server that can work with real devices
There was a problem hiding this comment.
I'm not sure I'm following what you mean, the device_ip is checked right below?
| callback(source_device_id, action, msg_value.get("params")) | ||
| if host in self.server._registered_devices: | ||
| return self._handle_datagram_from_registered_device(host, port, data) | ||
| else: |
There was a problem hiding this comment.
I am slightly worried that we might exedently send incorrect messages to a real device if that device is for instance not properly disconnected and the server is restarted or other edge cases.
Maybe we schould make a register_client function and check against that instead of just handeling everything which is not registerd as a miio_device as a client.
There was a problem hiding this comment.
The incorrect messages would be encrypted with a hardcoded token that should be different from a real device, so sending a message to the server with some other will simply cause the device not to respond. That is why I don't think it's a non-issue.
|
|
||
| if host not in self.server._registered_devices: |
There was a problem hiding this comment.
I would like to have this error back when the host is not registered as a miio_device or a client.
See comment below.
There was a problem hiding this comment.
This is now checked inside datagram_received, for registered devices. But you are right, we are responding to all non-registered devices as if they were real clients..
|
@rytilahti I just tested this code with gateway devices, and the event pushes still work just fine. |
|
@starkillerOG thanks for taking a look, and sorry for taking so long to respond... I changed the variable naming, but I'm not sure if we should do the other changes, see the linked PR and my commentary inline. |
Also, improve type hints and variable naming Related to @starkillerOG review in #1531
This generalizes the push server to allow using it for other uses besides event handling, e.g., to create device simulators.
When receiving a datagram from an unregistered device, the implementation will now try to decrypt the contents using a token full of 0s. If the decryption succeeds, the list of previously registered methods (using
add_method(name: str, response: Union[Dict, Callable)) is used to obtain a response to be send back to the requester. An error is send to the client if no method with the given name is found.The main motivation behind this change is to make it easier for developers to test miiocli & the library against devices they do not possess by providing easy-to-use device simulators for both miio and miot protocols. The simulator implementations will follow in separate PRs.
@starkillerOG could you please take a look at this PR to make sure I'm not breaking anything event related? I'll add some tests prior getting this merged so that we don't break it accidentally in the future.
TBD: