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.

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

feat: add mqtt v5 specific bindings to mqtt #201

Merged
merged 7 commits into from
Dec 4, 2023

Conversation

pearmaster
Copy link
Contributor

Description
According to #199 (comment) , I'm only adding fields to the mqtt binding. This is because we don't want to create new sets of bindings for each version of each protocol.

Some general changes:

  • Increased mqtt binding version to 0.2.0
  • Added a column to the tables to indicate which MQTT versions the binding applied to.
  • Create a new message binding
  • Added a deprecation warning to the mqtt5 readme to indicate a consolidation of bindings under mqtt only.

Here are specific fields being added:

  • sessionExpiryInterval added to the server binding
  • maximumPacketSize added to the server binding
  • messageExpiryInterval added to the operation binding
  • payloadFormatIndicator added to the message binding
  • correlationData added to the message binding
  • contentType added to the message binding
  • responseTopic added to the message binding.

Related issue(s)
Fixes #199 by adding new v5 properties to the mqtt binding
Fixes #198 by adding maximum packet size

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

fmvilas
fmvilas previously approved these changes Jun 26, 2023
@fmvilas
Copy link
Member

fmvilas commented Jun 26, 2023

@dalelane @derberg @smoya @char0n would love to know your opinion.

mqtt5/README.md Outdated Show resolved Hide resolved
KhudaDad414
KhudaDad414 previously approved these changes Jul 5, 2023
Copy link
Member

@KhudaDad414 KhudaDad414 left a comment

Choose a reason for hiding this comment

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

Documentation seems to match the JSON schema specification. 🙇

pearmaster and others added 2 commits July 21, 2023 10:52
Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
@pearmaster pearmaster dismissed stale reviews from KhudaDad414 and fmvilas via a785000 July 21, 2023 16:56
derberg
derberg previously approved these changes Aug 11, 2023
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

@smoya
Copy link
Member

smoya commented Aug 11, 2023

LGTM! 🚀🌔

Do we want to merge this into master and then adapt it to work with AsyncAPI v3? That already happened in #205, so we will need to update the binding in next-major-spec branch as well.
As an alternative, we could do the changes based on next-major-spec and will be released with v3 of the spec, which I think makes sense unless there is a really urgency.

Opinions? cc @jonaslagoni @dalelane @char0n @derberg

Copy link
Member

@KhudaDad414 KhudaDad414 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

Think there is reason to have this for v2 regardless of v3 is right around the corner.

Someone just needs to update these afterwards 😄

---|:---:|:---:|---
<a name="messageBindingObjectPayloadFormatIndicator"></a>`payloadFormatIndicator` | integer | 5 | Either: **0** (zero): Indicates that the payload is unspecified bytes, or **1**: Indicates that the payload is UTF-8 encoded character data. |
<a name="messageBindingObjectCorrelationData"></a>`correlationData` | [Schema Object][schemaObject] \| [Reference Object](referenceObject) | 5 | Correlation Data is used by the sender of the request message to identify which request the response message is for when it is received.
<a name="messageBindingObjectContentType"></a>`contentType` | string | 5 | String describing the content type of the message payload. This should not conflict with the `contentType` field of the associated AsyncAPI Message object.
Copy link
Member

@jonaslagoni jonaslagoni Aug 14, 2023

Choose a reason for hiding this comment

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

How come the contentType of the AsyncAPI Message object is not enough 🤔 Or rather whats the difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The contentType of the Message object is a requirement of the payload format.

Providing a contentType message binding requires that the MQTTv5 PUBLISH packet include the "Content Type" property.

If we were talking HTTP, it would be the difference between requiring the payload to be JSON and requiring the HTTP headers include Content-Type: application/json

Copy link
Member

Choose a reason for hiding this comment

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

Got it 👍

Field Name | Type | MQTT Versions | Description
---|:---:|:---:|---
<a name="messageBindingObjectPayloadFormatIndicator"></a>`payloadFormatIndicator` | integer | 5 | Either: **0** (zero): Indicates that the payload is unspecified bytes, or **1**: Indicates that the payload is UTF-8 encoded character data. |
<a name="messageBindingObjectCorrelationData"></a>`correlationData` | [Schema Object][schemaObject] \| [Reference Object](referenceObject) | 5 | Correlation Data is used by the sender of the request message to identify which request the response message is for when it is received.
Copy link
Member

Choose a reason for hiding this comment

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

How is this different from the correlation ID? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the same as a correlation ID. correlationData is the MQTT term. When this binding property is provided in the spec, it signals that the correlation ID should be provided in the MQTTv5 PUBLISH property designated for such.

I'd love to update the correlation ID runtime expression to indicate that a protocol binding field should be used for the correlation ID.

Copy link
Member

Choose a reason for hiding this comment

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

Alright 👍

Copy link
Member

derberg commented Sep 4, 2023

we should merge to master, so it is changed for v2 too

@jonaslagoni
Copy link
Member

ping ping @pearmaster 🙂

Copy link
Collaborator

@char0n char0n left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@smoya smoya left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀🌔

@smoya
Copy link
Member

smoya commented Nov 22, 2023

ping ping @pearmaster 🙂

I believe we can proceed with the merge of this PR. Nothing is expected from @pearmaster afaik. Anything else you expect @jonaslagoni ?

@jonaslagoni
Copy link
Member

@smoya nothing but the JSON Schema files need to be moved to spec-json-schema repo 🙂

@derberg
Copy link
Member

derberg commented Dec 4, 2023

@smoya I just opened asyncapi/spec-json-schemas#464 so I guess we can merge this one and the other PR?

@smoya
Copy link
Member

smoya commented Dec 4, 2023

/rtm

@asyncapi-bot asyncapi-bot merged commit e14c678 into asyncapi:master Dec 4, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MQTT5 CONNECT and PUBLISH Properties add message size/length to mqtt and mqtt5
8 participants