General: Cache course icons and profile pictures to improve performance
#9459
Conversation
github-actions
bot
added
server
WalkthroughThe pull request introduces several updates across multiple classes. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range comments (5)
src/main/java/de/tum/cit/aet/artemis/core/config/PublicResourcesConfiguration.java (1)
Line range hint
36-62: Summary: Caching implementation for static resources looks goodThe changes to
PublicResourcesConfigurationsuccessfully implement caching for course icons, profile pictures, and drag-and-drop quiz pictures. This aligns with the PR objective of improving performance by reducing the number of times these images are loaded from the server.Key points:
- The
fileUploadPathis correctly injected using@Value.- Resource handlers are added for each type of static resource.
- Caching is applied consistently using
defaultCacheControl.Suggestions for improvement:
- Address the TODO comment by extracting path constants to avoid duplication.
- Consider minor formatting adjustments for consistency.
Overall, these changes should contribute to improved performance as intended. Remember to update the relevant tests and documentation to reflect these changes.
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (2)
Line range hint
139-149: LGTM! Consider callingsubscribeToMetis()inngOnInit().The new
subscribeToMetis()method is well-implemented and follows Angular best practices. It correctly subscribes to the necessary observables and updates the component's state.To ensure this subscription is set up when the component initializes, consider adding a call to
subscribeToMetis()in thengOnInit()method.You can add the following line to the
ngOnInit()method:this.subscribeToMetis();
Line range hint
172-172: LGTM! Consider using arrow function syntax.The modification to reverse the order of posts before assignment is a good change that aligns with the PR objectives. This will likely improve the performance of the component when rendering posts.
To maintain consistency with the coding guidelines, consider using arrow function syntax for this method.
You can refactor the method as follows:
setPosts = (posts: Post[]): void => { if (this.content) { this.previousScrollDistanceFromTop = this.content.nativeElement.scrollHeight - this.content.nativeElement.scrollTop; } this.posts = posts.slice().reverse(); };src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java (2)
Line range hint
1-665: Consider reviewing and aligning caching across all relevant endpoints.While the changes in the
responseEntityForFilePathmethod are excellent, it might be beneficial to ensure consistency in caching across all relevant endpoints in this file.Review other methods that serve files, such as
getCourseIcon,getProfilePicture, and others, to ensure they also implement appropriate caching strategies. This would provide a consistent performance improvement across the application.For example, you might want to update the
getCourseIconmethod:@GetMapping("files/course/icons/{courseId}/*") @EnforceAtLeastStudent public ResponseEntity<byte[]> getCourseIcon(@PathVariable Long courseId) { log.debug("REST request to get icon for course : {}", courseId); Course course = courseRepository.findByIdElseThrow(courseId); - return responseEntityForFilePath(getActualPathFromPublicPathString(course.getCourseIcon())); + return responseEntityForFilePath(getActualPathFromPublicPathString(course.getCourseIcon())).cacheControl(CacheControl.maxAge(FILE_CACHE_DAYS, TimeUnit.DAYS)); }Apply similar changes to other relevant methods to ensure consistent caching behavior across the application.
Caching not properly implemented for course icons and profile pictures.
The current implementation of
getCourseIconandgetProfilePicturedoes not enable caching by setting thecacheparameter totruein theresponseEntityForFilePathmethod. This means caching headers are not applied, contrary to the PR objectives.Recommendation:
- Update the
getCourseIconandgetProfilePicturemethods to callresponseEntityForFilePathwith thecacheparameter set totrueto ensure proper caching.🔗 Analysis chain
Line range hint
1-665: Verify caching implementation for course icons and profile pictures.The PR objectives mention caching course icons and profile pictures. While the changes in the
responseEntityForFilePathmethod provide a good foundation for caching, we should verify that this implementation effectively caches these specific resources.To ensure that the caching is correctly applied to course icons and profile pictures, we can run the following script:
This script will help us verify that the caching implementation is correctly applied to the course icons and profile pictures, as specified in the PR objectives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify caching implementation for course icons and profile pictures # Test: Check if caching is applied to course icon endpoints echo "Checking course icon endpoints:" rg --type java -C 3 "files/course/icons.*ResponseEntity" # Test: Check if caching is applied to profile picture endpoints echo "Checking profile picture endpoints:" rg --type java -C 3 "files/user/profile-pictures.*ResponseEntity" # Test: Verify that responseEntityForFilePath is used in these methods echo "Verifying usage of responseEntityForFilePath:" rg --type java -C 3 "responseEntityForFilePath.*course.getCourseIcon" rg --type java -C 3 "responseEntityForFilePath.*user.getImageUrl"Length of output: 2178
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (4)
- src/main/java/de/tum/cit/aet/artemis/core/config/PublicResourcesConfiguration.java (3 hunks)
- src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java (2 hunks)
- src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (1 hunks)
- src/main/webapp/app/shared/metis/metis-conversation.service.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/main/java/de/tum/cit/aet/artemis/core/config/PublicResourcesConfiguration.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (1)
src/main/webapp/app/shared/metis/metis-conversation.service.ts (1)
🔇 Additional comments (5)
src/main/java/de/tum/cit/aet/artemis/core/config/PublicResourcesConfiguration.java (2)
36-37: LGTM: New field added correctlyThe
fileUploadPathfield is correctly added and follows the coding guidelines. It uses the@Valueannotation for property injection, which is appropriate for configuration values.
59-60: LGTM: Resource handler for profile pictures added correctlyThe resource handler for user profile pictures is well-implemented. It correctly uses the
fileUploadPathand applies caching. The code is also formatted for good readability.src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (1)
Line range hint
1-280: Overall, the changes look good and align with the PR objectives.The modifications to the
ConversationMessagesComponentimprove data management and should enhance performance. The newsubscribeToMetis()method and the changes tosetPosts()are well-implemented and follow Angular best practices.A few minor suggestions have been made to further improve the code:
- Call
subscribeToMetis()in thengOnInit()method to ensure the subscriptions are set up when the component initializes.- Consider using arrow function syntax for the
setPosts()method to maintain consistency with the coding guidelines.These changes contribute positively to the overall goal of caching course icons and profile pictures to improve performance.
src/main/webapp/app/shared/metis/metis-conversation.service.ts (1)
119-119: Improved type safety with explicit typingThe change to explicitly type
cachedConversationasConversationDTO | undefinedenhances code clarity and type safety. This aligns well with TypeScript best practices and Angular style guidelines.src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java (1)
Line range hint
1-665: Summary: Excellent caching implementation with room for further optimization.The changes made to the
responseEntityForFilePathmethod significantly improve the application's performance by implementing caching for file responses. This aligns well with the PR objectives of caching course icons and profile pictures.Key points:
- Caching is now implemented for file responses with a 30-day duration.
- The content type is dynamically set based on the filename, improving flexibility.
Suggestions for further improvement:
- Extract the cache duration into a constant for better maintainability.
- Review and align caching strategies across all relevant endpoints in the file.
- Verify the specific implementation for course icons and profile pictures.
Overall, these changes provide a solid foundation for improving the application's performance. Implementing the suggested improvements will further enhance the caching strategy and ensure consistency across the application.
src/main/java/de/tum/cit/aet/artemis/core/config/PublicResourcesConfiguration.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/config/PublicResourcesConfiguration.java
Show resolved
Hide resolved
PaRangger
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
krusche
dismissed stale reviews from coderabbitai[bot] and PaRangger
via
caddaec
github-actions
bot
added
tests
quiz
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range comments (6)
src/test/java/de/tum/cit/aet/artemis/core/util/CourseTestService.java (6)
Line range hint
487-488: Missing@TestAnnotation on Test MethodsThe test methods are missing the
@Testannotation from JUnit 5, which is necessary for the test runner to recognize and execute them.Apply this diff to add the
@Testannotation to each test method:+import org.junit.jupiter.api.Test; // Test +@Test public void testCreateCourseWithPermission() throws Exception { // test implementation } // Test +@Test public void testCreateCourseWithSameShortName() throws Exception { // test implementation } // Test +@Test private void testCreateCourseWithNegativeValue(Course course) throws Exception { // test implementation } // Test +@Test public void testCreateCourseWithNegativeMaxComplainNumber() throws Exception { // test implementation } // Continue adding @Test annotations to the remaining test methods...Also applies to: 495-496, 503-504, 511-512, 519-520, 527-528, 537-538, 545-546, 553-554
Line range hint
675-683: Avoid Using Magic Numbers in Test SetupIn the
setupmethod,NUMBER_OF_STUDENTS,NUMBER_OF_TUTORS,NUMBER_OF_EDITORS, andNUMBER_OF_INSTRUCTORSare hardcoded values. It's better to use constants or configuration parameters to improve readability and maintainability.Consider defining these numbers as configurable constants or parameters:
public class CourseTestService { private static final int NUMBER_OF_STUDENTS = 8; private static final int NUMBER_OF_TUTORS = 5; private static final int NUMBER_OF_EDITORS = 1; private static final int NUMBER_OF_INSTRUCTORS = 1; public void setup(String userPrefix, MockDelegate mockDelegate) { - userUtilService.addUsers(userPrefix, 8, 5, 1, 1); + userUtilService.addUsers(userPrefix, NUMBER_OF_STUDENTS, NUMBER_OF_TUTORS, NUMBER_OF_EDITORS, NUMBER_OF_INSTRUCTORS); // rest of the setup method... }
Line range hint
1824-1834: Handle Potential NullPointerExceptionIn the method
testGetCourseForDashboard, the variablecourses.getFirst()is used without checking if the listcoursesis not empty. This could lead to aNullPointerExceptionifcoursesis empty.Consider adding a check to ensure that
coursesis not empty before callinggetFirst():public void testGetCourseForDashboard(boolean userRefresh) throws Exception { List<Course> courses = courseUtilService.createCoursesWithExercisesAndLecturesAndLectureUnitsAndCompetencies(userPrefix, true, false, NUMBER_OF_TUTORS); + if (courses.isEmpty()) { + throw new IllegalStateException("Course list is empty."); + } CourseForDashboardDTO receivedCourseForDashboard = request.get("/api/courses/" + courses.getFirst().getId() + "/for-dashboard?refresh=" + userRefresh, HttpStatus.OK, CourseForDashboardDTO.class); Course receivedCourse = receivedCourseForDashboard.course(); // rest of the method... }
Line range hint
2446-2460: Possible Redundant Null CheckIn the method
testArchiveCourseWithTestModelingAndFileUploadExercises, after the archiving process, the code retrieves the updated course and checksassertThat(updatedCourse.getCourseArchivePath()).isNotEmpty();. SincegetCourseArchivePath()returns aString, which cannot be null whenisNotEmpty()is called, the null check might be redundant.Ensure that the null check is necessary; otherwise, consider removing it to clean up the code.
Line range hint
799-806: Unused Import StatementsThere are several unused import statements at the top of the file. Unused imports can clutter the code and may lead to confusion.
Consider removing unused imports to clean up the code.
For example:
-import org.mockito.MockedStatic;
Line range hint
2540-2546: Consistent Use of AssertionsIn
testDownloadCourseArchiveAsInstructor_not_found, the assertion checks forassertThat(downloadedArchive).isNull();. In other test methods, similar checks useassertThat(response).as("message").isNull();. For consistency, consider adding a description to the assertion.-assertThat(downloadedArchive).isNull(); +assertThat(downloadedArchive).as("No archive should be downloaded as it does not exist").isNull();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- src/test/java/de/tum/cit/aet/artemis/core/util/CourseTestService.java (1 hunks)
- src/test/java/de/tum/cit/aet/artemis/quiz/QuizExerciseIntegrationTest.java (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/test/java/de/tum/cit/aet/artemis/core/util/CourseTestService.java (1)
Pattern
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: truesrc/test/java/de/tum/cit/aet/artemis/quiz/QuizExerciseIntegrationTest.java (1)
Pattern
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
🔇 Additional comments (2)
src/test/java/de/tum/cit/aet/artemis/core/util/CourseTestService.java (2)
Line range hint
1592-1604: Exception Handling in Test MethodsThe method
testEnrollInCourse_notMeetsDatedoes not include assertions to verify that the expected exceptions are thrown when enrollment dates are not met.Add assertions to check that the correct exceptions or error responses are received.
// Add assertions to verify that enrollment fails due to date constraints request.post("/api/courses/" + notYetStartedCourse.getId() + "/enroll", User.class, HttpStatus.FORBIDDEN); request.post("/api/courses/" + finishedCourse.getId() + "/enroll", User.class, HttpStatus.FORBIDDEN);
Line range hint
3058-3060: Potential Misuse of Mocking LibrariesIn
testUpdateCourse_instructorNotInCourse, the method uses therequestobject to perform a request and expects aFORBIDDENresponse. Ensure that the mocking is appropriately set up to simulate the instructor not being in the course.Review the mocking setup to confirm that it correctly reflects the user permissions.
src/test/java/de/tum/cit/aet/artemis/quiz/QuizExerciseIntegrationTest.java
Show resolved
Hide resolved
Checklist
General
Motivation and Context
Improve performance by caching images that do not change
Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Summary by CodeRabbit
New Features
Bug Fixes
Refactor