Repository navigation
fix(client): validate outgoing topic aliases against server limit - #97
AndreyGatsuk wants to merge 1 commit into
Conversation
| if properties is not None and self._version != "5.0": | ||
| msg = "properties require MQTT 5.0" | ||
| raise RuntimeError(msg) | ||
| if properties is not None and properties.topic_alias is not None: | ||
| alias = properties.topic_alias | ||
| if not 1 <= alias <= _MAX_TOPIC_ALIAS: | ||
| msg = "topic_alias must be between 1 and 65535" | ||
| raise ValueError(msg) | ||
| connack_properties = self.connection_info.properties | ||
| maximum = 0 if connack_properties is None else (connack_properties.topic_alias_maximum or 0) | ||
| if alias > maximum: | ||
| msg = f"topic_alias {alias} exceeds server Topic Alias Maximum {maximum}" | ||
| raise ValueError(msg) |
There was a problem hiding this comment.
Let's refactor it into `self._validate_publish_properties(properties)
| @pytest.mark.parametrize("maximum", [None, 0, 1, 5]) | ||
| @pytest.mark.parametrize("resumed", [False, True]) | ||
| async def test_reconnect_uses_new_alias_limit(maximum: int | None, resumed: bool) -> None: |
There was a problem hiding this comment.
I think this tests is unnecessary, too complex.
| with pytest.raises(RuntimeError, match=r"properties require MQTT 5\.0"): | ||
| await client.publish("test/topic", b"payload", properties=PublishProperties(topic_alias=1)) |
There was a problem hiding this comment.
Can we test it using the e2e test case (for less coupling to library internals)?
|
|
||
| @pytest.mark.parametrize("maximum", [None, 0, 3]) | ||
| @pytest.mark.parametrize("properties", [None, PublishProperties(content_type="text/plain")]) | ||
| async def test_publication_without_alias_is_independent_of_server_limit( |
There was a problem hiding this comment.
What does this test measure? I believe it should be eliminated.
borisalekseev
left a comment
borisalekseev
left a comment
There was a problem hiding this comment.
Too many tests, and the tests are too complicated and coupled with the library's internals. We prefer writing only end-to-end (e2e) tests that measure the public API and behaviour of the library, as these tests are more compatible with refactoring. Unit tests that know about the protocol class and other internal details are allowed only if it is impossible to easily simulate the broker's behaviour in an e2e testing scenario.
I will extend the contribution guide and the project harness soon. Sorry that these concepts weren't explicitly described in the project.
This branch has not been deployed
2 participants
Summary
Implement outgoing Topic Alias validation according to MQTT 5.0 §3.2.2.3.8 (Topic Alias Maximum) and §3.3.2.3.4 (Topic Alias).
Before sending PUBLISH:
Closes #78
Validation
Added tests for invalid aliases (-1, 0, 65536), missing/zero/exceeded server limits, valid boundary values, updated limits after reconnect, publications without aliases, and MQTT 3.1.1 compatibility (6 test functions, 40 cases).
Full test suite on Python 3.11 with all five brokers: 732 passed, 24 skipped (existing protocol/broker-specific skips).
Ruff lint and formatting, mypy, mkdocs build --strict, uv build, and Bandit passed.
Final docstring and documentation edits passed git diff --check.
Tests were added or updated when behavior changed
Documentation was updated when the public API changed
The PR title follows Conventional Commits, for example
feat(client): add reconnect timeout