From 356c318e3f9ba907737654d568624950ba24b22f Mon Sep 17 00:00:00 2001 From: Cedric Ong <67156011+cedricongjh@users.noreply.github.com> Date: Fri, 29 Mar 2024 18:00:56 +0800 Subject: [PATCH] [#12048] Fix account request indexing (#12967) * Add isTransactionNeeded method to Action * Remove delay from taskqueuer * Change CreateAccountRequest to handle own transactions --- .../webapi/CreateAccountRequestActionIT.java | 11 +++++++++ .../java/teammates/logic/api/TaskQueuer.java | 6 ++--- .../teammates/logic/api/UserProvision.java | 11 +++++++++ .../java/teammates/sqllogic/api/Logic.java | 13 +++++++++++ .../sqllogic/core/AccountRequestsLogic.java | 23 +++++++++++++++++++ .../teammates/ui/servlets/WebApiServlet.java | 17 +++++++++++++- src/main/java/teammates/ui/webapi/Action.java | 14 ++++++++++- .../ui/webapi/CreateAccountRequestAction.java | 15 ++++++------ 8 files changed, 97 insertions(+), 13 deletions(-) diff --git a/src/it/java/teammates/it/ui/webapi/CreateAccountRequestActionIT.java b/src/it/java/teammates/it/ui/webapi/CreateAccountRequestActionIT.java index 70aa4ecc521..dcb1c279ae3 100644 --- a/src/it/java/teammates/it/ui/webapi/CreateAccountRequestActionIT.java +++ b/src/it/java/teammates/it/ui/webapi/CreateAccountRequestActionIT.java @@ -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; @@ -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 { @@ -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()); diff --git a/src/main/java/teammates/logic/api/TaskQueuer.java b/src/main/java/teammates/logic/api/TaskQueuer.java index 32a5ba9e13f..a3db7d7d359 100644 --- a/src/main/java/teammates/logic/api/TaskQueuer.java +++ b/src/main/java/teammates/logic/api/TaskQueuer.java @@ -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); } /** diff --git a/src/main/java/teammates/logic/api/UserProvision.java b/src/main/java/teammates/logic/api/UserProvision.java index 2401b871e68..140cfe0c34d 100644 --- a/src/main/java/teammates/logic/api/UserProvision.java +++ b/src/main/java/teammates/logic/api/UserProvision.java @@ -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; @@ -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. diff --git a/src/main/java/teammates/sqllogic/api/Logic.java b/src/main/java/teammates/sqllogic/api/Logic.java index a10fe1bae21..f83ff914ec7 100644 --- a/src/main/java/teammates/sqllogic/api/Logic.java +++ b/src/main/java/teammates/sqllogic/api/Logic.java @@ -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. * diff --git a/src/main/java/teammates/sqllogic/core/AccountRequestsLogic.java b/src/main/java/teammates/sqllogic/core/AccountRequestsLogic.java index fd7742b3a73..f0797adf034 100644 --- a/src/main/java/teammates/sqllogic/core/AccountRequestsLogic.java +++ b/src/main/java/teammates/sqllogic/core/AccountRequestsLogic.java @@ -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; @@ -126,4 +127,26 @@ public List 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; + } } diff --git a/src/main/java/teammates/ui/servlets/WebApiServlet.java b/src/main/java/teammates/ui/servlets/WebApiServlet.java index 60b4f5fa3dc..f4ee00059d8 100644 --- a/src/main/java/teammates/ui/servlets/WebApiServlet.java +++ b/src/main/java/teammates/ui/servlets/WebApiServlet.java @@ -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) { @@ -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. diff --git a/src/main/java/teammates/ui/webapi/Action.java b/src/main/java/teammates/ui/webapi/Action.java index 87e51ed74dc..ab32d4ed21d 100644 --- a/src/main/java/teammates/ui/webapi/Action.java +++ b/src/main/java/teammates/ui/webapi/Action.java @@ -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; @@ -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. */ diff --git a/src/main/java/teammates/ui/webapi/CreateAccountRequestAction.java b/src/main/java/teammates/ui/webapi/CreateAccountRequestAction.java index 27de0e8437b..f0f214a04f9 100644 --- a/src/main/java/teammates/ui/webapi/CreateAccountRequestAction.java +++ b/src/main/java/teammates/ui/webapi/CreateAccountRequestAction.java @@ -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; @@ -13,6 +12,11 @@ */ public class CreateAccountRequestAction extends AdminOnlyAction { + @Override + public boolean isTransactionNeeded() { + return false; + } + @Override public JsonResult execute() throws InvalidHttpRequestBodyException, InvalidOperationException { @@ -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.");