-
Notifications
You must be signed in to change notification settings - Fork 297
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
: Fix an issue with duplicated posts on course wide search
#9819
Communication
: Fix an issue with duplicated posts on course wide search
#9819
Conversation
WalkthroughThe changes in this pull request involve modifications to 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/communication/repository/CustomPostRepositoryImpl.java (2)
55-56
: Consider extracting deduplication logic and improving documentation.While the implementation is correct, consider these improvements:
- Extract the deduplication logic into a reusable utility method
- Enhance the comment to explain why duplicates occur (e.g., due to answer posts relationship)
Consider applying this refactor:
- // removes all duplicates from the answer posts - List<Long> uniquePostIds = new ArrayList<>(new LinkedHashSet<>(postIds)); + // Answer posts can appear multiple times in search results due to their relationship with parent posts + List<Long> uniquePostIds = PostRepositoryUtil.deduplicateIds(postIds);Add this utility class:
public class PostRepositoryUtil { public static List<Long> deduplicateIds(List<Long> ids) { return new ArrayList<>(new LinkedHashSet<>(ids)); } }
Line range hint
33-73
: Consider SQL-level deduplication for better performance.Instead of deduplicating in memory, consider using SQL DISTINCT in the query for better performance with large datasets. This would reduce the amount of data transferred from the database.
Consider modifying the query like this:
CriteriaQuery<Long> criteriaQuery = builder.createQuery(Long.class); Root<Post> root = criteriaQuery.from(Post.class); -criteriaQuery.select(root.get(Post_.ID)); +criteriaQuery.select(root.get(Post_.ID)).distinct(true);This would:
- Handle deduplication at the database level
- Reduce memory usage and processing time
- Ensure consistent counts for pagination
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/communication/repository/CustomPostRepositoryImpl.java
(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/communication/repository/CustomPostRepositoryImpl.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_imports
🔇 Additional comments (2)
src/main/java/de/tum/cit/aet/artemis/communication/repository/CustomPostRepositoryImpl.java (2)
5-5
: LGTM! Good choice of data structure.
LinkedHashSet
is an appropriate choice as it maintains insertion order while ensuring uniqueness.
73-73
: Verify pagination behavior with deduplicated results.
The count query might include duplicates while the returned list is deduplicated. This could lead to pagination inconsistencies where:
- The total count is higher than actual unique posts
- Pages might have fewer items than expected
Let's verify the impact:
Communication
Only show post once per search
Communication
Only show post once per searchCommunication
: Only show post once per search
Communication
: Only show post once per searchCommunication
: Remove post duplication on course wide search
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on TS5, worked just fine. No duplicated were shown
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on TS5, works as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on TS5. Works as described
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on TS5, search result only shows up once, works as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on TS5, all works as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx for looking into it again 👍
Communication
: Remove post duplication on course wide searchCommunication
: Fix an issue with duplicated posts on course wide search
Checklist
General
Server
Motivation and Context
Currently, the search returns for each answer post an occurences in the course wide search, since we do not filter on duplicates. This PR fixes the issue
Description
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
Code Review
Manual Tests
Test Coverage
Screenshots
Before:
After:
Summary by CodeRabbit