Add SubjectDataUpdateEvent for permission changes #1627
Conversation
|
I agree with Zidane here. Even if the perm plugin is used acros multiple servers as long as the change is syned to all servers (to update their perm cache) they should also give the oportunity to other plugins to update their caches. As for changes made offline. The perm plugin could post the event when the entry is loaded or just rely on the other plugin to update its cache on join. If this is done during a perm plugin reload the plugin should throw this event for all online users/loaded subjects because things could have changed. |
| protected void onUpdate() { | ||
| if (this.updateListener != null) { | ||
| this.updateListener.accept(this); | ||
| } |
There was a problem hiding this comment.
IMO this should just post the change event. Otherwise this might be forgotten.
There was a problem hiding this comment.
It gives the implementing plugin a chance to add any causes etc.
Also, since it is a requirement that the event is called async, a plugin instance is needed to schedule the event.
lucko
force-pushed
the
feature/perms-events
branch
from
6d73959
to
04a2b94
Compare
|
I've removed the clause about the event being optional. |
|
|
||
| @Override | ||
| public Subject getSubject() { | ||
| return subject; |
lucko
force-pushed
the
feature/perms-events
branch
from
04a2b94
to
c2cdcba
Compare
|
Umm, I've kinda forgotten where this got to exactly, so err... I think the issues remaining are:
I think it might make more sense to either:
I'm not sure if this be preferred over the current Consumer (defined listener) implementation. ¯\_(ツ)_/¯ (tagging @Zidane because I think it was you who asked me to PR this, maybe) |
|
@lucko can you please fix conflicts? |
lucko
force-pushed
the
feature/perms-events
branch
from
2a0361d
to
e4426f6
Compare
lucko
force-pushed
the
feature/perms-events
branch
from
e4426f6
to
b909c8c
Compare
|
@bloodmc Done I've swapped out the Consumer in favour of a protected update method. |
Continues on from #1524 (and #1411)
Why only an update event?
More specific update events are hard to (accurately) implement, and I personally don't think they'll get used. The PermissionService API is already pretty confusing to understand (especially to implement) and I think adding events into the mix will make things significantly more difficult.
Adding more specific stuff like
PermissionSetEventraises questions about what to do for bulk changes, changes which are made offline, etc.In my opinion, a simple update event is all that is needed. It allows plugins which use permissions for data storage to update any caches they may have when data is updated.
Should the event be called async?
The API was recently updated to use CompletableFutures for SubjectData changes, so imo, it makes sense for the event to always be called async.
Consistency is obviously important, it can't be sometimes async and sometimes not. I'm not sure if async events would be a new addition for Sponge, but I assume they're supported? (in other words, is the EventManager thread safe?)
I think that's about it.