Validation for Permission classes #1290
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1290 +/- ##
==========================================
- Coverage 98.05% 98.03% -0.02%
==========================================
Files 105 105
Lines 3759 3774 +15
Branches 529 532 +3
==========================================
+ Hits 3686 3700 +14
- Misses 41 42 +1
Partials 32 32 |
…ed uneeded comment
There was a problem hiding this comment.
Hi @Zlira, thanks for the PR.
I agree that BasePermission should be abstract with has_permission, but I'm not entirely sold on the quasi-requirement to force fields with permissions to be Optional. Can you split this into two PRs so that we can get the abstract stuff merged now, and we can start a discussion on the Optional stuff?
| """ | ||
| Base class for creating permissions | ||
| """ | ||
|
|
||
| message: Optional[str] = None | ||
|
|
||
| @abc.abstractclassmethod |
There was a problem hiding this comment.
| @abc.abstractclassmethod | |
| @abc.abstractmethod |
| # assert all abstact methods are implemented in a subclass | ||
| not_implemented_methods = getattr(cls, "__abstractmethods__", None) | ||
| if not_implemented_methods: | ||
| method_names = ", ".join(not_implemented_methods) | ||
| raise NotImplementedError( | ||
| f"Permission class {cls.__name__} " | ||
| f"should have the following methods implemented: {method_names}" | ||
| ) |
There was a problem hiding this comment.
This logic should already be performed by ABC. Why repeat it?
There was a problem hiding this comment.
@BryceBeagle Thanks for the feedback. This is also what I thought at first. But the ABC check is performed during instantiation. So if this check is skipped inside the validation method, the schema initialization will not fail. And instead, the user will get the error querying any field that uses that invalid Permission class:
{
"data": null,
"errors": [
{
"message": "Can't instantiate abstract class IsAuthenticated with abstract method has_permission",
...
}
]
}It is similar to how it works now just a different error message.
One idea I had was to try and instantiate the permission class inside the validation method:
@classmethod
def assert_valid_for_field(cls, field: "StrawberryField") -> None:
# assert all abstact methods are implemented in a subclass
cls()But that seemed less explicit, and if there are ever any parameters needed for the __init__ this approach might not work.
I'll split the PR but let's first discuss this here, ok?
There was a problem hiding this comment.
Oh good point. Can we use inspect.isabstract here instead of the manual check against __abstractmethods__?
| # assert all abstact methods are implemented in a subclass | ||
| not_implemented_methods = getattr(cls, "__abstractmethods__", None) | ||
| if not_implemented_methods: | ||
| method_names = ", ".join(not_implemented_methods) | ||
| raise NotImplementedError( | ||
| f"Permission class {cls.__name__} " | ||
| f"should have the following methods implemented: {method_names}" | ||
| ) |
There was a problem hiding this comment.
Oh good point. Can we use inspect.isabstract here instead of the manual check against __abstractmethods__?
|
@Zlira I'll close this for now, I think it might be nice to have this, but we might do some changes to permissions in future, we can think about this then 😊 |
Added two validation rules for Permission classes that should be checked on schema initialization:
has_permissionmethod implemented.Description
This is a draft PR because I'm not sure that these changes are even needed. As far as I understood, this page states that there are plans to deprecate Permission classes. But I've recently run into a problem with Permissions on non-optional fields on my project, so I figured the warning would be helpful to users while this feature is still in use. And if the Permission class lacks the
has_permissionmethod, it makes sense to fail during initialization, not while resolving queries.I also doubt that schema converter is the best place to run these checks, maybe you have suggestions about this?
Types of Changes
Issues Fixed or Closed by This PR
Checklist