-
-
Notifications
You must be signed in to change notification settings - Fork 114
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
Fix off-by-one error in upserts #423
Conversation
There are two off-by-one errors in the upsert functions for message ID and message type. UpsertMessageId should allow a value of 65535, while UpsertType should allow 255. Without this fix, a device sending a confirmable message to the server will receive an ACK message back with an incorrect Message ID. Co-authored-by: Vit Prajzler <28387418+vitprajzler@users.noreply.github.com>
b7514f4
to
a620e34
Compare
Codecov Report
@@ Coverage Diff @@
## master #423 +/- ##
==========================================
+ Coverage 71.34% 71.46% +0.12%
==========================================
Files 68 68
Lines 5255 5275 +20
==========================================
+ Hits 3749 3770 +21
+ Misses 1122 1119 -3
- Partials 384 386 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
message/getmid.go
Outdated
@@ -28,3 +29,8 @@ func RandMID() int32 { | |||
} | |||
return int32(uint16(binary.BigEndian.Uint32(b))) | |||
} | |||
|
|||
// ValidateMessageID validates a message id for UDP. (0 <= mid <= 65535) | |||
func ValidateMessageID(mid int32) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ValidateMID
would be more in line with the existing naming convention, but it's clear either way so I don't mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed 👍
@@ -46,10 +45,10 @@ func (c *Coder) Encode(m message.Message, buf []byte) (int, error) { | |||
|1 1 1 1 1 1 1 1| Payload (if any) ... | |||
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | |||
*/ | |||
if m.MessageID < 0 || m.MessageID > math.MaxUint16 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a test of Encode with the boundary values for messageID and type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
Kudos, SonarCloud Quality Gate passed! |
There are two off-by-one errors in the upsert functions for message ID and message type.
UpsertMessageId should allow a value of 65535, while UpsertType should allow 255.
Without this fix, a device sending a confirmable message to the server will receive an ACK message back with an incorrect Message ID.
FYI: @mniestroj @vitprajzler