Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Programming exercises: Remove unnecessary binary files from template and solution repository comparison #10091

Conversation

AjayvirS
Copy link
Contributor

@AjayvirS AjayvirS commented Jan 2, 2025

Checklist

General

Server

Client

  • Important: I implemented the changes with a very good performance, prevented too many (unnecessary) REST calls and made sure the UI is responsive, even with large data (e.g. using paging).
  • I strictly followed the principle of data economy for all client-server REST calls.

Changes affecting Programming Exercises

  • High priority: I tested all changes and their related features with all corresponding user types on a test server configured with the integrated lifecycle setup (LocalVC and LocalCI).
  • I tested all changes and their related features with all corresponding user types on a test server configured with Gitlab and Jenkins.

Motivation and Context

Currently, when comparing the template with the solution submission while editing an exercise, binary files such as .jar are unnecessarily included in the request, increasing its size. This leads to inefficient network usage and potential performance issues. Excluding common binary files (e.g., .jar, .exe) from these REST requests will optimize request payloads, improve responsiveness, and reduce unnecessary data transfer.
(see #9929)

Description

When comparing the template with the solution submission, while the binary files (e.g. jar) are not displayed, they are are included in the request unnecessarily increasing the request size.

Steps for Testing

Prerequisites:

  • 1 Instructor
  • 1 Programming Exercise
  1. Go to a programming exercise edit page
  2. Open your browser's dev tools
  3. Click on 'Review Changes' in the Template/Solution Comparison section
  4. Look at the network requests and see that binary files like .jar files are excluded from the request

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

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Test Coverage

Class/File Line Coverage Confirmation (assert/expect)
RepositoryService.java 90%
RepositoryProgrammingExerciseParticipationResource.java 87%
ProgrammingExerciseResource.java 89%
programming-exercise.service.ts 95%

Summary by CodeRabbit

  • New Features

    • Added ability to exclude binary files when retrieving repository files.
    • Introduced optional omitBinaries parameter for repository file retrieval endpoints.
  • Improvements

    • Enhanced file listing functionality to provide more granular control over file type inclusion.
    • Updated multiple service and resource layers to support binary file filtering.
    • Introduced a new configuration class for managing binary file extensions.
  • Tests

    • Added comprehensive test coverage for new binary file exclusion functionality, including specific test cases for omitted binary files.
    • Enhanced existing tests to validate the handling of binary file exclusions.

@github-actions github-actions bot added server Pull requests that update Java code. (Added Automatically!) client Pull requests that update TypeScript code. (Added Automatically!) iris Pull requests that affect the corresponding module programming Pull requests that affect the corresponding module labels Jan 2, 2025
@github-actions github-actions bot added the tests label Jan 2, 2025
@AjayvirS AjayvirS marked this pull request as ready for review January 8, 2025 18:26
@AjayvirS AjayvirS requested a review from a team as a code owner January 8, 2025 18:26
Copy link

coderabbitai bot commented Jan 8, 2025

Walkthrough

The pull request introduces a feature to exclude binary files from repository file listings across multiple components of the Artemis application. This change involves modifying several services and resources to add an omitBinaries parameter, which allows selective filtering of binary files (like .jar, .exe) when retrieving repository contents. The modifications span from backend services (GitService, RepositoryService) to web resources and frontend services, providing a consistent mechanism to reduce payload size and improve performance when handling repository files.

Changes

File Change Summary
src/main/java/de/tum/cit/aet/artemis/programming/service/GitService.java Added omitBinaries parameter to listFilesAndFolders method and created an overload for backward compatibility.
src/main/java/de/tum/cit/aet/artemis/programming/service/RepositoryService.java Updated getFilesContentFromWorkingCopy to include omitBinaries parameter and added an overload.
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseResource.java Added omitBinaries parameter to redirectGetSolutionRepositoryFiles and redirectGetTemplateRepositoryFiles methods.
src/main/java/de/tum/cit/aet/artemis/programming/web/repository/RepositoryProgrammingExerciseParticipationResource.java Updated getFilesWithContent to support omitBinaries parameter.
src/main/webapp/app/exercises/programming/manage/services/programming-exercise.service.ts Modified HTTP requests for retrieving test files to include omitBinaries query parameter.
src/test/java/de/tum/cit/aet/artemis/programming/GitServiceTest.java Enhanced tests to cover binary file omission functionality.
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseIntegrationTestService.java Added test for template repository file retrieval with binary omission.
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCIntegrationTest.java Introduced test for retrieving template files while omitting binaries.
src/test/java/de/tum/cit/aet/artemis/programming/RepositoryIntegrationTest.java Added test for file retrieval from repositories with binary exclusion.
src/test/java/de/tum/cit/aet/artemis/programming/util/GitUtilService.java Updated file handling to accommodate new binary file logic.
src/test/javascript/spec/service/programming-exercise.service.spec.ts Modified request matching in tests for dynamic URL handling.
src/main/java/de/tum/cit/aet/artemis/core/config/BinaryFileExtensionConfiguration.java Added a new configuration class for binary file extensions.

Possibly related issues

Possibly related PRs

  • Development: Optimize local git repository cleanup #9322: The changes in the main PR, which involve adding a parameter to the listFilesAndFolders method in GitService, are related to the modifications in the retrieved PR, where the getFilesContentFromWorkingCopy method in RepositoryService now calls gitService.listFilesAndFolders with the new omitBinaries parameter.
  • Development: Enable deactivated LocalVC integration tests #9942: The changes in the main PR, which involve adding and modifying the omitBinaries parameter in the listFilesAndFolders method of the GitService class, are directly related to the modifications in the retrieved PR, where the omitBinaries parameter is also integrated into various test methods that validate the functionality of file handling, including the use of listFilesAndFolders.
  • Integrated code lifecycle: Add auxiliary repository view #9321: The changes in the main PR, which involve adding a parameter to the listFilesAndFolders method in GitService to omit binary files, are related to the modifications in the retrieved PR, where the getFilesContentFromWorkingCopy method in RepositoryService is updated to utilize this new parameter when calling listFilesAndFolders.

Suggested labels

bugfix, performance

Suggested reviewers

  • SimonEntholzer
  • az108
  • coolchock
  • krusche
  • Feras797
  • BBesrour
  • HawKhiem

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/test/java/de/tum/cit/aet/artemis/programming/RepositoryIntegrationTest.java

[ERROR] Error at ruleset.xml:58:5
56|
57|
58|
^^^^^ Unable to find referenced rule BooleanInstantiation; perhaps the rule name is misspelled?

59|
60|
[WARN] Warning at ruleset.xml:66:5
64|
65|
66|
^^^^^ Use Rule name category/java/bestpractices.xml/DefaultLabelNotLastInSwitch instead of the deprecated Rule name category/java/bestpractices.xml/DefaultLabelNotLastInSwitchStmt. PMD 8.0.0 will remove support for this deprecated Rule name usage.

67|
68|
[ERROR] Error at ruleset.xml:71:5
69|
70|
71|
^^^^^ Unable to find referenced rule DontImportJavaLang; perhaps the rule name is misspelled?

72|
73|
[ERROR] Error at ruleset.xml:75:5
73|
74|
75|
^^^^^ Unable to find referenced rule DuplicateImports; perhaps the rule name is misspelled?

76|
77|
[ERROR] Error at ruleset.xml:78:5
76|
77|
78|
^^^^^ Unable to find referenced rule EmptyFinallyBlock; perhaps the rule name is misspelled?

79|
80|
[ERROR] Error at ruleset.xml:79:5
77|
78|
79|
^^^^^ Unable to find referenced rule EmptyIfStmt; perhaps the rule name is misspelled?

80|
81|
[ERROR] Error at ruleset.xml:81:5
79|
80|
81|
^^^^^ Unable to find referenced rule EmptyInitializer; perhaps the rule name is misspelled?

82|
83|
[ERROR] Error at ruleset.xml:82:5
80|
81|
82|
^^^^^ Unable to find referenced rule EmptyStatementBlock; perhaps the rule name is misspelled?

83|
84|
[ERROR] Error at ruleset.xml:83:5
81|
82|
83|
^^^^^ Unable to find referenced rule EmptyStatementNotInLoop; perhaps the rule name is misspelled?

84|
85|
[ERROR] Error at ruleset.xml:84:5
82|
83|
84|
^^^^^ Unable to find referenced rule EmptySwitchStatements; perhaps the rule name is misspelled?

85|
86|
[ERROR] Error at ruleset.xml:85:5
83|
84|
85|
^^^^^ Unable to find referenced rule EmptySynchronizedBlock; perhaps the rule name is misspelled?

86|
87|
[ERROR] Error at ruleset.xml:86:5
84|
85|
86|
^^^^^ Unable to find referenced rule EmptyTryBlock; perhaps the rule name is misspelled?

87|
88|
[ERROR] Error at ruleset.xml:87:5
85|
86|
87|
^^^^^ Unable to find referenced rule EmptyWhileStmt; perhaps the rule name is misspelled?

88|
89|
[ERROR] Error at ruleset.xml:90:5
88|
89|
90|
^^^^^ Unable to find referenced rule ExcessiveClassLength; perhaps the rule name is misspelled?

91|
92|
[ERROR] Error at ruleset.xml:91:5
89|
90|
91|
^^^^^ Unable to find referenced rule ExcessiveMethodLength; perhaps the rule name is misspelled?

92|
93|
[ERROR] Error at ruleset.xml:106:5
104|
105|
106|
^^^^^ Unable to find referenced rule ImportFromSamePackage; perhaps the rule name is misspelled?

107|
108|
[ERROR] Error at ruleset.xml:119:5
117|
118|
119|
^^^^^ Unable to find referenced rule MissingBreakInSwitch; perhaps the rule name is misspelled?

120|
121|
[WARN] Warning at ruleset.xml:124:5
122|
123|
124|
^^^^^ Use Rule name category/java/errorprone.xml/NonCaseLabelInSwitch instead of the deprecated Rule name category/java/errorprone.xml/NonCaseLabelInSwitchStatement. PMD 8.0.0 will remove support for this deprecated Rule name usage.

125|
126|
[ERROR] Error at ruleset.xml:134:9
132|
133| // It's okay to use short variable names in DTOs, e.g. "id" or "name"
134| ./de/tum/in/www1/artemis/web/rest/dto/.
^^^^^^^^^^^^^^^^ Unexpected element 'exclude-pattern' in rule ShortVariable

135|
136|
[ERROR] Error at ruleset.xml:137:5
135|
136|
137|
^^^^^ Unable to find referenced rule SimplifyBooleanAssertion; perhaps the rule name is misspelled?

138|
139|
[WARN] Warning at ruleset.xml:184:5
182|
183|
184|
^^^^^ Use Rule name category/ecmascript/errorprone.xml/InaccurateNumericLiteral instead of the deprecated Rule name category/ecmascript/errorprone.xml/InnaccurateNumericLiteral. PMD 8.0.0 will remove support for this deprecated Rule name usage.

185|
186|
[ERROR] Cannot load ruleset category/vm/bestpractices.xml: Cannot resolve rule/ruleset reference 'category/vm/bestpractices.xml'. Make sure the resource is a valid file or URL and is on the CLASSPATH. Use --debug (or a fine log level) to see the current classpath.
[WARN] Progressbar rendering conflicts with reporting to STDOUT. No progressbar will be shown. Try running with argument -r to output the report to a file instead.


📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 39d0572 and 998dd35.

📒 Files selected for processing (1)
  • src/test/java/de/tum/cit/aet/artemis/programming/RepositoryIntegrationTest.java (8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/test/java/**/*.java`: test_naming: descriptive; test_si...

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/RepositoryIntegrationTest.java
⏰ Context from checks skipped due to timeout of 90000ms (9)
  • GitHub Check: Call Build Workflow / Build .war artifact
  • GitHub Check: Call Build Workflow / Build and Push Docker Image
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: client-style
  • GitHub Check: client-tests-selected
  • GitHub Check: client-tests
  • GitHub Check: server-tests
  • GitHub Check: server-style
  • GitHub Check: Analyse
🔇 Additional comments (4)
src/test/java/de/tum/cit/aet/artemis/programming/RepositoryIntegrationTest.java (4)

97-98: LGTM! Well-structured setup for binary file testing.

The setup appropriately handles binary files by using byte arrays and proper file handling methods.

Also applies to: 149-153, 170-175


281-295: Well-structured test with comprehensive assertions!

The test method effectively verifies the binary file exclusion functionality with clear assertions for both positive and negative cases.


375-375: LGTM! Appropriate mock configuration update.

The mock configuration correctly reflects the updated method signature with the new boolean parameter.


409-413: LGTM! Improved code formatting.

The formatting changes enhance readability while maintaining consistency with the codebase style.

Also applies to: 431-434

✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (4)
src/test/java/de/tum/cit/aet/artemis/programming/util/GitUtilService.java (2)

205-206: Consider using Path.of() consistently.

The code uses string concatenation for file paths in some places. Consider using Path.of() consistently for better path handling.

-    public File getFile(REPOS repo, String fileToRead) {
-        Path path = Path.of(getCompleteRepoPathStringByType(repo), fileToRead);
+    public File getFile(REPOS repo, String fileName) {
+        return Path.of(getCompleteRepoPathStringByType(repo)).resolve(fileName).toFile();

-    public String getFileContent(REPOS repo, String fileToRead) {
-        try {
-            Path path = Path.of(getCompleteRepoPathStringByType(repo), fileToRead);
+    public String getFileContent(REPOS repo, String fileName) {
+        try {
+            Path path = Path.of(getCompleteRepoPathStringByType(repo)).resolve(fileName);

Also applies to: 210-212


260-271: Consider extracting file comparison logic.

The file comparison logic is becoming complex. Consider extracting it into a separate method for better maintainability.

+    private boolean compareFileContent(REPOS local, REPOS remote, String fileName) {
+        String localContent = getFileContent(local, fileName);
+        String remoteContent = getFileContent(remote, fileName);
+        return localContent.equals(remoteContent);
+    }
+
     public boolean isLocalEqualToRemote() {
-        String fileContentLocal1 = getFileContent(REPOS.LOCAL, GitUtilService.FILES.FILE1.toString());
-        String fileContentLocal2 = getFileContent(REPOS.LOCAL, GitUtilService.FILES.FILE2.toString());
-        String fileContentLocal3 = getFileContent(REPOS.LOCAL, GitUtilService.FILES.FILE3.toString());
-        String fileContentLocal4 = getFileContent(REPOS.LOCAL, GitUtilService.FILES.FILE4.toString() + ".jar");
-
-        String fileContentRemote1 = getFileContent(REPOS.REMOTE, GitUtilService.FILES.FILE1.toString());
-        String fileContentRemote2 = getFileContent(REPOS.REMOTE, GitUtilService.FILES.FILE2.toString());
-        String fileContentRemote3 = getFileContent(REPOS.REMOTE, GitUtilService.FILES.FILE3.toString());
-        String fileContentRemote4 = getFileContent(REPOS.REMOTE, GitUtilService.FILES.FILE4.toString() + ".jar");
-
-        return fileContentLocal1.equals(fileContentRemote1) && fileContentLocal2.equals(fileContentRemote2) && fileContentLocal3.equals(fileContentRemote3)
-                && fileContentLocal4.equals(fileContentRemote4);
+        return compareFileContent(REPOS.LOCAL, REPOS.REMOTE, FILES.FILE1.toString())
+            && compareFileContent(REPOS.LOCAL, REPOS.REMOTE, FILES.FILE2.toString())
+            && compareFileContent(REPOS.LOCAL, REPOS.REMOTE, FILES.FILE3.toString())
+            && compareFileContent(REPOS.LOCAL, REPOS.REMOTE, FILES.FILE4.toString() + ".jar");
     }
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseResource.java (1)

847-863: Inconsistent URL parameter handling between similar methods.

The URL parameter handling differs between the two methods:

  • redirectGetSolutionRepositoryFiles always includes the parameter: "?omitBinaries=" + omitBinaries
  • redirectGetTemplateRepositoryFiles conditionally includes it: (omitBinaries ? "?omitBinaries=" + omitBinaries : "")

Consider using the same approach in both methods for consistency.

-        return new ModelAndView("forward:/api/repository/" + participation.getId() + "/files-content" + (omitBinaries ? "?omitBinaries=" + omitBinaries : ""));
+        return new ModelAndView("forward:/api/repository/" + participation.getId() + "/files-content" + "?omitBinaries=" + omitBinaries);
src/main/java/de/tum/cit/aet/artemis/programming/service/GitService.java (1)

1035-1036: Consider making binary extensions configurable.

The list of binary extensions is currently hardcoded. Consider making it configurable through application properties to allow easier customization without code changes.

-        List<String> binaryExtensions = List.of(".exe", ".jar", ".dll", ".so", ".class", ".bin");
+        @Value("${artemis.version-control.binary-extensions}")
+        private List<String> binaryExtensions = List.of(".exe", ".jar", ".dll", ".so", ".class", ".bin");
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1fd1813 and 297416d.

📒 Files selected for processing (14)
  • src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisDTOService.java (2 hunks)
  • src/main/java/de/tum/cit/aet/artemis/programming/service/GitService.java (1 hunks)
  • src/main/java/de/tum/cit/aet/artemis/programming/service/RepositoryService.java (2 hunks)
  • src/main/java/de/tum/cit/aet/artemis/programming/service/hestia/TestwiseCoverageService.java (1 hunks)
  • src/main/java/de/tum/cit/aet/artemis/programming/service/hestia/behavioral/BehavioralTestCaseService.java (1 hunks)
  • src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseResource.java (2 hunks)
  • src/main/java/de/tum/cit/aet/artemis/programming/web/repository/RepositoryProgrammingExerciseParticipationResource.java (1 hunks)
  • src/main/webapp/app/exercises/programming/manage/services/programming-exercise.service.ts (2 hunks)
  • src/test/java/de/tum/cit/aet/artemis/programming/GitServiceTest.java (7 hunks)
  • src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseIntegrationTestService.java (3 hunks)
  • src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCIntegrationTest.java (1 hunks)
  • src/test/java/de/tum/cit/aet/artemis/programming/RepositoryIntegrationTest.java (2 hunks)
  • src/test/java/de/tum/cit/aet/artemis/programming/util/GitUtilService.java (4 hunks)
  • src/test/javascript/spec/service/programming-exercise.service.spec.ts (1 hunks)
👮 Files not reviewed due to content moderation or server errors (4)
  • src/main/java/de/tum/cit/aet/artemis/programming/service/hestia/TestwiseCoverageService.java
  • src/test/java/de/tum/cit/aet/artemis/programming/GitServiceTest.java
  • src/main/java/de/tum/cit/aet/artemis/programming/service/RepositoryService.java
  • src/main/java/de/tum/cit/aet/artemis/programming/web/repository/RepositoryProgrammingExerciseParticipationResource.java
🧰 Additional context used
📓 Path-based instructions (14)
src/test/javascript/spec/service/programming-exercise.service.spec.ts (1)

Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}

src/main/java/de/tum/cit/aet/artemis/programming/service/hestia/TestwiseCoverageService.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/repository/RepositoryProgrammingExerciseParticipationResource.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/hestia/behavioral/BehavioralTestCaseService.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/webapp/app/exercises/programming/manage/services/programming-exercise.service.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCIntegrationTest.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/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/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/test/java/de/tum/cit/aet/artemis/programming/RepositoryIntegrationTest.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/iris/service/pyris/PyrisDTOService.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/ProgrammingExerciseIntegrationTestService.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/GitServiceTest.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/ProgrammingExerciseResource.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/GitService.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 (1)
  • GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (15)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisDTOService.java (2)

69-69: Consider enabling binary file filtering for test repository.

The omitBinaries parameter is set to false, which means binary files will be included in the test repository content. Given the PR's objective to optimize request sizes by excluding binary files, consider setting this to true.


160-161: Consider enabling binary file filtering for repository content.

The omitBinaries parameter is set to false, which means binary files will be included in the repository content. Given the PR's objective to optimize request sizes by excluding binary files, consider setting this to true.

Run this script to verify the impact of including binary files:

✅ Verification successful

Consider enabling binary file filtering for repository content.

The omitBinaries parameter should be set to true to reduce response sizes by excluding binary files from repository content. This aligns with:

  • Existing support for binary filtering in repository endpoints
  • The PR's objective to optimize request sizes
  • Current practices in related services
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for binary files in repositories to assess potential impact.

# Search for common binary file extensions
echo "Searching for binary files in repositories..."
fd -e jar -e class -e war -e ear -e zip -e tar -e gz -e rar -e exe -e dll -e so -e dylib --exec ls -lh

Length of output: 2302


Script:

#!/bin/bash
# Find usages of getFilesContentFromWorkingCopy
rg "getFilesContentFromWorkingCopy" -B 2 -A 2

Length of output: 5642

src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCIntegrationTest.java (2)

187-188: LGTM!

The test method correctly tests the scenario where binary files are included.


190-193: LGTM!

Good addition of a test case to verify binary file filtering functionality.

src/main/java/de/tum/cit/aet/artemis/programming/service/hestia/behavioral/BehavioralTestCaseService.java (1)

195-195: Consider enabling binary file filtering for solution repository.

The omitBinaries parameter is set to false, which means binary files will be included in the solution repository content. Since this service is used for behavioral test case analysis, which typically focuses on source code, consider setting this to true to optimize performance.

src/test/java/de/tum/cit/aet/artemis/programming/util/GitUtilService.java (2)

51-51: LGTM!

Good addition of FILE4 to support binary file testing scenarios.


100-100: LGTM!

Good addition of a .jar file to test binary file filtering functionality.

src/test/javascript/spec/service/programming-exercise.service.spec.ts (1)

463-470: LGTM! Improved request matching flexibility.

The custom matcher function is a good improvement as it:

  1. Makes the tests more maintainable by allowing partial URL matches
  2. Separates the HTTP method check from the URL matching
  3. Reduces test brittleness
src/test/java/de/tum/cit/aet/artemis/programming/RepositoryIntegrationTest.java (2)

146-149: LGTM! Well-structured setup for binary file testing.

The binary file setup is implemented correctly using the Path API and follows the test setup pattern used in other parts of the code.


273-288: LGTM! Comprehensive test coverage for binary file exclusion.

The test method effectively verifies that:

  1. Binary files (.jar) are excluded when omitBinaries=true
  2. Non-binary files are still included
  3. The response is not empty
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseIntegrationTestService.java (2)

Line range hint 2312-2329: LGTM! Well-structured test method with good separation of concerns.

The method effectively handles both binary and non-binary test cases through the omitBinaries parameter, with clean delegation to specific test methods.


2341-2349: LGTM! Comprehensive test for binary file exclusion.

The test method:

  1. Sets up mixed content (binary and non-binary files)
  2. Verifies query parameter forwarding
  3. Uses proper assertions
src/main/webapp/app/exercises/programming/manage/services/programming-exercise.service.ts (1)

627-627: LGTM! Optimization to reduce request payload size.

The addition of ?omitBinaries=true parameter to HTTP requests will help reduce payload size by excluding binary files from the response.

Also applies to: 640-640

src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseResource.java (1)

821-837: LGTM! Well-structured parameter handling.

The addition of the optional omitBinaries parameter with a default value of false is well-implemented and follows RESTful practices.

src/main/java/de/tum/cit/aet/artemis/programming/service/GitService.java (1)

1025-1058: LGTM! Well-implemented binary file filtering.

The implementation correctly filters out binary files and maintains backward compatibility through method overloading. The logging of omitted files is helpful for debugging.

coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 8, 2025
Copy link
Contributor

@SimonEntholzer SimonEntholzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few code improvement suggestions:

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/programming/service/GitService.java (2)

99-100: Consider extending binary extensions list and moving to configuration.

While the current list covers common binary formats, consider:

  1. Adding more binary extensions (e.g., .war, .ear, .dylib, .o, .obj, .pyc)
  2. Moving this list to a configuration file for easier maintenance and environment-specific customization

1027-1050: LGTM! Consider a minor optimization for binary extension checks.

The implementation correctly filters binary files while maintaining the original functionality. For better performance with multiple files, consider:

-if (omitBinaries && nextFile.isFile() && BINARY_EXTENSIONS.stream().anyMatch(nextFile.getName()::endsWith)) {
+// Create a static Set for O(1) lookup
+private static final Set<String> BINARY_EXTENSIONS_SET = Set.of(".exe", ".jar", ".dll", ".so", ".class", ".bin", ".msi");
+
+// Use Set.contains for faster lookup
+if (omitBinaries && nextFile.isFile() && BINARY_EXTENSIONS_SET.stream().anyMatch(ext -> nextFile.getName().endsWith(ext))) {
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 297416d and 5a14ed9.

📒 Files selected for processing (4)
  • src/main/java/de/tum/cit/aet/artemis/programming/service/GitService.java (2 hunks)
  • src/main/java/de/tum/cit/aet/artemis/programming/service/RepositoryService.java (2 hunks)
  • src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseIntegrationTestService.java (3 hunks)
  • src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCIntegrationTest.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCIntegrationTest.java
  • src/main/java/de/tum/cit/aet/artemis/programming/service/RepositoryService.java
  • src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseIntegrationTestService.java
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/GitService.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

🔇 Additional comments (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/GitService.java (1)

1056-1059: LGTM! Good backward compatibility.

The overloaded method correctly maintains backward compatibility by defaulting omitBinaries to false.

coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 9, 2025
ole-ve
ole-ve previously approved these changes Jan 12, 2025
Copy link
Contributor

@ole-ve ole-ve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewed the code, looks good to me.

These changes will only affect programming-exercise worked on as part of the online editor/diff, right? i am wondering because there might be exercises for which it's valid to include executables (like a library.jar). but this should not be really relevant in this context, right?

coderabbitai[bot]
coderabbitai bot previously approved these changes Feb 8, 2025
coderabbitai[bot]
coderabbitai bot previously approved these changes Feb 9, 2025
@AjayvirS AjayvirS requested a review from HawKhiem February 9, 2025 01:14
@helios-aet helios-aet bot temporarily deployed to artemis-test4.artemis.cit.tum.de February 9, 2025 19:15 Inactive
Copy link

@flbrgit flbrgit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on TS4, binary files are excluded as expected

Copy link
Contributor

@ole-ve ole-ve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewed the code, changes look good to me.

Copy link
Contributor

@az108 az108 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code 👍

Copy link

@xHadie xHadie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good! 👍🏻

Copy link

@HawKhiem HawKhiem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reapprove

@helios-aet helios-aet bot temporarily deployed to artemis-test1.artemis.cit.tum.de February 10, 2025 22:25 Inactive
@helios-aet helios-aet bot temporarily deployed to artemis-test1.artemis.cit.tum.de February 10, 2025 22:52 Inactive
@helios-aet helios-aet bot temporarily deployed to artemis-test2.artemis.cit.tum.de February 10, 2025 22:59 Inactive
Copy link
Contributor

@MarkusPaulsen MarkusPaulsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good + tested on TS2

@MarkusPaulsen MarkusPaulsen added the maintainer-approved The feature maintainer has approved the PR label Feb 11, 2025
Copy link

@sachmii sachmii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on TS2, works as described.

Copy link

@SindiBuklaji SindiBuklaji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-approve

Copy link

@zagemello zagemello left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on TS2, all works as expected

@krusche krusche added this to the 7.10.1 milestone Feb 12, 2025
@krusche krusche merged commit 2aa1b8d into develop Feb 12, 2025
48 of 61 checks passed
@krusche krusche deleted the bugfix/programming-exercises/9929-omit-unnecessary-files-from-comparison branch February 12, 2025 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Pull requests that update TypeScript code. (Added Automatically!) core Pull requests that affect the corresponding module maintainer-approved The feature maintainer has approved the PR programming Pull requests that affect the corresponding module ready for review server Pull requests that update Java code. (Added Automatically!) tests
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

Programming Exercise: Template and solution repository comparison contains unnecessary binary files