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

Added authorizeForward #44

Merged
merged 10 commits into from
Apr 28, 2016
Merged

Added authorizeForward #44

merged 10 commits into from
Apr 28, 2016

Conversation

stochris
Copy link

Hello. I've added the method we talked about. However, I'm not very sure about the arguments of the callback. Right now, it only looks at err, the rest being modified on the original object.

I did see a slight drop in perfomance, about 2-5% after adding the method, without supplying anything custom in the config.

@coveralls
Copy link

coveralls commented Apr 26, 2016

Coverage Status

Coverage decreased (-0.3%) to 96.474% when pulling 3b56e00 on stoiKris:master into 4394240 on mcollina:master.

@stochris
Copy link
Author

Uhm, seems I didn't cover all my bases in the unit test. I could look into it a bit more if that's a problem

@mcollina
Copy link
Collaborator

A bad design decision I did in Mosca is making that function async. That
function needs to be synchronous, otherwise performance will be extremely
affected.

Would you mind making it sync?
Il giorno mar 26 apr 2016 alle 20:39 stoiKris notifications@github.com ha
scritto:

Uhm, seems I didn't cover all my bases in the unit test. I could look into
it a bit more if that's a problem


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#44 (comment)

@cristian-stoicescu
Copy link

cristian-stoicescu commented Apr 27, 2016

I thought it was strange the performance dropped after I added the callback.
Btw, in your opinion, should the function also return the package and client, or just modify it's parameters, and only return the error?
I'm thinking an elegant way would be to return undefined if there was an error else return the client and the packet.
What do you think?

@mcollina
Copy link
Collaborator

@cristian-stoicescu just return the packet, so the function can overwrite anything.

BTW, authorizeForward needs to be called only if it's a publish for QoS 1/2. Moreover you will need to call persistence.outgoingClearMessageId to remove the packet if the authorization is denied, if the client is not clean.

I know it's a lot of work for this feature, thanks for helping out!

… prevent the sending of the message to the client
@coveralls
Copy link

coveralls commented Apr 27, 2016

Coverage Status

Coverage increased (+0.04%) to 96.785% when pulling 159f8d3 on stoiKris:master into 4394240 on mcollina:master.

var _authPacket = that.broker.authorizeForward(that, _packet)
if (util.isNullOrUndefined(_authPacket)) {
// we consider this to be an error, since the packet is undefined, so there's nothing to send
that.broker.persistence.outgoingClearMessageId(that, _packet, function () {})
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't allocate a function here.

@@ -75,11 +75,17 @@ function Client (broker, conn) {
}

this.deliverQoS = function deliverQoS (_packet, cb) {
var _authPacket = that.broker.authorizeForward(that, _packet)
if (util.isNullOrUndefined(_authPacket)) {
Copy link
Collaborator

@mcollina mcollina Apr 27, 2016

Choose a reason for hiding this comment

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

just check if (!_authPacket)

@coveralls
Copy link

coveralls commented Apr 27, 2016

Coverage Status

Coverage increased (+0.04%) to 96.785% when pulling 75a9351 on stoiKris:master into 4394240 on mcollina:master.

@coveralls
Copy link

coveralls commented Apr 27, 2016

Coverage Status

Coverage increased (+0.04%) to 96.785% when pulling aea9ddf on stoiKris:master into 4394240 on mcollina:master.

@stochris
Copy link
Author

Sorry for the tons of commits, github has a habit of hiding "outdated" comments, so I see them too late

@coveralls
Copy link

coveralls commented Apr 27, 2016

Coverage Status

Coverage increased (+0.04%) to 96.785% when pulling 85da282 on stoiKris:master into 4394240 on mcollina:master.

@coveralls
Copy link

coveralls commented Apr 27, 2016

Coverage Status

Coverage increased (+0.04%) to 96.785% when pulling 804af2a on stoiKris:master into 4394240 on mcollina:master.

@coveralls
Copy link

coveralls commented Apr 27, 2016

Coverage Status

Coverage increased (+0.05%) to 96.8% when pulling d035c57 on stoiKris:master into 4394240 on mcollina:master.

@mcollina
Copy link
Collaborator

Actually no, there is no need to check, those methods are only used for
publishes :/. Sorry
Il giorno mer 27 apr 2016 alle 13:32 Coveralls notifications@github.com
ha scritto:

[image: Coverage Status] https://coveralls.io/builds/5951593

Coverage increased (+0.05%) to 96.8% when pulling d035c57
d035c57
on stoiKris:master
into 4394240
4394240
on mcollina:master
.


You are receiving this because you commented.

Reply to this email directly or view it on GitHub
#44 (comment)

… only packages of that type reach the function
@coveralls
Copy link

coveralls commented Apr 27, 2016

Coverage Status

Coverage increased (+0.04%) to 96.79% when pulling 7cba817 on stoiKris:master into 4394240 on mcollina:master.

@mcollina
Copy link
Collaborator

You need to do the same for the other deliver method, as one is used for QoS 0 and one for QoS 1/2.

@stochris
Copy link
Author

stochris commented Apr 27, 2016

Didn't you say earlier that the authorizeForward needs to be called only if it's a publish for QoS 1/2 ?

I'm confused

p.s.: sorry, closed by mistake

@stochris stochris closed this Apr 27, 2016
@stochris stochris reopened this Apr 27, 2016
@coveralls
Copy link

coveralls commented Apr 27, 2016

Coverage Status

Coverage increased (+0.04%) to 96.79% when pulling 7cba817 on stoiKris:master into 4394240 on mcollina:master.

@mcollina mcollina merged commit 7cba817 into moscajs:master Apr 28, 2016
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.

4 participants