Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Communication: Speedup messaging #7404

Merged
merged 8 commits into from
Oct 17, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@
* Stores the user of a conversation participant, who is supposed to receive a websocket message and stores whether
* the corresponding conversation is hidden by the user.
*
* @param user the user who is a member of the conversation
* @param userId the id of the user who is a member of the conversation
* @param userLogin the login of the user who is a member of the conversation
* @param isConversationHidden true if the user has hidden the conversation
* @param isAtLeastTutorInCourse true if the user is at least a tutor in the course
*/
public record ConversationWebSocketRecipientSummary(User user, boolean isConversationHidden, boolean isAtLeastTutorInCourse) {
public record ConversationWebSocketRecipientSummary(Long userId, String userLogin, boolean isConversationHidden, boolean isAtLeastTutorInCourse) {
}
8 changes: 8 additions & 0 deletions src/main/java/de/tum/in/www1/artemis/domain/User.java
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,14 @@
@Column(name = "iris_accepted")
private ZonedDateTime irisAccepted = null;

public User() {
}

Check warning on line 189 in src/main/java/de/tum/in/www1/artemis/domain/User.java

View check run for this annotation

Teamscale / teamscale-findings

src/main/java/de/tum/in/www1/artemis/domain/User.java#L188-L189

In this file 8 interface comments are missing. Consider adding explanatory comments or restricting the visibility. View in Teamscale: Line 161: https://teamscale.io/findings.html#details/GitHub-ls1intum-Artemis?t=performance%2Fmessages%3AHEAD&id=9A26871169C774229A2FCF09A01B4016 Line 188: https://teamscale.io/findings.html#details/GitHub-ls1intum-Artemis?t=performance%2Fmessages%3AHEAD&id=806CE3F6BF00A4227D0D450D45AF78FB Line 191: https://teamscale.io/findings.html#details/GitHub-ls1intum-Artemis?t=performance%2Fmessages%3AHEAD&id=1514CDDC1ED6A6F285E7BD22F213F8BF Line 201: https://teamscale.io/findings.html#details/GitHub-ls1intum-Artemis?t=performance%2Fmessages%3AHEAD&id=206D2A1D79428B4E4A790581E34B5BB4 Line 333: https://teamscale.io/findings.html#details/GitHub-ls1intum-Artemis?t=performance%2Fmessages%3AHEAD&id=546AA61D11FE5CD7291D53580F3B45B7 Line 397: https://teamscale.io/findings.html#details/GitHub-ls1intum-Artemis?t=performance%2Fmessages%3AHEAD&id=3018D3C1E89CDF65A721D5C35A953559 Line 402: https://teamscale.io/findings.html#details/GitHub-ls1intum-Artemis?t=performance%2Fmessages%3AHEAD&id=887219AFDCB426243F013C9C5420E48F Line 432: https://teamscale.io/findings.html#details/GitHub-ls1intum-Artemis?t=performance%2Fmessages%3AHEAD&id=116F587BD0849148B200D744F37642A0

Check warning on line 189 in src/main/java/de/tum/in/www1/artemis/domain/User.java

View check run for this annotation

Teamscale / teamscale-findings

src/main/java/de/tum/in/www1/artemis/domain/User.java#L188-L189

Empty block: constructor https://teamscale.io/findings.html#details/GitHub-ls1intum-Artemis?t=performance%2Fmessages%3AHEAD&id=090F9B2631B81B24376E42DE83C1469E

public User(Long id, String login) {
this.setId(id);
this.login = login;
}

public String getLogin() {
return login;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,22 +123,19 @@ OR lower(user.login) = lower(:#{#searchInput}))

@Query("""
SELECT NEW de.tum.in.www1.artemis.domain.ConversationWebSocketRecipientSummary (
user,
user.id,
user.login,
CASE WHEN cp.isHidden = true THEN true ELSE false END,
CASE WHEN atLeastTutors.id IS NOT null THEN true ELSE false END
CASE WHEN ug.group = :teachingAssistantGroupName OR ug.group = :editorGroupName OR ug.group = :instructorGroupName THEN true ELSE false END
)
FROM User user
JOIN UserGroup ug ON ug.userId = user.id
LEFT JOIN Course students ON ug.group = students.studentGroupName
LEFT JOIN Course atLeastTutors ON (atLeastTutors.teachingAssistantGroupName = ug.group
OR atLeastTutors.editorGroupName = ug.group
OR atLeastTutors.instructorGroupName = ug.group
)
LEFT JOIN ConversationParticipant cp ON cp.user.id = user.id AND cp.conversation.id = :conversationId
WHERE user.isDeleted = false
AND (students.id = :courseId OR atLeastTutors.id = :courseId)
WHERE user.isDeleted = false AND (ug.group = :studentGroupName OR ug.group = :teachingAssistantGroupName OR ug.group = :editorGroupName OR ug.group = :instructorGroupName)
""")
Set<ConversationWebSocketRecipientSummary> findAllWebSocketRecipientsInCourseForConversation(@Param("courseId") Long courseId, @Param("conversationId") Long conversationId);
Set<ConversationWebSocketRecipientSummary> findAllWebSocketRecipientsInCourseForConversation(@Param("conversationId") Long conversationId,
@Param("studentGroupName") String studentGroupName, @Param("teachingAssistantGroupName") String teachingAssistantGroupName,
@Param("editorGroupName") String editorGroupName, @Param("instructorGroupName") String instructorGroupName);

/**
* Searches for users in a group by their login or full name.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ public AnswerPost createAnswerPost(Long courseId, AnswerPost answerPost) {
AnswerPost savedAnswerPost = answerPostRepository.save(answerPost);
postRepository.save(post);

this.preparePostAndBroadcast(savedAnswerPost, course);
preparePostAndBroadcast(savedAnswerPost, course);
sendNotification(post, answerPost, course);

return savedAnswerPost;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,62 +78,67 @@
* @return the created message
*/
public Post createMessage(Long courseId, Post newMessage) {
if (newMessage.getId() != null) {
throw new BadRequestAlertException("A new message post cannot already have an ID", METIS_POST_ENTITY_NAME, "idexists");
}
if (newMessage.getConversation() == null || newMessage.getConversation().getId() == null) {
throw new BadRequestAlertException("A new message post must have a conversation", METIS_POST_ENTITY_NAME, "conversationnotset");
}

var author = this.userRepository.getUserWithGroupsAndAuthorities();
var author = userRepository.getUserWithGroupsAndAuthorities();
newMessage.setAuthor(author);
newMessage.setDisplayPriority(DisplayPriority.NONE);

conversationService.isMemberElseThrow(newMessage.getConversation().getId(), author.getId());
log.info(" createMessage:conversationService.isMemberElseThrow DONE");

var conversation = conversationRepository.findByIdElseThrow(newMessage.getConversation().getId());
log.info(" createMessage:conversationRepository.findByIdElseThrow DONE");
// IMPORTANT we don't need it in the conversation any more, so we reduce the amount of data sent to clients
conversation.setConversationParticipants(Set.of());
var course = preCheckUserAndCourseForMessaging(author, courseId);

// extra checks for channels
if (conversation instanceof Channel channel) {
channelAuthorizationService.isAllowedToCreateNewPostInChannel(channel, author);
}

log.debug(" createMessage:additional authorization DONE");
Set<User> mentionedUsers = parseUserMentions(course, newMessage.getContent());

log.debug(" createMessage:parseUserMentions DONE");
// update last message date of conversation
conversation.setLastMessageDate(ZonedDateTime.now());
conversation.setCourse(course);
Conversation savedConversation = conversationService.updateConversation(conversation);

// update last read date and unread message count of author
// invoke async due to db write access to avoid that the client has to wait
conversationParticipantRepository.updateLastReadAsync(author.getId(), conversation.getId(), ZonedDateTime.now());

var createdMessage = conversationMessageRepository.save(newMessage);
// set the conversation again, because it might have been lost during save
createdMessage.setConversation(conversation);
// reduce the payload of the response / websocket message: this is important to avoid overloading the involved subsystems
if (createdMessage.getConversation() != null) {
createdMessage.getConversation().hideDetails();
}

log.debug(" conversationMessageRepository.save DONE");
// TODO: we should consider invoking the following method async to avoid that authors wait for the message creation if many notifications are sent
notifyAboutMessageCreation(author, savedConversation, course, createdMessage, mentionedUsers);

log.debug(" notifyAboutMessageCreation DONE");
return createdMessage;

Check warning on line 128 in src/main/java/de/tum/in/www1/artemis/service/metis/ConversationMessagingService.java

View check run for this annotation

Teamscale / teamscale-findings

src/main/java/de/tum/in/www1/artemis/service/metis/ConversationMessagingService.java#L81-L128

This method is a bit lengthy [0]. Consider shortening it, e.g. by extracting code blocks into separate methods. [0] https://teamscale.io/findings.html#details/GitHub-ls1intum-Artemis?t=performance%2Fmessages%3AHEAD&id=4A73486D9BA142346E0CC91FFAC85773
}

private void notifyAboutMessageCreation(User author, Conversation conversation, Course course, Post createdMessage, Set<User> mentionedUsers) {
Set<ConversationWebSocketRecipientSummary> webSocketRecipients = getWebSocketRecipients(conversation).collect(Collectors.toSet());
Set<User> broadcastRecipients = webSocketRecipients.stream().map(ConversationWebSocketRecipientSummary::user).collect(Collectors.toSet());
log.debug(" getWebSocketRecipients DONE");
Set<User> broadcastRecipients = webSocketRecipients.stream().map(summary -> new User(summary.userId(), summary.userLogin())).collect(Collectors.toSet());
// Add all mentioned users, including the author (if mentioned). Since working with sets, there are no duplicate user entries
mentionedUsers = mentionedUsers.stream().map(user -> new User(user.getId(), user.getLogin())).collect(Collectors.toSet());
broadcastRecipients.addAll(mentionedUsers);

// Websocket notification 1: this notifies everyone including the author that there is a new message
broadcastForPost(new PostDTO(createdMessage, MetisCrudAction.CREATE), course, broadcastRecipients);
log.debug(" broadcastForPost DONE");

if (conversation instanceof OneToOneChat) {
var getNumberOfPosts = conversationMessageRepository.countByConversationId(conversation.getId());
Expand All @@ -143,17 +148,20 @@
}
}
conversationParticipantRepository.incrementUnreadMessagesCountOfParticipants(conversation.getId(), author.getId());
log.debug(" incrementUnreadMessagesCountOfParticipants DONE");
// ToDo: Optimization Idea: Maybe we can save this websocket call and instead get the last message date from the conversation object in the post somehow?
// send conversation with updated last message date to participants. This is necessary to show the unread messages badge in the client

// TODO: why do we need notification 2 and 3? we should definitely re-work this!
// Websocket notification 2
conversationService.notifyAllConversationMembersAboutNewMessage(course, conversation, broadcastRecipients);
log.debug(" conversationService.notifyAllConversationMembersAboutNewMessage DONE");

// creation of message posts should not trigger entity creation alert
// Websocket notification 3
Set<User> notificationRecipients = filterNotificationRecipients(author, conversation, webSocketRecipients, mentionedUsers);
conversationNotificationService.notifyAboutNewMessage(createdMessage, notificationRecipients, course);
log.debug(" conversationNotificationService.notifyAboutNewMessage DONE");
}

/**
Expand All @@ -172,12 +180,13 @@
private Set<User> filterNotificationRecipients(User author, Conversation conversation, Set<ConversationWebSocketRecipientSummary> webSocketRecipients,
Set<User> mentionedUsers) {
// Initialize filter with check for author
Predicate<ConversationWebSocketRecipientSummary> filter = recipientSummary -> !Objects.equals(recipientSummary.user().getId(), author.getId());
Predicate<ConversationWebSocketRecipientSummary> filter = recipientSummary -> !Objects.equals(recipientSummary.userId(), author.getId());

if (conversation instanceof Channel channel) {
// If a channel is not an announcement channel, filter out users, that hid the conversation
if (!channel.getIsAnnouncementChannel()) {
filter = filter.and(recipientSummary -> !recipientSummary.isConversationHidden() || mentionedUsers.contains(recipientSummary.user()));
filter = filter.and(
recipientSummary -> !recipientSummary.isConversationHidden() || mentionedUsers.contains(new User(recipientSummary.userId(), recipientSummary.userLogin())));
}

// If a channel is not visible to students, filter out participants that are only students
Expand All @@ -189,7 +198,7 @@
filter = filter.and(recipientSummary -> !recipientSummary.isConversationHidden());
}

return webSocketRecipients.stream().filter(filter).map(ConversationWebSocketRecipientSummary::user).collect(Collectors.toSet());
return webSocketRecipients.stream().filter(filter).map(summary -> new User(summary.userId(), summary.userLogin())).collect(Collectors.toSet());
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,11 +128,14 @@ else if (postConversation != null) {
*/
protected Stream<ConversationWebSocketRecipientSummary> getWebSocketRecipients(Conversation conversation) {
if (conversation instanceof Channel channel && channel.getIsCourseWide()) {
return userRepository.findAllWebSocketRecipientsInCourseForConversation(conversation.getCourse().getId(), conversation.getId()).stream();
Course course = conversation.getCourse();
return userRepository.findAllWebSocketRecipientsInCourseForConversation(conversation.getId(), course.getStudentGroupName(), course.getTeachingAssistantGroupName(),
course.getEditorGroupName(), course.getInstructorGroupName()).stream();
}

return conversationParticipantRepository.findConversationParticipantWithUserGroupsByConversationId(conversation.getId()).stream()
.map(participant -> new ConversationWebSocketRecipientSummary(participant.getUser(), participant.getIsHidden() != null && participant.getIsHidden(),
.map(participant -> new ConversationWebSocketRecipientSummary(participant.getUser().getId(), participant.getUser().getLogin(),
participant.getIsHidden() != null && participant.getIsHidden(),
authorizationCheckService.isAtLeastTeachingAssistantInCourse(conversation.getCourse(), participant.getUser())));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ public void deregisterUsersFromAConversation(Course course, Set<User> users, Con
var remainingUsers = existingUsers.stream().filter(user -> !usersToBeDeregistered.contains(user)).collect(Collectors.toSet());
var participantsToRemove = conversationParticipantRepository.findConversationParticipantsByConversationIdAndUserIds(conversation.getId(),
usersToBeDeregistered.stream().map(User::getId).collect(Collectors.toSet()));
if (participantsToRemove.size() > 0) {
if (!participantsToRemove.isEmpty()) {
conversationParticipantRepository.deleteAll(participantsToRemove);
broadcastOnConversationMembershipChannel(course, MetisCrudAction.DELETE, conversation, usersToBeDeregistered);
broadcastOnConversationMembershipChannel(course, MetisCrudAction.UPDATE, conversation, remainingUsers);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,23 @@
import java.net.URI;
import java.net.URISyntaxException;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.*;

import de.tum.in.www1.artemis.domain.metis.AnswerPost;
import de.tum.in.www1.artemis.security.annotations.EnforceAtLeastStudent;
import de.tum.in.www1.artemis.service.metis.AnswerMessageService;
import de.tum.in.www1.artemis.service.util.TimeLogUtil;

@RestController
@RequestMapping("/api")
public class AnswerMessageResource {

private final Logger log = LoggerFactory.getLogger(getClass());

private final AnswerMessageService answerMessageService;

public AnswerMessageResource(AnswerMessageService answerMessageService) {
Expand All @@ -31,10 +36,12 @@
*/
@PostMapping("courses/{courseId}/answer-messages")
@EnforceAtLeastStudent
public ResponseEntity<AnswerPost> createAnswerPost(@PathVariable Long courseId, @RequestBody AnswerPost answerMessage) throws URISyntaxException {
public ResponseEntity<AnswerPost> createAnswerMessage(@PathVariable Long courseId, @RequestBody AnswerPost answerMessage) throws URISyntaxException {
log.debug("POST createAnswerMessage invoked for course {} with message {}", courseId, answerMessage.getContent());
long start = System.nanoTime();
AnswerPost createdAnswerMessage = answerMessageService.createAnswerMessage(courseId, answerMessage);

// creation of answerMessage should not trigger alert
log.info("createAnswerMessage took {}", TimeLogUtil.formatDurationFrom(start));
return ResponseEntity.created(new URI("/api/courses" + courseId + "/answer-messages/" + createdAnswerMessage.getId())).body(createdAnswerMessage);
}

Expand All @@ -49,25 +56,30 @@
*/
@PutMapping("courses/{courseId}/answer-messages/{answerMessageId}")
@EnforceAtLeastStudent
public ResponseEntity<AnswerPost> updateAnswerPost(@PathVariable Long courseId, @PathVariable Long answerMessageId, @RequestBody AnswerPost answerMessage) {
public ResponseEntity<AnswerPost> updateAnswerMessage(@PathVariable Long courseId, @PathVariable Long answerMessageId, @RequestBody AnswerPost answerMessage) {
log.debug("PUT updateAnswerMessage invoked for course {} with message {}", courseId, answerMessage.getContent());
long start = System.nanoTime();
AnswerPost updatedAnswerMessage = answerMessageService.updateAnswerMessage(courseId, answerMessageId, answerMessage);
log.info("updateAnswerMessage took {}", TimeLogUtil.formatDurationFrom(start));
return new ResponseEntity<>(updatedAnswerMessage, null, HttpStatus.OK);
}

/**
* DELETE /courses/{courseId}/answer-messages/{id} : Delete an answer message by its id
*
* @param courseId id of the course the message belongs to
* @param answerMessageId id of the answer message to delete
* @return ResponseEntity with status 200 (OK),
* or 400 (Bad Request) if the checks on user or course validity fail
*/
@DeleteMapping("courses/{courseId}/answer-messages/{answerMessageId}")
@EnforceAtLeastStudent
public ResponseEntity<Void> deleteAnswerPost(@PathVariable Long courseId, @PathVariable Long answerMessageId) {
public ResponseEntity<Void> deleteAnswerMessage(@PathVariable Long courseId, @PathVariable Long answerMessageId) {
log.debug("PUT deleteAnswerMessage invoked for course {} on message {}", courseId, answerMessageId);
long start = System.nanoTime();
answerMessageService.deleteAnswerMessageById(courseId, answerMessageId);

log.info("deleteAnswerMessage took {}", TimeLogUtil.formatDurationFrom(start));
// deletion of answerMessages should not trigger alert
return ResponseEntity.ok().build();
}

Check warning on line 84 in src/main/java/de/tum/in/www1/artemis/web/rest/metis/AnswerMessageResource.java

View check run for this annotation

Teamscale / teamscale-findings

src/main/java/de/tum/in/www1/artemis/web/rest/metis/AnswerMessageResource.java#L59-L84

Clone with 3 instances of length 12 https://teamscale.io/findings.html#details/GitHub-ls1intum-Artemis?t=performance%2Fmessages%3AHEAD&id=E9ED34B8D42A00CF14529FFE5C9FF8FE
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,16 @@
import java.net.URI;
import java.net.URISyntaxException;

import org.springframework.beans.factory.annotation.Value;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.*;

import de.tum.in.www1.artemis.domain.metis.AnswerPost;
import de.tum.in.www1.artemis.security.annotations.EnforceAtLeastStudent;
import de.tum.in.www1.artemis.service.metis.AnswerPostService;
import de.tum.in.www1.artemis.service.util.TimeLogUtil;

/**
* REST controller for managing AnswerPost.
Expand All @@ -19,10 +21,9 @@
@RequestMapping("/api")
public class AnswerPostResource {

private final AnswerPostService answerPostService;
private final Logger log = LoggerFactory.getLogger(getClass());

@Value("${jhipster.clientApp.name}")
private String applicationName;
private final AnswerPostService answerPostService;

public AnswerPostResource(AnswerPostService answerPostService) {
this.answerPostService = answerPostService;
Expand All @@ -39,7 +40,10 @@
@PostMapping("courses/{courseId}/answer-posts")
@EnforceAtLeastStudent
public ResponseEntity<AnswerPost> createAnswerPost(@PathVariable Long courseId, @RequestBody AnswerPost answerPost) throws URISyntaxException {
log.debug("POST createAnswerPost invoked for course {} with post {}", courseId, answerPost.getContent());
long start = System.nanoTime();
AnswerPost createdAnswerPost = answerPostService.createAnswerPost(courseId, answerPost);
log.info("createAnswerPost took {}", TimeLogUtil.formatDurationFrom(start));
return ResponseEntity.created(new URI("/api/courses" + courseId + "/answer-posts/" + createdAnswerPost.getId())).body(createdAnswerPost);
}

Expand All @@ -54,23 +58,29 @@
*/
@PutMapping("courses/{courseId}/answer-posts/{answerPostId}")
@EnforceAtLeastStudent
public ResponseEntity<AnswerPost> updateAnswerPost(@PathVariable Long courseId, @PathVariable Long answerPostId, @RequestBody AnswerPost answerPost) {
log.debug("PUT updateAnswerPost invoked for course {} with post {}", courseId, answerPost.getContent());
long start = System.nanoTime();
AnswerPost updatedAnswerPost = answerPostService.updateAnswerPost(courseId, answerPostId, answerPost);
log.info("updatedAnswerPost took {}", TimeLogUtil.formatDurationFrom(start));
return new ResponseEntity<>(updatedAnswerPost, null, HttpStatus.OK);
}

/**
* DELETE /courses/{courseId}/posts/{id} : Delete an answer post by its id
*
* @param courseId id of the course the post belongs to
* @param answerPostId id of the answer post to delete
* @return ResponseEntity with status 200 (OK),
* or 400 (Bad Request) if the checks on user or course validity fail
*/
@DeleteMapping("courses/{courseId}/answer-posts/{answerPostId}")
@EnforceAtLeastStudent
public ResponseEntity<Void> deleteAnswerPost(@PathVariable Long courseId, @PathVariable Long answerPostId) {
log.debug("PUT deleteAnswerPost invoked for course {} on post {}", courseId, answerPostId);
long start = System.nanoTime();
answerPostService.deleteAnswerPostById(courseId, answerPostId);
log.info("deleteAnswerPost took {}", TimeLogUtil.formatDurationFrom(start));
return ResponseEntity.ok().build();
}

Check warning on line 85 in src/main/java/de/tum/in/www1/artemis/web/rest/metis/AnswerPostResource.java

View check run for this annotation

Teamscale / teamscale-findings

src/main/java/de/tum/in/www1/artemis/web/rest/metis/AnswerPostResource.java#L61-L85

Clone with 3 instances of length 12 https://teamscale.io/findings.html#details/GitHub-ls1intum-Artemis?t=performance%2Fmessages%3AHEAD&id=B7125DFB7B39BB98EF6E19C1387CC1A3
}
Loading
Loading