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: preUnsubscribe handler #605

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

chfritz
Copy link

@chfritz chfritz commented Mar 31, 2021

Invoked then a client unsubscribes from topics. Just like authorizeSubscribe, this function allows modifying the topics before executing the unsubscribe. This is needed in cases where topics are modified in authorizeSubscribe. For example:

aedes.authorizeSubscribe = (client, subscription, callback) => {
  // overwrite subscription: force client to its namespace
  subscription.topic = `/${client.id}/${subscription.topic}`;
  callback(null, subscription);
}

aedes.preUnsubscribe = (client, packet, callback) => {
  // overwrite unsubscriptions: force client to its namespace
  for (let i in packet.unsubscriptions) {
    packet.unsubscriptions[i] = `/${client.id}/${packet.unsubscriptions[i]}`;
  }
  callback(client, packet)
}

@coveralls
Copy link

coveralls commented Mar 31, 2021

Pull Request Test Coverage Report for Build 709159642

  • 5 of 5 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 99.555%

Totals Coverage Status
Change from base Build 689232499: 0.002%
Covered Lines: 804
Relevant Lines: 805

💛 - Coveralls

docs/Aedes.md Outdated Show resolved Hide resolved
@@ -69,6 +69,7 @@ function Aedes (opts) {
this.authorizePublish = opts.authorizePublish
this.authorizeSubscribe = opts.authorizeSubscribe
this.authorizeForward = opts.authorizeForward
this.preUnsubscribe = opts.preUnsubscribe
Copy link
Member

Choose a reason for hiding this comment

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

You should add a defaultPreSubscribe function and add it to defaultOptions

Copy link
Author

Choose a reason for hiding this comment

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

Ah, yes, good point. Thanks, done.

Copy link
Member

Choose a reason for hiding this comment

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

Should that allow some async stuff like the others do? maybe adding a callbackk to it?

@chfritz chfritz force-pushed the add_preunsubscribe_handler branch from 35986a5 to 1436c79 Compare April 1, 2021 14:49
@robertsLando robertsLando changed the title Added a preUnsubscribe handler, analog to authorizeSubscribe feat: preUnsubscribe handler Apr 1, 2021
@@ -69,6 +69,7 @@ function Aedes (opts) {
this.authorizePublish = opts.authorizePublish
this.authorizeSubscribe = opts.authorizeSubscribe
this.authorizeForward = opts.authorizeForward
this.preUnsubscribe = opts.preUnsubscribe
Copy link
Member

Choose a reason for hiding this comment

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

Should that allow some async stuff like the others do? maybe adding a callbackk to it?

@gnought
Copy link
Collaborator

gnought commented Apr 1, 2021

The main question is what's the use case of modifying subscription topic in authorizeSubscribe ?

@robertsLando
Copy link
Member

@gnought You mean preSubscribe ?

@chfritz
Copy link
Author

chfritz commented Apr 1, 2021

@gnought the example I'm providing in the description is the use-case I need it for: I need to force clients into their own namespace, without the clients needing to know about their namespaces. Clients may subscribe to #, i.e., get everything, but will actually be reduced to /${client.id}/#, i.e., everything in their namespace only.

@chfritz chfritz force-pushed the add_preunsubscribe_handler branch 2 times, most recently from 4fbc84c to 6deb055 Compare April 1, 2021 16:23
@chfritz
Copy link
Author

chfritz commented Apr 1, 2021

@robertsLando I've made preUnsubscribe async as you suggested.

docs/Aedes.md Outdated Show resolved Hide resolved
@chfritz chfritz force-pushed the add_preunsubscribe_handler branch from 6deb055 to c00dc6c Compare April 1, 2021 16:25
Invoked then a client unsubscribes from topics. Just like authorizeSubscribe, this function allows modifying the topics because executing the unsubscribe. This is needed in cases where topics are modified in authorizeSubscribe. For example:

```js
aedes.authorizeSubscribe = (client, subscription, callback) => {
  // overwrite subscription: force client to its namespace
  subscription.topic = `/${client.id}/${subscription.topic}`;
  callback(null, subscription);
}

aedes.preUnsubscribe = (client, packet, callback) => {
  // overwrite unsubscriptions: force client to its namespace
  for (let i in packet.unsubscriptions) {
    packet.unsubscriptions[i] = `/${client.id}/${packet.unsubscriptions[i]}`;
  }
  callback(client, packet)
}
```

Includes fix-ups:
- Update docs/Aedes.md
  Co-authored-by: Daniel Lando <[email protected]>
- added defaultPreUnsubscribe
- made preUnsubscribe async like authorizeSubscribe
@chfritz chfritz force-pushed the add_preunsubscribe_handler branch from c00dc6c to 1e96245 Compare April 1, 2021 16:27
@gnought
Copy link
Collaborator

gnought commented Apr 1, 2021

I believe you also hook authorizePublish to modify the PUBLISH topic, am I right?

@chfritz
Copy link
Author

chfritz commented Apr 1, 2021

yes, exactly, @gnought .

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Can you please add a unit test?

@gnought
Copy link
Collaborator

gnought commented Apr 3, 2021

How about if we add the support of client isolation? Will it fit in your case? @chfritz

@chfritz
Copy link
Author

chfritz commented Apr 3, 2021

@mcollina : will do.
@gnought : maybe. What do you mean by support for client isolation?

@gnought
Copy link
Collaborator

gnought commented Apr 3, 2021

Just like your use case, we prepend some client-specific string in client topics.

@chfritz
Copy link
Author

chfritz commented Apr 3, 2021

I see. Yes, I think that would work for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants