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

Differentiate CONNECT packets properties and CONNACK packets properties #1200

Closed
wants to merge 2 commits into from
Closed

Differentiate CONNECT packets properties and CONNACK packets properties #1200

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4e54fd2
Differentiate CONNECT packets properties and CONNACK packets properties
heunghingwan Nov 3, 2020
35a9716
Clarify properties meaning
heunghingwan Nov 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions lib/client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -503,11 +503,11 @@ MqttClient.prototype.publish = function (topic, message, opts, callback) {
if (options.protocolVersion === 5) {
packet.properties = opts.properties
if ((!options.properties && packet.properties && packet.properties.topicAlias) || ((opts.properties && options.properties) &&
((opts.properties.topicAlias && options.properties.topicAliasMaximum && opts.properties.topicAlias > options.properties.topicAliasMaximum) ||
(!options.properties.topicAliasMaximum && opts.properties.topicAlias)))) {
((opts.properties.topicAlias && options.properties.serverTopicAliasMaximum && opts.properties.topicAlias > options.properties.serverTopicAliasMaximum) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

do you know that this change honors the MQTTv5 Spec definition for topicAliasMaximum:

34 (0x22) Byte, Identifier of the Topic Alias Maximum.
811 Followed by the Two Byte Integer representing the Topic Alias Maximum value. It is a Protocol Error to
812 include the Topic Alias Maximum value more than once. If the Topic Alias Maximum property is absent,
813 the default value is 0.
814
815 This value indicates the highest value that the Client will accept as a Topic Alias sent by the Server. The
816 Client uses this value to limit the number of Topic Aliases that it is willing to hold on this Connection. The
817 Server MUST NOT send a Topic Alias in a PUBLISH packet to the Client greater than Topic Alias
818 Maximum [MQTT-3.1.2-26]. A value of 0 indicates that the Client does not accept any Topic Aliases on
819 this connection. If Topic Alias Maximum is absent or zero, the Server MUST NOT send any Topic Aliases
820 to the Client [MQTT-3.1.2-27]

Copy link
Author

Choose a reason for hiding this comment

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

It should honor topicAliasMaximum property in CONNACK packet, not CONNECT packets property sent by the client, as shown below: https://docs.oasis-open.org/mqtt/mqtt/v5.0/os/mqtt-v5.0-os.html#_Toc3901088

(!options.properties.serverTopicAliasMaximum && opts.properties.topicAlias)))) {
/*
if we are don`t setup topic alias or
topic alias maximum less than topic alias or
server's topic alias maximum less than topic alias or
server don`t give topic alias maximum,
we are removing topic alias from packet
*/
Expand Down Expand Up @@ -1171,15 +1171,15 @@ MqttClient.prototype._handleConnack = function (packet) {
if (packet.properties) {
if (packet.properties.topicAliasMaximum) {
if (!options.properties) { options.properties = {} }
options.properties.topicAliasMaximum = packet.properties.topicAliasMaximum
options.properties.serverTopicAliasMaximum = packet.properties.topicAliasMaximum
}
if (packet.properties.serverKeepAlive && options.keepalive) {
options.keepalive = packet.properties.serverKeepAlive
this._shiftPingInterval()
}
if (packet.properties.maximumPacketSize) {
if (!options.properties) { options.properties = {} }
options.properties.maximumPacketSize = packet.properties.maximumPacketSize
options.properties.serverMaximumPacketSize = packet.properties.maximumPacketSize
Copy link
Contributor

Choose a reason for hiding this comment

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

you've changed this but where is options.properties.serverMaximumPacketSize referenced?

Copy link
Author

Choose a reason for hiding this comment

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

It's not use anywhere, It should be use when client sending message, but I don't know the code enough to do it properly, may be this will better translate it meaning?

Suggested change
options.properties.serverMaximumPacketSize = packet.properties.maximumPacketSize
options.serverProperties.maximumPacketSize = packet.properties.maximumPacketSize

}
}

Expand Down
19 changes: 14 additions & 5 deletions test/client_mqtt5.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,23 +17,32 @@ describe('MQTT 5.0', function () {

var topicAliasTests = [
{properties: {}, name: 'should allow any topicAlias when no topicAliasMaximum provided in settings'},
{properties: { topicAliasMaximum: 15 }, name: 'should not allow topicAlias > topicAliasMaximum when topicAliasMaximum provided in settings'}
{properties: { topicAliasMaximum: 15 }, name: 'should not allow topicAlias > serverTopicAliasMaximum when TopicAliasMaximum provided in CONNACK'}
]

topicAliasTests.forEach(function (test) {
it(test.name, function (done) {
var server215 = new MqttServer(function (serverClient) {
serverClient.on('connect', function (packet) {
serverClient.connack({
reasonCode: 0,
properties: test.properties
})
})
}).listen(ports.PORTAND215)
this.timeout(15000)
server.once('client', function (serverClient) {
server215.once('client', function (serverClient) {
serverClient.on('publish', function (packet) {
if (packet.properties && packet.properties.topicAlias) {
done(new Error('Packet should not have topicAlias'))
return false
} else {
server215.close()
serverClient.end(done)
}
})
})
var opts = {host: 'localhost', port: ports.PORTAND115, protocolVersion: 5, properties: test.properties}
var opts = {host: 'localhost', port: ports.PORTAND215, protocolVersion: 5, properties: {}}
var client = mqtt.connect(opts)
client.publish('t/h', 'Message', { properties: { topicAlias: 22 } })
})
Expand Down Expand Up @@ -105,8 +114,8 @@ describe('MQTT 5.0', function () {
var client = mqtt.connect(opts)
client.on('connect', function () {
assert.strictEqual(client.options.keepalive, 16)
assert.strictEqual(client.options.properties.topicAliasMaximum, 15)
assert.strictEqual(client.options.properties.maximumPacketSize, 95)
assert.strictEqual(client.options.properties.serverTopicAliasMaximum, 15)
assert.strictEqual(client.options.properties.serverMaximumPacketSize, 95)
server116.close()
client.end(true, done)
})
Expand Down
2 changes: 2 additions & 0 deletions test/helpers/port_list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ var PORTAND116 = PORT + 116
var PORTAND117 = PORT + 117
var PORTAND118 = PORT + 118
var PORTAND119 = PORT + 119
var PORTAND215 = PORT + 215
var PORTAND316 = PORT + 316
var PORTAND326 = PORT + 326
var PORTAND327 = PORT + 327
Expand All @@ -39,6 +40,7 @@ module.exports = {
PORTAND117,
PORTAND118,
PORTAND119,
PORTAND215,
PORTAND316,
PORTAND326,
PORTAND327
Expand Down