Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement Permissions API #229
base: dev/v2
Are you sure you want to change the base?
Implement Permissions API #229
Changes from 32 commits
11e0492
2dba238
6fde59f
3cea14e
5856160
4ca5ce8
a12c1b3
2978084
94defc8
4cbbc25
73f0695
e698aeb
4afceec
c62b477
bb411e9
9aee46c
7779ac5
5b399c6
4082e78
4abc1ed
5fcbdd7
16eb615
06b8cc2
49f8120
c102ce0
ddef41d
213884c
4d47ce8
46e594a
a966915
5b13b61
cee38b7
3726e34
067f29b
2d79a88
adb9c9a
98269eb
3891a02
3a7eb23
68e90f3
eb81eae
dac47d7
f37c380
235db89
82bb05e
7159a0f
789878c
f633b43
2c870d2
49ce899
46ef940
4bd2284
afe4506
3deb463
8817874
c0540a4
e235362
516f31d
9c2d517
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering whether using CompletableFuture here is the best approach. Rather than have a data request be performed every time
hasPermission
is called, why not create a method to pre-load user permission data? The client plugin may request permission data for a user; once loaded, this permission data is in-memory sohasPermission
calls using the data object return synchronously. I've never used the LuckPerms API but I believe it allows something like this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that should not be in the hands of the API, and rather, the permissions service provider. Regardless, it would not be ideal to add a synchronous cache-assuming
hasPermission
method since that will be abused by developers instead of using the CF version, even when they are not certain that permissions have been cached for a user.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of this idea? We might design the API so that it is impossible to use a cache-backed
hasPermission
without first loading such cache.For example, a
loadUser(UUID)
method returns aCompletableFuture<LoadedUser>
. Then, the synchronousLoadedUser#hasPermission
allows checking permissions synchronously.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API could handle caching, though it is imperative that permission service provider developers fully understand that they must update the cache of a permission holder when some relevant part of their context changes.
For instance, if permission X is only given to player Y in world Z, and player Y changes to world W, then the cache should be updated primarily to ensure that permission X only remains allocated if the player remains in world Z.
(Whether this updates the entire cache, or just re-checks that individual permission, is another matter.)
Another consideration to make is that the rest of the APIs do not offer inbuilt caching. Ideally, we keep things consistent between our APIs.
Overall, I'm uncertain about that idea, and more inclined to think it would not be the best route forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current design allows for caching. Changing the design to something like
loadUser
will often lead to confusion between both permission plugins and plugins which use the api.Consider this: plugin x uses the api and calls
loadUser
each time they need it, which causes multiple database queries to be performed, because developer y of plugin x doesn't know the implementation cache is being updated (if it really is, because permission plugin z may choose not to update it). The current design significantly reduces database queries, because one may not need the whole information about the account.