[#11346] [#11348] Fix NPE/array OOB in logs and OOM in session published email cron job (#11349)

* Add branches to cater for exceptions with no stack trace and logs with no source

* Add time limit when querying sessions to be sent published email

Author: wkurniawan07

src/main/appengine/index.yaml

@@ -35,3 +35,11 @@ indexes:
     name: isPublishedEmailEnabled
   - direction: asc
     name: sentPublishedEmail
+- kind: FeedbackSession
+  properties:
+  - direction: asc
+    name: isPublishedEmailEnabled
+  - direction: asc
+    name: sentPublishedEmail
+  - direction: asc
+    name: resultsVisibleFromTime

src/main/java/teammates/common/util/Logger.java

@@ -141,17 +141,21 @@ public void severe(String message, Throwable t) {
     }
 
     private String getLogMessageWithStackTrace(String message, Throwable t, LogSeverity severity) {
-        String logMessage;
         if (Config.isDevServer()) {
             StringWriter sw = new StringWriter();
             try (PrintWriter pw = new PrintWriter(sw)) {
                 t.printStackTrace(pw);
             }
 
-            logMessage = formatLogMessageForHumanDisplay(message) + " stack_trace: "
+            return formatLogMessageForHumanDisplay(message) + " stack_trace: "
                     + System.lineSeparator() + sw.toString();
-        } else {
-            StackTraceElement tSource = t.getStackTrace()[0];
+        }
+
+        StackTraceElement[] stackTraces = t.getStackTrace();
+        Map<String, Object> payload = getBaseCloudLoggingPayload(message, severity);
+
+        if (stackTraces.length > 0) {
+            StackTraceElement tSource = stackTraces[0];
             SourceLocation tSourceLocation = new SourceLocation(
                     tSource.getClassName(), (long) tSource.getLineNumber(), tSource.getMethodName());
 
@@ -168,8 +172,6 @@ private String getLogMessageWithStackTrace(String message, Throwable t, LogSever
                 currentT = currentT.getCause();
             }
 
-            Map<String, Object> payload = getBaseCloudLoggingPayload(message, severity);
-
             // Replace the source location with the Throwable's source location instead
             SourceLocation loggerSourceLocation = (SourceLocation) payload.get("logging.googleapis.com/sourceLocation");
             payload.put("logging.googleapis.com/sourceLocation", tSourceLocation);
@@ -184,11 +186,9 @@ private String getLogMessageWithStackTrace(String message, Throwable t, LogSever
             Map<String, Object> detailsSpecificPayload =
                     JsonUtils.fromJson(JsonUtils.toCompactJson(details), new TypeToken<Map<String, Object>>(){}.getType());
             payload.putAll(detailsSpecificPayload);
-
-            logMessage = JsonUtils.toCompactJson(payload);
         }
 
-        return logMessage;
+        return JsonUtils.toCompactJson(payload);
     }
 
     private List<String> getStackTraceToDisplay(Throwable t) {

src/main/java/teammates/logic/core/GoogleCloudLoggingService.java

@@ -155,9 +155,17 @@ public QueryLogsResults queryLogs(QueryLogsParams queryLogsParams) throws LogSer
             Payload<?> payload = entry.getPayload();
             long timestamp = entry.getTimestamp();
 
+            String file = "";
+            Long line = 0L;
+            String function = "";
+            if (sourceLocation != null) {
+                file = sourceLocation.getFile();
+                line = sourceLocation.getLine();
+                function = sourceLocation.getFunction();
+            }
+
             GeneralLogEntry logEntry = new GeneralLogEntry(convertSeverity(severity), trace, insertId,
-                    resourceIdentifier, new SourceLocation(sourceLocation.getFile(),
-                            sourceLocation.getLine(), sourceLocation.getFunction()), timestamp);
+                    resourceIdentifier, new SourceLocation(file, line, function), timestamp);
             if (payload.getType() == Payload.Type.JSON) {
                 Map<String, Object> jsonPayloadMap = ((Payload.JsonPayload) payload).getDataAsMap();
                 logEntry.setDetails(JsonUtils.fromJson(JsonUtils.toCompactJson(jsonPayloadMap), LogDetails.class));

src/main/java/teammates/storage/api/FeedbackSessionsDb.java

@@ -376,6 +376,7 @@ private List<FeedbackSession> getFeedbackSessionEntitiesPossiblyNeedingClosedEma
 
     private List<FeedbackSession> getFeedbackSessionEntitiesPossiblyNeedingPublishedEmail() {
         return load()
+                .filter("resultsVisibleFromTime >", TimeHelper.getInstantDaysOffsetFromNow(-2))
                 .filter("sentPublishedEmail =", false)
                 .filter("isPublishedEmailEnabled =", true)
                 .list();

src/main/java/teammates/storage/entity/FeedbackSession.java

@@ -52,7 +52,6 @@ public class FeedbackSession extends BaseEntity {
     @Translate(InstantTranslatorFactory.class)
     private Instant sessionVisibleFromTime;
 
-    @Unindex
     @Translate(InstantTranslatorFactory.class)
     private Instant resultsVisibleFromTime;
 

src/test/java/teammates/storage/api/FeedbackSessionsDbTest.java

@@ -385,7 +385,7 @@ public void testGetFeedbackSessionsPossiblyNeedingPublishedEmail() throws Except
 
         List<FeedbackSessionAttributes> fsaList = fsDb.getFeedbackSessionsPossiblyNeedingPublishedEmail();
 
-        assertEquals(11, fsaList.size());
+        assertEquals(8, fsaList.size());
         for (FeedbackSessionAttributes fsa : fsaList) {
             assertFalse(fsa.isSentPublishedEmail());
             assertTrue(fsa.isPublishedEmailEnabled());
@@ -399,7 +399,7 @@ public void testGetFeedbackSessionsPossiblyNeedingPublishedEmail() throws Except
         fsDb.softDeleteFeedbackSession(feedbackSession.getFeedbackSessionName(), feedbackSession.getCourseId());
 
         fsaList = fsDb.getFeedbackSessionsPossiblyNeedingPublishedEmail();
-        assertEquals(10, fsaList.size());
+        assertEquals(7, fsaList.size());
     }
 
     @Test

src/test/java/teammates/storage/search/StudentSearchTest.java

@@ -1,7 +1,7 @@
 package teammates.storage.search;
 
-import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.List;
 
 import org.testng.annotations.Test;
@@ -141,8 +141,10 @@ public void testSearchStudents_noSearchService_shouldThrowException() {
             return;
         }
 
+        List<InstructorAttributes> ins1OfCourse1 = Collections.singletonList(
+                dataBundle.instructors.get("instructor1OfCourse1"));
         assertThrows(SearchServiceException.class,
-                () -> studentsDb.search("anything", new ArrayList<>()));
+                () -> studentsDb.search("anything", ins1OfCourse1));
         assertThrows(SearchServiceException.class,
                 () -> studentsDb.searchStudentsInWholeSystem("anything"));
     }

src/test/java/teammates/ui/webapi/FeedbackSessionPublishedRemindersActionTest.java

@@ -32,12 +32,12 @@ protected void testAccessControl() {
     @Test
     public void testExecute() throws Exception {
 
-        ______TS("default state of typical data bundle: 1 session published with email unsent");
+        ______TS("default state of typical data bundle: no sessions needing published email");
 
         FeedbackSessionPublishedRemindersAction action = getAction();
         action.execute();
 
-        verifySpecifiedTasksAdded(Const.TaskQueue.FEEDBACK_SESSION_PUBLISHED_EMAIL_QUEUE_NAME, 2);
+        verifyNoTasksAdded();
 
         ______TS("1 session published by moving automated publish time, "
                  + "1 session published similarly with disabled published reminder, "
@@ -84,7 +84,7 @@ public void testExecute() throws Exception {
         action = getAction();
         action.execute();
 
-        verifySpecifiedTasksAdded(Const.TaskQueue.FEEDBACK_SESSION_PUBLISHED_EMAIL_QUEUE_NAME, 4);
+        verifySpecifiedTasksAdded(Const.TaskQueue.FEEDBACK_SESSION_PUBLISHED_EMAIL_QUEUE_NAME, 2);
 
         ______TS("1 session unpublished manually");
 
@@ -93,7 +93,7 @@ public void testExecute() throws Exception {
         action = getAction();
         action.execute();
 
-        verifySpecifiedTasksAdded(Const.TaskQueue.FEEDBACK_SESSION_PUBLISHED_EMAIL_QUEUE_NAME, 3);
+        verifySpecifiedTasksAdded(Const.TaskQueue.FEEDBACK_SESSION_PUBLISHED_EMAIL_QUEUE_NAME, 1);
 
         ______TS("1 session published with emails sent");
 
@@ -107,7 +107,7 @@ public void testExecute() throws Exception {
         action = getAction();
         action.execute();
 
-        verifySpecifiedTasksAdded(Const.TaskQueue.FEEDBACK_SESSION_PUBLISHED_EMAIL_QUEUE_NAME, 2);
+        verifyNoTasksAdded();
 
     }
 

src/test/resources/emails/sessionReminderEmailForInstructorNotJoinedYet.html

@@ -54,7 +54,7 @@
   <ul>
     <li>
       <strong>To submit, edit or view your feedback for the above session, please go to this Web address: </strong>
-      <a href="http://localhost:4200/web/instructor/sessions/submission?courseid=idOfTypicalCourse1&fsname=First%20feedback%20session">http://localhost:4200/web/instructor/sessions/submission?courseid=idOfTypicalCourse1&fsname=First%20feedback%20session</a>
+      <a href="${app.url}/web/instructor/sessions/submission?courseid=idOfTypicalCourse1&fsname=First%20feedback%20session">${app.url}/web/instructor/sessions/submission?courseid=idOfTypicalCourse1&fsname=First%20feedback%20session</a>
     </li>
     <li>
       The above link is unique to you. Please do not share it with others.
@@ -75,7 +75,7 @@
 <p>=====</p>
 <p>
     <strong>To "join" the course, please go to this Web address: </strong>
-    <a href="http://localhost:4200/web/join?key=${regkey.enc}&entitytype=instructor">http://localhost:4200/web/join?key=${regkey.enc}&entitytype=instructor</a>
+    <a href="${app.url}/web/join?key=${regkey.enc}&entitytype=instructor">${app.url}/web/join?key=${regkey.enc}&entitytype=instructor</a>
 <ul>
     <li>
         If prompted to log in, use your Google account to log in.
@@ -97,7 +97,7 @@
     </li>
     <li>
       <strong>Technical help regarding TEAMMATES</strong> (e.g. submission link is not working):
-      Email TEAMMATES support team at app_admin@gmail.com.
+      Email TEAMMATES support team at ${support.email}.
     </li>
   </ul>
 </p>