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

utils: improve group create or update #343

Merged
merged 2 commits into from
Dec 3, 2024

Conversation

jrcastro2
Copy link
Contributor

No description provided.

else:
roles_ids.add(existing_role.id)

db.session.flush() # Ensure changes are written before committing
Copy link
Member

Choose a reason for hiding this comment

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

Isn't that happening eitherway on exit of db.session.begin_nested ?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes:

When the context manager yielded by Session.begin_nested() completes, it “commits” the savepoint, which includes the usual behavior of flushing all pending state. When an error is raised, the savepoint is rolled back and the state of the Session local to the objects that were changed is expired.

I am wondering if we should delegate to the caller to call this method inside the begin_nested, WDYT?

Copy link
Contributor Author

@jrcastro2 jrcastro2 Dec 3, 2024

Choose a reason for hiding this comment

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

I thought about this, however kept it here as it was the same behaviour as before. I will change it to be the caller 👍

EDIT: Actually moving it to the caller would not be possible because the for loop is inside this func and now we are creating SAVEPOINTS with the begin_nested for each value, plus the datastore.commit() is at the end of this func, it would imply calling this after every time we call this func which might be hard to remember

@jrcastro2 jrcastro2 merged commit 8b2e6e1 into inveniosoftware:master Dec 3, 2024
2 checks passed
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.

3 participants