Development: Remove atlas dependency on jgrapht and apfloat
#9342
Conversation
WalkthroughThe changes involve the removal of two dependencies from the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UnionFind
participant Element
Client->>UnionFind: Initialize with elements
UnionFind->>Element: Set each element as its own parent
Client->>UnionFind: Add Element
UnionFind->>Element: Ensure no duplicates
Client->>UnionFind: Find Element
UnionFind->>Element: Retrieve representative
Client->>UnionFind: Union Elements
UnionFind->>Element: Merge sets
Client->>UnionFind: Check inSameSet
UnionFind->>Element: Verify membership
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
|
github-actions
bot
added
tests
server
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (4)
- build.gradle (0 hunks)
- src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathNgxService.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/atlas/service/util/UnionFind.java (1 hunks)
- src/test/java/de/tum/cit/aet/artemis/UnionFindTest.java (1 hunks)
Files not reviewed due to no reviewable changes (1)
- build.gradle
Additional context used
Path-based instructions (3)
src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathNgxService.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/atlas/service/util/UnionFind.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/test/java/de/tum/cit/aet/artemis/UnionFindTest.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 not posted (15)
src/test/java/de/tum/cit/aet/artemis/UnionFindTest.java (7)
24-27: LGTM!The test is well-written, follows the naming convention, and uses specific assertions to verify the initial state of the
UnionFindinstance.
30-34: LGTM!The test is well-written, follows the naming convention, and uses specific assertions to verify the behavior of adding a new element to the
UnionFindinstance.
37-40: LGTM!The test is well-written, follows the naming convention, and uses specific assertions to verify the behavior of finding the representative of individual elements.
43-47: LGTM!The test is well-written, follows the naming convention, and uses specific assertions to verify the behavior of merging sets using the
unionmethod.
50-54: LGTM!The test is well-written, follows the naming convention, and uses specific assertions to verify the behavior of checking if elements belong to the same set using the
inSameSetmethod.
57-61: LGTM!The test is well-written, follows the naming convention, and uses specific assertions to verify the behavior of counting the number of sets after performing multiple unions.
64-68: LGTM!The test is well-written, follows the naming convention, and uses specific assertions to verify the behavior of checking the size of the
UnionFindinstance before and after adding an element.src/main/java/de/tum/cit/aet/artemis/atlas/service/util/UnionFind.java (7)
16-22: LGTM!The constructor correctly initializes the
UnionFinddata structure with the provided elements, setting each element as its own parent with a rank of 0 and updating thecountvariable accordingly.
29-36: LGTM!The
addElementmethod correctly handles the addition of a new element to theUnionFinddata structure, ensuring that duplicates are not added and updating the necessary data structures and variables.
44-68: LGTM!The
findmethod correctly implements the find operation with path compression, efficiently retrieving the representative element of the set containing the given element. The method also properly handles the case when the input element is not found in the data structure.
76-101: LGTM!The
unionmethod correctly implements the union operation with union by rank, efficiently merging the sets containing the given elements. The method properly handles the case when either of the input elements is not found in the data structure and updates thecountvariable to reflect the merging of sets.
110-112: LGTM!The
inSameSetmethod correctly checks if the two given elements belong to the same set by comparing their representative elements (roots) obtained using thefindmethod.
119-121: LGTM!The
numberOfSetsmethod correctly returns the current number of disjoint sets by returning the value of thecountvariable.
128-130: LGTM!The
sizemethod correctly returns the total number of elements in theUnionFinddata structure by returning the size of theparentMap.src/main/java/de/tum/cit/aet/artemis/atlas/service/learningpath/LearningPathNgxService.java (1)
26-26: Verify correctness of the custom UnionFind implementation.The import statement change aligns with the PR objective to remove the dependency on the
jgraphtlibrary. However, please ensure that the customUnionFindimplementation located in thede.tum.cit.aet.artemis.atlas.service.utilpackage has been thoroughly tested for correctness and provides equivalent behavior to the previous implementation fromjgrapht.To verify the correctness of the custom
UnionFindimplementation, you can run the following script:The script performs the following verifications:
- Checks if both the
UnionFindclass and its corresponding test classUnionFindTestexist in the codebase.- Searches for usage of core methods specific to the
UnionFindclass (union,find,connected) to ensure that the custom implementation provides the same functionality.- Checks if the
UnionFindTestclass contains multiple test methods annotated with@Testto verify the correctness of theUnionFindimplementation.Please review the test results and ensure that the custom
UnionFindimplementation is thoroughly tested and provides the same behavior as the previous implementation.
MaximilianAnzinger
dismissed stale reviews from JohannesWt and coderabbitai
via
40feab5
MaximilianAnzinger
changed the title
Development: Remove jgrapht and apfloat dependencyDevelopment: Remove atlas dependency on jgrapht and apfloat
coolchock
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
Checklist
General
Server
Motivation and Context
jgrapht relies on apfloat which may cause serious security issues.
Description
In this PR we add our own implementation of a generic UnionFind datastructure that utilizes path compression and accounts for the rank of the nodes to achieve an amotized runtime O($\alpha$ (n)) and O(n) space complexity.
Steps for Testing
Prerequisites:
1 Instructor
Course with learning paths enabled
Competencies that contain relations of the type "Matching"
navigate to the learning paths tab of the instructor view
view a students learning path
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
Test Coverage
Summary by CodeRabbit
New Features
UnionFindclass for efficient management of disjoint sets.UnionFindfunctionality.Bug Fixes
Refactor
UnionFindimplementation.