Instructor: view results: fix IndexOutOfBoundsException

[damithc]

I'm getting this error when I try to view results of an ongoing session.

teammates.common.util.Logger severe: Unexpected exception caught by ControllerServlet :  (Logger.java:52)
java.lang.IndexOutOfBoundsException: Index: 0, Size: 0
	at java.util.ArrayList.rangeCheck(ArrayList.java:654)
	at java.util.ArrayList.get(ArrayList.java:430)
	at teammates.common.datatransfer.questions.FeedbackMsqQuestionDetails.getQuestionAdditionalInfoHtml(FeedbackMsqQuestionDetails.java:460)
	at teammates.ui.pagedata.InstructorFeedbackResultsPageData.buildQuestionTableAndResponseRows(InstructorFeedbackResultsPageData.java:924)
	at teammates.ui.pagedata.InstructorFeedbackResultsPageData.buildQuestionTableWithoutResponseRows(InstructorFeedbackResultsPageData.java:865)
	at teammates.ui.pagedata.InstructorFeedbackResultsPageData.lambda$initForViewByQuestion$0(InstructorFeedbackResultsPageData.java:120)
	at java.util.LinkedHashMap.forEach(LinkedHashMap.java:684)
	at teammates.ui.pagedata.InstructorFeedbackResultsPageData.initForViewByQuestion(InstructorFeedbackResultsPageData.java:117)
	at teammates.ui.controller.InstructorFeedbackResultsPageAction.execute(InstructorFeedbackResultsPageAction.java:137)
	at teammates.ui.controller.Action.executeAndPostProcess(Action.java:474)
	at teammates.ui.controller.ControllerServlet.doPost(ControllerServlet.java:76)
	at teammates.ui.controller.ControllerServlet.doGet(ControllerServlet.java:51)
	at javax.servlet.http.HttpServlet.service(HttpServlet.java:687)
	at javax.servlet.http.HttpServlet.service(HttpServlet.java:790)
	at org.eclipse.jetty.servlet.ServletHolder.handle(ServletHolder.java:848)
	at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1772)
	at com.googlecode.objectify.ObjectifyFilter.doFilter(ObjectifyFilter.java:48)
	at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1759)
	at com.google.apphosting.utils.servlet.ParseBlobUploadFilter.doFilter(ParseBlobUploadFilter.java:125)
	at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1759)
	at com.google.apphosting.runtime.jetty9.SaveSessionFilter.doFilter(SaveSessionFilter.java:37)
	at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1759)
	at com.google.apphosting.utils.servlet.JdbcMySqlConnectionCleanupFilter.doFilter(JdbcMySqlConnectionCleanupFilter.java:60)
	at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1759)
	at com.google.apphosting.utils.servlet.TransactionCleanupFilter.doFilter(TransactionCleanupFilter.java:48)
	at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1759)
	at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:582)
	at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:143)
	at org.eclipse.jetty.security.SecurityHandler.handle(SecurityHandler.java:524)
	at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:226)
	at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:1180)
	at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:512)
	at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:185)
	at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:1112)
	at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:141)
	at com.google.apphosting.runtime.jetty9.AppVersionHandlerMap.handle(AppVersionHandlerMap.java:297)
	at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:134)
	at org.eclipse.jetty.server.Server.handle(Server.java:534)
	at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:320)
	at com.google.apphosting.runtime.jetty9.RpcConnection.handle(RpcConnection.java:202)
	at com.google.apphosting.runtime.jetty9.RpcConnector.serviceRequest(RpcConnector.java:81)
	at com.google.apphosting.runtime.jetty9.JettyServletEngineAdapter.serviceRequest(JettyServletEngineAdapter.java:108)
	at com.google.apphosting.runtime.JavaRuntime$RequestRunnable.dispatchServletRequest(JavaRuntime.java:680)
	at com.google.apphosting.runtime.JavaRuntime$RequestRunnable.dispatchRequest(JavaRuntime.java:642)
	at com.google.apphosting.runtime.JavaRuntime$RequestRunnable.run(JavaRuntime.java:612)
	at com.google.apphosting.runtime.JavaRuntime$NullSandboxRequestRunnable.run(JavaRuntime.java:806)
	at com.google.apphosting.runtime.ThreadGroupPool$PoolEntry.run(ThreadGroupPool.java:274)
	at java.lang.Thread.run(Thread.java:745)

[damithc]

I have several multiple-select, multiple-answers questions. In one, the options are generated based on the students in the class. In others, options are fixed.

[damithc]

Need to fix quite soon as I need to access results of that session.

[whipermr5]

Seems like there's a discrepancy between numOfMsqChoices and msqChoices. Both fields are deserialised from the question metadata stored in the datastore and can possibly be inconsistent. Would it be possible to view the FeedbackQuestion entity of the corresponding question in the Google Cloud console? I'm interested to see what is stored in the questionText field that contains the metadata. It may be inconsistent, e.g. this existing test data is malformed (it's an MSQ question with msqChoices but numOfMcqChoices instead of numOfMsqChoices in the metadata):

teammates/src/test/resources/data/FeedbackSessionQuestionTypeTest.json

Line 711 in 85faa78

"value": "{\"msqChoices\":[\"It's good\",\"It's perfect\"],\"numOfMcqChoices\":2,\"questionText\":\"What do you like best about the class' product?\",\"questionType\":\"MSQ\",\"otherEnabled\":false}"

This issue might even be related to the issue of response deletion (both may be due to inconsistencies in the view of questions stored in the datastore).

Meanwhile, I've updated log-response-deletion with a hotfix. It has passed the Travis build.

[damithc]

The hot fix seem to have fixed the issue. I can see the results now. In any case, we might need a test to cover this case.

[bqnguyen94]

In this method, is there any reason why numOfMsqChoices is explicitly set instead of being taken from the size of msqChoices?

And does anyone know how to insert embedded code snippets into comments?

[whipermr5]

In this method, is there any reason why numOfMsqChoices is explicitly set instead of being taken from the size of msqChoices?

Yes; that's what I was wondering too. The problematic question had an empty msqChoices (the options are generated based on students in the team) but numOfMsqChoices was set to 25.

And does anyone know how to insert embedded code snippets into comments?

https://help.github.com/articles/creating-a-permanent-link-to-a-code-snippet/

[bqnguyen94]

I think it's safe to use msqChoices.size here instead of numOfMsqChoices, which was only needed in extractQuestionDetails to iterate through msqChoices. Then again, what could trigger a request with discrepancy between numOfMsqChoices and msqChoices?

[whipermr5]

Likely a bug in the code related to generating options based on students in the team.

[bqnguyen94]

In this method

teammates/src/main/java/teammates/common/datatransfer/questions/FeedbackMsqQuestionDetails.java

Lines 119 to 128 in a1e79ad

	private void setMsqQuestionDetails(FeedbackParticipantType generateOptionsFor, String courseId) {
	this.msqChoices = new ArrayList<>();
	this.otherEnabled = false;
	this.generateOptionsFor = generateOptionsFor;
	this.numOfMsqChoices = generateOptionList(courseId).size();
	Assumption.assertTrue("Can only generate students, teams or instructors",
	generateOptionsFor == FeedbackParticipantType.STUDENTS
	|| generateOptionsFor == FeedbackParticipantType.TEAMS
	|| generateOptionsFor == FeedbackParticipantType.INSTRUCTORS);
	}

I'm quite baffled as msqChoices is set to empty then not populated back (not in generateOptionList as well) while numOfMsqChoices is set to the size of the generated list for courseId. Am I missing something here?

[whipermr5]

I believe you have found the bug! This was done in #7509 (specifically, this commit). @VamsiSangam was this intentional? This leads to a non-zero numOfMsqChoices being stored in the datastore even though msqChoices is stored as empty.

[Shashwat-Garg]

@whipermr5 Anyone working on this issue ?

[whipermr5]

@Shashwat-Garg Don't think so.

[Shashwat-Garg]

I'll try to do what's left. :)
By the way, @whipermr5 what's left to be done in this ?
Can you please point to some things ?

[whipermr5]

@Shashwat-Garg Investigate why the current code was written this way #8415 (comment), and whether this solution #8415 (comment) has any side effects.

[Haozhe321]

Here's what I found:

The offending line for the Exception comes from the method getQuestionAdditionalInfoHtml which is used to generate the additional information for the questions like this:

Inside the FeedbackMsqQuestionDetails class, there are 2 attributes:

private int numOfMsqChoices;
private List<String> msqChoices;

When the MSQ's choices are generated from the instructors' input, both fields are assigned accordingly in this method:

teammates/src/main/java/teammates/common/datatransfer/questions/FeedbackMsqQuestionDetails.java

Lines 113 to 118 in cd9a536

	private void setMsqQuestionDetails(int numOfMsqChoices, List<String> msqChoices, boolean otherEnabled) {
	this.numOfMsqChoices = numOfMsqChoices;
	this.msqChoices = msqChoices;
	this.otherEnabled = otherEnabled;
	this.generateOptionsFor = FeedbackParticipantType.NONE;
	}

However when MSQ's choices are generated from say students in the class, or teams, etc.,
the method below assigns a value for numOfMsqChoices but not for msqChoices.

teammates/src/main/java/teammates/common/datatransfer/questions/FeedbackMsqQuestionDetails.java

Lines 120 to 134 in cd9a536

	private void setMsqQuestionDetails(FeedbackParticipantType generateOptionsFor, String courseId) {
	this.msqChoices = new ArrayList<>();
	this.otherEnabled = false;
	this.generateOptionsFor = generateOptionsFor;
	if (generateOptionsFor.equals(FeedbackParticipantType.STUDENTS_EXCLUDING_SELF)) {
	this.numOfMsqChoices = generateNumOfChoicesForStudentsExcludingSelf(courseId);
	} else {
	this.numOfMsqChoices = generateOptionList(courseId).size();
	}
	Assumption.assertTrue("Can only generate students, students (excluding self), teams or instructors",
	generateOptionsFor == FeedbackParticipantType.STUDENTS
	|| generateOptionsFor == FeedbackParticipantType.STUDENTS_EXCLUDING_SELF
	|| generateOptionsFor == FeedbackParticipantType.TEAMS
	|| generateOptionsFor == FeedbackParticipantType.INSTRUCTORS);
	}

Hence when the instructor wants to view the result of a session which contains MSQ questions with generated options(2nd scenario above), the above lines will trigger the problem in the execution of the lines below, as @whipermr5 has identified and discussed in the comments in this thread.

teammates/src/main/java/teammates/common/datatransfer/questions/FeedbackMsqQuestionDetails.java

Lines 482 to 487 in cd9a536

	if (numOfMsqChoices > 0) {
	optionListHtml.append("<ul style=\"list-style-type: disc;margin-left: 20px;\" >");
	for (int i = 0; i < numOfMsqChoices; i++) {
	String optionFragment =
	Templates.populateTemplate(optionFragmentTemplate,
	Slots.MSQ_CHOICE_VALUE, SanitizationHelper.sanitizeForHtml(msqChoices.get(i)));

In fact, for a question that has generated options, this additional information should not be generated, and the UI below should just be shown.

This is why MCQ questions do not face such Exceptions because in MCQ, numOfMcqChoices is not assigned and that code block in the MCQ class will not be executed when the question generates options from students, or teams, etc.

teammates/src/main/java/teammates/common/datatransfer/questions/FeedbackMcqQuestionDetails.java

Lines 357 to 366 in cd9a536

	if (numOfMcqChoices > 0) {
	optionListHtml.append("<ul style=\"list-style-type: disc;margin-left: 20px;\" >");
	for (int i = 0; i < numOfMcqChoices; i++) {
	String optionFragment =
	Templates.populateTemplate(optionFragmentTemplate,
	Slots.MCQ_CHOICE_VALUE, SanitizationHelper.sanitizeForHtml(mcqChoices.get(i)));
	optionListHtml.append(optionFragment);
	}
	}

This is also why MSQ with choices that the instructor created manually does not cause the exception, as there is no discrepancy between numOfMsqChoices and msqChoices. The UI displays correct as shown in the first screenshot in this comment. The Exception is due to the program trying to display all the choices of students, or teams, etc., which should not be displayed anyway.

TLDR:
The fix should not cause other problems because generating the responses are done in FeedbackMsqResponseDetails. The cause for the problem is because of generating the additional information for display for the questions.

[whipermr5]

Is there even a use case for storing numOfMsqChoices when we can just read msqChoices.size() all the time?

[VamsiSangam]

I believe you have found the bug! This was done in #7509 (specifically, this commit). @VamsiSangam was this intentional? This leads to a non-zero numOfMsqChoices being stored in the datastore even though msqChoices is stored as empty.

Sorry for the late reply. As far as I remember, when the MSQ/MCQ question has its options generated from students/teams etc., we don't store the options in the FeedbackMsqQuestionDetails class object. Precisely speaking, we don't store it in private List<String> msqChoices; member variable. Whenever we want to generate the HTML for the question, we directly call private List<String> generateOptionList(String courseId). msqChoices will be non-empty when the instructor has given custom options as input (not generated from students/teams etc.)

But I wanted numOfMsqChoices to have the correct value since I needed it for my min/max selectable choices feature. So, in the code mentioned, the msqChoices List was empty but numOfMsqChoices was not.

Is there even a use case for storing numOfMsqChoices when we can just read msqChoices.size() all the time?

As explained above, I believe they differ in the case where options are generated.

I may be wrong though, my knowledge of Teammates is pretty outdated 😅

[whipermr5]

@VamsiSangam Thanks for the reply! We've moved the discussion to #8714. Am I right in saying that numOfMsqChoices is used only for validation? I feel it should not be persisted to the datastore if that is so.