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

[feature] better session management for newSyncedAdapter() #66

Open
CheaterScript opened this issue Mar 22, 2023 · 4 comments
Open

[feature] better session management for newSyncedAdapter() #66

CheaterScript opened this issue Mar 22, 2023 · 4 comments
Assignees
Labels
enhancement New feature or request

Comments

@CheaterScript
Copy link

Hi!
I am a MongoDB novice and would like to ask two questions. Why doesn’t the newSyncedAdapter API return the current session or call the current session.endSession?
Because I want to perform some operations before submitting the transaction and found that I need to manage the session myself, which doesn’t seem reasonable. Also, I see that people online will call session.endSession after a transaction is completed, but this library only calls session.endSession in the close method. Other APIs do not call session.endSession after the session is committed or discarded. Is there any problem with this? Hope to get your help.

@casbin-bot
Copy link

@casbin-bot casbin-bot added the question Further information is requested label Mar 22, 2023
@hsluoyz hsluoyz added enhancement New feature or request and removed question Further information is requested labels Mar 22, 2023
@hsluoyz hsluoyz changed the title Ask two questions about newSyncedAdapter. [feature] better session management for newSyncedAdapter() Mar 27, 2023
@hsluoyz
Copy link
Member

hsluoyz commented Mar 28, 2023

@CheaterScript hi, can you make a PR to return the current session in the function?

@kohtala
Copy link
Contributor

kohtala commented Sep 12, 2023

I share the confusion on the transaction support.

I understood @CheaterScript, and I, have other database changes to put in the same transaction with casbin policy changes. For example, if the policy refers to a user, and I'd like to have the user created/deleted/modified in the same transaction such that the user and policy would both be changed or neither would be changed.

As @CheaterScript points out, keeping the session in an instance attribute is confusing. I do not find what in the design would enforce the rule that at any given time, you can have at most one open transaction for a session.

Looking at the interfaces, I'd expect I should get a copy of the enforcer for policy changes with a copy of the adapter that performs all database operations in the transaction session. The session may have been created earlier for example to create the user before the policy. Also, the session would be committed or aborted outside and independent of the adapter. Once transaction is committed, the changes would be seen in the enforcer used by other concurrent requests.

The gorm-adapter seems to implement the Transaction method almost like this with copies. It wants to own the transaction, but I do not see why. If two libraries were to want to own the transaction, you could not use them in the same transaction.

Transactions have also the complexity of when the enforcer will take the new rules into use. You'd want the enforcer to be part of the transaction.

@hsluoyz
Copy link
Member

hsluoyz commented Sep 13, 2023

@kohtala hi, can you make a PR to fix it?

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

No branches or pull requests

4 participants