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

More descriptive decoder exceptions #2260

Merged
merged 3 commits into from
Jul 24, 2024
Merged

More descriptive decoder exceptions #2260

merged 3 commits into from
Jul 24, 2024

Conversation

alexrudd2
Copy link
Collaborator

@alexrudd2 alexrudd2 commented Jul 23, 2024

Closes #2259.

Three current tests are relevant for badly encoded Response.

(1)
test_client_factory_fails() calls ClientDecoder.decode(None) and expects None.
This would cause a TypeError, but it's monkey-patched to throw a ModbusException instead. (The monkey patching was never necessary with the catch-all Except)

(2) test_server_factory_fails() calls ServerDecoder.decode(None) and expects None.
This would cause a TypeError, but it's monkey-patched to throw a ModbusException instead. This is caught normally.

I'm assuming that in (1) and (2) we should actually throw the TypeError to the caller, so I changed as such. I see no valid use case for decoding None...

(3) test_processincomingpacket_not_ok() calls ClientDecoder.decode(b'\x03\x01\x00\n') and expects a ModbusIOException. The real exception is a struct.error because the payload has only 3 bytes. The register values must be an even number of bytes i.e. 6 bytes for 3 registers.
image

This bad message is caught by decode(), losing information on the error. Later on frameProcessIncomingPacket notices that decode returned None and raise ModbusIOException.

I think it's better to explicitly raise a descriptive exception in this case.

@alexrudd2 alexrudd2 force-pushed the decoder-exceptions branch from cd0050c to 33d2754 Compare July 23, 2024 22:30
@alexrudd2
Copy link
Collaborator Author

alexrudd2 commented Jul 24, 2024

Regarding (3)

@pytest.mark.parametrize(
("framer", "message"),
[
(ModbusAsciiFramer, b':01270001000ACD\r\n',),
(ModbusRtuFramer, b"\x01\x03\x03\x01\x00\n\x94\x49",),
(ModbusSocketFramer, b'\x00\x00\x00\x00\x00\x06\x01\x27\x00\x01\x00\n',),
(ModbusTlsFramer, b'\x54\x00\x7c\x00\x02',),
]
)
def test_processincomingpacket_not_ok(self, framer, message):
"""Test processIncomingPacket."""
test_framer = framer(ClientDecoder())
with pytest.raises(ModbusIOException):
test_framer.processIncomingPacket(message, mock.Mock(), 0x01)

RapidScada's parser claims it's valid.... but I don't trust it. It's parsing the last two bytes twice (once as data and once as CRC!)
image

Anyways, this is all in test_old_framer so maybe it's going to get deleted... anyways I got distracted hunting down #2261

Copy link
Collaborator

@janiversen janiversen left a comment

Choose a reason for hiding this comment

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

Another way to reach the same goal, but nicer.

@janiversen janiversen merged commit 4dbefa2 into dev Jul 24, 2024
@janiversen janiversen deleted the decoder-exceptions branch July 24, 2024 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Server and client decoders handle exceptions differently
2 participants