Making these checks more granular will help in a future where read only is not a single flag.
Details
BK, existing read-only tests
Diff Detail
- Repository
- R1 dagster
- Lint
Lint Passed - Unit
No Test Coverage
Event Timeline
This is a really good change - we should more explicitly test it and be a little bit more defensive w/r/t passing strings around.
python_modules/dagster-graphql/dagster_graphql/implementation/utils.py | ||
---|---|---|
9–12 | I think this util would benefit from a few explicit unit test cases: - test_check_permission_has_permission() - test_check_permission_does_not_have_permission() - test_check_permission_permission_does_not_exist() Instead of implicitly testing it via our read-only tests. At that point, we could probably even remove some of the read-only tests (unless there's specific behavior they test beyond just whether or not read-only permissions are applied). | |
10 | Smart to generalize this 🤓 | |
15 | I think we want to raise a different, more general error class. Perhaps we need a function that maps error class to permission? Or a more general PermissionDeniedError? | |
python_modules/dagster-graphql/dagster_graphql/schema/roots/mutation.py | ||
148 | Given that we're just passing strings in, do we want to add an additional check inside of check_permission to ensure that the permission actually exists? That you didn't typo the permission name. Alternatively, we could pass in an enum or a constant to make it less prone to typos. |
python_modules/dagster-graphql/dagster_graphql/implementation/utils.py | ||
---|---|---|
15 | yeah my next diff is going to change this to GrapheneAuthorizationError |