Development: Modernize Java server code
#10221
Conversation
github-actions
bot
added
tests
server
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 PMD (7.8.0)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerService.java[ERROR] Error at ruleset.xml:58:5 59| 67| 72| 76| 79| 80| 82| 83| 84| 85| 86| 87| 88| 91| 92| 107| 120| 125| 135| 138| 185| src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIIntegrationTest.java[ERROR] Error at ruleset.xml:58:5 59| 67| 72| 76| 79| 80| 82| 83| 84| 85| 86| 87| 88| 91| 92| 107| 120| 125| 135| 138| 185| src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java[ERROR] Error at ruleset.xml:58:5 59| 67| 72| 76| 79| 80| 82| 83| 84| 85| 86| 87| 88| 91| 92| 107| 120| 125| 135| 138| 185| WalkthroughThis pull request introduces a comprehensive modernization of file handling across multiple Java classes in the Artemis project. The changes primarily involve transitioning from traditional Changes
Suggested labels
Suggested reviewers
Possibly related PRs
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: 6
🔭 Outside diff range comments (3)
src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCServletService.java (2)
Line range hint
48-50: Address the TODO comment about class renaming.The TODO comment suggests that the class name doesn't accurately reflect its responsibilities, as it handles both HTTPS and SSH git operations. Consider splitting the class to better follow the Single Responsibility Principle.
Consider creating separate services:
GitAuthenticationService: For authentication logicGitRepositoryService: For repository managementGitOperationService: For git operation handling
Line range hint
156-156: Improve repository caching mechanism.The current implementation using a simple HashMap for repository caching has several potential issues:
- Not thread-safe for concurrent access
- No cache eviction strategy
- Potential resource leaks
Consider using a thread-safe cache implementation:
- private final Map<String, Repository> repositories = new HashMap<>(); + private final Map<String, Repository> repositories = new ConcurrentHashMap<>();Also consider adding:
- Cache eviction strategy using Guava Cache or Caffeine
- Proper repository cleanup in a
@PreDestroymethodsrc/test/java/de/tum/cit/aet/artemis/fileupload/util/ZipFileTestUtilService.java (1)
Line range hint
44-48: Strengthen zip slip protection and improve error message.While the security check for path traversal (zip slip) is present, it could be enhanced:
- Move the check before any file operations
- Provide more detailed error message
- Use Path API consistently
- File destFile = parentFolder.toPath().resolve(currentEntry).toFile(); - - if (!destFile.getCanonicalPath().startsWith(parentFolder.getCanonicalPath())) { - fail("Bad zip entry"); - } + Path destPath = parentFolder.resolve(currentEntry).normalize(); + if (!destPath.startsWith(parentFolder)) { + fail("Zip entry '" + currentEntry + "' is trying to write outside target directory"); + } + File destFile = destPath.toFile();
🧹 Nitpick comments (33)
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseTemplateIntegrationTest.java (1)
142-143: Consider combining path operations for better readability.While the current implementation is correct, we can make it more concise and readable.
- File binFolder = Path.of(path).toFile().getParentFile(); - binFolder = binFolder.toPath().resolve("Home/bin").toFile(); + File binFolder = Path.of(path) + .getParent() + .resolve("Home/bin") + .toFile();src/main/java/de/tum/cit/aet/artemis/core/service/ResourceLoaderService.java (2)
Line range hint
222-228: Improve temporary file handling strategy.The current implementation creates temporary files that are only deleted on JVM exit. Consider using try-with-resources or maintaining a collection of temporary files for explicit cleanup.
Here's a suggested improvement:
else if ("jar".equals(resourceUrl.getProtocol())) { - // Resource is in a jar file. - Path resourcePath = Files.createTempFile(UUID.randomUUID().toString(), ""); - File file = resourcePath.toFile(); - file.deleteOnExit(); - FileUtils.copyInputStreamToFile(resource.getInputStream(), file); - return resourcePath; + // Resource is in a jar file - create a temporary file that will be deleted when the JVM exits + Path tempFile = Files.createTempFile(UUID.randomUUID().toString(), ""); + try { + Files.copy(resource.getInputStream(), tempFile, StandardCopyOption.REPLACE_EXISTING); + tempFile.toFile().deleteOnExit(); + return tempFile; + } catch (IOException e) { + Files.deleteIfExists(tempFile); + throw new IOException("Failed to extract resource from jar: " + path, e); + }
Line range hint
217-229: Enhance error messages for better debugging.The error messages could be more descriptive to help with troubleshooting.
Here's a suggested improvement:
if ("file".equals(resourceUrl.getProtocol())) { - // Resource is in the file system. + // Resource is in the file system - return direct file path return Path.of(resourceUrl.toURI()); } else if ("jar".equals(resourceUrl.getProtocol())) { // Resource is in a jar file. Path resourcePath = Files.createTempFile(UUID.randomUUID().toString(), ""); File file = resourcePath.toFile(); file.deleteOnExit(); FileUtils.copyInputStreamToFile(resource.getInputStream(), file); return resourcePath; } - throw new IllegalArgumentException("Unsupported protocol: " + resourceUrl.getProtocol()); + throw new IllegalArgumentException(String.format( + "Unsupported protocol '%s' for resource '%s'. Only 'file' and 'jar' protocols are supported.", + resourceUrl.getProtocol(), path));src/test/java/de/tum/cit/aet/artemis/communication/linkpreview/LinkPreviewIntegrationTest.java (1)
84-87: LGTM! Consider enhancing test data organization.The modernization to use Path API improves maintainability. The implementation follows test guidelines by using fixed test data and JUnit 5 features.
Consider these improvements:
- Extract test URLs as constants to improve maintainability
- Create an enum or record to pair URLs with their corresponding mock files
- Add comments describing the test scenarios each mock file represents
Example refactor:
+ private static final String GITHUB_PR_6615_URL = "https://github.com/ls1intum/Artemis/pull/6615"; + private static final String GITHUB_PR_6618_URL = "https://github.com/ls1intum/Artemis/pull/6618"; + private static final String GITHUB_HOME_URL = "https://github.com/"; + + private record MockData(String url, String fileName) { + // Test data with PR containing multiple comments + static final MockData GITHUB_PR_6615 = new MockData(GITHUB_PR_6615_URL, "github_pull_request_6615.txt"); + // Test data with PR containing no comments + static final MockData GITHUB_PR_6618 = new MockData(GITHUB_PR_6618_URL, "github_pull_request_6618.txt"); + // Test data for GitHub homepage + static final MockData GITHUB_HOME = new MockData(GITHUB_HOME_URL, "github_home.txt"); + // Test data for site without OG tags + static final MockData GOOGLE = new MockData(GOOGLE_URL, "google.txt"); + } private static Stream<Arguments> provideUrls() { var mockPath = Path.of(MOCK_FILE_PATH_PREFIX); - return Stream.of(Arguments.of("https://github.com/ls1intum/Artemis/pull/6615", mockPath.resolve("github_pull_request_6615.txt").toFile()), - Arguments.of("https://github.com/ls1intum/Artemis/pull/6618", mockPath.resolve("github_pull_request_6618.txt").toFile()), - Arguments.of("https://github.com/", mockPath.resolve("github_home.txt").toFile()), - Arguments.of(GOOGLE_URL, mockPath.resolve("google.txt").toFile())); + return Stream.of(MockData.values()) + .map(data -> Arguments.of(data.url(), mockPath.resolve(data.fileName()).toFile())); }src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCRepositoryUri.java (1)
293-293: Consider extracting the Git suffix to a constant.The modernization to
Path.of()looks good. Consider extracting the ".git" suffix to a constant for better maintainability, as it's used in multiple places throughout the class.+ private static final String GIT_SUFFIX = ".git"; + public Path getRelativeRepositoryPath() { - return Path.of(projectKey, repositorySlug + ".git"); + return Path.of(projectKey, repositorySlug + GIT_SUFFIX); }src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseExportImportResource.java (2)
514-514: Consider adding try-with-resources for automatic resource management.While the modernization to
Files.newInputStreamis good, consider wrapping it in a try-with-resources statement to ensure proper resource cleanup.- InputStreamResource resource = new InputStreamResource(Files.newInputStream(zipFile.toPath())); + var inputStream = Files.newInputStream(zipFile.toPath()); + InputStreamResource resource = new InputStreamResource(inputStream) { + @Override + public void finalize() { + try { + inputStream.close(); + } catch (IOException e) { + log.warn("Failed to close input stream", e); + } + } + };
Line range hint
496-496: Consider addressing the TODO comment.The TODO comment indicates a need to inform users when no participations are found. Consider implementing this user feedback mechanism.
Would you like me to help implement the user feedback mechanism or create an issue to track this enhancement?
src/test/java/de/tum/cit/aet/artemis/programming/util/GitUtilService.java (1)
305-309: Consider fully modernizing the file creation logic.While using
Files.newBufferedWriteris good, consider modernizing the directory creation as well.public void writeEmptyJsonFileToPath(Path path) throws Exception { var fileContent = "{}"; - path.toFile().getParentFile().mkdirs(); + Files.createDirectories(path.getParent()); try (var writer = Files.newBufferedWriter(path, StandardCharsets.UTF_8)) { writer.write(fileContent); } }src/main/java/de/tum/cit/aet/artemis/modeling/service/ModelingSubmissionExportService.java (2)
38-40: Good modernization, but can be improved further.The changes improve resource management by using NIO.2's
Files.newBufferedWriterwith explicit charset. Consider these additional improvements:
- Use
Files.createFile()instead ofFile.createNewFile().- Extract the model casting to reduce duplication.
- Consider updating the method signature to use
Pathinstead ofFile.Here's a suggested refactor:
- protected void saveSubmissionToFile(Exercise exercise, Submission submission, File file) throws IOException { - if (((ModelingSubmission) submission).getModel() == null) { - if (!file.exists()) { - file.createNewFile(); // create empty file if submission is empty - } - } - else { - try (BufferedWriter writer = Files.newBufferedWriter(file.toPath(), StandardCharsets.UTF_8)) { - writer.write(((ModelingSubmission) submission).getModel()); // TODO: save explanation text - } - } + protected void saveSubmissionToFile(Exercise exercise, Submission submission, File file) throws IOException { + var modelingSubmission = (ModelingSubmission) submission; + var path = file.toPath(); + + if (modelingSubmission.getModel() == null) { + if (!Files.exists(path)) { + Files.createFile(path); // create empty file if submission is empty + } + return; + } + + try (BufferedWriter writer = Files.newBufferedWriter(path, StandardCharsets.UTF_8)) { + writer.write(modelingSubmission.getModel()); + } + }
39-39: Address the TODO comment about explanation text.The comment indicates missing functionality for saving explanation text.
Would you like me to help implement the explanation text saving functionality or create an issue to track this task?
src/main/java/de/tum/cit/aet/artemis/core/service/export/CourseExamExportService.java (2)
Line range hint
521-527: Consider enhancing the file writing implementation.While the modernization to use
Files.newBufferedWriteris good, consider these improvements:
- Add file existence check before writing
- Use buffered writing with a larger buffer size for better performance
- Add explicit error handling for specific IO exceptions
private Path writeFile(List<String> data, Path outputDir, String fileName) throws IOException { Path outputFile = outputDir.resolve(fileName); - try (BufferedWriter writer = Files.newBufferedWriter(outputFile, StandardCharsets.UTF_8)) { + if (Files.exists(outputFile)) { + log.warn("File {} already exists and will be overwritten", outputFile); + } + try (BufferedWriter writer = Files.newBufferedWriter(outputFile, StandardCharsets.UTF_8, + StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING)) { + writer.write(String.join(System.lineSeparator(), data)); + writer.write(System.lineSeparator()); for (String line : data) { writer.write(line); writer.write(System.lineSeparator()); } + } catch (IOException e) { + log.error("Failed to write to file {}: {}", outputFile, e.getMessage()); + throw new IOException("Failed to write to file: " + e.getMessage(), e); } return outputFile; }
432-433: Enhance error handling in the default case.While the simplified default case is good, consider improving error handling by:
- Using a dedicated exception type for unsupported exercise types
- Including more context in the error message
- default -> logMessageAndAppendToList( - "Failed to export exercise '" + exercise.getTitle() + "' (id: " + exercise.getId() + "): Exercise type not supported for export", exportErrors, null); + default -> { + var message = String.format("Exercise type %s is not supported for export. Exercise: '%s' (id: %d)", + exercise.getClass().getSimpleName(), exercise.getTitle(), exercise.getId()); + logMessageAndAppendToList(message, exportErrors, + new UnsupportedOperationException("Unsupported exercise type: " + exercise.getClass().getSimpleName())); + }src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCServletService.java (2)
Line range hint
824-825: Improve exception handling in VCS access log methods.Empty catch blocks silently ignore exceptions, which could hide important issues. At minimum, these exceptions should be logged.
- catch (Exception ignored) { + catch (Exception e) { + log.debug("Failed to update VCS access log: {}", e.getMessage()); }
Line range hint
48-914: Consider architectural improvements to better align with coding guidelines.The class currently handles multiple responsibilities including:
- Authentication & Authorization
- Repository Management
- VCS Access Logging
- Git Operations
This violates the Single Responsibility Principle specified in the coding guidelines.
Consider:
- Breaking down the class into smaller, focused services
- Using a facade pattern if these operations need to be coordinated
- Moving the repository caching logic to a dedicated caching service
src/test/java/de/tum/cit/aet/artemis/fileupload/util/ZipFileTestUtilService.java (2)
30-30: Consider using Path API consistently throughout the method.While converting to Path.of() is a good start, converting back to File immediately with toFile() doesn't fully leverage the benefits of the modern Path API. Consider refactoring the method to work with Path objects throughout.
- File file = Path.of(zipFile).toFile(); + Path file = Path.of(zipFile);
Line range hint
24-29: Enhance test utility with better error handling and assertions.As per the test coding guidelines:
- Consider using more specific assertions (assertThat)
- Add validation for input parameters
- Consider adding performance tracking for large zip files
public Path extractZipFileRecursively(String zipFile) throws IOException { + assertThat(zipFile).isNotNull().isNotEmpty(); + assertThat(new File(zipFile)).exists().isFile(); + // Rest of the method... }src/main/java/de/tum/cit/aet/artemis/core/config/WebConfigurer.java (1)
99-99: Enhance path handling for better cross-platform compatibility.While using
Path.of()is a step in the right direction, we can further improve the path handling:Consider this alternative implementation that better utilizes the Path API:
- File root = Path.of(prefixPath + "build/resources/main/static/").toFile(); + Path root = Path.of(prefixPath).resolve("build").resolve("resources").resolve("main").resolve("static"); + File rootFile = root.toFile(); + if (rootFile.exists() && rootFile.isDirectory()) { + servletWebServer.setDocumentRoot(rootFile); + }Benefits:
- Uses
resolve()for more robust path composition- Avoids hard-coded path separators
- More maintainable and clearer path construction
- Better cross-platform compatibility
Note: In the future, consider updating
ConfigurableServletWebServerFactoryto acceptPathinstead ofFileto fully embrace modern Java NIO.2 API.src/main/java/de/tum/cit/aet/artemis/programming/domain/Repository.java (1)
Line range hint
1-93: Consider architectural improvements for better maintainability and consistency.The class shows inconsistent usage of file handling APIs (
FilevsPath) and would benefit from architectural improvements:
- Standardize on
PathAPI throughout the class for consistency- Document thread safety guarantees
- Add null handling documentation
- Consider extracting file validation logic to a separate validator class
Would you like me to provide a detailed refactoring plan to address these architectural concerns?
src/main/java/de/tum/cit/aet/artemis/programming/service/JavaTemplateUpgradeService.java (1)
300-300: Consider using Java NIO for file copy operations.While
FileUtils.copyToFileworks, consider usingFiles.copyfor consistency with other NIO operations in the codebase:- FileUtils.copyToFile(inputStream, repoFile.get().getFile()); + Files.copy(inputStream, repoFile.get().getFile().toPath(), StandardCopyOption.REPLACE_EXISTING);src/main/java/de/tum/cit/aet/artemis/programming/domain/File.java (4)
6-6: Consider renaming the class to avoid overshadowingjava.io.File.
Defining a domain class namedFilein the same package scope asjava.io.Filecan lead to confusion. Renaming this class (e.g.,ArtemisFile) would improve clarity.- public class File { + public class ArtemisFile {
27-29: Returning the underlyingjava.io.Filemay break encapsulation.
Exposing the internalfileobject directly could allow external code to operate on the underlying file without going through the domain class methods. If you need to maintain strict control, consider providing higher-level methods to manipulate files.
41-44: Optimize string replacement for path unification.
Using regex-based replacement might be slower or risk special characters. If only a single character is being replaced,String.replace(char, char)might be more efficient and clearer.- path = path.replaceAll(Pattern.quote(java.io.File.separator), "/"); + path = path.replace(java.io.File.separatorChar, '/');
57-59: Consider usingjava.nio.file.Files.movefor more reliable renaming.
renameTocan exhibit platform-dependent quirks and may fail silently across filesystems.Files.moveprovides better error handling and clearer exceptions.-public boolean renameTo(File newFile) { - return file.renameTo(newFile.getFile()); +public boolean renameTo(File newFile) throws IOException { + Files.move(this.file.toPath(), newFile.getFile().toPath()); + return true; }src/main/java/de/tum/cit/aet/artemis/core/util/ResponseUtil.java (1)
29-32: Avoid conflating IO errors with “File not found”.
By catching allIOExceptiontypes and throwing anEntityNotFoundException, real issues such as permission errors or read failures may be hidden. Consider reporting a more descriptive error message.catch (IOException e) { - throw new EntityNotFoundException("File not found"); + throw new EntityNotFoundException("Unable to access or read file: " + e.getMessage(), e); }src/main/java/de/tum/cit/aet/artemis/text/service/TextSubmissionExportService.java (1)
54-55: Unify file creation logic for clarity.
Currently, you callsubmissionExportFile.createNewFile()before usingFiles.newBufferedWriter(submissionPath). You might omit the explicit call for file creation sincenewBufferedWritercan create the file if needed.if (!submissionExportFile.exists()) { - submissionExportFile.createNewFile(); } try (BufferedWriter writer = Files.newBufferedWriter(submissionPath, StandardCharsets.UTF_8)) { writer.write(submission.getText()); }Also applies to: 61-61
src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/ssh/SshConfiguration.java (1)
69-69: LGTM! Consider making the host key path configurable.The update to
Path.of()aligns with modern Java practices. However, consider making the host key path configurable through properties to support different deployment environments.- sshd.setKeyPairProvider(new SimpleGeneratorHostKeyProvider(Path.of("tmp", "hostkey.ser"))); + @Value("${artemis.ssh.host-key-path:tmp/hostkey.ser}") + private String sshHostKeyPath; + ... + sshd.setKeyPairProvider(new SimpleGeneratorHostKeyProvider(Path.of(sshHostKeyPath)));src/main/java/de/tum/cit/aet/artemis/core/config/SAML2Configuration.java (1)
120-121: LGTM! Consider adding path validation for security.The updates to use modern NIO.2 APIs (
Path.of(),Files.newInputStream(),Files.newBufferedReader()) improve resource management and align with best practices.Consider adding path validation to prevent path traversal attacks:
+ private static void validatePath(String path) { + Path normalizedPath = Path.of(path).normalize(); + if (!normalizedPath.startsWith(Path.of(System.getProperty("user.dir")))) { + throw new SecurityException("Path traversal attempt detected"); + } + } private boolean checkFiles(SAML2Properties.RelyingPartyProperties config) { if (config.getCertFile() == null || config.getKeyFile() == null || config.getCertFile().isBlank() || config.getKeyFile().isBlank()) { log.debug("No Config for SAML2"); return false; } + validatePath(config.getKeyFile()); + validatePath(config.getCertFile()); File keyFile = Path.of(config.getKeyFile()).toFile(); File certFile = Path.of(config.getCertFile()).toFile();Also applies to: 132-132, 140-140
src/test/java/de/tum/cit/aet/artemis/assessment/util/GradingScaleUtilService.java (1)
208-209: LGTM! Modern file reading approach with explicit charset.The change to use
Files.newBufferedReaderwith explicit charset is a good modernization that aligns with Java's NIO.2 API and ensures proper character encoding.Consider using try-with-resources with CSVParser directly:
- try (var reader = Files.newBufferedReader(ResourceUtils.getFile("classpath:" + path).toPath(), StandardCharsets.UTF_8); - var csvParser = CSVParser.parse(reader, CSVFormat.DEFAULT)) { + try (var csvParser = CSVParser.parse(ResourceUtils.getFile("classpath:" + path).toPath(), StandardCharsets.UTF_8, CSVFormat.DEFAULT)) {src/main/java/de/tum/cit/aet/artemis/programming/service/AbstractGitService.java (1)
121-122: LGTM! Modern path handling approach.The change to use
Path.ofis a good modernization that aligns with Java's NIO.2 API, providing better path manipulation capabilities.Consider extracting the path creation to separate methods for better readability:
- .setSshDirectory(Path.of(gitSshPrivateKeyPath.orElseThrow()).toFile()) - .setHomeDirectory(Path.of(System.getProperty("user.home")).toFile()); + .setSshDirectory(getSshDirectory(gitSshPrivateKeyPath)) + .setHomeDirectory(getHomeDirectory()); + private static File getSshDirectory(Optional<String> gitSshPrivateKeyPath) { + return Path.of(gitSshPrivateKeyPath.orElseThrow()).toFile(); + } + + private static File getHomeDirectory() { + return Path.of(System.getProperty("user.home")).toFile(); + }src/test/java/de/tum/cit/aet/artemis/programming/TestRepositoryResourceIntegrationTest.java (3)
Line range hint
516-517: Add test coverage for remaining repository states.The TODO comment indicates missing test coverage. Consider adding test cases for other repository states to ensure comprehensive coverage.
Would you like me to help create test cases for the remaining repository states? I can generate test methods that cover states like:
- Merge conflicts
- Ahead/Behind remote
- Detached HEAD
- etc.
Line range hint
4-516: Enhance test assertions for better error detection.While the tests use assertThat, some assertions could be more specific:
Examples:
- In testGetFiles:
-assertThat(files).isNotEmpty(); +assertThat(files) + .isNotEmpty() + .containsKey(currentLocalFileName) + .hasEntrySatisfying(currentLocalFileName, type -> assertThat(type).isEqualTo(FileType.FILE));
- In testCommitChanges:
-assertThat(testRepoCommits).hasSize(1); +assertThat(testRepoCommits) + .hasSize(1) + .first() + .satisfies(commit -> { + assertThat(commit.getFullMessage()).isNotEmpty(); + assertThat(commit.getAuthorIdent().getWhen()).isCloseTo( + ZonedDateTime.now().toInstant(), + within(1, ChronoUnit.MINUTES) + ); + });
Line range hint
32-516: Consider splitting large test methods into smaller, focused tests.Some test methods cover multiple scenarios and could be split for better maintainability and clarity.
For example,
testPullChangescould be split into:
- testPullChanges_withNewFile
- testPullChanges_withModifiedFile
- testPullChanges_withDeletedFile
This would improve test isolation and make failures more specific.
src/main/java/de/tum/cit/aet/artemis/core/service/FileService.java (1)
Line range hint
671-681: Consider using Files.walk for more efficient file traversal.While the current changes correctly use Path API, consider using
Files.walk()orFiles.find()for more efficient and cleaner file traversal, similar to how it's used in thereplaceVariablesInFilenamemethod.- String[] files = directory.list((current, name) -> current.toPath().resolve(name).toFile().isFile()); - if (files != null) { - // filter out files that should be ignored - files = Arrays.stream(files).filter(Predicate.not(filesToIgnore::contains)).toArray(String[]::new); - for (String file : files) { - replaceVariablesInFile(directory.toPath().toAbsolutePath().resolve(file), replacements); - } - } + try (var files = Files.find(startPath, 1, (path, attr) -> attr.isRegularFile())) { + files.filter(path -> !filesToIgnore.contains(path.getFileName().toString())) + .forEach(path -> replaceVariablesInFile(path, replacements)); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (52)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobExecutionService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/config/SAML2Configuration.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/core/config/WebConfigurer.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/service/FileService.java(6 hunks)src/main/java/de/tum/cit/aet/artemis/core/service/ResourceLoaderService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/service/export/CourseExamExportService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/core/service/export/DataExportService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/core/util/ResponseUtil.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/exam/web/ExamResource.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/modeling/service/ModelingSubmissionExportService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/plagiarism/service/TextPlagiarismDetectionService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/domain/File.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/domain/Repository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/AbstractGitService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/JavaTemplateUpgradeService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/PlantUmlService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseExportService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/RepositoryService.java(9 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/scaparser/strategy/PMDCPDParser.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCRepositoryUri.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCServletService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/SshGitLocationResolverService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/ssh/SshConfiguration.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseExportImportResource.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExercisePlagiarismResource.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/repository/RepositoryResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/quiz/service/QuizExerciseService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/text/service/TextSubmissionExportService.java(3 hunks)src/test/java/de/tum/cit/aet/artemis/assessment/util/GradingScaleUtilService.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/athena/service/connectors/AthenaRepositoryExportServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/communication/linkpreview/LinkPreviewIntegrationTest.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/communication/notification/NotificationPlaceholderSignatureTest.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/core/service/DataExportCreationServiceTest.java(5 hunks)src/test/java/de/tum/cit/aet/artemis/core/service/FileServiceTest.java(3 hunks)src/test/java/de/tum/cit/aet/artemis/core/service/ResourceLoaderServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/core/service/UriServiceTest.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/exam/ExamUserIntegrationTest.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/programming/AuxiliaryRepositoryResourceIntegrationTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/fileupload/util/ZipFileTestUtilService.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/plagiarism/PlagiarismDetectionServiceTest.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseIntegrationTestService.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseTemplateIntegrationTest.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/programming/StaticCodeAnalysisParserUnitTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/programming/TestRepositoryResourceIntegrationTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIIntegrationTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalVCLocalCITestService.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/RepositoryUriTest.java(4 hunks)src/test/java/de/tum/cit/aet/artemis/programming/util/GitUtilService.java(3 hunks)src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseUtilService.java(1 hunks)
✅ Files skipped from review due to trivial changes (13)
- src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalVCLocalCITestService.java
- src/test/java/de/tum/cit/aet/artemis/core/service/ResourceLoaderServiceTest.java
- src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseUtilService.java
- src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/SshGitLocationResolverService.java
- src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalCIIntegrationTest.java
- src/main/java/de/tum/cit/aet/artemis/quiz/service/QuizExerciseService.java
- src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseExportService.java
- src/test/java/de/tum/cit/aet/artemis/programming/icl/RepositoryUriTest.java
- src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerService.java
- src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobExecutionService.java
- src/main/java/de/tum/cit/aet/artemis/programming/service/PlantUmlService.java
- src/test/java/de/tum/cit/aet/artemis/programming/StaticCodeAnalysisParserUnitTest.java
- src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseIntegrationTestService.java
🧰 Additional context used
📓 Path-based instructions (39)
src/test/java/de/tum/cit/aet/artemis/communication/notification/NotificationPlaceholderSignatureTest.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
src/test/java/de/tum/cit/aet/artemis/athena/service/connectors/AthenaRepositoryExportServiceTest.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
src/main/java/de/tum/cit/aet/artemis/core/config/WebConfigurer.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
src/main/java/de/tum/cit/aet/artemis/plagiarism/service/TextPlagiarismDetectionService.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
src/test/java/de/tum/cit/aet/artemis/core/service/DataExportCreationServiceTest.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
src/test/java/de/tum/cit/aet/artemis/programming/TestRepositoryResourceIntegrationTest.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
src/main/java/de/tum/cit/aet/artemis/programming/service/AbstractGitService.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
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExercisePlagiarismResource.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
src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCServletService.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
src/main/java/de/tum/cit/aet/artemis/core/service/export/CourseExamExportService.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
src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/ssh/SshConfiguration.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
src/test/java/de/tum/cit/aet/artemis/exercise/programming/AuxiliaryRepositoryResourceIntegrationTest.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
src/main/java/de/tum/cit/aet/artemis/modeling/service/ModelingSubmissionExportService.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
src/test/java/de/tum/cit/aet/artemis/communication/linkpreview/LinkPreviewIntegrationTest.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
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseTemplateIntegrationTest.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
src/test/java/de/tum/cit/aet/artemis/fileupload/util/ZipFileTestUtilService.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
src/test/java/de/tum/cit/aet/artemis/assessment/util/GradingScaleUtilService.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
src/main/java/de/tum/cit/aet/artemis/programming/domain/Repository.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
src/main/java/de/tum/cit/aet/artemis/core/service/ResourceLoaderService.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
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.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
src/main/java/de/tum/cit/aet/artemis/core/util/ResponseUtil.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
src/test/java/de/tum/cit/aet/artemis/programming/util/GitUtilService.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
src/main/java/de/tum/cit/aet/artemis/programming/web/repository/RepositoryResource.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
src/test/java/de/tum/cit/aet/artemis/plagiarism/PlagiarismDetectionServiceTest.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
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/scaparser/strategy/PMDCPDParser.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
src/main/java/de/tum/cit/aet/artemis/core/service/export/DataExportService.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
src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCRepositoryUri.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
src/main/java/de/tum/cit/aet/artemis/core/config/SAML2Configuration.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
src/main/java/de/tum/cit/aet/artemis/programming/service/JavaTemplateUpgradeService.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
src/main/java/de/tum/cit/aet/artemis/core/service/FileService.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
src/main/java/de/tum/cit/aet/artemis/programming/service/RepositoryService.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
src/test/java/de/tum/cit/aet/artemis/core/service/UriServiceTest.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
src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.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
src/test/java/de/tum/cit/aet/artemis/exam/ExamUserIntegrationTest.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
src/main/java/de/tum/cit/aet/artemis/exam/web/ExamResource.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
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseExportImportResource.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
src/test/java/de/tum/cit/aet/artemis/core/service/FileServiceTest.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
src/main/java/de/tum/cit/aet/artemis/text/service/TextSubmissionExportService.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
src/main/java/de/tum/cit/aet/artemis/programming/domain/File.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
📓 Learnings (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/scaparser/strategy/PMDCPDParser.java (1)
Learnt from: magaupp
PR: ls1intum/Artemis#9609
File: src/main/java/de/tum/cit/aet/artemis/programming/service/localci/scaparser/strategy/sarif/SarifParser.java:76-77
Timestamp: 2024-11-12T12:51:40.391Z
Learning: In `SarifParser.java`, the path obtained from the URI is only displayed and not otherwise used, so cross-platform path handling is unnecessary in this context.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: server-tests
- GitHub Check: Build and Push Docker Image
- GitHub Check: Analyse
🔇 Additional comments (50)
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseTemplateIntegrationTest.java (1)
127-127: LGTM! Good use of modern Path API.The change from direct
Fileconstructor toPath.of().toFile()follows modern Java practices for file path handling.src/main/java/de/tum/cit/aet/artemis/core/service/ResourceLoaderService.java (1)
218-218: LGTM! Good modernization of Path creation.The change from
Paths.get()toPath.of()aligns with modern Java practices. This static factory method was introduced in Java 11 as the preferred way to create Path instances.src/test/java/de/tum/cit/aet/artemis/communication/linkpreview/LinkPreviewIntegrationTest.java (1)
7-7: LGTM!Clean addition of Path import for modernized file handling.
src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCRepositoryUri.java (2)
75-75: LGTM! Good modernization of path handling.The change from
Paths.get()toPath.of()aligns with modern Java practices while maintaining the same functionality.
149-154: LGTM! Well-documented examples.The Javadoc examples have been updated to reflect the modern path handling API, providing clear and accurate usage scenarios.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/scaparser/strategy/PMDCPDParser.java (1)
4-4: LGTM!The specific import follows the coding guidelines by avoiding star imports.
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseExportImportResource.java (3)
9-10: LGTM! Good modernization of imports.The addition of
java.nio.fileimports aligns with the modernization goal and enables the use of the more robust Path API.
338-344: LGTM! Improved file handling with modern APIs.The changes effectively modernize the file handling by:
- Using
Files.newInputStreaminstead ofFileInputStream- Properly handling paths with the Path API
- Using consistent variable naming
405-405: LGTM! Consistent modernization of file handling.The change to
Files.newInputStreamwith Path API is consistent with the modernization pattern.src/test/java/de/tum/cit/aet/artemis/programming/util/GitUtilService.java (2)
167-167: LGTM! Good modernization of file handling.The change from direct
Filecreation to usingPath.of()aligns with modern Java practices.
287-287: LGTM! Consistent modernization approach.The change maintains consistency with other modernized file handling code in the class.
src/main/java/de/tum/cit/aet/artemis/modeling/service/ModelingSubmissionExportService.java (1)
9-9: LGTM! Import changes align with modernization.The addition of
java.nio.file.Filesimport supports the transition to modern file handling while maintaining backward compatibility.src/main/java/de/tum/cit/aet/artemis/core/service/export/CourseExamExportService.java (1)
Line range hint
1-50: Well-structured class with proper dependency injection!The class follows best practices with constructor-based dependency injection and appropriate profile scoping.
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExercisePlagiarismResource.java (1)
7-7: LGTM!The specific import for
java.nio.file.Filesfollows the coding guidelines by avoiding star imports.src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCServletService.java (1)
173-173: LGTM: Modern path handling approach.The change from
Paths.get()toPath.of()aligns with modern Java practices and maintains OS independence.src/main/java/de/tum/cit/aet/artemis/programming/domain/Repository.java (1)
29-29: 🛠️ Refactor suggestion
⚠️ Potential issueRemove redundant
getFile()call and consider modernizing the API.The call to
getFile()on aFileobject is redundant as the parameter is already aFile. Additionally, this change doesn't align with the PR's modernization goals.Consider these improvements:
- Remove the redundant call
- Modernize the API to use
Pathinstead ofFile- Add null check for defensive programming
Here's how to fix the immediate issue:
- super(gitDir.getFile()); + super(gitDir);Consider modernizing the entire constructor to use
Path:- public Repository(File gitDir, VcsRepositoryUri remoteRepositoryUri) throws IOException { - super(gitDir); + public Repository(Path gitDir, VcsRepositoryUri remoteRepositoryUri) throws IOException { + super(gitDir.toFile()); this.remoteRepositoryUri = remoteRepositoryUri; }Let's verify the usage of this constructor across the codebase to assess the impact of modernization:
src/test/java/de/tum/cit/aet/artemis/core/service/UriServiceTest.java (1)
7-7: LGTM!The import for
java.nio.file.Pathis correctly added to support the modernized file path handling.src/main/java/de/tum/cit/aet/artemis/programming/service/JavaTemplateUpgradeService.java (3)
8-9: LGTM! Good use of modern Java NIO package.The addition of
java.nio.fileimports aligns with the modernization effort and best practices for file handling in Java.
160-163: LGTM! Improved stream handling with modern Java NIO.Good use of:
Files.newOutputStream/newInputStreamfor better resource management- try-with-resources for automatic resource cleanup
Also applies to: 167-168
105-105: Verify consistent usage of file.getFile() pattern across the codebase.While the change to
file.getFile().getName()works, the double method call pattern might impact performance in loops. Let's verify this pattern's usage:✅ Verification successful
The file.getFile().getName() pattern is correct and necessary
The double method call pattern is intentional and proper in this context. The outer
fileis a wrapper class that provides repository-specific functionality, whilegetFile()returns the underlyingjava.io.Fileobject for standard file operations. This abstraction layer is necessary for repository validation and safety.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for inconsistent file name access patterns rg --type java "\.getName\(\)" -A 2 -B 2 # Search for the wrapper pattern usage ast-grep --pattern 'file.getFile().getName()'Length of output: 76531
Script:
#!/bin/bash # Search for the file.getFile() pattern rg "file\.getFile\(\)" --type java -B 2 -A 2 # Search for different file name access patterns rg "\.getName\(\)" --type java --files-with-matches # Try different ast-grep pattern with single quotes ast-grep --pattern 'file.getFile()'Length of output: 28054
src/main/java/de/tum/cit/aet/artemis/programming/domain/File.java (2)
49-51: Convenient path conversion.
This direct conversion toPathis a straightforward improvement for modern file-handling usage.
53-55: Simple file check is correct.
TheisFile()behavior aligns well with the underlyingjava.io.File. No issues detected.src/test/java/de/tum/cit/aet/artemis/athena/service/connectors/AthenaRepositoryExportServiceTest.java (1)
84-84: LGTM!The update to
Path.of()aligns with modern Java practices. The hardcoded path is acceptable in this test context.src/test/java/de/tum/cit/aet/artemis/plagiarism/PlagiarismDetectionServiceTest.java (1)
123-123: LGTM!The update to use
Path.of().toFile()aligns with modern Java practices. The empty path is appropriate for this test mock scenario.src/main/java/de/tum/cit/aet/artemis/core/service/export/DataExportService.java (1)
104-104: LGTM! Modern file handling approach.The change to use
Files.newInputStreamis a good modernization that aligns with Java's NIO.2 API, providing better error handling and resource management.src/main/java/de/tum/cit/aet/artemis/plagiarism/service/TextPlagiarismDetectionService.java (1)
102-102: LGTM! Modern path handling approach.The change to use
Path.ofis a good modernization that aligns with Java's NIO.2 API, providing better path manipulation capabilities.src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java (1)
Line range hint
917-927: Well done on modernizing the file handling code!The changes follow Java best practices by:
- Using
Path.of()for path handling- Using
Files.newInputStream()for file I/O- Improved exception handling by using
IOExceptionsrc/test/java/de/tum/cit/aet/artemis/communication/notification/NotificationPlaceholderSignatureTest.java (1)
69-69: Good use of modern Java Path API!The change from
Paths.get()toPath.of()follows the recommended approach for path handling in modern Java.src/test/java/de/tum/cit/aet/artemis/core/service/DataExportCreationServiceTest.java (1)
219-219: Consistent modernization of file handling across test cleanup.Good job updating all file deletion operations to use
Path.of(). This makes the code more consistent with modern Java practices and improves path handling safety.Also applies to: 547-547, 573-573, 596-596, 652-652
src/test/java/de/tum/cit/aet/artemis/exam/ExamUserIntegrationTest.java (1)
342-344: LGTM! Good modernization of file handling.The changes improve the code by:
- Using the modern
PathAPI for file resolution- Using
Files.newInputStreaminstead ofFileInputStream- Following better resource management practices
src/main/java/de/tum/cit/aet/artemis/exam/web/ExamResource.java (1)
1241-1241: LGTM! Good improvements to file handling.The changes improve the code by:
- Using more specific exception handling with
IOException- Modernizing file handling with
Files.newInputStream- Following best practices for resource management
Also applies to: 1258-1258
src/test/java/de/tum/cit/aet/artemis/programming/TestRepositoryResourceIntegrationTest.java (1)
294-294: LGTM! The mock setup aligns with the File class structure.The change correctly reflects the updated File class structure where getName is accessed through getFile(). This maintains proper encapsulation.
src/test/java/de/tum/cit/aet/artemis/core/service/FileServiceTest.java (4)
72-74: LGTM! Good use of static final Path variables.The introduction of static final Path variables improves code maintainability by centralizing the path definitions and follows Java best practices.
196-207: Test method correctly uses the new Path variable.The test method properly utilizes the new
lineEndingsUnixPathvariable for file size verification.
214-225: Test method correctly uses the new Path variable.The test method properly utilizes the new
lineEndingsWindowsPathvariable for file size verification.
239-244: Good use of local variable for path.The introduction of
exportTestPathimproves readability and reduces duplication in the test method.src/main/java/de/tum/cit/aet/artemis/programming/web/repository/RepositoryResource.java (1)
417-417: LGTM! Proper use of File abstraction.The change correctly uses
file.get().getFile()to access the underlying file object, maintaining proper encapsulation of the File class.src/main/java/de/tum/cit/aet/artemis/programming/service/RepositoryService.java (6)
164-164: LGTM! Proper use of File abstraction for reading content.The change correctly uses
file.getFile()to access the underlying file for reading content.
246-248: Good modernization of file input handling.The change properly uses
Files.newInputStreamfrom java.nio.file package, which is the recommended approach for file I/O operations.
277-277: LGTM! Proper file comparison using File abstraction.The change correctly uses
file.getFile()for content comparison between files.
303-303: LGTM! Consistent use of File abstraction for file operations.The changes maintain consistency in using
file.getFile()for file operations.Also applies to: 320-320
Line range hint
373-403: LGTM! Proper validation using File abstraction.The changes correctly use
file.getFile()for file validation and operations, maintaining consistency with the File abstraction.
428-435: LGTM! Proper file operations using File abstraction.The changes correctly use
file.get().getFile()for file validation and deletion operations.src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java (3)
11-11: LGTM! Proper import for modern file I/O.Added import for java.nio.file.Files to support modern file I/O operations.
368-368: LGTM! Modern path creation.The change correctly uses
Path.of()which is the modern replacement forPaths.get().
391-394: Good modernization of file input handling.The change properly uses
Files.newInputStreamfrom java.nio.file package, which is the recommended approach for file I/O operations.src/test/java/de/tum/cit/aet/artemis/exercise/programming/AuxiliaryRepositoryResourceIntegrationTest.java (1)
337-337: LGTM! Test updated to match new File API.The test mock has been correctly updated to reflect the change from direct inheritance to composition in the File class, now accessing
getName()throughgetFile().src/main/java/de/tum/cit/aet/artemis/core/service/FileService.java (3)
519-524: LGTM! Modernized file handling using NIO.2.The changes appropriately use modern Java NIO.2 APIs (
Path.ofandFiles.newBufferedReader/Writer) which provide better platform independence and resource management.
597-599: LGTM! Consistent use of modern path handling.The change to
Path.ofmaintains consistency with the codebase modernization and improves cross-platform compatibility.
1039-1039: LGTM! Improved resource handling with Files.newInputStream.The change to
Files.newInputStreamis consistent with the modernization effort and provides better resource management when used with try-with-resources.
...java/de/tum/cit/aet/artemis/programming/service/localci/scaparser/strategy/PMDCPDParser.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExercisePlagiarismResource.java
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/fileupload/util/ZipFileTestUtilService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/programming/domain/File.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/text/service/TextSubmissionExportService.java
Show resolved
Hide resolved
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/config/SAML2Configuration.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/plagiarism/service/TextPlagiarismDetectionService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java (1)
Line range hint
367-383: Improve resource handling for TarArchiveOutputStream.The
TarArchiveOutputStreamis not properly closed, which could lead to resource leaks. Use try-with-resources to ensure proper cleanup.private ByteArrayOutputStream createTarArchive(Path sourcePath) { ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream(); - TarArchiveOutputStream tarArchiveOutputStream = new TarArchiveOutputStream(byteArrayOutputStream); - // This needs to be done in case the files have a long name - tarArchiveOutputStream.setLongFileMode(TarArchiveOutputStream.LONGFILE_POSIX); - try { - addFileToTar(tarArchiveOutputStream, sourcePath, ""); - } - catch (IOException e) { - throw new LocalCIException("Could not create tar archive", e); - } - return byteArrayOutputStream; + try (TarArchiveOutputStream tarArchiveOutputStream = new TarArchiveOutputStream(byteArrayOutputStream)) { + // This needs to be done in case the files have a long name + tarArchiveOutputStream.setLongFileMode(TarArchiveOutputStream.LONGFILE_POSIX); + addFileToTar(tarArchiveOutputStream, sourcePath, ""); + tarArchiveOutputStream.finish(); + return byteArrayOutputStream; + } catch (IOException e) { + throw new LocalCIException("Could not create tar archive for: " + sourcePath, e); + } }
🧹 Nitpick comments (10)
src/main/java/de/tum/cit/aet/artemis/core/config/SAML2Configuration.java (1)
Line range hint
139-143: Consider enhancing error handling for better security.The implementation correctly uses modern file reading with proper character encoding. However, consider adding specific error handling for security-related exceptions.
private RSAPrivateKey readPrivateKey(String file) throws IOException { // Read PKCS#8 File! try (var keyReader = Files.newBufferedReader(Path.of(file), StandardCharsets.UTF_8); var pemParser = new PEMParser(keyReader)) { JcaPEMKeyConverter converter = new JcaPEMKeyConverter(); PrivateKeyInfo privateKeyInfo = PrivateKeyInfo.getInstance(pemParser.readObject()); return (RSAPrivateKey) converter.getPrivateKey(privateKeyInfo); + } catch (IllegalArgumentException e) { + throw new IOException("Invalid private key format", e); } }src/main/java/de/tum/cit/aet/artemis/athena/service/AthenaRepositoryExportService.java (2)
Line range hint
85-120: Consider breaking down this method for better maintainability.The method handles multiple responsibilities: checking permissions, creating directories, and handling different repository types. Consider extracting these into separate methods.
public Path exportRepository(long exerciseId, Long submissionId, RepositoryType repositoryType) throws IOException { log.debug("Exporting repository for exercise {}, submission {}", exerciseId, submissionId); - var programmingExercise = programmingExerciseRepository.findByIdElseThrow(exerciseId); - checkFeedbackSuggestionsOrAutomaticFeedbackEnabledElseThrow(programmingExercise); + var programmingExercise = validateAndGetExercise(exerciseId); // Athena currently does not support individual due dates var exportOptions = new RepositoryExportOptionsDTO(false, true, false, programmingExercise.getDueDate(), false, false, false, true, false); - if (!Files.exists(repoDownloadClonePath)) { - Files.createDirectories(repoDownloadClonePath); - } + ensureExportDirectoryExists(); Path exportDir = fileService.getTemporaryUniqueSubfolderPath(repoDownloadClonePath, 15); - Path zipFilePath = null; + return exportRepositoryByType(programmingExercise, submissionId, repositoryType, exportOptions, exportDir); +} - if (repositoryType == null) { // Export student repository - var submission = programmingSubmissionRepository.findById(submissionId).orElseThrow(); - // Load participation with eager submissions - var participation = programmingExerciseStudentParticipationRepository.findWithSubmissionsById(submission.getParticipation().getId()).getFirst(); - zipFilePath = programmingExerciseExportService.getRepositoryWithParticipation(programmingExercise, participation, exportOptions, exportDir, exportDir, true); - } - else { - List<String> exportErrors = List.of(); - var exportFile = programmingExerciseExportService.exportInstructorRepositoryForExercise(programmingExercise.getId(), repositoryType, exportDir, exportDir, - exportErrors); - if (exportFile.isPresent()) { - zipFilePath = exportFile.get().toPath(); - } - } +private Path exportRepositoryByType(ProgrammingExercise exercise, Long submissionId, RepositoryType repositoryType, + RepositoryExportOptionsDTO exportOptions, Path exportDir) throws IOException { + Path zipFilePath = repositoryType == null + ? exportStudentRepository(exercise, submissionId, exportOptions, exportDir) + : exportInstructorRepository(exercise, repositoryType, exportDir); if (zipFilePath == null) { throw new IOException("Failed to export repository"); } return zipFilePath; }
116-118: Enhance error handling with specific exceptions.The generic IOException doesn't provide enough context about what failed. Consider creating a custom exception or including more details in the error message.
- if (zipFilePath == null) { - throw new IOException("Failed to export repository"); - } + if (zipFilePath == null) { + throw new IOException("Failed to export repository: " + + (repositoryType == null ? "student repository" : repositoryType.toString())); + }src/main/java/de/tum/cit/aet/artemis/exercise/service/SubmissionExportService.java (1)
Line range hint
214-214: Consider modernizing the method signature to use Path.For consistency with the modernization efforts, consider updating this method to use
Pathinstead ofFile.- protected abstract void saveSubmissionToFile(Exercise exercise, Submission submission, File file) throws IOException; + protected abstract void saveSubmissionToFile(Exercise exercise, Submission submission, Path path) throws IOException;src/main/java/de/tum/cit/aet/artemis/core/util/ResponseUtil.java (2)
27-36: Improve resource handling with try-with-resources.The current implementation could leak resources if an exception occurs after creating the
InputStreamResource. Consider using try-with-resources to ensure proper cleanup.public static ResponseEntity<Resource> ok(Path filepath) { - try { - InputStreamResource resource = new InputStreamResource(Files.newInputStream(filepath)); - return ResponseEntity.ok().contentLength(Files.size(filepath)).contentType(MediaType.APPLICATION_OCTET_STREAM) - .header("filename", String.valueOf(filepath.getFileName())).body(resource); + try (var inputStream = Files.newInputStream(filepath)) { + InputStreamResource resource = new InputStreamResource(inputStream); + return ResponseEntity.ok() + .contentLength(Files.size(filepath)) + .contentType(MediaType.APPLICATION_OCTET_STREAM) + .header("filename", String.valueOf(filepath.getFileName())) + .body(resource); }
33-35: Enhance error message with file details.The generic "File not found" message could be more helpful by including the filepath that caused the error.
- catch (IOException e) { - throw new EntityNotFoundException("File not found"); + catch (IOException e) { + throw new EntityNotFoundException("File not found: " + filepath); }src/main/java/de/tum/cit/aet/artemis/core/service/FileService.java (4)
568-569: Maintain consistency in file operation APIs.Consider using
Files.move()instead ofFileUtils.moveFile()to maintain consistency with the NIO.2 API:-Files.delete(filePath); -FileUtils.moveFile(tempFile.toFile(), filePath.toFile()); +Files.delete(filePath); +Files.move(tempFile, filePath);
622-634: Good modernization but maintain API consistency.Excellent use of
Files.find()for recursive file search, but consider replacingFileUtils.moveFile()withFiles.move()for consistency:-FileUtils.moveFile(filePath.toFile(), Path.of(cleanFileName).toFile()); +Files.move(filePath, Path.of(cleanFileName));
748-755: Consider using modern Files.walk for recursive file listing.Replace
FileUtils.listFileswithFiles.walkfor consistency with NIO.2:-Collection<File> files = FileUtils.listFiles(startPath.toFile(), FileFilterUtils.trueFileFilter(), directoryFileFilter); +List<Path> files = Files.walk(startPath) + .filter(path -> !path.getFileName().toString().equals(".git")) + .filter(Files::isRegularFile) + .collect(Collectors.toList());
Line range hint
1-1049: Consider implementing a consistent file operation strategy.While the modernization to NIO.2 is good, the codebase mixes different file operation APIs (NIO.2, Commons IO, File). Consider:
- Creating a unified file operation strategy
- Documenting when to use each API
- Adding security checks for all file operations
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/main/java/de/tum/cit/aet/artemis/athena/service/AthenaRepositoryExportService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java(6 hunks)src/main/java/de/tum/cit/aet/artemis/core/config/SAML2Configuration.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/core/config/WebConfigurer.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/core/service/FileService.java(10 hunks)src/main/java/de/tum/cit/aet/artemis/core/util/ResponseUtil.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/service/SubmissionExportService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/fileupload/web/FileUploadExerciseResource.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/athena/service/connectors/AthenaRepositoryExportServiceTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/java/de/tum/cit/aet/artemis/athena/service/connectors/AthenaRepositoryExportServiceTest.java
- src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerService.java
- src/main/java/de/tum/cit/aet/artemis/core/config/WebConfigurer.java
🧰 Additional context used
📓 Path-based instructions (7)
src/main/java/de/tum/cit/aet/artemis/core/util/ResponseUtil.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
src/main/java/de/tum/cit/aet/artemis/core/config/SAML2Configuration.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
src/main/java/de/tum/cit/aet/artemis/core/service/FileService.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
src/main/java/de/tum/cit/aet/artemis/exercise/service/SubmissionExportService.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
src/main/java/de/tum/cit/aet/artemis/fileupload/web/FileUploadExerciseResource.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
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.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
src/main/java/de/tum/cit/aet/artemis/athena/service/AthenaRepositoryExportService.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
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: client-style
- GitHub Check: client-tests
- GitHub Check: Build and Push Docker Image
- GitHub Check: Analyse
🔇 Additional comments (14)
src/main/java/de/tum/cit/aet/artemis/core/config/SAML2Configuration.java (4)
6-7: LGTM! Clean import additions.The new imports for
java.nio.fileAPIs are specific and align with the modernization goals.
119-122: LGTM! Modern path handling implemented correctly.The changes properly implement modern Java path handling:
- Using
Path.of()for path creation- Using
Files.exists()for existence checks
131-134: LGTM! Proper use of modern file streaming.The change to
Files.newInputStreamwith try-with-resources ensures proper resource management.
Line range hint
51-52: Address the TODO comment about BouncyCastle provider.The TODO indicates missing documentation about why the BouncyCastle provider is needed. Consider adding a brief explanation about PKCS#8 support.
Would you like me to provide the documentation for why the BouncyCastle provider is necessary?
src/main/java/de/tum/cit/aet/artemis/athena/service/AthenaRepositoryExportService.java (1)
81-85: LGTM! Good modernization of file handling.The change from
FiletoPathaligns with modern Java practices and improves cross-platform compatibility.src/main/java/de/tum/cit/aet/artemis/exercise/service/SubmissionExportService.java (1)
69-76: LGTM! Clean modernization of file handling.The change from
FiletoPathis well-implemented with appropriate documentation updates. The simplified return statement improves code clarity.src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java (1)
Line range hint
358-365: LGTM!The implementation properly handles resources and provides clear error messages. The transition to using Path is well implemented.
src/main/java/de/tum/cit/aet/artemis/core/service/FileService.java (5)
32-32: LGTM! Stream import supports modern Java practices.The addition of the Stream import aligns with the modernization goals and enables more functional programming approaches.
586-605: Well-implemented modernization changes!Good improvements:
- Using NIO.2 Path API
- Proper resource management with try-with-resources
- Modern directory listing with Files.list
Line range hint
646-693: Well-structured implementation with good practices!Positive aspects:
- Proper exception handling
- Efficient directory listing with Files.list
- Thoughtful handling of .git directory exclusion
1041-1041:⚠️ Potential issueVerify input path security.
While the file input handling is modernized well, ensure the path is properly validated:
✅ Verification successful
Path security is properly implemented ✅
The implementation follows secure path handling practices:
- Filenames are sanitized using
sanitizeFilename()- Paths are contained within a controlled temporary directory via
FilePathService.getTempFilePath()- Safe path resolution using
Path.resolve()🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for path validation before file operations rg -A 5 "Files\.newInputStream.*outputFile\.toPath\(\)"Length of output: 775
Script:
#!/bin/bash # Get the full method context rg -B 20 -A 10 "convertByteArrayToMultipart.*\{" src/main/java/de/tum/cit/aet/artemis/core/service/FileService.javaLength of output: 1657
516-521:⚠️ Potential issueVerify path traversal protection.
While the modernization to Path API is good, ensure proper path sanitization to prevent path traversal attacks.
✅ Verification successful
Path handling is secure in this context
The path traversal concern is adequately addressed as the method is only called with controlled paths constructed using Path.resolve() for specific build files (pom.xml, build.gradle) within repository-managed directories.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for path sanitization or validation before file operations ast-grep --pattern 'sanitizeByCheckingIfPathStartsWithSubPathElseThrow($path, $_)'Length of output: 208
Script:
#!/bin/bash # Get the full context of the FileService class and the method rg -U "class FileService.*?replacePlaceholderSections.*?\{.*?\}" -A 50 --multiline # Look for path validation patterns rg -l "normalize|toRealPath|getCanonicalPath|isAbsolute|startsWith|relativize" src/main/java/de/tum/cit/aet/artemis/core/service/FileService.java # Check how filePath is used and validated ast-grep --pattern 'Path $_ = Path.of($path)'Length of output: 471
Script:
#!/bin/bash # Get the method implementation rg "replacePlaceholderSections.*?\{" -A 30 src/main/java/de/tum/cit/aet/artemis/core/service/FileService.java # Look for security validation methods rg "validate|check|secure|sanitize" src/main/java/de/tum/cit/aet/artemis/core/service/FileService.java # Check for path normalization rg "Path\.normalize|toAbsolutePath|toRealPath" src/main/java/de/tum/cit/aet/artemis/core/service/FileService.javaLength of output: 5561
Script:
#!/bin/bash # Check how replacePlaceholderSections is called rg "replacePlaceholderSections\(" -B 5 --multilineLength of output: 2553
src/main/java/de/tum/cit/aet/artemis/fileupload/web/FileUploadExerciseResource.java (2)
8-8: LGTM! Good modernization of imports.The addition of
java.nio.file.Pathimport aligns with the PR's objective to modernize file handling.
388-389: LGTM! Good modernization of file handling.The change from
FiletoPathaligns with modern Java practices and the PR objectives.Consider these improvements:
- Add Javadoc to document the returned Path object
- Add debug logging before returning the response
+ // Add logging + log.debug("Exporting submissions for exercise {} to path: {}", exerciseId, zipFilePath); return ResponseUtil.ok(zipFilePath);Verify that ResponseUtil properly handles Path objects:
✅ Verification successful
ResponseUtil properly handles Path objects with robust implementation
The
ResponseUtil.ok(Path)method correctly:
- Streams file content using NIO.2 APIs
- Sets proper headers for file download
- Handles errors appropriately
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check ResponseUtil implementation for Path handling ast-grep --pattern 'class ResponseUtil { $$$ ok(Path $path) { $$$ } $$$ }'Length of output: 212
Script:
#!/bin/bash # Find ResponseUtil class and check its content echo "=== Finding ResponseUtil class ===" fd -e java "ResponseUtil.java" echo -e "\n=== Checking ResponseUtil implementation ===" for f in $(fd -e java "ResponseUtil.java"); do echo "=== Content of $f ===" cat "$f" done echo -e "\n=== Looking for Path response patterns ===" rg "Path.*Response" -A 3Length of output: 3584
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportService.java (1)
Line range hint
238-242: Consider improving error handling to maintain consistency.The current error handling in
adjustProjectNamesonly logs the error when repository adjustments fail. This could lead to an inconsistent state if some repositories are updated while others fail. Consider:
- Rolling back successful changes when an error occurs
- Propagating the error to allow proper error handling at a higher level
- try { - // Adjust placeholders that were replaced during creation of template exercise - adjustProjectNames(templateExercise, newExercise); - } - catch (GitAPIException | IOException e) { - log.error("Error during adjustment of placeholders of ProgrammingExercise {}", newExercise.getTitle(), e); - } + try { + // Adjust placeholders that were replaced during creation of template exercise + adjustProjectNames(templateExercise, newExercise); + } + catch (GitAPIException | IOException e) { + log.error("Error during adjustment of placeholders of ProgrammingExercise {}. Rolling back changes...", newExercise.getTitle(), e); + // TODO: Add rollback logic here + throw new RuntimeException("Failed to adjust project names. Changes have been rolled back.", e); + }src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportFromFileService.java (2)
Line range hint
1-324: Consider decomposing the service class to improve maintainability.The class has multiple responsibilities including file operations, Git operations, and exercise management. Consider splitting it into smaller, focused services:
ProgrammingExerciseFileService: Handle file operationsProgrammingExerciseGitService: Handle Git operationsProgrammingExerciseImportService: Orchestrate the import processAdd protection against path traversal and zip slip vulnerabilities.
The file operations could be vulnerable to path traversal and zip slip attacks. Consider:
- Validating all file paths are within the expected directory
- Using
java.nio.file.Path.normalize()before file operations- Adding zip slip protection when extracting files
Example protection for zip slip:
private void copyEmbeddedFiles(Path importExerciseDir) throws IOException { Path embeddedFilesDir = importExerciseDir.resolve("files"); if (!Files.exists(embeddedFilesDir)) { return; } try (var embeddedFiles = Files.list(embeddedFilesDir)) { for (Path file : embeddedFiles.toList()) { + // Protect against zip slip + Path normalizedFile = file.normalize(); + if (!normalizedFile.startsWith(embeddedFilesDir)) { + throw new SecurityException("Attempted path traversal in zip file"); + } Path targetPath = FilePathService.getMarkdownFilePath().resolve(file.getFileName()); if (!Files.exists(targetPath)) { FileUtils.copyFile(file.toFile(), targetPath.toFile()); } } } }
Test coverage missing for ProgrammingExerciseImportFromFileService
The service lacks dedicated test coverage despite handling critical operations (file, Git, repository management). Similar services in the codebase have comprehensive test coverage (e.g., ProgrammingExerciseServiceTest, ProgrammingExerciseRepositoryServiceTest).
Required test scenarios:
- Importing valid programming exercise zip files
- Handling invalid/malformed zip files
- Repository management operations
- Git operations and error cases
- File system operations and cleanup
🔗 Analysis chain
Line range hint
1-324: Verify test coverage for the import service.Given the complexity of operations (file, Git, JSON parsing), ensure comprehensive test coverage including:
- Happy path scenarios
- Error cases (invalid zip, missing files)
- Security scenarios (invalid paths, malformed zip)
- Edge cases (empty repositories, large files)
Run this script to check existing test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for ProgrammingExerciseImportFromFileService # Find test files echo "Looking for test files..." fd -e java -g "*Test*.java" | grep -i "ProgrammingExerciseImportFromFileService" # Find test methods echo -e "\nLooking for test methods..." rg -l "ProgrammingExerciseImportFromFileService" | xargs rg "@Test"Length of output: 278
Script:
#!/bin/bash # Cast a wider net to find any references to the class and test patterns echo "=== Looking for any mentions of the service ===" rg "ProgrammingExerciseImportFromFileService" -l echo -e "\n=== Finding similar service test patterns ===" fd -e java -g "*Service*Test.java" echo -e "\n=== Checking test directory structure ===" fd "test" -t d echo -e "\n=== Looking for programming exercise related tests ===" rg "class.*ProgrammingExercise.*Test" -g "*.java"Length of output: 20741
🧹 Nitpick comments (6)
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportService.java (1)
Line range hint
1-391: Overall code quality is good with room for minor improvements.The class demonstrates good practices:
- Proper dependency injection
- Clear documentation
- Single responsibility principle
- Modern Java file handling with
PathConsider these minor improvements:
- Add debug logging before critical operations
- Consider using
@Transactionalfor operations that modify multiple repositories- Add metrics/monitoring for import operations
src/test/java/de/tum/cit/aet/artemis/fileupload/util/ZipFileTestUtilService.java (4)
31-31: Consider using Path consistently throughout the method.Converting to Path and then back to File defeats the purpose of modernization. Consider using ZipFile's constructor that accepts Path (available since Java 13).
- File file = Path.of(zipFile).toFile(); + Path file = Path.of(zipFile); - try (ZipFile zip = new ZipFile(file)) { + try (ZipFile zip = new ZipFile(file.toFile())) { // Until upgrading to Java 13+
45-49: Enhance zip slip vulnerability check.While the security check is good, consider these improvements:
- Use a more descriptive error message
- Extract the check into a separate method for reusability
+ private void validateZipEntry(Path destFile, Path parentFolder) { + if (!destFile.toAbsolutePath().toString().startsWith(parentFolder.toAbsolutePath().toString())) { + fail("Potential zip slip attack detected - entry path is outside target directory"); + } + } public Path extractZipFileRecursively(String zipFile) throws IOException { // ... Path destFile = parentFolder.resolve(currentEntry); - if (!destFile.toAbsolutePath().toString().startsWith(parentFolder.toAbsolutePath().toString())) { - fail("Bad zip entry"); - } + validateZipEntry(destFile, parentFolder); // ...
56-56: Consider using Files.copy for consistency.For consistency with the modern Path API, consider using Files.copy instead of FileUtils.
- FileUtils.copyInputStreamToFile(zip.getInputStream(entry), destFile.toFile()); + Files.copy(zip.getInputStream(entry), destFile, java.nio.file.StandardCopyOption.REPLACE_EXISTING);
61-61: Consider overloading the method to accept Path.To avoid unnecessary string conversions, consider adding an overloaded method that accepts Path.
+ public Path extractZipFileRecursively(Path zipFile) throws IOException { + return extractZipFileRecursively(zipFile.toString()); + } public Path extractZipFileRecursively(String zipFile) throws IOException { // ... existing implementation ... - extractZipFileRecursively(destFile.toAbsolutePath().toString()); + extractZipFileRecursively(destFile); }src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportFromFileService.java (1)
Line range hint
201-205: LGTM! Consider adding debug logging.The addition of
throws IOExceptionproperly declares the potential file operation failures. The method follows best practices with proper error propagation and modern file handling.Consider adding debug logging to improve observability:
private void replaceImportedExerciseShortName(Map<String, String> replacements, List<Repository> repositories) throws IOException { + log.debug("Replacing exercise short name in {} repositories with replacements: {}", repositories.size(), replacements); for (Repository repository : repositories) { + log.debug("Processing repository at path: {}", repository.getLocalPath()); fileService.replaceVariablesInFileRecursive(repository.getLocalPath(), replacements, SHORT_NAME_REPLACEMENT_EXCLUSIONS); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/de/tum/cit/aet/artemis/modeling/web/ModelingExerciseResource.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportFromFileService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/text/web/TextExerciseResource.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/fileupload/util/ZipFileTestUtilService.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
src/main/java/de/tum/cit/aet/artemis/modeling/web/ModelingExerciseResource.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
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportService.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
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportFromFileService.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
src/main/java/de/tum/cit/aet/artemis/text/web/TextExerciseResource.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
src/test/java/de/tum/cit/aet/artemis/fileupload/util/ZipFileTestUtilService.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
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: client-tests
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Build and Push Docker Image
- GitHub Check: Analyse
- GitHub Check: Build .war artifact
🔇 Additional comments (9)
src/main/java/de/tum/cit/aet/artemis/modeling/web/ModelingExerciseResource.java (2)
8-8: LGTM! Good modernization of imports.The change from
java.io.Filetojava.nio.file.Pathaligns with modern Java practices for file handling.
395-395: LGTM! Improved file handling with Path API.The transition from
FiletoPathenhances OS independence and provides better file handling capabilities. This change:
- Improves cross-platform compatibility
- Provides more robust path manipulation
- Aligns with modern Java best practices
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportService.java (1)
266-266: LGTM! Appropriate exception handling for file operations.The addition of
IOExceptionto the method signature properly declares the checked exception that can be thrown during file operations, improving error handling transparency.src/test/java/de/tum/cit/aet/artemis/fileupload/util/ZipFileTestUtilService.java (4)
36-37: Improved directory creation using Path API.The changes align with the previous suggestion to use Files.createDirectories for better error handling.
51-53: LGTM! Modern Path API usage.The changes correctly use Path.getParent() and Files.createDirectories() for parent directory handling.
64-64: LGTM! Appropriate return type.Returning Path directly aligns with the modern API approach.
Line range hint
16-24: Class complies with test coding guidelines.The class follows the required guidelines for test utilities:
- Uses @service annotation (utility service factory pattern)
- Has a specific, focused purpose
- Avoids database access
- Includes proper documentation
src/main/java/de/tum/cit/aet/artemis/text/web/TextExerciseResource.java (2)
8-8: LGTM!The import of
java.nio.file.Pathaligns with the modernization effort to use the newer NIO.2 API.
563-563: LGTM! Verify the Path usage in TextSubmissionExportService.The change from
FiletoPathis correct and aligns with modern Java practices. ThePathAPI provides more powerful and flexible file operations.Let's verify that the
TextSubmissionExportServicehas been updated to returnPath:✅ Verification successful
Path usage in TextSubmissionExportService is correctly implemented ✅
The service extends SubmissionExportService which properly returns Path from exportStudentSubmissionsElseThrow. The implementation is consistent with modern Java practices, using both Path API for new code while maintaining compatibility with existing File operations where needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if TextSubmissionExportService has been updated to use Path ast-grep --pattern 'class TextSubmissionExportService { $$$ exportStudentSubmissionsElseThrow($$$) { $$$ } $$$ }'Length of output: 251
Script:
#!/bin/bash # Find the TextSubmissionExportService file rg -l "class TextSubmissionExportService" # Then examine the method signature and surrounding context rg -A 5 "exportStudentSubmissionsElseThrow"Length of output: 3340
Script:
#!/bin/bash # Check TextSubmissionExportService implementation cat src/main/java/de/tum/cit/aet/artemis/text/service/TextSubmissionExportService.javaLength of output: 2888
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/main/java/de/tum/cit/aet/artemis/core/service/FileService.java (2)
632-632:⚠️ Potential issueCritical: Potential path traversal vulnerability.
The code moves files without proper path validation, which could lead to path traversal attacks.
This issue was previously flagged by the security scanner. Apply this fix:
- FileUtils.moveFile(filePath.toFile(), Path.of(cleanFileName).toFile()); + Path targetPath = Path.of(cleanFileName).normalize(); + if (!targetPath.startsWith(startPath)) { + throw new SecurityException("Invalid target path: possible path traversal attempt"); + } + Files.move(filePath, targetPath);
1038-1038:⚠️ Potential issueCritical: Potential path traversal vulnerability in input stream handling.
The code opens an input stream without proper path validation.
This issue was previously flagged by the security scanner. Apply this fix:
- try (InputStream input = Files.newInputStream(outputFile.toPath()); OutputStream fileItemOutputStream = fileItem.getOutputStream()) { + Path normalizedPath = outputFile.toPath().normalize(); + if (!normalizedPath.startsWith(FilePathService.getTempFilePath())) { + throw new SecurityException("Invalid file path: possible path traversal attempt"); + } + try (InputStream input = Files.newInputStream(normalizedPath); OutputStream fileItemOutputStream = fileItem.getOutputStream()) {
🧹 Nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/core/service/FileService.java (2)
670-670: Consider using Files.list() for better performance.As noted in a past review comment by tobias-lippert, we should use modern NIO.2 APIs.
Consider this improvement:
- String[] files = directory.list((current, name) -> current.toPath().resolve(name).toFile().isFile()); + try (var files = Files.list(directory.toPath())) { + return files + .filter(Files::isRegularFile) + .map(Path::getFileName) + .map(Path::toString) + .toArray(String[]::new); + }
680-680: Modernize directory listing using NIO.2.Similar to the previous comment, directory listing should use modern APIs.
Consider this improvement:
- String[] subDirectories = directory.list((current, name) -> current.toPath().resolve(name).toFile().isDirectory()); + try (var subDirectories = Files.list(directory.toPath())) { + return subDirectories + .filter(Files::isDirectory) + .map(Path::getFileName) + .map(Path::toString) + .filter(name -> !name.equalsIgnoreCase(".git")) + .toArray(String[]::new); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/core/service/FileService.java(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/core/service/FileService.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
⏰ 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-tests
- GitHub Check: client-style
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (1)
src/main/java/de/tum/cit/aet/artemis/core/service/FileService.java (1)
596-598: LGTM: Path handling improvement.The code correctly uses
Path.of()for path manipulation.
# Conflicts: # src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java
Checklist
General
Server
Motivation and Context
The modernizer gradle tasks showed a lot of outdated code, in particular related to file handling.
All of them relate to the usage of
java.io.Fileinstead of the modernjava.nio.file.Path. In the long term we should fully migrate towardsjava.nio.file.Pathand only usejava.io.Filewhen invoking library methods that to not supportjava.nio.file.Path.This is a first stetp towards this goal.
Description
Replaced java.io.File with java.nio.file.Path whenever possible and sugggested by the modernizer plugin.
Initially we also wanted to remove that
programming.dmain.Fileinherits fromjava.io.Filebut this turned out to be trickier than expected and is therefore postponed to a followup.The modernizer issues in
programming.domain.Fileare the only ones that aren't adressed in this PR.Only code review is needed
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
Summary by CodeRabbit
Summary by CodeRabbit
Based on the comprehensive summary of changes, here are the release notes:
Refactor
FiletoPathclass.Path.of()instead ofPaths.get().Filesutility class.Chores
These changes represent a systematic update to file handling mechanisms, focusing on leveraging modern Java NIO (New Input/Output) package capabilities. The modifications do not introduce new features but significantly improve the codebase's maintainability and align with contemporary Java development practices.