Skip to content

Commit

Permalink
[#12048] Fix account request indexing (#12967)
Browse files Browse the repository at this point in the history
* Add isTransactionNeeded method to Action

* Remove delay from taskqueuer

* Change CreateAccountRequest to handle own transactions
  • Loading branch information
cedricongjh committed Mar 29, 2024
1 parent c4fe140 commit 356c318
Show file tree
Hide file tree
Showing 8 changed files with 97 additions and 13 deletions.
@@ -1,10 +1,12 @@
package teammates.it.ui.webapi;

import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;

import teammates.common.util.Const;
import teammates.common.util.EmailType;
import teammates.common.util.EmailWrapper;
import teammates.common.util.HibernateUtil;
import teammates.storage.sqlentity.AccountRequest;
import teammates.ui.output.JoinLinkData;
import teammates.ui.request.AccountCreateRequest;
Expand All @@ -26,6 +28,13 @@ String getRequestMethod() {
return POST;
}

@BeforeMethod
@Override
protected void setUp() {
// CreateAccountRequestAction handles its own transactions;
// There is thus no need to setup a transaction.
}

@Override
@Test
protected void testExecute() throws Exception {
Expand All @@ -37,7 +46,9 @@ protected void testExecute() throws Exception {
CreateAccountRequestAction action = getAction(request);
JsonResult result = getJsonResult(action);
JoinLinkData output = (JoinLinkData) result.getOutput();
HibernateUtil.beginTransaction();
AccountRequest accountRequest = logic.getAccountRequest("ring-bearer@fellowship.net", "The Fellowship of the Ring");
HibernateUtil.commitTransaction();
assertEquals("ring-bearer@fellowship.net", accountRequest.getEmail());
assertEquals("Frodo Baggins", accountRequest.getName());
assertEquals("The Fellowship of the Ring", accountRequest.getInstitute());
Expand Down
6 changes: 2 additions & 4 deletions src/main/java/teammates/logic/api/TaskQueuer.java
Expand Up @@ -228,10 +228,8 @@ public void scheduleAccountRequestForSearchIndexing(String email, String institu
paramMap.put(ParamsNames.INSTRUCTOR_EMAIL, email);
paramMap.put(ParamsNames.INSTRUCTOR_INSTITUTION, institute);

// TODO: change the action CreateAccountRequestAction to call scheduleAccountRequestForSearchIndexing
// after AccountRequest is inserted in the DB
addDeferredTask(TaskQueue.SEARCH_INDEXING_QUEUE_NAME, TaskQueue.ACCOUNT_REQUEST_SEARCH_INDEXING_WORKER_URL,
paramMap, null, 60);
addTask(TaskQueue.SEARCH_INDEXING_QUEUE_NAME, TaskQueue.ACCOUNT_REQUEST_SEARCH_INDEXING_WORKER_URL,
paramMap, null);
}

/**
Expand Down
11 changes: 11 additions & 0 deletions src/main/java/teammates/logic/api/UserProvision.java
Expand Up @@ -3,6 +3,7 @@
import teammates.common.datatransfer.UserInfo;
import teammates.common.datatransfer.UserInfoCookie;
import teammates.common.util.Config;
import teammates.common.util.HibernateUtil;
import teammates.logic.core.InstructorsLogic;
import teammates.logic.core.StudentsLogic;
import teammates.sqllogic.core.UsersLogic;
Expand Down Expand Up @@ -48,6 +49,16 @@ public UserInfo getCurrentUser(UserInfoCookie uic) {
return user;
}

/**
* Gets the information of the current logged in user, with an SQL transaction.
*/
public UserInfo getCurrentUserWithTransaction(UserInfoCookie uic) {
HibernateUtil.beginTransaction();
UserInfo userInfo = getCurrentUser(uic);
HibernateUtil.commitTransaction();
return userInfo;
}

// TODO: method visibility to package-private after migration
/**
* Gets the current logged in user.
Expand Down
13 changes: 13 additions & 0 deletions src/main/java/teammates/sqllogic/api/Logic.java
Expand Up @@ -94,6 +94,19 @@ public AccountRequest createAccountRequest(String name, String email, String ins
return accountRequestLogic.createAccountRequest(name, email, institute);
}

/**
* Creates a or gets an account request.
*
* @return newly created account request.
* @throws InvalidParametersException if the account request details are invalid.
* @throws EntityAlreadyExistsException if the account request already exists.
*/
public AccountRequest createAccountRequestWithTransaction(String name, String email, String institute)
throws InvalidParametersException {

return accountRequestLogic.createOrGetAccountRequestWithTransaction(name, email, institute);
}

/**
* Gets the account request with the given email and institute.
*
Expand Down
23 changes: 23 additions & 0 deletions src/main/java/teammates/sqllogic/core/AccountRequestsLogic.java
Expand Up @@ -6,6 +6,7 @@
import teammates.common.exception.EntityDoesNotExistException;
import teammates.common.exception.InvalidParametersException;
import teammates.common.exception.SearchServiceException;
import teammates.common.util.HibernateUtil;
import teammates.storage.sqlapi.AccountRequestsDb;
import teammates.storage.sqlentity.AccountRequest;
import teammates.storage.sqlsearch.AccountRequestSearchManager;
Expand Down Expand Up @@ -126,4 +127,26 @@ public List<AccountRequest> searchAccountRequestsInWholeSystem(String queryStrin
throws SearchServiceException {
return accountRequestDb.searchAccountRequestsInWholeSystem(queryString);
}

/**
* Creates an or gets an account request.
*/
public AccountRequest createOrGetAccountRequestWithTransaction(String name, String email, String institute)
throws InvalidParametersException {
AccountRequest toCreate = new AccountRequest(email, name, institute);
HibernateUtil.beginTransaction();
AccountRequest accountRequest;
try {
accountRequest = accountRequestDb.createAccountRequest(toCreate);
HibernateUtil.commitTransaction();
} catch (InvalidParametersException ipe) {
HibernateUtil.rollbackTransaction();
throw new InvalidParametersException(ipe);
} catch (EntityAlreadyExistsException eaee) {
// Use existing account request
accountRequest = getAccountRequest(email, institute);
HibernateUtil.commitTransaction();
}
return accountRequest;
}
}
17 changes: 16 additions & 1 deletion src/main/java/teammates/ui/servlets/WebApiServlet.java
Expand Up @@ -60,7 +60,14 @@ private void invokeServlet(HttpServletRequest req, HttpServletResponse resp) thr

try {
action = ActionFactory.getAction(req, req.getMethod());
ActionResult result = executeWithTransaction(action, req);
ActionResult result;

if (action.isTransactionNeeded()) {
result = executeWithTransaction(action, req);
} else {
result = executeWithoutTransaction(action, req);
}

statusCode = result.getStatusCode();
result.send(resp);
} catch (ActionMappingException e) {
Expand Down Expand Up @@ -127,6 +134,14 @@ private ActionResult executeWithTransaction(Action action, HttpServletRequest re
}
}

private ActionResult executeWithoutTransaction(Action action, HttpServletRequest req)
throws InvalidOperationException, InvalidHttpRequestBodyException, UnauthorizedAccessException {
action.init(req);
action.checkAccessControl();

return action.execute();
}

private void throwErrorBasedOnRequester(HttpServletRequest req, HttpServletResponse resp, Exception e, int statusCode)
throws IOException {
// The header X-AppEngine-QueueName cannot be spoofed as GAE will strip any user-sent X-AppEngine-QueueName headers.
Expand Down
14 changes: 13 additions & 1 deletion src/main/java/teammates/ui/webapi/Action.java
Expand Up @@ -216,7 +216,11 @@ private void initAuthInfo() {
} else {
String cookie = HttpRequestHelper.getCookieValueFromRequest(req, Const.SecurityConfig.AUTH_COOKIE_NAME);
UserInfoCookie uic = UserInfoCookie.fromCookie(cookie);
userInfo = userProvision.getCurrentUser(uic);
if (isTransactionNeeded()) {
userInfo = userProvision.getCurrentUser(uic);
} else {
userInfo = userProvision.getCurrentUserWithTransaction(uic);
}
}

authType = userInfo == null ? AuthType.PUBLIC : AuthType.LOGGED_IN;
Expand Down Expand Up @@ -480,6 +484,14 @@ InstructorPermissionSet constructInstructorPrivileges(Instructor instructor, Str
return privilege;
}

/**
* Checks if the action requires a SQL transaction when executed.
* If false, the action will have to handle its own SQL transactions.
*/
public boolean isTransactionNeeded() {
return true;
}

/**
* Gets the minimum access control level required to access the resource.
*/
Expand Down
@@ -1,6 +1,5 @@
package teammates.ui.webapi;

import teammates.common.exception.EntityAlreadyExistsException;
import teammates.common.exception.InvalidParametersException;
import teammates.common.util.EmailWrapper;
import teammates.storage.sqlentity.AccountRequest;
Expand All @@ -13,6 +12,11 @@
*/
public class CreateAccountRequestAction extends AdminOnlyAction {

@Override
public boolean isTransactionNeeded() {
return false;
}

@Override
public JsonResult execute()
throws InvalidHttpRequestBodyException, InvalidOperationException {
Expand All @@ -25,16 +29,13 @@ public JsonResult execute()
AccountRequest accountRequest;

try {
accountRequest = sqlLogic.createAccountRequest(instructorName, instructorEmail, instructorInstitution);
taskQueuer.scheduleAccountRequestForSearchIndexing(instructorEmail, instructorInstitution);
accountRequest =
sqlLogic.createAccountRequestWithTransaction(instructorName, instructorEmail, instructorInstitution);
} catch (InvalidParametersException ipe) {
throw new InvalidHttpRequestBodyException(ipe);
} catch (EntityAlreadyExistsException eaee) {
// Use existing account request
accountRequest = sqlLogic.getAccountRequest(instructorEmail, instructorInstitution);
}

assert accountRequest != null;
taskQueuer.scheduleAccountRequestForSearchIndexing(instructorEmail, instructorInstitution);

if (accountRequest.getRegisteredAt() != null) {
throw new InvalidOperationException("Cannot create account request as instructor has already registered.");
Expand Down

0 comments on commit 356c318

Please sign in to comment.