Development: Introduce module API for Atlas

[ole-ve]

Checklist

General

• I tested all changes and their related features with all corresponding user types on a test server.
• I chose a title conforming to the naming conventions for pull requests.

Motivation and Context

As part of my master thesis, I am trying to create more well-defined boundaries between different modules. The main motivation is to improve the developer experience and test efficiency.

Description

Each module (subpackages of de.tum.cit.aet.artemis) will have its own api-subpackage. Classes from other modules are only allowed to access functionality from this api folder which acts as a facade to services (and repositories) within each module. Apart from the api-subpackage, other modules can access all classes/interfaces in domain and dto. This module API can be seen as part of the presentation layer - same as the Resources (Rest Controllers). However, instead of exposing a REST-API to the client, this module API exposes a Java-API (i.e. method calls) to other modules.

Traditional Layered Architecture Diagram:

This diagram might fall short in explaining that JavaAPI and REST-API are part of the presentation layer, but have a different way of exposing their API. The below diagram might be more explicit about this:

Hexagonal Architecture Diagram:

Access Within a Module

• A RestController can services, and repositories from the same module. We will restrict the access to repositories in the future. A RestController cannot access module APIs from the same module
• A module API can access services and repositories from the same module
• A service can access other services and repositories from the same module

Access Between Modules

• A RestController can access module APIs from other modules, but cannot access services/repositories from other modules
• A service can access other module APIs from other modules, but cannot access services/repositories from other modules
• RestControllers, ModuleAPIs, and Services can access Entities/DTOs from other modules

The access to module-external services will also enforced be enforced by Arch-Tests. Currently, only enforced for the atlas module.

Out of scope / Future

• Module APIs use repositories from the same module - we will change
  Currently, the APIs are using repositories from the same module. In the future, we will change to only use services instead of repositories.
• Extend the arch tests to enforce that controllers cannot access module APIs from the same module
• Warnings for modules that do not fullfil these requirements (with these changes, one can only enable the tests for the module which would cause the test step to fail)
• We will add the concept of "SimpleServices" (not attached to the name yet) that aim to move functionality from the Repositories to the Service-Layer). This, for instance, includes default methods or logic beyond the unimplemented interface-methods or queries).

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

Summary by CodeRabbit

• New Features

  • Introduced multiple new API controllers for managing competency progress, prerequisites, learning paths, and science events.
  • Added methods for updating competency progress asynchronously and synchronously.
  • Added new CompetencyApi for managing competencies and a LearningMetricsApi for accessing student learning metrics.

• Refactor

  • Replaced CompetencyProgressService with CompetencyProgressApi across various services and controllers, enhancing modularity and API interactions.
  • Updated service dependencies in CourseService, ExerciseService, LectureService, and others to utilize the new API structure.

• Tests

  • Updated integration tests to reflect changes in competency progress handling, ensuring they interact with CompetencyProgressApi instead of CompetencyProgressService.

[github-actions]

There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions.

[ole-ve]

What is the rational for preventing RestControllers from accessing the API from its module?

The idea is that the moduleAPI is part of the presentation layer - same as the RestControllers. They serve two different purposes. The REST Controller exposes a REST API, the module API exposes a Java API to the "outside world".

This might become relevant in the future when the moduleAPI might rather be sth like an adapter on Artemis side, whereas the RestControllers are the API on atlas' side.

[coderabbitai]

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (4)

src/main/java/de/tum/cit/aet/artemis/atlas/api/LearningMetricsApi.java (1)

11-13: Consider adding REST-specific annotations and documentation.

While the class is marked as a @controller, it might need additional annotations if it's meant to serve REST endpoints. Also, consider adding Javadoc to describe the API's purpose and usage.

Consider adding:

+/**
+ * API facade for accessing learning metrics functionality.
+ * Part of the Atlas module's public API, providing access to student metrics.
+ */
 @Controller
+@RequestMapping("/api/atlas/metrics")
 @Profile(PROFILE_CORE)
 public class LearningMetricsApi extends AbstractAtlasApi {

src/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyProgressApi.java (2)

29-32: Add null validation for constructor parameters.

While constructor injection is correctly used, consider adding null validation to ensure the dependencies are properly provided:

 public CompetencyProgressApi(CompetencyProgressService competencyProgressService, CompetencyRepository competencyRepository) {
+    if (competencyProgressService == null || competencyRepository == null) {
+        throw new IllegalArgumentException("Dependencies cannot be null");
+    }
     this.competencyProgressService = competencyProgressService;
     this.competencyRepository = competencyRepository;
 }

---

54-60: Consider moving repository access to service layer.

Direct repository access in the API layer might violate the layering principles. Consider moving these operations to the service layer:

-public long countByCourse(Course course) {
-    return competencyRepository.countByCourse(course);
-}
+public long countByCourse(Course course) {
+    return competencyProgressService.countCompetenciesByCourse(course);
+}

-public void deleteAll(Set<Competency> competencies) {
-    competencyRepository.deleteAll(competencies);
-}
+public void deleteAll(Set<Competency> competencies) {
+    competencyProgressService.deleteCompetencies(competencies);
+}

src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java (1)

30-30: Implementation aligns with module-API architecture

The usage of CompetencyApi from the atlas module demonstrates the intended architectural pattern where RestControllers access other modules through their API facades rather than directly accessing their services or repositories. This promotes loose coupling and clear module boundaries.

Also applies to: 70-70

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c8ffd55 and 07fae72.

📒 Files selected for processing (6)

• src/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyApi.java (1 hunks)
• src/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyProgressApi.java (1 hunks)
• src/main/java/de/tum/cit/aet/artemis/atlas/api/LearningMetricsApi.java (1 hunks)
• src/main/java/de/tum/cit/aet/artemis/core/config/migration/entries/MigrationEntry20240614_140000.java (2 hunks)
• src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisPipelineService.java (3 hunks)
• src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java (6 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• src/main/java/de/tum/cit/aet/artemis/core/config/migration/entries/MigrationEntry20240614_140000.java
• src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisPipelineService.java

🧰 Additional context used

📓 Path-based instructions (4)

src/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyApi.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/atlas/api/CompetencyProgressApi.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/atlas/api/LearningMetricsApi.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/lecture/web/LectureResource.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 (13)

src/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyApi.java (3)

1-10: LGTM! Package structure aligns with module API design.

The package structure correctly implements the new module-API approach, and imports follow best practices.

---

15-19: LGTM! Constructor injection properly implemented.

The implementation follows best practices with constructor injection and final field.

---

1-24: Verify module boundary implementation.

To ensure this API properly implements the module boundary pattern, we should verify that no other modules directly access the CompetencyService.

✅ Verification successful

Based on the search results, I can now provide a final response:

Module boundary implementation is correctly enforced

The CompetencyService is properly encapsulated within the atlas module:

• The service is defined in atlas/service/competency/CompetencyService.java
• All access to the service is through the CompetencyApi controller in the atlas module
• No direct usage of CompetencyService is found outside the atlas module
• The service hierarchy (CompetencyService extends CourseCompetencyService) is contained within the atlas module

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for direct CompetencyService usage outside the atlas module
rg -l "CompetencyService" | grep -v "atlas/" | grep "src/main/java"

Length of output: 63

---

Script:

#!/bin/bash
# Let's try a more comprehensive search to verify the module boundary
# 1. Find all references to CompetencyService
rg "CompetencyService" -A 3 -B 1

# 2. Also check for any potential imports of the service
rg "import.*CompetencyService"

# 3. Use ast-grep to find any field declarations or constructor parameters of CompetencyService type
ast-grep --pattern 'CompetencyService $_'

Length of output: 109795

---

Script:

#!/bin/bash
# Let's search for direct usage of CompetencyService in the atlas module
rg -l "CompetencyService" "src/main/java/de/tum/cit/aet/artemis/atlas/"

# Also check for any classes that extend or implement CompetencyService
ast-grep --pattern 'class $_ implements CompetencyService'
ast-grep --pattern 'class $_ extends CompetencyService'

# Check for any other references to CompetencyService in Java files
rg "CompetencyService" "src/main/java" -t java

Length of output: 10411

src/main/java/de/tum/cit/aet/artemis/atlas/api/LearningMetricsApi.java (3)

1-10: LGTM! Package structure and imports are well-organized.

The package structure aligns with the PR objectives of creating clear module boundaries through api subpackages. Imports are specific and follow best practices.

---

15-19: LGTM! Clean dependency injection implementation.

The code follows best practices with constructor injection and proper encapsulation of the service dependency.

---

1-24: Verify module access rules compliance.

Let's verify that this API implementation properly enforces the module boundaries described in the PR objectives.

✅ Verification successful

Module boundaries are properly implemented

The implementation correctly enforces module boundaries:

• LearningMetricsService is only accessed within the atlas module through:
  • LearningMetricsApi (facade)
  • MetricsResource (REST controller)
• External modules (like iris) correctly use the LearningMetricsApi instead of accessing the service directly
• The REST endpoint properly encapsulates the service access with appropriate authorization checks

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that only appropriate classes access the LearningMetricsService directly

# Check for direct service access from outside the atlas module
rg -t java "import.*LearningMetricsService" --glob "!src/main/java/de/tum/cit/aet/artemis/atlas/**"

# Check for proper API usage pattern in REST controllers
ast-grep --pattern 'class $_ implements $_ {
  $$$
  private final LearningMetricsApi $_;
  $$$
}'

Length of output: 326

---

Script:

#!/bin/bash
# Let's try a different approach to verify module boundaries

# 1. Check all imports of LearningMetricsService across the codebase
rg -l "LearningMetricsService"

# 2. Check all usages of LearningMetricsApi across the codebase
rg -l "LearningMetricsApi"

# 3. Look for any direct instantiation of LearningMetricsService
ast-grep --pattern 'new LearningMetricsService'

# 4. Check if there are any other classes in the atlas module that expose LearningMetricsService
fd -e java -p "src/main/java/de/tum/cit/aet/artemis/atlas" --exec grep -l "LearningMetricsService" {}

Length of output: 928

---

Script:

#!/bin/bash
# Let's check the specific implementations to verify the module boundary pattern

# Check MetricsResource implementation to see how it uses LearningMetricsService
rg -A 10 "class MetricsResource" src/main/java/de/tum/cit/aet/artemis/atlas/web/MetricsResource.java

# Check PyrisPipelineService to see how it uses LearningMetricsApi
rg -A 10 "class PyrisPipelineService" src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisPipelineService.java

# Look for any REST endpoints that might expose the service directly
ast-grep --pattern '@$_(value = $_)
class MetricsResource {
  $$$
}'

Length of output: 1235

---

Script:

#!/bin/bash
# Let's verify the REST endpoint implementation and service usage

# Check the full MetricsResource implementation for endpoint definitions
rg -g "MetricsResource.java" "@.*Mapping.*|@RestController" -A 5

# Check if LearningMetricsService is properly encapsulated
rg "private.*LearningMetricsService" src/main/java/de/tum/cit/aet/artemis/atlas/service/LearningMetricsService.java

# Check if there are any other direct usages of StudentMetricsDTO
rg -l "StudentMetricsDTO"

Length of output: 2999

src/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyProgressApi.java (4)

21-23: Consider using @Service instead of @Controller.

Since this class acts as a facade API rather than handling HTTP requests directly, @Service would be more appropriate than @Controller. This better reflects its role in the application architecture and aligns with Spring's component stereotypes.

---

1-23: LGTM: Package structure aligns with module boundary objectives.

The placement of this API class in the de.tum.cit.aet.artemis.atlas.api package perfectly aligns with the PR's objective of establishing clearer module boundaries. This structure helps enforce the architectural rule that other modules can only access atlas functionality through this API facade.

---

34-52: 🛠️ Refactor suggestion

Enhance async method implementations.

The async methods should:

1. Be annotated with @Async
2. Return CompletableFuture for proper async handling
3. Include error handling

Example for one method:

+@Async
-public void updateProgressByLearningObjectForParticipantAsync(LearningObject learningObject, Participant participant) {
+public CompletableFuture<Void> updateProgressByLearningObjectForParticipantAsync(LearningObject learningObject, Participant participant) {
-    competencyProgressService.updateProgressByLearningObjectForParticipantAsync(learningObject, participant);
+    return CompletableFuture.runAsync(() -> {
+        try {
+            competencyProgressService.updateProgressByLearningObjectForParticipantAsync(learningObject, participant);
+        } catch (Exception e) {
+            throw new CompletionException("Failed to update progress", e);
+        }
+    });
 }

Likely invalid or redundant comment.

---

67-73: 🛠️ Refactor suggestion

Optimize batch processing in updateProgressForCoursesAsync.

The current implementation has performance concerns:

1. Nested loops could be inefficient for large datasets
2. No batch processing for database operations
3. No progress tracking or error handling

Consider this optimization:

-public void updateProgressForCoursesAsync(List<Course> activeCourses) {
+public CompletableFuture<Void> updateProgressForCoursesAsync(List<Course> activeCourses) {
+    return CompletableFuture.runAsync(() -> {
+        List<CompletableFuture<Void>> futures = new ArrayList<>();
         activeCourses.forEach(course -> {
-            List<Competency> competencies = competencyRepository.findByCourseIdOrderById(course.getId());
-            competencies.forEach(competencyProgressService::updateProgressByCompetencyAsync);
+            // Batch fetch competencies
+            List<Competency> competencies = competencyProgressService.findByCourseIdBatch(course.getId());
+            
+            // Process in batches of 100
+            Lists.partition(competencies, 100).forEach(batch -> {
+                futures.add(CompletableFuture.allOf(
+                    batch.stream()
+                        .map(competencyProgressService::updateProgressByCompetencyAsync)
+                        .toArray(CompletableFuture[]::new)
+                ));
+            });
         });
+        
+        // Wait for all batches to complete
+        CompletableFuture.allOf(futures.toArray(new CompletableFuture[0])).join();
+    });
 }

Likely invalid or redundant comment.

src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java (3)

30-30: LGTM! Import change aligns with module-API architecture

The import of CompetencyApi from the atlas module's API package follows the new architectural boundaries.

---

70-70: LGTM! Field declaration follows best practices

The field declaration follows Java best practices:

• Proper encapsulation with private
• Immutability with final
• Clear naming convention

---

Line range hint 95-105: LGTM! Constructor follows dependency injection best practices

The constructor properly initializes all dependencies through constructor injection, maintaining immutability and clear dependency management.

[JohannesStoehr]

Code

[raffifasaro]

re-approve

[MaximilianAnzinger]

Maintainer approved

[N0W0RK]

deleted

[coderabbitai]

Actionable comments posted: 2

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 07fae72 and a93d358.

📒 Files selected for processing (3)

• src/main/java/de/tum/cit/aet/artemis/core/service/CourseService.java (11 hunks)
• src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java (6 hunks)
• src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractArtemisIntegrationTest.java (3 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java
• src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractArtemisIntegrationTest.java

🧰 Additional context used

📓 Path-based instructions (1)

src/main/java/de/tum/cit/aet/artemis/core/service/CourseService.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 (2)

src/main/java/de/tum/cit/aet/artemis/core/service/CourseService.java (2)

621-621: Ensure proper error handling for generateLearningPathForUser call

Consider handling potential exceptions from learningPathApi.generateLearningPathForUser(course, user) to prevent enrollment failures if the learning path generation fails.

Apply this diff to add exception handling:

+        try {
            learningPathApi.generateLearningPathForUser(course, user);
+        } catch (Exception e) {
+            log.error("Could not generate learning path for user {}: {}", user.getLogin(), e.getMessage());
+        }

---

654-654: Ensure proper error handling when generating learning paths for users

Consider handling potential exceptions from learningPathApi.generateLearningPathForUser(course, optionalStudent.get()) to prevent registration failures if the learning path generation encounters an error.

Apply this diff to add exception handling:

                     } else if (courseGroupRole == Role.STUDENT && course.getLearningPathsEnabled()) {
+                    try {
                        learningPathApi.generateLearningPathForUser(course, optionalStudent.get());
+                    } catch (Exception e) {
+                        log.error("Could not generate learning path for user {}: {}", optionalStudent.get().getLogin(), e.getMessage());
+                    }
                     }