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

Add OnSendToSubscriber Event Hooks #383

Closed
wants to merge 4 commits into from
Closed

Add OnSendToSubscriber Event Hooks #383

wants to merge 4 commits into from

Conversation

leijux
Copy link

@leijux leijux commented Apr 7, 2024

See #377

@werbenhu
Copy link
Member

werbenhu commented Apr 10, 2024

@leijux Would merging the onAclCheck and OnSendToSubscriber into one make it cleaner and tidier? Just need to modify the parameters of onAclCheck.
Change

func (h *HookBase) OnACLCheck(cl *Client, topic string, write bool) bool

to

func (h *HookBase) OnACLCheck(cl *Client, pk packets.Packet, write bool) bool

@werbenhu
Copy link
Member

If we directly modify the parameters of OnACLCheck, won't it affect compatibility with older versions? @mochi-co @thedevop , what are your thoughts on this?

@thedevop
Copy link
Collaborator

If we directly modify the parameters of OnACLCheck, won't it affect compatibility with older versions? @mochi-co @thedevop , what are your thoughts on this?

I do like merging this into OnACLCheck, in addition to the benefits you outlined, it is also a bit more performant, as hooks are not "free".

Note, this capability is only usable for embedded use case as current auth hook doesn't provide mechanism to examine packet.

@mochi-co
Copy link
Collaborator

@werbenhu @thedevop @leijux I'm also in favour of changing the signature of OnACLCheck, as we will otherwise be creating significant duplicated functionality. However, it will be a breaking change to OnACLCheck (and server references and tests), so I recommend opening a new PR which specifically targets this method.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 8585944022

Details

  • 11 of 14 (78.57%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.05%) to 98.756%

Changes Missing Coverage Covered Lines Changed/Added Lines %
server.go 1 4 25.0%
Totals Coverage Status
Change from base Build 8510971796: -0.05%
Covered Lines: 6115
Relevant Lines: 6192

💛 - Coveralls

@leijux
Copy link
Author

leijux commented Apr 17, 2024

@mochi-co @werbenhu @thedevop Thank you everyone for your opinions. I agree very much with merging into OnACLCheck. I will close this PR and try to merge into OnACLCheck and submit a new PR.

@leijux leijux closed this Apr 17, 2024
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