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.

Sign up for GitHub

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

Jump to bottom

MQTT5: support multiple Subscription ID properties #10734

Merged
merged 3 commits into from
Oct 30, 2020
Merged

MQTT5: support multiple Subscription ID properties #10734

merged 3 commits into from
Oct 30, 2020

Conversation

paul-lysak
Copy link
Contributor

Motivation:

Subscription ID property of the PUBLISH message may be repeated multiple times, which wasn't taken into account when developing MqttProperties API.

Modification:

Store Subscription ID properties separately from others - in MqttProperties.subscriptionIds.
Add MqttProperties.getProperties method to retrieve properties that may be repeated.
Change internal representation of User Properties for uniformity with Subscription ID - now they're stored in MqttProperties.userProperties rather than the common hash map.

Result:

Multiple Subscription ID properties can be set or retrieved.

@netty-bot
Copy link

Can one of the admins verify this patch?

@paul-lysak
Copy link
Contributor Author

Also, now I think that introducing MqttProperties.UserProperties class to wrap multiple occurrences of MqttProperties.UserProperty was a bad idea. If MqttProperties.listAll returned a collection of MqttProperty instead, MqttProperties.getProperty returned first occurence of UserProperty and in order to retrieve multiple properties users would need to call MqttProperties.getProperties (introduced in this PR) we would have a cleaner API.
However, the changes are already released and I'm not sure if it's OK to change the way properties behave now.

@normanmaurer
Copy link
Member

@netty-bot test this please

@paul-lysak thats right we can not break the API anymore... This ship has sailed :/

normanmaurer
normanmaurer requested changes Oct 26, 2020
View reviewed changes
codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttProperties.java Outdated Show resolved Hide resolved
codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttProperties.java Outdated Show resolved Hide resolved
codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttProperties.java Outdated Show resolved Hide resolved
codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttProperties.java Outdated Show resolved Hide resolved
codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttProperties.java Outdated Show resolved Hide resolved
codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttProperties.java Outdated Show resolved Hide resolved
codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttProperties.java Outdated Show resolved Hide resolved
codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttProperties.java Outdated Show resolved Hide resolved
codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttProperties.java Outdated Show resolved Hide resolved
codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttProperties.java Outdated Show resolved Hide resolved
@normanmaurer
Copy link
Member

@netty-bot test this please

chrisvest
chrisvest reviewed Oct 29, 2020
View reviewed changes
Copy link
Contributor

@chrisvest chrisvest left a comment

Choose a reason for hiding this comment

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

Some small nits, but looks fine.

codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttProperties.java Outdated Show resolved Hide resolved
codec-mqtt/src/test/java/io/netty/handler/codec/mqtt/MqttPropertiesTest.java Outdated Show resolved Hide resolved
codec-mqtt/src/test/java/io/netty/handler/codec/mqtt/MqttPropertiesTest.java Outdated Show resolved Hide resolved
@normanmaurer
Copy link
Member

@netty-bot test this please

@normanmaurer normanmaurer merged commit 7b736a3 into netty:4.1 Oct 30, 2020
@normanmaurer normanmaurer added this to the 4.1.54.Final milestone Oct 30, 2020
normanmaurer pushed a commit that referenced this pull request Oct 30, 2020
@paul-lysak @normanmaurer
MQTT5: support multiple Subscription ID properties (#10734)
30e9b68 
Motivation:

Subscription ID property of the PUBLISH message may be repeated multiple times, which wasn't taken into account when developing `MqttProperties` API.

Modification:

Store Subscription ID properties separately from others - in `MqttProperties.subscriptionIds`. 
Add `MqttProperties.getProperties` method to retrieve properties that may be repeated.
Change internal representation of User Properties for uniformity with Subscription ID - now they're stored in `MqttProperties.userProperties` rather than the common hash map.

Result:

Multiple Subscription ID properties can be set or retrieved.
raidyue pushed a commit to raidyue/netty that referenced this pull request Jul 8, 2022
@paul-lysak
MQTT5: support multiple Subscription ID properties (netty#10734)
c365972 
Motivation:

Subscription ID property of the PUBLISH message may be repeated multiple times, which wasn't taken into account when developing `MqttProperties` API.

Modification:

Store Subscription ID properties separately from others - in `MqttProperties.subscriptionIds`. 
Add `MqttProperties.getProperties` method to retrieve properties that may be repeated.
Change internal representation of User Properties for uniformity with Subscription ID - now they're stored in `MqttProperties.userProperties` rather than the common hash map.

Result:

Multiple Subscription ID properties can be set or retrieved.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisvest chrisvest chrisvest approved these changes

@normanmaurer normanmaurer normanmaurer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.1.54.Final
Development

Successfully merging this pull request may close these issues.
4 participants
@paul-lysak @netty-bot @normanmaurer @chrisvest