Skip to content
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

feat: add interface stubs for async adapters #335

Merged
merged 7 commits into from
Dec 28, 2023
Merged

feat: add interface stubs for async adapters #335

merged 7 commits into from
Dec 28, 2023

Conversation

thearchitector
Copy link
Contributor

@thearchitector thearchitector commented Dec 20, 2023

Closes #334.

This PR adds asyncio stubs for all the adapter interfaces in order to draw a strict line between asyncio-based adapters (where all functions are expected to be coroutines) and the sync ones (where they are not).

To make things make more sense, I've renamed the existing casbin.persist.adapters.adapter_filtered::FilteredAdapter to casbin.persist.adapters.filtered_file_adapter::FilteredFileAdapter, since the latter is more accurate and better organizes the module structure.

All of the new interfaces + the existing async_file_adapter are now within casbin.persist.adapters.asyncio as to partition them better; i've tried retaining most of the imports as to avoid too many breaking changes.

but, there are 3 breaking changes:

  • (bugfix) the AsyncEnforcer now requires the adapter instance you pass to be an instance of AsyncAdapter not Adapter.
  • (refactor) adapters.FilteredAdapter is now adapters.FilteredFileAdapter ---> the new adapters.FilteredAdapter is now the old persist.FilteredAdapter interface.
  • (refactor) anyone using direct non-module direct file imports (ie from casbin.persist.update_adapter import UpdateAdapter vs. from casbin.persist import UpdateAdapter) will have to change to the new imports.

@casbin-bot
Copy link
Member

@Nekotoxin please review

@CLAassistant
Copy link

CLAassistant commented Dec 20, 2023

CLA assistant check
All committers have signed the CLA.

@thearchitector thearchitector changed the title add: interface stubs for async adapters feat!: add interface stubs for async adapters Dec 20, 2023
@hsluoyz
Copy link
Member

hsluoyz commented Dec 20, 2023

@thearchitector fix:

image

@hsluoyz
Copy link
Member

hsluoyz commented Dec 20, 2023

@thearchitector breaking changes are not allowed. Python is a dynamic language. I believe there must be ways to do this without any breaking changes (like via dynamic casting). So closed for now

@hsluoyz hsluoyz closed this Dec 20, 2023
@thearchitector
Copy link
Contributor Author

thearchitector commented Dec 20, 2023

@hsluoyz the function definitions themselves are different between the example implementation (async file adapter) and the rest. that is not what Python supports when you say "dynamic"; so at the very least with no other changes, that impl change will be required --- technically the only "non-breaking" kinda feasible way given your constraints is to have the async implementations be synchronous functions that return coroutines vs already being coroutines.

in the greater scope, though, I think the position is somewhat foolish; no breaking changes means there is an implicit trust that the original and initial implementation was the best way to do it -- having looked through the code, I can guarantee that is not true (nor is it ever true with anything).... So what you're actually saying is "foundational improvements are not allowed," and that is the textbook definition of tech debt.

if that's the position and sacrifice, that's fine - nothing I can do about it anyways - but it should be made explicitly clear in the docs and CLA.

@hsluoyz
Copy link
Member

hsluoyz commented Dec 20, 2023

@leeqvip

@thearchitector
Copy link
Contributor Author

it is worth mentioning, I suppose, that doing the "sync returns coro" approach will work, but will also mean any existing adapters that use asynccasbin will have to introduce breaking changes to migrate over to pycasbin

of course, the problem can also just be ignored completely since it's not a runtime bug, in which case no PR is necessary. that's always an option

@leeqvip
Copy link
Member

leeqvip commented Dec 20, 2023

@thearchitector Hi,
I support the following points:

  • Added casbin.persist.adapters.asyncio and AsyncEnforcer, etc., and verified them in AsyncInternalEnforcer
  • Rename adapters.FilteredAdapter to adapters.FilteredFileAdapter, but to ensure compatibility, adapters.FilteredAdapter should be retained to inherit adapters.FilteredFileAdapter

What I don't support are:

  • Change persist.FilteredAdapter interface to adapters.FilteredAdapter. The interface should be placed in the persist directory

I look forward to your revision and I will reopen this PR.

@hsluoyz hsluoyz reopened this Dec 20, 2023
@hsluoyz
Copy link
Member

hsluoyz commented Dec 20, 2023

@thearchitector fix:

image

@thearchitector
Copy link
Contributor Author

@thearchitector Hi, I support the following points:

* Added `casbin.persist.adapters.asyncio` and `AsyncEnforcer`, etc., and verified them in `AsyncInternalEnforcer`

* Rename `adapters.FilteredAdapter` to `adapters.FilteredFileAdapter`, but to ensure compatibility, `adapters.FilteredAdapter` should be retained to inherit `adapters.FilteredFileAdapter`

What I don't support are:

* Change `persist.FilteredAdapter` interface to `adapters.FilteredAdapter`. The` interface` should be placed in the `persist` directory

I look forward to your revision and I will reopen this PR.

@leeqvip @hsluoyz thanks for the feedback. to be clear, you support everything except moving the interfaces, correct? So all the existing interfaces (adapter, adapter_filtered, batch_adapter, etc) should remain in persist? And the new asyncio ones will go in persist.adapters.asyncio?

@hsluoyz
Copy link
Member

hsluoyz commented Dec 23, 2023

@thearchitector yes

@thearchitector
Copy link
Contributor Author

@hsluoyz @leeqvip i've moved the interfaces back to persist and included import aliases for backwards compatibility. i did notice, though, that update_adapter lives within persist.adapters instead of the top level persist -- should it be moved to be with the other interfaces? i think that would make sense. i can do the same import alias pattern as well to maintain backwards compat if you agree.

also, running black i kept running into this bug and had to fiddle with my venv to get it to work. Since Black is stable now, I think it would make sense to bump the version of super-linter and update the black dep in requirements_dev.txt to match? I scanned through the super linter changelog and dont see any reason why we couldn't bump it to 5.7.2, (and update the black dep to 23.11.0)

@thearchitector thearchitector changed the title feat!: add interface stubs for async adapters feat: add interface stubs for async adapters Dec 24, 2023
Copy link
Member

@leeqvip leeqvip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.
In the same way, please move update_adapter to the persist directory.
Regarding the linter warning you mentioned, I didn’t see it?
https://github.com/casbin/pycasbin/actions/runs/7312454795/job/19923199403#step:4:1

@thearchitector
Copy link
Contributor Author

thearchitector commented Dec 27, 2023

Thanks. In the same way, please move update_adapter to the persist directory. Regarding the linter warning you mentioned, I didn’t see it? https://github.com/casbin/pycasbin/actions/runs/7312454795/job/19923199403#step:4:1

@leeqvip it doesn't happen in the github action because super linter pins the broken dependency to an old version, but shows up locally because the dependency isn't pinned in requirements_dev.txt:

image

we can solve it by pinning the hidden dep in the local req.txt file to an older version, but i think it would be best to just bump the super linter version to 5.7.2 and black to 23.11.0

@leeqvip
Copy link
Member

leeqvip commented Dec 27, 2023

Thanks. In the same way, please move update_adapter to the persist directory. Regarding the linter warning you mentioned, I didn’t see it? https://github.com/casbin/pycasbin/actions/runs/7312454795/job/19923199403#step:4:1

@leeqvip it doesn't happen in the github action because super linter pins the broken dependency to an old version, but shows up locally because the dependency isn't pinned in requirements_dev.txt:

image

we can solve it by pinning the hidden dep in the local req.txt file to an older version, but i think it would be best to just bump the super linter version to 5.7.2 and black to 23.11.0

So, bump to the new version plz.

@hsluoyz
Copy link
Member

hsluoyz commented Dec 27, 2023

@thearchitector fix CI errors first:

image

@thearchitector
Copy link
Contributor Author

@hsluoyz huh, gh actions cannot find the super linter version. weird, it definitely exists...

do I need to address the coveralls stuff too? Not sure what the error is there (the test percentage regression?)

.github/workflows/build.yml Outdated Show resolved Hide resolved
@leeqvip leeqvip merged commit d557189 into casbin:master Dec 28, 2023
14 of 24 checks passed
github-actions bot pushed a commit that referenced this pull request Dec 28, 2023
# [1.34.0](v1.33.0...v1.34.0) (2023-12-28)

### Features

* add interface stubs for async adapters ([#335](#335)) ([d557189](d557189))
Copy link

🎉 This PR is included in version 1.34.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

asyncio Adapter support
5 participants