-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fixed possible constraint violation with an existed organization. #6674
Conversation
Usecase is following: If we have already an organization with name, let's say "org" and the new user also has name "org" then we will have constraint violation. In this case, we will try to create the user with name "org"+random string.
AccountImpl account = accounts[0]; | ||
account.setName(accounts[1].getName()); | ||
|
||
accountDao.create(account); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You try to create user with the existing name and id. To ensure that conflict is thrown because of names conflct it is needed to rewrite this test as
AccountImpl account = new AccountImpl(NameGenerator.generate("account", 5), accounts[0].getName(), "test");
accountDao.create(account);
managerProvider.get().persist(account); | ||
final EntityManager manager = managerProvider.get(); | ||
manager.persist(account); | ||
manager.flush(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add flushing to doRemove
method too. See UserDao.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
cheUser.setName(generate(cheUser.getName(), 4)); | ||
return userManager.create(cheUser, false); | ||
} catch (ServerException | ConflictException e1) { | ||
throw new ServletException("Unable to create new user", e1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With such try catches this method looks unreadable. Please consider to rewrite it somehow, e.g.
private synchronized User getOrCreateUser(String id, String email, String username)
throws ServletException {
try {
Optional<User> user = doGetUser(id);
if (user.isPresent()) {
return user.get();
}
final UserImpl cheUser = new UserImpl(id, email, username, generate("", 12), emptyList());
try {
return userManager.create(cheUser, false);
} catch (ConflictException ex) {
cheUser.setName(generate(cheUser.getName(), 4));
return userManager.create(cheUser, false);
}
} catch (ServerException | ConflictException e) {
throw new ServletException("Unable to create new user", e);
}
}
private Optional<User> doGetUser(String id) throws ServerException {
try {
return Optional.of(userManager.getById(id));
} catch (NotFoundException e) {
return Optional.empty();
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1:1 by readability imo...:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW throw new ServletException("Unable to create new user", e);
this line is one time in my variant 😃
managerProvider.get().persist(account); | ||
final EntityManager manager = managerProvider.get(); | ||
manager.persist(account); | ||
manager.flush(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pls fix AccountDao.doRemove() in the same way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/4095/ |
ci-test build report: |
What does this PR do?
Usecase is following:
If we have already an organization with name, let's say "org" and the new user also has name "org" then we will have constraint violation.
In this case, we will try to create the user with name "org"+random string.
What issues does this PR fix or reference?
#6650
n/a
Release Notes
n/a
Docs PR
n/a