feat: session expiry interval to mqtt5 bindings #157

sudoshreyansh · 2022-10-11T13:51:05Z

Description

Adds sessionExpiryInterval to MQTT5 server bindings, which is required for asyncapi/glee#323

github-actions

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

LGTM but you'll have to update the binding version and upgrade it to 0.2.0.

sudoshreyansh · 2022-10-11T16:47:30Z

@fmvilas updated the binding version. Can you please re-review?

fmvilas

LGTM 👍 Let's wait for others to review too.

Pinging @KhudaDad414 @derberg @dalelane @smoya @char0n

fmvilas · 2022-10-14T09:56:34Z

mqtt5/json_schemas/server.json

+    }
+  },
+  "properties": {
+


Suggested change

dalelane

@rcoppen is better placed to comment on this than me, as my MQTT knowledge is pretty rusty... but I'll ask some ignorant questions anyway :-)

Is session expiry an attribute of the server? I thought it was something the client could choose, with different clients connecting to the same server allowed to choose their own (and different) expiries?

rcoppen · 2022-10-14T14:22:18Z

I agree with @dalelane, is the intention to define a server configuration that always enforces a single Session Expiry for every client connection?

dan-r · 2022-10-14T14:29:32Z

As @dalelane pointed out, the session expiry is sent by the client in the CONNECT packet, although I believe it can technically be overridden by the broker in the CONNACK.

@fmvilas is there a current distinction in the bindings between client-side connection properties and server-side configuration? IMO it would be more logical to define this as something set by a client rather than an 'enforced' value.

smoya · 2022-10-17T17:21:41Z

I agree @dalelane and @rcoppen. Session expiration is only a client decision. I tried to think on some way of adding this to the binding, like setting some "max" session TTL, but still it doesn't fit IMHO.
Regarding Glee, you could still use extensions.

rcoppen · 2022-10-18T15:46:30Z

Looking at the mqtt binding a similar problem exists for clientId. If a value is presented in the AsyncAPI doc then should every client connect with that clientId? This would limit the server to one active client per doc instance. One approach would be to assert a server default in the binding spec should the client not present value. For clientId (assuming MQTT 3.1.1 or 5.0) then that could be a default of "not present" and the server would then create one.

smoya · 2022-10-19T07:59:59Z

Looking at the mqtt binding a similar problem exists for clientId. If a value is presented in the AsyncAPI doc then should every client connect with that clientId? This would limit the server to one active client per doc instance.

Good catch!

One approach would be to assert a server default in the binding spec should the client not present value. For clientId (assuming MQTT 3.1.1 or 5.0) then that could be a default of "not present" and the server would then create one.

I wouldn't either set a default, since then the same issue you described earlier could happen.
I would suggest that the type becomes a Schema instead, so users can configure the schema the value should fulfill, including things like default value, regex, etc etc.

But that's for another Issue/PR. Are you happy opening a new issue with this @rcoppen ? I can do otherwise.

rcoppen · 2022-10-19T08:45:42Z

@smoya np, will open an issue for clientId. My initial thinking was that since MQTT 3.1.1 states that the server will autogenerate a clientId if the client doesn't provide one, then that could be one way to overcome the problem. However I agree that's a discussion for the new issue.

fmvilas · 2022-10-20T04:52:01Z

Yeah, these things need to be clarified. Y'all are making a good point that this is something the client should decide, maybe even on runtime. I think it could be a good idea to follow the direction of Kafka and WS bindings, and allow either a scalar value or a Schema Object value. The Schema Object should give the client enough flexibility to calculate things on runtime but still make sure it follows a specific pattern or limits. The scalar value would be great for those cases in which we want to enforce a specific value. For instance, when generating code.

sudoshreyansh · 2022-11-03T13:46:11Z

I agree with @fmvilas. I have made the required changes.

mqtt5/README.md

fmvilas · 2022-11-08T17:52:07Z

mqtt5/json_schemas/server.json

+          "$ref": "https://raw.githubusercontent.com/asyncapi/spec-json-schemas/v2.14.0/schemas/2.4.0.json#/definitions/schema"
+        }
+      ],
+      "description": "Session Expiry Interval in seconds."


Same as above. Something to tweak for the Schema Object case.

fmvilas · 2022-12-09T10:17:06Z

@derberg @dalelane @char0n please review 🙏

fmvilas · 2022-12-12T04:54:42Z

@KhudaDad414 can you have a look as well, please? 🙏

KhudaDad414

LGTM!

fmvilas · 2022-12-12T16:15:51Z

🏓 @derberg

char0n

LGTM. I've suggested couple of changes, due to #143

char0n · 2022-12-19T16:40:09Z

mqtt5/README.md

+
+Field Name | Type | Description
+---|:---:|---
+<a name="serverBindingObjectSessionExpiryInterval"></a>`sessionExpiryInterval` | [Schema Object][schemaObject] \| integer | Session Expiry Interval in seconds or a Schema Object containing the definition of the interval.


Suggested change

<a name="serverBindingObjectSessionExpiryInterval"></a>`sessionExpiryInterval` | [Schema Object][schemaObject] \| integer | Session Expiry Interval in seconds or a Schema Object containing the definition of the interval.

<a name="serverBindingObjectSessionExpiryInterval"></a>`sessionExpiryInterval` | [Schema Object][schemaObject] \| [Reference Object](referenceObject) \| integer | Session Expiry Interval in seconds or a Schema Object containing the definition of the interval.

char0n · 2022-12-19T16:41:00Z

mqtt5/README.md

+This object MUST NOT contain any properties. Its name is reserved for future use.
+
+[schemaObject]: https://www.asyncapi.com/docs/specifications/2.4.0/#schemaObject


Suggested change

[schemaObject]: https://www.asyncapi.com/docs/specifications/2.4.0/#schemaObject

[schemaObject]: https://github.com/asyncapi/spec/blob/master/spec/asyncapi.md#schemaObject

[referenceObject]: https://github.com/asyncapi/spec/blob/master/spec/asyncapi.md#referenceObject

char0n · 2022-12-19T16:43:59Z

mqtt5/json_schemas/server.json

+          "minimum": 0
+        },
+        {
+          "$ref": "https://raw.githubusercontent.com/asyncapi/spec-json-schemas/v2.14.0/schemas/2.4.0.json#/definitions/schema"


Can we replace the $ref with following two objects?

{ "$ref": "https://asyncapi.com/definitions/2.4.0/schema.json" }, { "$ref": "https://asyncapi.com/definitions/2.4.0/Reference.json" }

sudoshreyansh · 2023-01-01T06:38:36Z

I have made the changes @char0n
Could you please review?

fmvilas · 2023-01-10T17:17:45Z

@KhudaDad414 would you mind reapproving if you agree with the changes?

KhudaDad414

LGTM!

fmvilas · 2023-01-12T15:16:59Z

/rtm

add session expiry interval to mqtt5 bindings

7229add

sudoshreyansh requested review from KhudaDad414, derberg, fmvilas and asyncapi-bot-eve as code owners October 11, 2022 13:51

github-actions bot reviewed Oct 11, 2022

View reviewed changes

fmvilas reviewed Oct 11, 2022

View reviewed changes

fmvilas requested review from smoya and dalelane October 11, 2022 15:29

sudoshreyansh added 2 commits October 11, 2022 22:11

update binding version

752a856

Update server.json

98c4591

sudoshreyansh requested review from fmvilas and removed request for smoya and dalelane October 11, 2022 16:47

fmvilas previously approved these changes Oct 14, 2022

View reviewed changes

mqtt5/json_schemas/server.json

}

},

"properties": {

Copy link

Member

fmvilas Oct 14, 2022

Choose a reason for hiding this comment

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

Suggested change

dalelane reviewed Oct 14, 2022

View reviewed changes

sudoshreyansh added 2 commits November 3, 2022 19:07

added schema object option in sessionExpiryInterval

3670f3b

updated version

74c740f

sudoshreyansh dismissed fmvilas’s stale review via 74c740f November 3, 2022 13:42

sudoshreyansh requested review from smoya and char0n as code owners November 3, 2022 13:42

fmvilas requested changes Nov 8, 2022

View reviewed changes

updated description and examples

d953a5e

dalelane previously approved these changes Dec 10, 2022

View reviewed changes

KhudaDad414 previously approved these changes Dec 12, 2022

View reviewed changes

char0n reviewed Dec 19, 2022

View reviewed changes

added reference object as an allowed value

a3911c6

sudoshreyansh dismissed stale reviews from KhudaDad414 and dalelane via a3911c6 January 1, 2023 06:35

sudoshreyansh requested a review from fmvilas January 1, 2023 06:39

dalelane approved these changes Jan 2, 2023

View reviewed changes

fmvilas approved these changes Jan 10, 2023

View reviewed changes

KhudaDad414 approved these changes Jan 10, 2023

View reviewed changes

sudoshreyansh and others added 2 commits January 11, 2023 15:13

Merge branch 'master' into session-expiry-interval-mqtt5

d6b6878

Merge branch 'master' into session-expiry-interval-mqtt5

298b115

asyncapi-bot added the ready-to-merge label Jan 12, 2023

asyncapi-bot merged commit de3fe35 into asyncapi:master Jan 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: session expiry interval to mqtt5 bindings #157

feat: session expiry interval to mqtt5 bindings #157

sudoshreyansh commented Oct 11, 2022

github-actions bot left a comment

fmvilas left a comment

sudoshreyansh commented Oct 11, 2022

fmvilas left a comment

fmvilas Oct 14, 2022

dalelane left a comment

rcoppen commented Oct 14, 2022

dan-r commented Oct 14, 2022

smoya commented Oct 17, 2022

rcoppen commented Oct 18, 2022

smoya commented Oct 19, 2022

rcoppen commented Oct 19, 2022

fmvilas commented Oct 20, 2022

sudoshreyansh commented Nov 3, 2022

fmvilas Nov 8, 2022

fmvilas commented Dec 9, 2022

fmvilas commented Dec 12, 2022

KhudaDad414 left a comment

fmvilas commented Dec 12, 2022

char0n left a comment

char0n Dec 19, 2022

char0n Dec 19, 2022

char0n Dec 19, 2022

sudoshreyansh commented Jan 1, 2023

fmvilas commented Jan 10, 2023

KhudaDad414 left a comment

fmvilas commented Jan 12, 2023

	<a name="serverBindingObjectSessionExpiryInterval"></a>`sessionExpiryInterval` \| [Schema Object][schemaObject] \\| integer \| Session Expiry Interval in seconds or a Schema Object containing the definition of the interval.
	<a name="serverBindingObjectSessionExpiryInterval"></a>`sessionExpiryInterval` \| [Schema Object][schemaObject] \\| [Reference Object](referenceObject) \\| integer \| Session Expiry Interval in seconds or a Schema Object containing the definition of the interval.

		This object MUST NOT contain any properties. Its name is reserved for future use.

		[schemaObject]: https://www.asyncapi.com/docs/specifications/2.4.0/#schemaObject

	[schemaObject]: https://www.asyncapi.com/docs/specifications/2.4.0/#schemaObject
	[schemaObject]: https://github.com/asyncapi/spec/blob/master/spec/asyncapi.md#schemaObject
	[referenceObject]: https://github.com/asyncapi/spec/blob/master/spec/asyncapi.md#referenceObject

feat: session expiry interval to mqtt5 bindings #157

feat: session expiry interval to mqtt5 bindings #157

Conversation

sudoshreyansh commented Oct 11, 2022

github-actions bot left a comment

Choose a reason for hiding this comment

fmvilas left a comment

Choose a reason for hiding this comment

sudoshreyansh commented Oct 11, 2022

fmvilas left a comment

Choose a reason for hiding this comment

fmvilas Oct 14, 2022

Choose a reason for hiding this comment

dalelane left a comment

Choose a reason for hiding this comment

rcoppen commented Oct 14, 2022

dan-r commented Oct 14, 2022

smoya commented Oct 17, 2022

rcoppen commented Oct 18, 2022

smoya commented Oct 19, 2022

rcoppen commented Oct 19, 2022

fmvilas commented Oct 20, 2022

sudoshreyansh commented Nov 3, 2022

fmvilas Nov 8, 2022

Choose a reason for hiding this comment

fmvilas commented Dec 9, 2022

fmvilas commented Dec 12, 2022

KhudaDad414 left a comment

Choose a reason for hiding this comment

fmvilas commented Dec 12, 2022

char0n left a comment

Choose a reason for hiding this comment

char0n Dec 19, 2022

Choose a reason for hiding this comment

char0n Dec 19, 2022

Choose a reason for hiding this comment

char0n Dec 19, 2022

Choose a reason for hiding this comment

sudoshreyansh commented Jan 1, 2023

fmvilas commented Jan 10, 2023

KhudaDad414 left a comment

Choose a reason for hiding this comment

fmvilas commented Jan 12, 2023