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

Fix #6124 Permissions lost when doing something transactional in the … #6127

Merged
merged 2 commits into from
May 2, 2017

Conversation

fdlk
Copy link
Contributor

@fdlk fdlk commented Apr 26, 2017

…afterCommit handler of a Job

The idea is to turn the order around, do the permission switch outside the transaction instead of the other way around.

Before:
Before

After:
After

Checklist

  • Functionality works & meets specifications
  • Code reviewed
  • Code unit/integration/system tested
  • User documentation updated
  • (If you have changed REST API interface) view-swagger.ftl updated
  • Test plan template updated
  • Clean commits

}
else
{
return doCallInTransaction();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we actually doing the call in a transaction here? I'm a bit confused

Copy link
Contributor Author

@fdlk fdlk Apr 26, 2017

Choose a reason for hiding this comment

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

The Jobs run running on a separate executor thread.
If we do nothing, it will run without permissions and not in a transaction.
It is no bean so annotating the call() method with @transactional won't do anything.
So we need to manually set the permissions and manually call the TransactionTemplate.

This PR turns the order inside out.
Call the transaction template within the user switch.
Used to be switch the user within the transaction.
The bug was that the afterCommit then runs after the permissions get restored/removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or do you mean the case where no transaction template is specified? There we don't run in a transaction.

@fdlk fdlk assigned fdlk and unassigned tommydeboer and fdlk May 1, 2017
@fdlk fdlk assigned tommydeboer and unassigned fdlk May 1, 2017
@tommydeboer tommydeboer merged commit 2f8dfb1 into molgenis:master May 2, 2017
@fdlk fdlk deleted the fix/6124 branch May 5, 2017 09:09
@dennishendriksen
Copy link
Contributor

@fdlk How does this relate to #6127? Is #6427 the proper solution and can we undo the previous solution?

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

Successfully merging this pull request may close these issues.

3 participants