Skip to content

Conversation

CristopherH95
Copy link

This PR adds two new methods to the ObjectPermissionsBackend class, to support async environments: ahas_perm and ahas_module_perms. These implementations just wrap the original sync versions in sync_to_async, which comes from asgiref (and should be included in all currently supported versions of Django).

Resolves #199.

This PR used #197 as a base, so that PR should probably be merged before this one.

@federicobond
Copy link
Collaborator

Hi @CristopherH95 can you fix the linter issues so we can see the tests running in CI?

@CristopherH95
Copy link
Author

I updated the module sort, to address the isort issue, but I will need to spend a little more time on the check-manifest issue. I am not 100% sure yet what is causing it, asgiref is an install dependency for Django so I'm not clear yet on why the ModuleNotFoundError is being raised.

@CristopherH95
Copy link
Author

I think I have figured out what the issue is. When check-manifest runs, it imports the package version number from rules/__init__.py. When that happens, imports for permissions.py, predicates.py, and rulesets.py run. Django is not a listed dependency for the package, so asgiref is not installed in a fresh environment. Ordinarily, this did not cause issues because permissions.py, predicates.py, and rulesets.py do not use anything from Django. However, this code uses asgiref for its sync_to_async decorator, which is installed with Django, so the error occurs.

I have three ideas for how to address this (though I am open to any other suggestions):

  1. List Django as a dependency (more package-wide change).
  2. I can write a fallback sync_to_async implementation that just runs the sync code with run_in_executor (a change with less broad impact, though my fallback code will likely be less robust than what sync_to_async has to offer).
  3. Lazy import sync_to_async in the specific async methods (probably the simplest change).

Is there any particular preference for the maintainers?

@federicobond
Copy link
Collaborator

I think it makes sense to add a dependency on the minimum version of Django we support, as other Django packages do.

Could you open a separate pull request for that?

@CristopherH95
Copy link
Author

@federicobond I opened a separate PR for adding Django as a dependency here: #201. However, I have run into another issue probably related to the lint problem here. Doing some more tests locally I find that, because VERSION is imported from rules in setup.py: permissions.py, predicates.py, and rulesets.py all appear to be imported when attempting to install or run sdist from setup.py. This can then lead to continued ModuleNotFoundError exceptions.

Looking at the Django project itself, they also define VERSION in their root __init__.py. However, they use pyproject.toml to dynamically retrieve it. I am thinking it may be worth considering also changing the way the version is retrieved for the package metadata here (for example, either by defining it somewhere that does not import modules that might have dependencies that aren't installed yet or by using pyproject.toml).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rules are skipped in ahas_perm check
3 participants