Development: Export programming repositories in-memory
#11027
Conversation
github-actions
bot
added
tests
server
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
…yResource for zip operations.
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
ekayandan
changed the title
Development: Export programming repositories in-memory
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary |
![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
♻️ Duplicate comments (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryRepositoryBuilder.java (1)
172-179: Method name is misleading: rename to reflect pack/index creation.
createGitIndexactually writes the object pack and index, not the working-tree index. Rename for clarity.Apply:
- private static void createGitIndex(InMemoryRepository repo, ObjectId commitId, ZipArchiveOutputStream zipOutputStream, Set<String> createdDirs) throws IOException { + private static void writePackAndIndex(InMemoryRepository repo, ObjectId commitId, ZipArchiveOutputStream zipOutputStream, Set<String> createdDirs) throws IOException {and
- createGitIndex(repo, commitId, zipOutputStream, createdDirs); + writePackAndIndex(repo, commitId, zipOutputStream, createdDirs);Also applies to: 225-263
🧹 Nitpick comments (6)
src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryDirCache.java (3)
57-69: Avoid re-sorting DirCache entries; rely on Builder’s canonical order.DirCacheBuilder#finish already sorts entries correctly (including stage ordering). The custom byte-wise sort risks breaking canonical ordering for edge-cases (e.g., non-zero stages). Iterate in the existing order instead.
Apply:
- Arrays.sort(entries, (cacheEntryA, cacheEntryB) -> { - byte[] pathABytes = cacheEntryA.getPathString().getBytes(StandardCharsets.UTF_8); - byte[] pathBBytes = cacheEntryB.getPathString().getBytes(StandardCharsets.UTF_8); - int minLength = Math.min(pathABytes.length, pathBBytes.length); - for (int i = 0; i < minLength; i++) { - int diff = (pathABytes[i] & 0xff) - (pathBBytes[i] & 0xff); - if (diff != 0) { - return diff; - } - } - return pathABytes.length - pathBBytes.length; - }); + // DirCache.Builder#finish already produced canonical order; no extra sort needed.
83-90: Consider populating mtime/ctime and size when available.Zeroing times and size is valid but can cause immediate “stat dirty” states. If you can cheaply obtain stat info (or reuse commit time for mtime), write those for better UX. Otherwise, keep as-is.
Also applies to: 100-106
142-155: SHA-1 is mandated by Git index trailer; add a clarifying comment/suppression.Static analysis will flag SHA-1, but Git index v2 requires a SHA-1 trailer. Add a short comment or suppression to avoid false positives in SAST.
Apply:
- MessageDigest messageDigest = MessageDigest.getInstance("SHA-1"); + // SHA-1 is required by the Git index format (v2 trailer); not used as a security primitive. + MessageDigest messageDigest = MessageDigest.getInstance("SHA-1");src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryRepositoryBuilder.java (3)
121-123: Verify DirCache construction with null File/FS.
new InMemoryDirCache(null, null)assumes JGit’s DirCache never dereferences these. If JGit changes or a future call path touches them, this could NPE. Consider a dedicated factory (e.g.,InMemoryDirCache.newInCore()) or pass a harmless placeholder FS.I can provide a small
newInCore()factory and update call sites if you want.
153-162: Symlink materialization choice: document portability trade-off.Emitting symlinks as plain files improves Windows portability but loses link semantics on Unix. A short comment in the API/JavaDoc would help set expectations.
109-116: Minor: BEST_COMPRESSION may be CPU heavy; consider DEFAULT.Re-using pack compression inside a ZIP yields diminishing returns. DEFAULT compression often strikes a better CPU/size balance.
Also applies to: 174-181
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryDirCache.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryRepositoryBuilder.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
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
Files:
src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryRepositoryBuilder.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryDirCache.java
🧠 Learnings (5)
📚 Learning: 2025-08-27T09:46:36.480Z
Learnt from: ekayandan
PR: ls1intum/Artemis#11027
File: src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java:1839-1839
Timestamp: 2025-08-27T09:46:36.480Z
Learning: In the Artemis codebase, when stubbing GitService.getBareRepository() in tests, it's valid to return a JGit Repository (org.eclipse.jgit.lib.Repository) even though the method signature returns the domain Repository (de.tum.cit.aet.artemis.programming.domain.Repository), because the domain Repository extends JGit's FileRepository, making them polymorphically compatible.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java
📚 Learning: 2025-08-19T09:13:51.235Z
Learnt from: ekayandan
PR: ls1intum/Artemis#11027
File: src/main/java/de/tum/cit/aet/artemis/programming/service/GitArchiveHelper.java:0-0
Timestamp: 2025-08-19T09:13:51.235Z
Learning: The domain Repository class (de.tum.cit.aet.artemis.programming.domain.Repository) extends JGit's FileRepository, making it polymorphically compatible with JGit APIs while providing additional Artemis-specific methods like getParticipation() and getLocalPath().
Applied to files:
src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java
📚 Learning: 2025-08-11T13:21:25.531Z
Learnt from: ekayandan
PR: ls1intum/Artemis#11027
File: src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java:184-186
Timestamp: 2025-08-11T13:21:25.531Z
Learning: The `getBareRepository` method in `GitService` returns a domain `Repository` object (from `de.tum.cit.aet.artemis.programming.domain.Repository`), not a JGit `Repository` (from `org.eclipse.jgit.lib.Repository`), even though its JavaDoc mentions "JGit repository".
Applied to files:
src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java
📚 Learning: 2025-08-11T13:21:25.531Z
Learnt from: ekayandan
PR: ls1intum/Artemis#11027
File: src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java:184-186
Timestamp: 2025-08-11T13:21:25.531Z
Learning: The `getBareRepository` method in `GitService` returns a domain `Repository` object (from `de.tum.cit.aet.artemis.programming.domain.Repository`), not a JGit `Repository` (from `org.eclipse.jgit.lib.Repository`).
Applied to files:
src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java
📚 Learning: 2025-08-11T12:57:51.535Z
Learnt from: ekayandan
PR: ls1intum/Artemis#11027
File: src/main/java/de/tum/cit/aet/artemis/programming/service/GitService.java:1455-1465
Timestamp: 2025-08-11T12:57:51.535Z
Learning: In the Artemis project, when ekayandan responds with "same as above" to a code review suggestion, it means they want to defer the suggested change to a follow-up PR to keep the current PR scope minimal and focused on the core functionality being implemented.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java
🧬 Code graph analysis (2)
src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryRepositoryBuilder.java (2)
src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryDirCache.java (1)
InMemoryDirCache(21-170)src/main/java/de/tum/cit/aet/artemis/programming/domain/Repository.java (1)
Repository(17-84)
src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java (4)
src/main/java/de/tum/cit/aet/artemis/core/util/FileUtil.java (1)
FileUtil(57-932)src/main/java/de/tum/cit/aet/artemis/programming/domain/Repository.java (1)
Repository(17-84)src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryRepositoryBuilder.java (1)
InMemoryRepositoryBuilder(56-368)src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCRepositoryUri.java (1)
LocalVCRepositoryUri(17-318)
🪛 ast-grep (0.38.6)
src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryRepositoryBuilder.java
[warning] 241-241: Detected SHA1 hash algorithm which is considered insecure. SHA1 is not collision resistant and is therefore not suitable as a cryptographic signature. Instead, use PBKDF2 for password hashing or SHA256 or SHA512 for other hash function applications.
Context: MessageDigest.getInstance("SHA-1")
Note: [CWE-328] Use of Weak Hash. [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(use-of-sha1-java)
src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryDirCache.java
[warning] 145-145: Detected SHA1 hash algorithm which is considered insecure. SHA1 is not collision resistant and is therefore not suitable as a cryptographic signature. Instead, use PBKDF2 for password hashing or SHA256 or SHA512 for other hash function applications.
Context: MessageDigest.getInstance("SHA-1")
Note: [CWE-328] Use of Weak Hash. [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(use-of-sha1-java)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: server-tests
- GitHub Check: server-style
- GitHub Check: Analyse
- GitHub Check: bean-instantiation-check
🔇 Additional comments (10)
src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java (7)
56-62: Proper JGit ZIP format registration with error handling.The ArchiveCommand.registerFormat call is correctly placed in the constructor to ensure the ZIP format is available for archive operations. Good error handling prevents the service from failing to initialize if registration fails.
101-107: Good filename sanitization implementation.The method correctly uses FileUtil.sanitizeFilename and handles the .zip extension case-insensitively. This addresses previous review feedback about comprehensive filename sanitization.
142-155: Well-designed InputStreamResource factory method.This helper method properly creates an InputStreamResource with accurate filename and content length information, which is essential for proper HTTP responses.
167-171: Efficient snapshot export implementation.The method correctly uses the domain Repository type (which extends JGit's FileRepository) and delegates to the in-memory archive creation. The approach avoids disk I/O while providing working tree content only.
183-187: Excellent use of InMemoryRepositoryBuilder for full history exports.This method leverages the InMemoryRepositoryBuilder to create a complete Git repository with working tree and .git directory, all in memory. Perfect for instructor repository exports that need full history.
239-240: Appropriate export strategy for student repositories.Using snapshot export for student repositories is a good privacy decision - it excludes the .git directory and provides only the working tree content, which is what students typically need.
272-284: Robust archive creation with proper error handling.The method correctly handles the case where HEAD cannot be resolved (returning empty byte array) and uses JGit's ArchiveCommand for efficient ZIP creation.
src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryDirCache.java (1)
116-129: Fix index v2 long-name semantics (0xFFF) and padding.For names >= 0xFFF bytes the NUL terminator must be omitted, and padding must be computed accordingly. Current code always writes a NUL and pads based on +1, which is incorrect for very long paths.
Apply:
- // flags: [15..12]=stage, [11..0]=name length (or 0xFFF if >= 0xFFF) + // flags: bit15=assume-valid(0), bit14=extended(0), bits13..12=stage, bits11..0=name length (0xFFF if >= 0xFFF) int stage = entry.getStage() & 0x3; byte[] pathBytes = entry.getPathString().getBytes(StandardCharsets.UTF_8); - int nameLength = Math.min(pathBytes.length, 0xFFF); - int flags = (stage << 12) | (nameLength == 0xFFF ? 0x0FFF : nameLength); + boolean longName = pathBytes.length >= 0xFFF; + int nameLength = longName ? 0x0FFF : pathBytes.length; + int flags = (stage << 12) | nameLength; buffer.write((flags >>> 8) & 0xFF); buffer.write(flags & 0xFF); buffer.write(pathBytes); - buffer.write(0); // NUL - // The entry (from ctime sec to NUL) must make the total entry size a multiple of 8 - int entryLength = 62 /* fixed */ + pathBytes.length + 1; // 62 = 10*4 + 20 + 2 + if (!longName) { + buffer.write(0); // NUL only for short names + } + // Pad entry to 8-byte boundary (from ctime sec up to end-of-name (and optional NUL)) + int entryLength = 62 /* fixed */ + pathBytes.length + (longName ? 0 : 1); // 62 = 10*4 + 20 + 2 int padding = (8 - (entryLength % 8)) % 8; - IntStream.range(0, padding).map(i -> 0).forEach(buffer::write); + for (int i = 0; i < padding; i++) { + buffer.write(0); + }Likely an incorrect or invalid review comment.
src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryRepositoryBuilder.java (2)
134-143: Index finalize/write is correct; nice separation.The build-flow adds entries, calls
finish(), then serializes once. Looks good.Also applies to: 164-170
274-305: Directory entry helpers look solid.Idempotent mkdir + parent-dir materialization with normalization and POSIX modes is neat and robust.
Also applies to: 315-333, 345-352
src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryRepositoryBuilder.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryRepositoryBuilder.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryRepositoryBuilder.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/de/tum/cit/aet/artemis/programming/web/repository/RepositoryResource.java (1)
281-285: Avoid hard dependency on LocalVC; commit fails on non-LocalVC profilesUsing
localVCServletService.orElseThrow()will throw when LocalVC is not active (e.g., Jenkins/GitLab CI), causing 500s on commit. Previously, aProfileService.isLocalCIActive()guard guaranteed bean presence; that protection is gone after removing ProfileService. Call LocalVC only if present.Apply:
- localVCServletService.orElseThrow().processNewPush(null, repository, user, Optional.empty(), Optional.empty(), vcsAccessLog); + // Only available in LocalVC/LocalCI. Remote CI (e.g., Jenkins) is triggered via VCS webhooks. + if (localVCServletService.isPresent()) { + localVCServletService.get().processNewPush(null, repository, user, Optional.empty(), Optional.empty(), vcsAccessLog); + }
🧹 Nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/programming/web/repository/AuxiliaryRepositoryResource.java (1)
197-199: Wrong repository label passed to access check ("test" vs "auxiliary")The human-readable repo type is used for error/log messages. Passing "test" here is misleading.
Apply:
- repositoryAccessService.checkAccessTestOrAuxRepositoryElseThrow(true, exercise, userRepository.getUserWithGroupsAndAuthorities(principal.getName()), "test"); + repositoryAccessService.checkAccessTestOrAuxRepositoryElseThrow(true, exercise, userRepository.getUserWithGroupsAndAuthorities(principal.getName()), "auxiliary");src/main/java/de/tum/cit/aet/artemis/programming/web/repository/RepositoryProgrammingExerciseParticipationResource.java (1)
208-216: JavaDoc route and access-level mismatch with implementationComment says
GET /repository/{participationId}/files/{commitId}and “enforces at least instructor”, but the mapping isrepository-files-content/{commitId}with@EnforceAtLeastStudent.Apply:
- /** - * GET /repository/{participationId}/files/{commitId} : Gets the files of the repository with the given participationId at the given commitId. - * This enforces at least instructor access rights. + /** + * GET /repository-files-content/{commitId} : Gets the files of the repository at the given commitId. + * Requires at least student access (checked at runtime against repository ownership/permissions).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/de/tum/cit/aet/artemis/programming/web/repository/AuxiliaryRepositoryResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/repository/RepositoryProgrammingExerciseParticipationResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/repository/RepositoryResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/repository/TestRepositoryResource.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
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
Files:
src/main/java/de/tum/cit/aet/artemis/programming/web/repository/AuxiliaryRepositoryResource.javasrc/main/java/de/tum/cit/aet/artemis/programming/web/repository/RepositoryResource.javasrc/main/java/de/tum/cit/aet/artemis/programming/web/repository/TestRepositoryResource.javasrc/main/java/de/tum/cit/aet/artemis/programming/web/repository/RepositoryProgrammingExerciseParticipationResource.java
🧠 Learnings (5)
📚 Learning: 2025-06-15T13:49:43.096Z
Learnt from: ahmetsenturk
PR: ls1intum/Artemis#10916
File: src/main/java/de/tum/cit/aet/artemis/atlas/web/LearnerProfileResource.java:36-41
Timestamp: 2025-06-15T13:49:43.096Z
Learning: In the Artemis codebase, the development team has decided to allow direct injection of repositories into REST resources rather than always delegating to service layers. This architectural decision was communicated and decided with peer developers.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/programming/web/repository/AuxiliaryRepositoryResource.javasrc/main/java/de/tum/cit/aet/artemis/programming/web/repository/RepositoryResource.javasrc/main/java/de/tum/cit/aet/artemis/programming/web/repository/TestRepositoryResource.javasrc/main/java/de/tum/cit/aet/artemis/programming/web/repository/RepositoryProgrammingExerciseParticipationResource.java
📚 Learning: 2025-08-08T08:56:49.931Z
Learnt from: tobias-lippert
PR: ls1intum/Artemis#11248
File: src/main/java/de/tum/cit/aet/artemis/programming/web/repository/RepositoryResource.java:288-291
Timestamp: 2025-08-08T08:56:49.931Z
Learning: In the Artemis codebase, the LocalCI profile is only active when LocalVC is active (LocalCI ⇒ LocalVC). Therefore, in RepositoryResource.commitChanges, guarding the LocalVCServletService invocation with profileService.isLocalCIActive() is correct and guarantees the bean’s presence.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/programming/web/repository/RepositoryResource.java
📚 Learning: 2025-08-19T09:13:51.235Z
Learnt from: ekayandan
PR: ls1intum/Artemis#11027
File: src/main/java/de/tum/cit/aet/artemis/programming/service/GitArchiveHelper.java:0-0
Timestamp: 2025-08-19T09:13:51.235Z
Learning: The domain Repository class (de.tum.cit.aet.artemis.programming.domain.Repository) extends JGit's FileRepository, making it polymorphically compatible with JGit APIs while providing additional Artemis-specific methods like getParticipation() and getLocalPath().
Applied to files:
src/main/java/de/tum/cit/aet/artemis/programming/web/repository/RepositoryResource.java
📚 Learning: 2025-04-10T20:38:13.424Z
Learnt from: florian-glombik
PR: ls1intum/Artemis#10656
File: src/main/java/de/tum/cit/aet/artemis/core/repository/webauthn/ArtemisUserCredentialRepository.java:114-137
Timestamp: 2025-04-10T20:38:13.424Z
Learning: In the ArtemisUserCredentialRepository class, the findByUserId method intentionally returns an empty list rather than throwing an exception when not fully implemented, as throwing an exception would cause authentication to fail.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/programming/web/repository/RepositoryResource.java
📚 Learning: 2024-06-10T19:44:09.116Z
Learnt from: pzdr7
PR: ls1intum/Artemis#8703
File: src/main/java/de/tum/in/www1/artemis/web/rest/QuizParticipationResource.java:39-39
Timestamp: 2024-06-10T19:44:09.116Z
Learning: The `Profile(PROFILE_CORE)` annotation in `QuizParticipationResource.java` is used to ensure that all nodes, not just those with the scheduling profile, can handle quiz participations.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/programming/web/repository/RepositoryProgrammingExerciseParticipationResource.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: client-style
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: server-style
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: bean-instantiation-check
- GitHub Check: Analyse
🔇 Additional comments (4)
src/main/java/de/tum/cit/aet/artemis/programming/web/repository/RepositoryResource.java (1)
73-74: Constructor DI refactor — LGTMRemoving ProfileService from the constructor and standardizing DI matches the current pattern used across resources.
src/main/java/de/tum/cit/aet/artemis/programming/web/repository/TestRepositoryResource.java (1)
59-62: Constructor signature update — LGTMConstructor/super call now align with the new base class signature without ProfileService.
src/main/java/de/tum/cit/aet/artemis/programming/web/repository/AuxiliaryRepositoryResource.java (1)
63-67: Constructor signature update — LGTMMatches the base class change and keeps wiring consistent.
src/main/java/de/tum/cit/aet/artemis/programming/web/repository/RepositoryProgrammingExerciseParticipationResource.java (1)
87-94: Constructor changes — LGTMDI updates (dropping ProfileService, adjusting super call) are consistent with the refactor.
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
- small typo fix
…ntum/Artemis into chore/in-memory-repo-export-artemis
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryRepositoryBuilder.java (4)
153-162: Symlink handling: confirm policy or encode as real symlinkYou currently materialize symlinks as plain files. If symlinks are disallowed in Artemis anyway, skip them; otherwise encode as symlink (Unix mode 0120000) so Git restores correctly.
Would you like me to switch this to real symlink entries, or skip entirely?
#!/bin/bash # Verify repo-wide symlink policy/usages rg -n --type java -C2 'symlink|core\.symlinks|reject.*symlink|FileMode\.SYMLINK'
79-87: Close InMemoryRepository via try-with-resources to prevent leaks
InMemoryRepositoryholds buffers/readers; currently never closed.Apply:
- InMemoryRepository repo = new InMemoryRepository.Builder().setRepositoryDescription(new DfsRepositoryDescription("inmem")).setFS(FS.DETECTED).build(); + try (InMemoryRepository repo = new InMemoryRepository.Builder() + .setRepositoryDescription(new DfsRepositoryDescription("inmem")) + .setFS(FS.DETECTED) + .build()) { ... - return outputStream.toByteArray(); + } + return outputStream.toByteArray();Also applies to: 181-184
90-92: Add network timeout on Transport.fetch to avoid hanging exportsUnbounded fetch can hang threads on IO glitches.
- try (Transport transport = Transport.open(repo, remoteUri.toString())) { + try (Transport transport = Transport.open(repo, remoteUri.toString())) { + transport.setTimeout(60); // seconds transport.fetch(NullProgressMonitor.INSTANCE, List.of(new RefSpec("+refs/heads/*:refs/remotes/origin/*"))); }
239-253: Use JGit’s canonical pack name; drop manual SHA-1 digestPack name must match JGit’s canonical name; computing SHA-1 yourself risks mismatches and triggers crypto lint noise.
- // Write .pack while computing its SHA-1 (Git pack filename = sha1 of content) - MessageDigest sha1; - try { - sha1 = MessageDigest.getInstance("SHA-1"); - } - catch (NoSuchAlgorithmException e) { - throw new RuntimeException(e); - } - ByteArrayOutputStream packOut = new ByteArrayOutputStream(); - try (DigestOutputStream digestOut = new DigestOutputStream(packOut, sha1)) { - packWriter.writePack(NullProgressMonitor.INSTANCE, NullProgressMonitor.INSTANCE, digestOut); - } - packBytes = packOut.toByteArray(); - packHashHex = toHex(sha1.digest()); + // Write .pack and use JGit's computed name + ByteArrayOutputStream packOut = new ByteArrayOutputStream(); + packWriter.writePack(NullProgressMonitor.INSTANCE, NullProgressMonitor.INSTANCE, packOut); + packBytes = packOut.toByteArray(); + packHashHex = packWriter.computeName().name();Also applies to: 255-258, 260-263
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseRepositoryExportResource.java (1)
118-133: IDOR: ensure auxiliary repository belongs to the exerciseCurrently any auxiliary repository id can be exported if the caller has TA rights on the provided exercise id. Validate association to prevent cross-exercise data leakage.
- AuxiliaryRepository auxiliaryRepository = auxiliaryRepositoryRepository.findByIdElseThrow(repositoryId); + AuxiliaryRepository auxiliaryRepository = auxiliaryRepositoryRepository.findByIdElseThrow(repositoryId); + if (auxiliaryRepository.getProgrammingExercise() == null + || !auxiliaryRepository.getProgrammingExercise().getId().equals(programmingExercise.getId())) { + return ResponseEntity.notFound() + .headers(HeaderUtil.createFailureAlert(applicationName, true, ENTITY_NAME, "auxiliaryRepositoryNotFound", + "Auxiliary repository " + repositoryId + " was not found for this exercise.")) + .build(); + }
🧹 Nitpick comments (5)
src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryRepositoryBuilder.java (2)
172-173: Rename createGitIndex(...) → writePackedObjects(...) for clarityThe method writes pack and idx, not the Git index; name is misleading.
- private static void createGitIndex(InMemoryRepository repo, ObjectId commitId, ZipArchiveOutputStream zipOutputStream, Set<String> createdDirs) throws IOException { + private static void writePackedObjects(InMemoryRepository repo, ObjectId commitId, ZipArchiveOutputStream zipOutputStream, Set<String> createdDirs) throws IOException {And at call site:
- createGitIndex(repo, commitId, zipOutputStream, createdDirs); + writePackedObjects(repo, commitId, zipOutputStream, createdDirs);Also applies to: 225-226
109-111: Reproducibility: pin ZIP timestampsTo get deterministic ZIPs across runs, set a stable timestamp (e.g., commit time or epoch) on entries (files and dirs).
Example:
- ZipArchiveEntry zipEntry = new ZipArchiveEntry(path.replace('\\', '/')); + ZipArchiveEntry zipEntry = new ZipArchiveEntry(path.replace('\\', '/')); + zipEntry.setTime(0L); // or commit.getCommitTime()*1000LApply similarly in
mkdir(...).Also applies to: 324-329
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseRepositoryExportResource.java (3)
18-19: Add missing imports for Content-Disposition headerimport org.springframework.http.MediaType; import org.springframework.http.ResponseEntity; +import org.springframework.http.HttpHeaders; +import org.springframework.http.ContentDisposition;
189-191: Use consistent duration logging helperElsewhere you use
formatDurationFrom(start); keep it consistent here.- log.info("Successfully exported repository for programming exercise {} with title {} in {} ms", programmingExercise.getId(), programmingExercise.getTitle(), - (System.nanoTime() - start) / 1000000); + log.info("Successfully exported repository for programming exercise {} with title '{}' in {}.", + programmingExercise.getId(), programmingExercise.getTitle(), formatDurationFrom(start));
87-104: Add preflight size/guardrails to avoid OOM on in-memory exportsGiven fully in-memory zips, add a conservative size check or circuit breaker (e.g., estimate pack/archive size, cap at configured MB, reject with 413) to protect the node.
Happy to draft a small helper in
GitRepositoryExportServiceto estimate size and enforce a configurable limit (e.g.,artemis.exports.max-bytes).Also applies to: 114-143, 153-200, 210-236
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryRepositoryBuilder.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseRepositoryExportResource.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
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
Files:
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseRepositoryExportResource.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryRepositoryBuilder.java
🧠 Learnings (1)
📚 Learning: 2024-11-26T20:43:17.588Z
Learnt from: magaupp
PR: ls1intum/Artemis#9751
File: src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseFactory.java:143-148
Timestamp: 2024-11-26T20:43:17.588Z
Learning: In `src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseFactory.java`, the test package name assigned in `populateUnreleasedProgrammingExercise` does not need to adhere to naming conventions.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseRepositoryExportResource.java
🧬 Code graph analysis (1)
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseRepositoryExportResource.java (6)
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
Constants(8-516)src/main/java/de/tum/cit/aet/artemis/core/util/TimeLogUtil.java (1)
TimeLogUtil(5-52)src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java (1)
Profile(41-285)src/main/java/de/tum/cit/aet/artemis/core/util/FileUtil.java (1)
FileUtil(57-932)src/main/java/de/tum/cit/aet/artemis/exam/config/ExamApiNotPresentException.java (1)
ExamApiNotPresentException(11-19)src/main/java/de/tum/cit/aet/artemis/programming/domain/VcsRepositoryUri.java (1)
VcsRepositoryUri(11-180)
🪛 ast-grep (0.38.6)
src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryRepositoryBuilder.java
[warning] 241-241: Detected SHA1 hash algorithm which is considered insecure. SHA1 is not collision resistant and is therefore not suitable as a cryptographic signature. Instead, use PBKDF2 for password hashing or SHA256 or SHA512 for other hash function applications.
Context: MessageDigest.getInstance("SHA-1")
Note: [CWE-328] Use of Weak Hash. [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(use-of-sha1-java)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: client-style
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: server-style
- GitHub Check: bean-instantiation-check
- GitHub Check: Analyse
src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryRepositoryBuilder.java
Show resolved
Hide resolved
...java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseRepositoryExportResource.java
Show resolved
Hide resolved
ekayandan
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||||||||
ekayandan
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
- improved code quality - removed unreachable statements
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryDirCache.java (1)
48-138: Consider extracting complex sorting and entry writing logic into helper methods.The
writeTomethod handles multiple responsibilities: sorting entries, writing header, processing each entry's fields, and buffering content. This could benefit from decomposition for better maintainability.
🧹 Nitpick comments (3)
src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryDirCache.java (2)
62-73: Optimize path comparison with direct byte array comparison.The current sorting implementation converts paths to UTF-8 bytes on every comparison, which is inefficient for large repositories. Consider caching the byte arrays or using a more efficient comparison.
- Arrays.sort(entries, (cacheEntryA, cacheEntryB) -> { - byte[] pathABytes = cacheEntryA.getPathString().getBytes(StandardCharsets.UTF_8); - byte[] pathBBytes = cacheEntryB.getPathString().getBytes(StandardCharsets.UTF_8); - int minLength = Math.min(pathABytes.length, pathBBytes.length); - for (int i = 0; i < minLength; i++) { - int diff = (pathABytes[i] & 0xff) - (pathBBytes[i] & 0xff); - if (diff != 0) { - return diff; - } - } - return pathABytes.length - pathBBytes.length; - }); + // Cache path bytes to avoid repeated UTF-8 conversion + Arrays.sort(entries, (cacheEntryA, cacheEntryB) -> { + return cacheEntryA.getPathString().compareTo(cacheEntryB.getPathString()); + });Alternatively, if byte-level sorting is required for Git compatibility, cache the byte arrays in a map before sorting.
134-134: Replace functional stream with simple loop for padding.The padding logic uses an unnecessarily complex functional approach that's harder to read.
- IntStream.range(0, padding).map(i -> 0).forEach(buffer::write); + for (int i = 0; i < padding; i++) { + buffer.write(0); + }src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java (1)
286-298: Add null check for empty repositories to prevent potential issues.While the method handles the case where HEAD cannot be resolved, it should also verify that the repository is not empty before attempting archive operations.
public byte[] createInMemoryZipArchive(Repository repository) throws GitAPIException, IOException { ObjectId treeId = repository.resolve("HEAD"); if (treeId == null) { log.debug("Could not resolve tree for HEAD"); return new byte[0]; } + // Verify the tree exists and is not empty + try (var objectReader = repository.newObjectReader()) { + if (!objectReader.has(treeId)) { + log.debug("Tree object does not exist for HEAD"); + return new byte[0]; + } + } + ByteArrayOutputStream archiveData = new ByteArrayOutputStream(); try (Git git = new Git(repository)) { git.archive().setFormat("zip").setTree(treeId).setOutputStream(archiveData).call(); } return archiveData.toByteArray(); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/de/tum/cit/aet/artemis/core/service/ZipStreamHelper.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryDirCache.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryRepositoryBuilder.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseRepositoryExportResource.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryRepositoryBuilder.java
- src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseRepositoryExportResource.java
- src/main/java/de/tum/cit/aet/artemis/core/service/ZipStreamHelper.java
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
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
Files:
src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryDirCache.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java
🧠 Learnings (5)
📚 Learning: 2025-08-27T09:46:36.480Z
Learnt from: ekayandan
PR: ls1intum/Artemis#11027
File: src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java:1839-1839
Timestamp: 2025-08-27T09:46:36.480Z
Learning: In the Artemis codebase, when stubbing GitService.getBareRepository() in tests, it's valid to return a JGit Repository (org.eclipse.jgit.lib.Repository) even though the method signature returns the domain Repository (de.tum.cit.aet.artemis.programming.domain.Repository), because the domain Repository extends JGit's FileRepository, making them polymorphically compatible.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java
📚 Learning: 2025-08-19T09:13:51.235Z
Learnt from: ekayandan
PR: ls1intum/Artemis#11027
File: src/main/java/de/tum/cit/aet/artemis/programming/service/GitArchiveHelper.java:0-0
Timestamp: 2025-08-19T09:13:51.235Z
Learning: The domain Repository class (de.tum.cit.aet.artemis.programming.domain.Repository) extends JGit's FileRepository, making it polymorphically compatible with JGit APIs while providing additional Artemis-specific methods like getParticipation() and getLocalPath().
Applied to files:
src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java
📚 Learning: 2025-08-11T13:21:25.531Z
Learnt from: ekayandan
PR: ls1intum/Artemis#11027
File: src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java:184-186
Timestamp: 2025-08-11T13:21:25.531Z
Learning: The `getBareRepository` method in `GitService` returns a domain `Repository` object (from `de.tum.cit.aet.artemis.programming.domain.Repository`), not a JGit `Repository` (from `org.eclipse.jgit.lib.Repository`), even though its JavaDoc mentions "JGit repository".
Applied to files:
src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java
📚 Learning: 2025-08-11T13:21:25.531Z
Learnt from: ekayandan
PR: ls1intum/Artemis#11027
File: src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java:184-186
Timestamp: 2025-08-11T13:21:25.531Z
Learning: The `getBareRepository` method in `GitService` returns a domain `Repository` object (from `de.tum.cit.aet.artemis.programming.domain.Repository`), not a JGit `Repository` (from `org.eclipse.jgit.lib.Repository`).
Applied to files:
src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java
📚 Learning: 2025-08-11T12:57:51.535Z
Learnt from: ekayandan
PR: ls1intum/Artemis#11027
File: src/main/java/de/tum/cit/aet/artemis/programming/service/GitService.java:1455-1465
Timestamp: 2025-08-11T12:57:51.535Z
Learning: In the Artemis project, when ekayandan responds with "same as above" to a code review suggestion, it means they want to defer the suggested change to a follow-up PR to keep the current PR scope minimal and focused on the core functionality being implemented.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java
🧬 Code graph analysis (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java (3)
src/main/java/de/tum/cit/aet/artemis/core/util/FileUtil.java (1)
FileUtil(57-932)src/main/java/de/tum/cit/aet/artemis/programming/domain/VcsRepositoryUri.java (1)
VcsRepositoryUri(11-180)src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryRepositoryBuilder.java (1)
InMemoryRepositoryBuilder(53-363)
🪛 ast-grep (0.38.6)
src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryDirCache.java
[warning] 150-150: Detected SHA1 hash algorithm which is considered insecure. SHA1 is not collision resistant and is therefore not suitable as a cryptographic signature. Instead, use PBKDF2 for password hashing or SHA256 or SHA512 for other hash function applications.
Context: MessageDigest.getInstance("SHA-1")
Note: [CWE-328] Use of Weak Hash. [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(use-of-sha1-java)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Analyse
🔇 Additional comments (8)
src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryDirCache.java (2)
26-46: LGTM! Solid foundation for in-memory Git index representation.The class extends DirCache appropriately and the constructor follows JGit conventions. The constants for file modes are correctly defined.
150-151: SHA-1 usage is appropriate for Git index format compatibility.While static analysis flags SHA-1 as insecure, this is a false positive. The Git index format specification requires SHA-1 checksums, making this usage necessary for compatibility rather than cryptographic security.
src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java (6)
55-76: LGTM! Proper service configuration and JGit archive format registration.The Spring annotations are correct for the CORE profile, and the constructor properly registers the ZIP format for JGit's ArchiveCommand with appropriate error handling.
115-121: LGTM! Consistent filename sanitization implementation.The private helper method provides consistent sanitization logic and follows the established pattern from past review feedback.
156-169: LGTM! Clean InputStreamResource wrapper with proper metadata.The anonymous class extension properly provides filename and content length metadata, which is essential for HTTP responses.
181-185: LGTM! Proper repository snapshot export implementation.The method correctly delegates to the appropriate services and uses the domain Repository type as confirmed in retrieved learnings.
197-201: LGTM! Consistent full-history export using InMemoryRepositoryBuilder.The method properly integrates with the new InMemoryRepositoryBuilder for comprehensive repository exports including Git history.
236-263: LGTM! Comprehensive student repository export with proper error handling.The method includes proper null checks, error logging, and uses snapshot export (without .git directory) for student privacy as intended in the PR objectives.
src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java
Outdated
Show resolved
Hide resolved
…positoryExportService.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java (8)
69-75: Guard ArchiveCommand format registration against duplicatesRegistering the ZIP format on each bean construction can throw on duplicates and pollute logs. Treat duplicate registration as benign.
- try { - ArchiveCommand.registerFormat("zip", new ZipFormat()); - } - catch (Exception e) { - log.error("Could not register zip format", e); - } + try { + ArchiveCommand.registerFormat("zip", new ZipFormat()); + } + catch (IllegalArgumentException alreadyRegistered) { + log.debug("JGit zip format already registered"); + } + catch (Exception e) { + log.warn("Could not register JGit zip format", e); + }
186-195: Fix JavaDoc: not using ArchiveCommand for full-history exportThis method relies on InMemoryRepositoryBuilder, not ArchiveCommand; update docs to avoid misleading maintenance.
- /** - * Exports a repository with full history including the .git directory directly to memory. - * This method uses JGit's ArchiveCommand to create a zip of the working tree and combines it - * with the .git directory for full history, all done in memory without disk checkout. - * + /** + * Exports a repository with full history (including a synthetic .git directory) directly to memory. + * This uses InMemoryRepositoryBuilder to materialize a full, usable Git working copy without disk I/O. + *
203-212: JavaDoc claims “or null if export failed” but method throws on failureThe method does not return null; it propagates IOException. Adjust the JavaDoc for correctness.
- * @return an InputStreamResource containing the zipped repository, or null if export failed + * @return an InputStreamResource containing the zipped repository + * @throws IOException if the export fails
214-225: JavaDoc: remove “or null if export failed” (method throws)- * @return an InputStreamResource containing the zipped repository, or null if export failed + * @return an InputStreamResource containing the zipped repository + * @throws IOException if the export fails
122-131: Doc example: use a segment-aware .git exclusionThe suggested
path.toString().contains(".git")can over/under-match. Provide a segment-aware example.- * // Exclude .git directory - * Predicate<Path> excludeGit = path -> !path.toString().contains(".git"); + * // Exclude any path that is inside a .git directory (segment-aware) + * Predicate<Path> excludeGit = path -> { + * for (Path seg : path) { + * if (".git".equals(seg.toString())) { + * return false; + * } + * } + * return true; + * };
155-166: Sanitize response filename before returning itEven though call sites sanitize, defense-in-depth: sanitize here to avoid accidental unsafe filenames leaking to Content-Disposition.
- private InputStreamResource createZipInputStreamResource(byte[] zipData, String filename) { + private InputStreamResource createZipInputStreamResource(byte[] zipData, String filename) { + final String safeBase = FileUtil.sanitizeFilename(filename); return new InputStreamResource(new ByteArrayInputStream(zipData)) { @Override public String getFilename() { - return filename + ".zip"; + return safeBase + ".zip"; } @Override public long contentLength() { return zipData.length; } }; }
235-254: Optional: enforce an upper bound on in-memory export sizeTo prevent OOM on very large repos, consider rejecting oversized responses early. The PR discussion mentions a deferred size check; wiring it here is straightforward since we know the byte length via
contentLength().@@ - // For student repositories, we use snapshot export to exclude .git directory for privacy - return exportRepositorySnapshot(participation.getVcsRepositoryUri(), repoName); + // For student repositories, we use snapshot export to exclude .git directory for privacy + var resource = exportRepositorySnapshot(participation.getVcsRepositoryUri(), repoName); + // Optional guardrail: reject overly large exports (value configurable) + long maxBytes = /* inject via config, e.g., ExportProperties.getMaxZipBytes() */ 0L; + if (maxBytes > 0 && resource.contentLength() > maxBytes) { + String msg = "Export exceeds max size (" + resource.contentLength() + " > " + maxBytes + " bytes) for participation " + participation.getId(); + log.warn(msg); + exportErrors.add(msg); + return null; + } + return resource;If you prefer to keep this out of service, implement the check in the REST resource and return HTTP 413.
87-112: Folder exports named “...-student-submission.git” may be confusingWhen
zipOutput == false, creating a directory that ends with “.git” is surprising (folders with “.git” often indicate Git internals). Consider appending the “.git” suffix only for archive names, not directory copies.@@ - if (hideStudentName) { - sanitizedRepoName += "-student-submission.git"; - } + if (hideStudentName) { + sanitizedRepoName += zipOutput ? "-student-submission.git" : "-student-submission"; + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
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
Files:
src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java
🧠 Learnings (5)
📚 Learning: 2025-08-27T09:46:36.480Z
Learnt from: ekayandan
PR: ls1intum/Artemis#11027
File: src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java:1839-1839
Timestamp: 2025-08-27T09:46:36.480Z
Learning: In the Artemis codebase, when stubbing GitService.getBareRepository() in tests, it's valid to return a JGit Repository (org.eclipse.jgit.lib.Repository) even though the method signature returns the domain Repository (de.tum.cit.aet.artemis.programming.domain.Repository), because the domain Repository extends JGit's FileRepository, making them polymorphically compatible.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java
📚 Learning: 2025-08-19T09:13:51.235Z
Learnt from: ekayandan
PR: ls1intum/Artemis#11027
File: src/main/java/de/tum/cit/aet/artemis/programming/service/GitArchiveHelper.java:0-0
Timestamp: 2025-08-19T09:13:51.235Z
Learning: The domain Repository class (de.tum.cit.aet.artemis.programming.domain.Repository) extends JGit's FileRepository, making it polymorphically compatible with JGit APIs while providing additional Artemis-specific methods like getParticipation() and getLocalPath().
Applied to files:
src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java
📚 Learning: 2025-08-11T13:21:25.531Z
Learnt from: ekayandan
PR: ls1intum/Artemis#11027
File: src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java:184-186
Timestamp: 2025-08-11T13:21:25.531Z
Learning: The `getBareRepository` method in `GitService` returns a domain `Repository` object (from `de.tum.cit.aet.artemis.programming.domain.Repository`), not a JGit `Repository` (from `org.eclipse.jgit.lib.Repository`), even though its JavaDoc mentions "JGit repository".
Applied to files:
src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java
📚 Learning: 2025-08-11T13:21:25.531Z
Learnt from: ekayandan
PR: ls1intum/Artemis#11027
File: src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java:184-186
Timestamp: 2025-08-11T13:21:25.531Z
Learning: The `getBareRepository` method in `GitService` returns a domain `Repository` object (from `de.tum.cit.aet.artemis.programming.domain.Repository`), not a JGit `Repository` (from `org.eclipse.jgit.lib.Repository`).
Applied to files:
src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java
📚 Learning: 2025-08-11T12:57:51.535Z
Learnt from: ekayandan
PR: ls1intum/Artemis#11027
File: src/main/java/de/tum/cit/aet/artemis/programming/service/GitService.java:1455-1465
Timestamp: 2025-08-11T12:57:51.535Z
Learning: In the Artemis project, when ekayandan responds with "same as above" to a code review suggestion, it means they want to defer the suggested change to a follow-up PR to keep the current PR scope minimal and focused on the core functionality being implemented.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java
🧬 Code graph analysis (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java (4)
src/main/java/de/tum/cit/aet/artemis/core/util/FileUtil.java (1)
FileUtil(57-932)src/main/java/de/tum/cit/aet/artemis/programming/domain/Repository.java (1)
Repository(17-84)src/main/java/de/tum/cit/aet/artemis/programming/service/git/InMemoryRepositoryBuilder.java (1)
InMemoryRepositoryBuilder(53-363)src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCRepositoryUri.java (1)
LocalVCRepositoryUri(17-318)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Analyse
- GitHub Check: bean-instantiation-check
🔇 Additional comments (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java (1)
180-200: Type usage between domain Repository and JGit is correctAcknowledging retrieved learnings: domain
Repositoryextends JGit’s FileRepository, so using it withGitandresolveis valid here. No action needed.
src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java
Show resolved
Hide resolved
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
github-project-automation
bot
moved this from Ready For Review
to Merged
in Artemis Development
Checklist
General
Server
Motivation and Context
This branch introduces in-memory repository exports to avoid creating temporary files on disk. This increases export performance and prevents possible i/o errors.
Description
Affected Endpoints
ProgrammingExerciseExportImportResource.java now exposes these routes using completely in-memory methods:
Instructor repository export – repositoryWithFullHistory
@GetMapping("programming-exercises/{exerciseId}/export-instructor-repository/{repositoryType}")
Instructor auxiliary repository export - repositoryWithFullHistory
@GetMapping("programming-exercises/{exerciseId}/export-instructor-auxiliary-repository/{repositoryId}")
Student-requested repository export - repositorySnapshot
@GetMapping("programming-exercises/{exerciseId}/export-student-requested-repository")
Student repository export - repositorySnapshot
@GetMapping("programming-exercises/{exerciseId}/export-student-repository/{participationId}")
Steps for Testing
Prerequisites:
For testing repositoryWithFullHistory endpoints(Instructor repository export, Instructor auxiliary repository export):
For testing Student-requested repository (exportRepositorySnapshot):
For testing Student repository (exportRepositorySnapshot):
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
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
Chores