-
Notifications
You must be signed in to change notification settings - Fork 521
Implement Fabric Permission API #5226
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
Open
Patbox
wants to merge
22
commits into
FabricMC:26.1
Choose a base branch
from
Patbox:permission-api
base: 26.1
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 8 commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
ca75625
Create Fabric Permission API
Patbox 63f2ba3
Improve javadocs, move from interface to a final class, safer Command…
Patbox 4b87796
Merge branch '26.1' into permission-api
Patbox 5f31c28
Merge branch '26.1' into permission-api
Patbox 98e6e08
Async permission check utility methods, mention possible slowness of …
Patbox 07cc1e1
Provide better support for async permission checks on provider side
Patbox a83a17b
Merge branch '26.1' into permission-api
Patbox 13d5cda
Fix async check (oops)
Patbox 2bf037a
Wrap permission key and codec into a single object, swap from tristat…
Patbox 050baef
Write package-info, test key serialization
Patbox e002854
Intern keys, don't use supply async
Patbox ee133e5
Fix interner being initialized too late
Patbox 60cc5eb
a
Patbox b9d0c87
Mark as experimental
Patbox 77f1229
Merge branch '26.1' into permission-api
Patbox 703a71e
Merge remote-tracking branch 'origin/permission-api' into permission-api
Patbox 1d1cfc3
Drop serializable context keys for now
Patbox 73eab46
Merge branch '26.1' into permission-api
Patbox 9d52cb9
Rework how permissions are resolved, remove async methods, add server…
Patbox 856cf52
Merge branch '26.1' into permission-api
Patbox 49648a4
Checkstyle fix
Patbox cbeeb1a
Fix not being marked as experimental in fabric.mod.json
Patbox File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| version = getSubprojectVersion(project) | ||
|
|
||
| loom { | ||
| accessWidenerPath = file('src/main/resources/fabric-permission-api-v1.classtweaker') | ||
| } | ||
|
|
||
| moduleDependencies(project, [ | ||
| ":fabric-api-base" | ||
| ]) | ||
|
|
||
| testDependencies(project, [ | ||
| ":fabric-command-api-v2" | ||
| ]) |
26 changes: 26 additions & 0 deletions
26
...-api-v1/src/main/java/net/fabricmc/fabric/api/permission/v1/MutablePermissionContext.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| /* | ||
| * Copyright (c) 2016, 2017, 2018, 2019 FabricMC | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package net.fabricmc.fabric.api.permission.v1; | ||
|
|
||
| import org.jspecify.annotations.Nullable; | ||
|
|
||
| /** | ||
| * Mutable version of {@link PermissionContext}, intended for creation of custom context conditions. | ||
| */ | ||
| public interface MutablePermissionContext extends PermissionContext { | ||
| <T> void set(PermissionContext.Key<T> key, @Nullable T value); | ||
| } |
108 changes: 108 additions & 0 deletions
108
...n-api-v1/src/main/java/net/fabricmc/fabric/api/permission/v1/PermissionCheckCallback.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| /* | ||
| * Copyright (c) 2016, 2017, 2018, 2019 FabricMC | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package net.fabricmc.fabric.api.permission.v1; | ||
|
|
||
| import java.util.Objects; | ||
| import java.util.concurrent.CompletableFuture; | ||
|
|
||
| import com.mojang.serialization.Codec; | ||
| import org.jspecify.annotations.Nullable; | ||
|
|
||
| import net.minecraft.resources.Identifier; | ||
|
|
||
| import net.fabricmc.fabric.api.event.Event; | ||
| import net.fabricmc.fabric.impl.permission.PermissionCheckCallbackImpl; | ||
|
|
||
| /** | ||
| * The event used for getting the permission result for given context. | ||
| * Implemented callbacks for this event should be thread safe, as permission methods can be called from another thread. | ||
| * Additionally, the execution should be reasonably fast for non-player and online player cases. | ||
| * Offline player checks are allowed to be slower and can happen asynchronously. | ||
| * | ||
| * <p>When implementing this callback, only {@link PermissionCheckCallback#onPermissionCheck} needs to be implemented, | ||
| * but for better performance in case of support for async lookup, the {@link PermissionCheckCallback#onAsyncPermissionCheck} | ||
| * method should also be implemented. In case it wasn't it will default to running {@link PermissionCheckCallback#onPermissionCheck} | ||
| * on current thread. | ||
| * | ||
| * <p>To check for permissions, you should use dedicated methods from {@link PermissionContextOwner} interface | ||
| * and it's implementations over invoking this event. | ||
| */ | ||
| public interface PermissionCheckCallback { | ||
| /** | ||
| * Registers the permission callback. | ||
| * | ||
| * @param callback permission check callback to register | ||
| */ | ||
| static void register(PermissionCheckCallback callback) { | ||
| register(Event.DEFAULT_PHASE, callback); | ||
| } | ||
|
|
||
| /** | ||
| * Registers the permission callback. | ||
| * | ||
| * @param phase ordering phase to place the callback | ||
| * @param callback permission check callback to register | ||
| */ | ||
| static void register(Identifier phase, PermissionCheckCallback callback) { | ||
| Objects.requireNonNull(phase, "phase can't be null!"); | ||
| Objects.requireNonNull(callback, "callback can't be null!"); | ||
|
|
||
| PermissionCheckCallbackImpl.MAIN_EVENT.register(phase, callback::onPermissionCheck); | ||
| PermissionCheckCallbackImpl.ASYNC_EVENT.register(phase, callback::onAsyncPermissionCheck); | ||
| } | ||
|
|
||
| /** | ||
| * Orders the phases in provided order. | ||
| * | ||
| * @param firstPhase the id of the phase that should happen first | ||
| * @param lastPhase the id of the phase that should happen last | ||
| */ | ||
| static void addPhaseOrdering(Identifier firstPhase, Identifier lastPhase) { | ||
| Objects.requireNonNull(firstPhase, "firstPhase can't be null!"); | ||
| Objects.requireNonNull(lastPhase, "lastPhase can't be null!"); | ||
|
|
||
| PermissionCheckCallbackImpl.MAIN_EVENT.addPhaseOrdering(firstPhase, lastPhase); | ||
| PermissionCheckCallbackImpl.ASYNC_EVENT.addPhaseOrdering(firstPhase, lastPhase); | ||
| } | ||
|
|
||
| /** | ||
| * Main check method, executes on current thread. | ||
| * | ||
| * @param context context to check for | ||
| * @param permission identifier of the permission | ||
| * @param permissionType codec representing type if permissions | ||
| * @param <T> type of permission | ||
| * @return value of type T if present, null to pass through. | ||
| */ | ||
| @Nullable | ||
| <T> T onPermissionCheck(PermissionContext context, Identifier permission, Codec<T> permissionType); | ||
|
|
||
| /** | ||
| * Async permission check method. | ||
| * | ||
| * @param context context to check for | ||
| * @param permission identifier of the permission | ||
| * @param permissionType codec representing type if permissions | ||
| * @param <T> type of permission | ||
| * @return a completable future value of type T if present, null or null containing completable future to quickly pass through to next callback. | ||
| */ | ||
| @Nullable | ||
| default <T> CompletableFuture<@Nullable T> onAsyncPermissionCheck(PermissionContext context, Identifier permission, Codec<T> permissionType) { | ||
| T value = this.onPermissionCheck(context, permission, permissionType); | ||
| return value != null ? CompletableFuture.completedFuture(value) : null; | ||
| } | ||
| } | ||
44 changes: 44 additions & 0 deletions
44
...rmission-api-v1/src/main/java/net/fabricmc/fabric/api/permission/v1/PermissionCodecs.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| /* | ||
| * Copyright (c) 2016, 2017, 2018, 2019 FabricMC | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package net.fabricmc.fabric.api.permission.v1; | ||
|
|
||
| import com.mojang.serialization.Codec; | ||
|
|
||
| import net.minecraft.util.TriState; | ||
|
|
||
| /** | ||
| * Set of <i>suggested</i> codecs used for permission checks. | ||
| * While custom codecs are also allowed, using one below (or other minimal codecs) is encouraged. | ||
| */ | ||
| public final class PermissionCodecs { | ||
| private PermissionCodecs() { } | ||
|
|
||
| /** | ||
| * TriState codec, can be used for simple boolean / ability checks. | ||
| */ | ||
| public static Codec<TriState> TRI_STATE = TriState.CODEC; | ||
|
|
||
| /** | ||
| * Integer codec, can be used for limit checks. | ||
| */ | ||
| public static Codec<Integer> INT = Codec.INT; | ||
|
|
||
| /** | ||
| * String codec, can be used for display. | ||
| */ | ||
| public static Codec<String> STRING = Codec.STRING; | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
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.
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 worry this is needlessly complex having two methods like this. Why not just have the method that returns a CompletableFuture and let the implimentation use
CompletableFuture.completedFutureas it needs?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.
In most cases generally you want to complete it synchroniously (player is online and you just want to check some permission), so needing to use async method is way less common. Mostly included them as I wanted permission providers to have option for better handling of them (and as it was requested from lucko). Also it allows to have them run with higher priority in cases when they are actually needed just now
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 have no objection to having them async and it makes sense why you may need this, I am suggesting to go full in on CompletableFuture's. Maybe this is the way to go with the default that calls a none async version.
Maybe the middle ground is for the impl to always use the CompletableFuture path, to save the complex duplication. The onPermissionCheck function could be @ApiStatus.OverrideOnly
It doesnt make much sense to me for a CompletableFuture to be nullable either.
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.
CompletableFuture being nullable was mostly intended as shortcut to not require it going over
res.thenComposechain. Also downside of async one is that it will always go through all callbacks (but only use first result), while sync/regular will do an early return. I think keeping them as seperate still makes sense through, as async implementation will likely only be needed by few providers/users. It also allows it to work at "lower priority" / execute slower compared to sync where it's needed "right now".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 worry this premature optimisation is just making everything a lot harder to follow. I tottaly get having a synchronous user facing API on either end, but Im not keen on making everything even harder to follow maybe async maybe not like behaviour. CompletableFuture's can already be quite hard to follow and making them nulltable with a nullable value makes it really hard to reason about.
Both of these functions could be @ApiStatus.OverrideOnly, making it quite easy to change this in the future if it does turn out to be a performance issue.
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.
After looking into this I still think it makes sense to keep sync and async calls separate. Primarily since it keeps the expectations of the behaviour of called methods (sync calls should only happen on calling thread, while async ones can be executed on a different one) and allows to have them prioritized if needed (example offline player check that need to lookup into the database). Even more so with sync checks being primary ones to be used (since most checks happen when player is online or happening to non-players), them just being
.join()ed async calls could become noticeable (in cases where a lot of entities/players/blocks/whatever request a permission).