[#12594] Fix deadline extensions update issue #12601
Conversation
cedricongjh
changed the title
|
Folks, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢 |
2 similar comments
|
Folks, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢 |
|
Folks, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢 |
There was a problem hiding this comment.
Thanks for proposing your changes @cedricongjh . Perhaps we should update the unit tests too, such that it fails with the old code, but passes with your proposed changes. If this code is the cause of the issue, I think your changes should be mostly okay, albeit with a few nits.
However, are we actually certain that this is the actual cause of the issue? It has been a while since I've worked on this project so correct me if I am wrong. I do agree that the current code updates the deadline maps in the FeedbackSessionAttributes object in memory. However, doesn't the FeedbackSessionsDb::updateFeedbackSession method load the FeedbackSession from the database (FeedbackSessionsDb.java line 207 ... getFeedbackSessionEntity ...) before comparing (FeedbackSessionsDb.java line 221 onwards boolean hasSameAttributes ...)? Unlike the one in the memory, the one from the database should not have been updated yet at that point in time that we are checking if the attributes have changed.
In other words, I do not think we are comparing the updated feedback session with the feedback session in memory (with already updated deadline maps, which would be the same). I think we are actually comparing the updated feedback session with the feedback session in storage (with still unchanged deadline maps, which would be different). If so, I think the current code should not cause the bug, right? I think that's why the unit tests pass (testUpdateFeedbackSessionsStudentDeadlinesWithNewEmail and the instructor one). That is unless the unit tests are wrong, but they do not look wrong, at least to me.
| deadlines -> { | ||
| Map<String, Instant> newDeadlines = new HashMap<String, Instant>(deadlines); | ||
| newDeadlines.put(newEmailAddress, newDeadlines.remove(oldEmailAddress)); | ||
| return newDeadlines; | ||
| }); |
There was a problem hiding this comment.
I think we are trying to create a copy of the Map object instead of using the object directly, right? There is a simpler way to do it. Similarly for the Map for students. See below.
| Map<String, Instant> newInstructorDeadlines = deadlinesUpdater.apply(instructorDeadlines); | ||
| updateOptionsBuilder.withInstructorDeadlines(newInstructorDeadlines); |
There was a problem hiding this comment.
We can just copy the Map object here, then pass the copy into the accept method. Something like the following:
if (!instructorDeadlines.containsKey(emailAddress)) {
return;
}
Map<String, Instant> newInstructorDeadlines = new HashMap<>(instructorDeadlines);
deadlinesUpdater.accept(newInstructorDeadlines);
updateOptionsBuilder.withInstructorDeadlines(newInstructorDeadlines);Similarly for the Map for students.
Yes, this is due to the session cache in Objectify (https://github.com/objectify/objectify/wiki/Caching): 
In particular, "The session cache holds your specific entity object instances. If you load() the same entity, you will receive the exact same Java entity object instance" Hence, when updating the student deadlines in the current code, while the java entity object changes, the test case passes, as it is not fetching the entity from the db, rather it is using the current cached entity that was loaded previously, whose student deadlines were updated, but not saved to the db. To make the test case fail with the current code, I added Objectify.clear(); between the loading of the old and new feedback sessions. Moreover, testing on the frontend on this branch does resolve the issue. |
|
I see. I was not aware Objectify had that cache. That makes a lot of sense now; thank you for the insight. Then I think these changes should be fine. Let's just simplify your changes according to my nits (unless you protest), and let's add the |
|
Thanks @jayasting98, I've made the changes requested I've added a method in
|
|
Folks, This PR seems to be stalling (no activities for the past 8 days). 🐌 😢 |
|
Folks, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢 |
|
Folks, This PR seems to be stalling (no activities for the past 11 days). 🐌 😢 |
wkurniawan07
added
c.Bug
* return new map to ensure updates are persisted to db * make changes * revert indentation * add clear Objectify cache method to BaseTestCase * fix checkstyle issues
Fixes #12594
Outline of Solution
Deadline extensions map in
FeedbackSessionwas not being updated when a student's email was updated.This was due to updating of the feedback session's map, and then checking if the feedback session's attributes had changed, which would always be false since we were just comparing the same map.
Creating a new map and passing it into the update options fixes this.