Programming exercises: Improve repository diff report generation performance
#10374
Conversation
add TODOs refactor method names
…ion bug" This reverts commit 34264d0.
# Conflicts: # src/main/java/de/tum/cit/aet/artemis/programming/web/repository/RepositoryProgrammingExerciseParticipationResource.java
github-actions
bot
added
server
WalkthroughThe pull request refactors repository handling and diff report generation across multiple components. It modifies how test repository contents are fetched in the Pyris service by replacing an Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant R as GitDiffReportResource
participant S as GitDiffReportService
participant G as Git Engine
C->>R: GET /git-diff-report/{exerciseId}/{commitHash1}/{commitHash2}
R->>S: generateReportForCommits(repositoryUri, commitHash1, commitHash2)
S->>S: prepareRepository(repositoryUri)
S->>G: Retrieve repository state and commit details
G-->>S: Commit details, diff data
S->>S: createReport(commit details)
S-->>R: ProgrammingExerciseGitDiffReport
R-->>C: 200 OK with diff report
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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: 0
🧹 Nitpick comments (6)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisDTOService.java (1)
69-87: Refactor to avoid duplicating repository-fetching logic.
These lines largely replicate the logic already encapsulated ingetRepositoryContents(...)(lines 163–189). Extract a shared method or reusegetRepositoryContents(...)to maintain consistency and reduce code duplication.src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseGitDiffReportService.java (4)
185-185: Consider implementing LocalVC approach
The TODO comments reference bypassing a full checkout for LocalVC. I can help propose a bare repository approach or partial clone to improve performance.Also applies to: 189-190
191-192: Potential concurrency or performance overhead
Performing checkouts in parallel might warrant concurrency safeguards. Consider a bare repository approach for improved performance under load.
259-260: Rename local variable
Since this is a genericprepareRepositorymethod, renaming the variable for clarity would align better with its usage.-Repository templateRepo = gitService.getOrCheckoutRepository(vcsRepositoryUri, true); +Repository repository = gitService.getOrCheckoutRepository(vcsRepositoryUri, true);
324-343: Graceful handling of missing commits
Returningnullcould lead to null-pointer issues. Throwing a dedicated exception or returning anOptionalmight be safer.src/main/java/de/tum/cit/aet/artemis/programming/service/RepositoryService.java (1)
192-192: Consider implementing the suggested enhancement.The TODO comment suggests offering the method without requiring a commitId parameter. This could help improve performance by avoiding unnecessary commit lookups when the latest state is sufficient.
Would you like me to help implement this enhancement? I can propose a solution that handles both cases (with and without commitId).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisDTOService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/CommitHistoryService.java(0 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/GitService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseGitDiffReportService.java(9 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/RepositoryService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseGitDiffReportResource.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/repository/RepositoryResource.java(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/de/tum/cit/aet/artemis/programming/service/CommitHistoryService.java
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
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
src/main/java/de/tum/cit/aet/artemis/programming/web/repository/RepositoryResource.javasrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisDTOService.javasrc/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseGitDiffReportResource.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/GitService.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/RepositoryService.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseGitDiffReportService.java
🧠 Learnings (1)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisDTOService.java (3)
Learnt from: julian-christl
PR: ls1intum/Artemis#9322
File: src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java:0-0
Timestamp: 2024-11-12T12:51:51.201Z
Learning: In Artemis, an `ExerciseGroup` always has an associated `Exam`, so `exerciseGroup.exam` is never null.
Learnt from: julian-christl
PR: ls1intum/Artemis#9322
File: src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java:170-172
Timestamp: 2024-11-12T12:51:46.554Z
Learning: In Artemis, `exercise.exerciseGroup` may be null, as not all exercises belong to an `ExerciseGroup`.
Learnt from: Hialus
PR: ls1intum/Artemis#8607
File: src/main/java/de/tum/in/www1/artemis/web/rest/programming/ProgrammingExerciseResource.java:64-64
Timestamp: 2024-11-12T12:51:51.201Z
Learning: For the Artemis project, import statements are automatically managed by formatters and should not be commented on.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: client-style
- GitHub Check: client-tests
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (23)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisDTOService.java (2)
65-65: Initialization of an empty Map looks good.
This ensures a safe default if the repository retrieval fails.
67-68: Consider null checks for the test repository URI.
Ifexercise.getVcsTestRepositoryUri()can ever be null, it may cause unexpected exceptions further down the pipeline.src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseGitDiffReportService.java (11)
7-7: Imports look good
The usage ofStandardCharsetsand JGit diff classes is appropriate.Also applies to: 17-19
28-28: ProfileService injection
InjectingProfileServiceis coherent with the local VCS logic. Its usage at line 325 to check local VCS mode is consistent.Also applies to: 66-66, 70-71, 79-79
117-117: Report creation invoked
The call tocreateReportForTemplateWithSolutionfits the new refactor. Assigning commit hashes afterward is clear.
195-195: FileTreeIterator usage
Initializing a new tree parser for the submission’s repository is correct for diffing.
197-197: Switching back to default branch
This ensures the repository returns to a clean state post-diff.
232-233: Documentation for template/solution URIs
These parameters are described well.
237-245: Refactored method for template/solution
Encapsulating the diff creation logic between template and solution helps maintain clarity. The TODO for LocalVC remains valid.
253-253: Documentation clarified
Specifying that the repository is checked out and reset to HEAD helps explain the approach.Also applies to: 255-255
313-323: Method documentation
Great explanation of how to generate a diff for two commits.
345-356: Method documentation
The JavaDoc clearly outlines the commit-based diff logic.
357-383: Diff creation logic
This is a straightforward usage ofDiffFormatter; rename detection and handling of identical commits is well-managed.src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseGitDiffReportResource.java (5)
65-65: Constructor injection
AcceptingParticipationAuthorizationCheckServiceandRepositoryServicein the constructor aligns with the refactored structure.
125-125: Doc improvement
The endpoint’s description is clearer with the new path details.
129-130: Parameter documentation
Providing explicit parameter descriptions benefits API consumers.
137-137: Method signature rename
Switching tosubmissionIdinstead ofsubmissionId1simplifies understanding of the parameter’s role.
142-142: Submission retrieval
LeveragingorElseThrowensures valid submission handling.src/main/java/de/tum/cit/aet/artemis/programming/web/repository/RepositoryResource.java (1)
333-333: Renamed method usage
AdoptingisWorkingCopyCleanaligns with the updated naming convention and clarifies the purpose of the check.Also applies to: 337-337
src/main/java/de/tum/cit/aet/artemis/programming/service/RepositoryService.java (2)
486-488: LGTM! The new method name is more descriptive.The rename from
isCleantoisWorkingCopyCleanbetter describes the method's purpose, making it clear that it checks the cleanliness of the working copy.
499-501: LGTM! The new method name is more descriptive.The rename from
isCleantoisWorkingCopyCleanbetter describes the method's purpose, making it clear that it checks the cleanliness of the working copy. This change maintains consistency with the single-parameter version.src/main/java/de/tum/cit/aet/artemis/programming/service/GitService.java (2)
346-346: LGTM! Improved encapsulation by making the method private.The visibility change from
publictoprivatebetter encapsulates this internal helper method, following the principle of least privilege.
1111-1116: LGTM! The new method name is more descriptive.The rename from
isCleantoisWorkingCopyCleanbetter describes the method's purpose, making it clear that it checks the cleanliness of the working copy. This change maintains consistency with the rest of the codebase.
Checklist
General
Server
Changes affecting Programming Exercises
Motivation and Context
The performance of endpoint
programming-exercises/{exerciseId}/submissions/{submissionId1}/diff-report/{submissionId2}can be improved for LocalVC, as we can avoid checking out the repository.Description
Improves performance of the diff report generation, (between commits of one repository) by not checking out the repos, but using the local path for LocalVC repositories.
I didn't find a way to make diff generation work with this method between two different repositories (e.g. solution and template)
Steps for Testing
Make sure the repository diff view works everywhere as intended.
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Code Review
Manual Tests
Test Coverage
Screenshots
Summary by CodeRabbit
New Features
Refactor