-
Notifications
You must be signed in to change notification settings - Fork 217
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 the content-type
issue for the MQTT protocol
#1063
Conversation
fdc7415
to
ab4b6b9
Compare
This is a bug of the MQTT protocol in the structured mode. Please take a look @embano1 ~ |
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.
Just a nit and a question
protocol/mqtt_paho/v2/message.go
Outdated
@@ -42,7 +41,7 @@ func NewMessage(msg *paho.Publish) *Message { | |||
var v spec.Version | |||
if msg.Properties != nil { | |||
// Use properties.User["Content-type"] to determine if message is structured |
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.
should this comment be removed then?
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.
Done.
} | ||
if err != nil { | ||
return | ||
} | ||
} | ||
|
||
contentType := m.internal.Properties.ContentType |
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.
question: no changes needed for structured encoding?
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.
We still need it here, cause the attribute datacontenttype
of the event is decoded from this field with a binary message
89a0e0f
to
563be8b
Compare
Signed-off-by: myan <myan@redhat.com>
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 (merging once tests pass)
Signed-off-by: myan myan@redhat.com
Fixed: #1062