Programming exercises: Create a ExerciseVersion on Exercise creation/updates on new flush to database. (all exercise types)
#11351
Conversation
…ersioning/versioning-basics-and-text-exercise-versioning
…ated exercise version creation
github-actions
bot
added
tests
server
Elfari1028
changed the title
…ercise-snapshot-during-hibernate-post-flush
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
bensofficial
left a comment
bensofficial
left a comment
There was a problem hiding this comment.
DB migration and new exercise_version table looks good
|
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. |
…ersioning/create-new-exercise-snapshot-during-hibernate-post-flush
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
…ercise-snapshot-during-hibernate-post-flush
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/versioning/config/ExerciseVersioningEnabled.java (1)
13-19: LGTM with optional optimization.The implementation correctly instantiates
ArtemisConfigHelperin the constructor. While this doesn't follow typical dependency injection patterns, it's appropriate for SpringConditionclasses, which are evaluated before the full application context is available.Optional optimization: Since
ArtemisConfigHelperappears stateless, consider making the field static to avoid creating multiple instances if this condition is evaluated multiple times:- private final ArtemisConfigHelper artemisConfigHelper; + private static final ArtemisConfigHelper ARTEMIS_CONFIG_HELPER = new ArtemisConfigHelper(); public ExerciseVersioningEnabled() { - this.artemisConfigHelper = new ArtemisConfigHelper(); }Then update the
matchesmethod to use the static reference:@Override public boolean matches(ConditionContext context, AnnotatedTypeMetadata metadata) { - return artemisConfigHelper.isExerciseVersioningEnabled(context.getEnvironment()); + return ARTEMIS_CONFIG_HELPER.isExerciseVersioningEnabled(context.getEnvironment()); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/versioning/config/ExerciseVersioningEnabled.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
Files:
src/main/java/de/tum/cit/aet/artemis/versioning/config/ExerciseVersioningEnabled.java
🧬 Code graph analysis (1)
src/main/java/de/tum/cit/aet/artemis/versioning/config/ExerciseVersioningEnabled.java (1)
src/main/java/de/tum/cit/aet/artemis/core/config/ArtemisConfigHelper.java (1)
ArtemisConfigHelper(16-116)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: server-tests
- GitHub Check: Analyse
- GitHub Check: bean-instantiation-check
🔇 Additional comments (2)
src/main/java/de/tum/cit/aet/artemis/versioning/config/ExerciseVersioningEnabled.java (2)
1-7: LGTM!Package and imports are correctly structured with no wildcard imports.
21-24: LGTM!The
matchesmethod correctly delegates toArtemisConfigHelperand properly retrieves theEnvironmentfrom theConditionContext.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/versioning/service/ExerciseVersionEntityListener.java (1)
45-51: Consider documenting the Exercise exclusion logic.The early return for
Exerciseinstances inhandlePostRemoveappears intentional (likely to avoid versioning exercise deletions), but this logic would benefit from a brief comment explaining the rationale.Example:
@PostRemove public void handlePostRemove(Object entity) { + // Exercise deletions are not versioned; only changes to related entities trigger versions if (entity instanceof Exercise) { return; } handleEntityChange(entity); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/versioning/service/ExerciseVersionEntityListener.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
Files:
src/main/java/de/tum/cit/aet/artemis/versioning/service/ExerciseVersionEntityListener.java
🧬 Code graph analysis (1)
src/main/java/de/tum/cit/aet/artemis/versioning/service/ExerciseVersionEntityListener.java (1)
src/main/webapp/app/atlas/shared/entities/competency.model.ts (1)
CompetencyExerciseLink(103-110)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: client-style
- GitHub Check: client-tests
- GitHub Check: bean-instantiation-check
- GitHub Check: Analyse
| else if (entity instanceof CompetencyExerciseLink link) { | ||
| if (link.getExercise() == null) { | ||
| return; | ||
| } | ||
| publishExerciseChangedEvent(link.getExercise().getId(), link.getExercise().getExerciseType()); | ||
| } | ||
| else if (entity instanceof AuxiliaryRepository repository) { | ||
| if (repository.getExercise() == null) { | ||
| return; | ||
| } | ||
| publishExerciseChangedEvent(repository.getExercise().getId(), repository.getExercise().getExerciseType()); | ||
| } | ||
| else if (entity instanceof StaticCodeAnalysisCategory category) { | ||
| if (category.getExercise() == null) { | ||
| return; | ||
| } | ||
| publishExerciseChangedEvent(category.getExercise().getId(), category.getExercise().getExerciseType()); | ||
| } | ||
| else if (entity instanceof SubmissionPolicy submissionPolicy) { | ||
| if (submissionPolicy.getProgrammingExercise() == null) { | ||
| return; | ||
| } | ||
| publishExerciseChangedEvent(submissionPolicy.getProgrammingExercise().getId(), submissionPolicy.getProgrammingExercise().getExerciseType()); | ||
| } | ||
| else if (entity instanceof ProgrammingExerciseBuildConfig config) { | ||
| if (config.getProgrammingExercise() == null) { | ||
| return; | ||
| } | ||
| publishExerciseChangedEvent(config.getProgrammingExercise().getId(), config.getProgrammingExercise().getExerciseType()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Eliminate code duplication with a helper method.
Lines 57–86 repeat the same pattern five times: null-check an exercise reference, then call its getters twice. This violates DRY and the code reuse guideline. Extract the pattern into a helper method to improve maintainability.
Example refactor:
private void handleEntityChange(Object entity) {
if (entity instanceof Exercise exercise) {
publishExerciseChangedEvent(exercise.getId(), exercise.getExerciseType());
- }
- else if (entity instanceof CompetencyExerciseLink link) {
- if (link.getExercise() == null) {
- return;
- }
- publishExerciseChangedEvent(link.getExercise().getId(), link.getExercise().getExerciseType());
- }
- else if (entity instanceof AuxiliaryRepository repository) {
- if (repository.getExercise() == null) {
- return;
- }
- publishExerciseChangedEvent(repository.getExercise().getId(), repository.getExercise().getExerciseType());
- }
- else if (entity instanceof StaticCodeAnalysisCategory category) {
- if (category.getExercise() == null) {
- return;
- }
- publishExerciseChangedEvent(category.getExercise().getId(), category.getExercise().getExerciseType());
- }
- else if (entity instanceof SubmissionPolicy submissionPolicy) {
- if (submissionPolicy.getProgrammingExercise() == null) {
- return;
- }
- publishExerciseChangedEvent(submissionPolicy.getProgrammingExercise().getId(), submissionPolicy.getProgrammingExercise().getExerciseType());
- }
- else if (entity instanceof ProgrammingExerciseBuildConfig config) {
- if (config.getProgrammingExercise() == null) {
- return;
- }
- publishExerciseChangedEvent(config.getProgrammingExercise().getId(), config.getProgrammingExercise().getExerciseType());
+ }
+ else if (entity instanceof CompetencyExerciseLink link) {
+ publishEventForExercise(link.getExercise());
+ }
+ else if (entity instanceof AuxiliaryRepository repository) {
+ publishEventForExercise(repository.getExercise());
+ }
+ else if (entity instanceof StaticCodeAnalysisCategory category) {
+ publishEventForExercise(category.getExercise());
+ }
+ else if (entity instanceof SubmissionPolicy submissionPolicy) {
+ publishEventForExercise(submissionPolicy.getProgrammingExercise());
+ }
+ else if (entity instanceof ProgrammingExerciseBuildConfig config) {
+ publishEventForExercise(config.getProgrammingExercise());
}
}
+
+private void publishEventForExercise(Exercise exercise) {
+ if (exercise == null) {
+ return;
+ }
+ publishExerciseChangedEvent(exercise.getId(), exercise.getExerciseType());
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| else if (entity instanceof CompetencyExerciseLink link) { | |
| if (link.getExercise() == null) { | |
| return; | |
| } | |
| publishExerciseChangedEvent(link.getExercise().getId(), link.getExercise().getExerciseType()); | |
| } | |
| else if (entity instanceof AuxiliaryRepository repository) { | |
| if (repository.getExercise() == null) { | |
| return; | |
| } | |
| publishExerciseChangedEvent(repository.getExercise().getId(), repository.getExercise().getExerciseType()); | |
| } | |
| else if (entity instanceof StaticCodeAnalysisCategory category) { | |
| if (category.getExercise() == null) { | |
| return; | |
| } | |
| publishExerciseChangedEvent(category.getExercise().getId(), category.getExercise().getExerciseType()); | |
| } | |
| else if (entity instanceof SubmissionPolicy submissionPolicy) { | |
| if (submissionPolicy.getProgrammingExercise() == null) { | |
| return; | |
| } | |
| publishExerciseChangedEvent(submissionPolicy.getProgrammingExercise().getId(), submissionPolicy.getProgrammingExercise().getExerciseType()); | |
| } | |
| else if (entity instanceof ProgrammingExerciseBuildConfig config) { | |
| if (config.getProgrammingExercise() == null) { | |
| return; | |
| } | |
| publishExerciseChangedEvent(config.getProgrammingExercise().getId(), config.getProgrammingExercise().getExerciseType()); | |
| } | |
| private void handleEntityChange(Object entity) { | |
| if (entity instanceof Exercise exercise) { | |
| publishExerciseChangedEvent(exercise.getId(), exercise.getExerciseType()); | |
| } | |
| else if (entity instanceof CompetencyExerciseLink link) { | |
| publishEventForExercise(link.getExercise()); | |
| } | |
| else if (entity instanceof AuxiliaryRepository repository) { | |
| publishEventForExercise(repository.getExercise()); | |
| } | |
| else if (entity instanceof StaticCodeAnalysisCategory category) { | |
| publishEventForExercise(category.getExercise()); | |
| } | |
| else if (entity instanceof SubmissionPolicy submissionPolicy) { | |
| publishEventForExercise(submissionPolicy.getProgrammingExercise()); | |
| } | |
| else if (entity instanceof ProgrammingExerciseBuildConfig config) { | |
| publishEventForExercise(config.getProgrammingExercise()); | |
| } | |
| } | |
| private void publishEventForExercise(Exercise exercise) { | |
| if (exercise == null) { | |
| return; | |
| } | |
| publishExerciseChangedEvent(exercise.getId(), exercise.getExerciseType()); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/de/tum/cit/aet/artemis/versioning/service/ExerciseVersionEntityListener.java
around lines 57 to 86, multiple branches repeat the same pattern of
null-checking an exercise (or programmingExercise) and then calling its getters
twice; extract this into a single private helper method that accepts the
exercise (or programming exercise) object, performs a single null-check, reads
id and exerciseType into local variables, and calls
publishExerciseChangedEvent(id, exerciseType); replace each duplicated branch
with a call to that helper so getters are not invoked twice and the logic is
centralized.
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
…able event publishing when set to false.
…ersioning/create-new-exercise-snapshot-during-hibernate-post-flush
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
…ercise-snapshot-during-hibernate-post-flush
…t-during-hibernate-post-flush' of github.com:ls1intum/Artemis into feature/exercise-versioning/create-new-exercise-snapshot-during-hibernate-post-flush
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
github-actions
bot
removed
the
config-change
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/test/java/de/tum/cit/aet/artemis/versioning/service/ExerciseVersionServiceTest.java (1)
109-112: Restore base teardown cleanup.This overrides
AbstractProgrammingIntegrationLocalCILocalVCTestBase.tearDown()but never callssuper, so build-job cleanup from the base class stops running. Subsequent programming tests will see leftover build jobs.Delegate to the superclass before deleting exercise versions:
- @AfterEach - void tearDown() { - exerciseVersionRepository.deleteAll(); - } + @AfterEach + @Override + void tearDown() { + super.tearDown(); + exerciseVersionRepository.deleteAll(); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/de/tum/cit/aet/artemis/versioning/service/ExerciseVersionEntityListener.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/versioning/service/ExerciseVersionService.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/versioning/service/ExerciseVersionServiceTest.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
Files:
src/main/java/de/tum/cit/aet/artemis/versioning/service/ExerciseVersionEntityListener.javasrc/main/java/de/tum/cit/aet/artemis/versioning/service/ExerciseVersionService.java
src/test/java/**/*.java
⚙️ CodeRabbit configuration file
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
Files:
src/test/java/de/tum/cit/aet/artemis/versioning/service/ExerciseVersionServiceTest.java
🧬 Code graph analysis (2)
src/test/java/de/tum/cit/aet/artemis/versioning/service/ExerciseVersionServiceTest.java (2)
src/test/java/de/tum/cit/aet/artemis/programming/AbstractProgrammingIntegrationLocalCILocalVCTestBase.java (1)
AbstractProgrammingIntegrationLocalCILocalVCTestBase(51-234)src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseFactory.java (1)
ProgrammingExerciseFactory(50-512)
src/main/java/de/tum/cit/aet/artemis/versioning/service/ExerciseVersionService.java (1)
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
Constants(8-526)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: client-tests
- GitHub Check: server-style
- GitHub Check: client-style
- GitHub Check: server-tests
- GitHub Check: Analyse
- GitHub Check: bean-instantiation-check
| private void createExerciseVersion(Long exerciseId, ExerciseType exerciseType, String userLogin) { | ||
| try { | ||
| User author = userRepository.findOneByLogin(userLogin).orElse(null); | ||
| if (author == null) { | ||
| log.error("No active user during exercise version creation check"); | ||
| return; | ||
| } | ||
| Exercise exercise = fetchExerciseEagerly(exerciseId, exerciseType); | ||
| if (exercise == null) { | ||
| log.error("Exercise with id {} not found", exerciseId); | ||
| return; | ||
| } | ||
| ExerciseVersion exerciseVersion = new ExerciseVersion(); | ||
| exerciseVersion.setExercise(exercise); | ||
| exerciseVersion.setAuthor(author); | ||
| ExerciseSnapshotDTO exerciseSnapshot = ExerciseSnapshotDTO.of(exercise, gitService); | ||
| Optional<ExerciseVersion> previousVersion = exerciseVersionRepository.findTopByExerciseIdOrderByCreatedDateDesc(exercise.getId()); | ||
| if (previousVersion.isPresent()) { | ||
| ExerciseSnapshotDTO previousVersionSnapshot = previousVersion.get().getExerciseSnapshot(); | ||
| boolean equal = previousVersionSnapshot.equals(exerciseSnapshot); | ||
| if (equal) { | ||
| log.info("Exercise {} has no versionable changes from last version", exercise.getId()); | ||
| return; | ||
| } | ||
| } | ||
| exerciseVersion.setExerciseSnapshot(exerciseSnapshot); | ||
| ExerciseVersion savedExerciseVersion = exerciseVersionRepository.save(exerciseVersion); | ||
| log.info("Exercise version {} has been created for exercise {}", savedExerciseVersion.getId(), exercise.getId()); |
There was a problem hiding this comment.
Race condition creates duplicate snapshots.
createExerciseVersion runs outside a shared transaction: two concurrent events can both read the same “latest” version, see no changes, and each save a new ExerciseVersion. We need a transactional section plus a pessimistic lock on the latest row before comparing.
Please wrap the compare-and-save in a transaction and lock the latest version, e.g.:
@@
- private void createExerciseVersion(Long exerciseId, ExerciseType exerciseType, String userLogin) {
+ @Transactional
+ public void createExerciseVersion(Long exerciseId, ExerciseType exerciseType, String userLogin) {
@@
- Optional<ExerciseVersion> previousVersion = exerciseVersionRepository.findTopByExerciseIdOrderByCreatedDateDesc(exercise.getId());
+ Optional<ExerciseVersion> previousVersion = exerciseVersionRepository
+ .findTopByExerciseIdOrderByCreatedDateDescForUpdate(exercise.getId());and add in ExerciseVersionRepository:
@Lock(LockModeType.PESSIMISTIC_WRITE)
@Query("""
select v
from ExerciseVersion v
where v.exercise.id = :exerciseId
order by v.createdDate desc
""")
Optional<ExerciseVersion> findTopByExerciseIdOrderByCreatedDateDescForUpdate(@Param("exerciseId") Long exerciseId);Call the transactional method from the async listener via the Spring proxy (e.g., inject ExerciseVersionService into itself or use TransactionTemplate) so the lock actually applies.
🤖 Prompt for AI Agents
In
src/main/java/de/tum/cit/aet/artemis/versioning/service/ExerciseVersionService.java
around lines 90 to 117, the compare-and-save flow is vulnerable to a race that
can create duplicate ExerciseVersion rows; fix it by moving the portion that
reads the latest version, compares snapshots and saves the new version into a
dedicated @Transactional method and acquire a pessimistic write lock on the
latest version row before comparing; add a repository query method annotated
with @Lock(LockModeType.PESSIMISTIC_WRITE) that selects the latest
ExerciseVersion for an exercise (ordered by createdDate desc) and use that
method inside the new transactional method, then ensure the transactional method
is invoked through the Spring proxy (self-inject the service or use
TransactionTemplate) from the async listener so the transaction and lock are
actually applied.
There was a problem hiding this comment.
when using order by, using Optional<> is not allowed. return type must be list. therefore we cannot fetch single most recent exercise version with jpql.
There was a problem hiding this comment.
@Elfari1028, I appreciate your feedback, but I need to clarify: Spring Data JPA does support Optional<> as a return type with ORDER BY in both derived queries and @Query methods. Your current code already uses this pattern successfully:
Optional<ExerciseVersion> findTopByExerciseIdOrderByCreatedDateDesc(Long exerciseId);For the pessimistic lock query, this should work:
@Lock(LockModeType.PESSIMISTIC_WRITE)
@Query("""
SELECT v FROM ExerciseVersion v
WHERE v.exercise.id = :exerciseId
ORDER BY v.createdDate DESC
""")
Optional<ExerciseVersion> findTopByExerciseIdOrderByCreatedDateDescForUpdate(@Param("exerciseId") Long exerciseId);However, if you prefer using List<> or encountered a specific error, here's an alternative:
@Lock(LockModeType.PESSIMISTIC_WRITE)
@Query("""
SELECT v FROM ExerciseVersion v
WHERE v.exercise.id = :exerciseId
ORDER BY v.createdDate DESC
""")
List<ExerciseVersion> findByExerciseIdOrderByCreatedDateDescForUpdate(@Param("exerciseId") Long exerciseId);Then in the service:
List<ExerciseVersion> versions = exerciseVersionRepository.findByExerciseIdOrderByCreatedDateDescForUpdate(exercise.getId());
Optional<ExerciseVersion> previousVersion = versions.isEmpty() ? Optional.empty() : Optional.of(versions.get(0));Important: Regardless of the return type, the race condition I flagged is critical and must be addressed. Without proper locking and transactional boundaries, concurrent events will create duplicate versions. The solution requires:
- A
@Transactionalmethod for the compare-and-save logic - Pessimistic locking on the latest version row
- Proper proxy invocation to ensure the transaction applies
If you encountered a specific error message when trying Optional<> with ORDER BY, please share it so I can help diagnose the actual issue.
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
…ersioning/create-new-exercise-snapshot-during-hibernate-post-flush
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/de/tum/cit/aet/artemis/versioning/service/ExerciseVersionService.java (1)
90-117: Transactional gap allows duplicate snapshots under load
createExerciseVersionstill runs outside a shared transaction: two async events for the same exercise can both read the same “latest” version, pass the equality check, and eachsavea new row. We need the compare+save to execute within a single transaction that holds a pessimistic write lock on the latest version record. Please refactor so the logic executes via a proxied@Transactionalmethod (e.g. self-injected proxy orTransactionTemplate) and replace the plain lookup with afindTop...ForUpdate(...)query annotated with@Lock(PESSIMISTIC_WRITE), handling the ordered result. This guarantees only one writer proceeds per exercise.
🧹 Nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/versioning/domain/ExerciseVersion.java (1)
61-75: Keep entity reference fields in sync with their IDs
setExercise/setAuthoronly copy the IDs, sogetExercise()/getAuthor()still returnnulluntil the entity is reloaded. That’s surprising for callers and breaks the usual setter/getter contract. Please store the association as well (null-safe), e.g.:- public void setExercise(Exercise exercise) { - this.exerciseId = exercise.getId(); - } + public void setExercise(Exercise exercise) { + this.exercise = exercise; + this.exerciseId = exercise != null ? exercise.getId() : null; + } ... - public void setAuthor(User author) { - this.authorId = author.getId(); - } + public void setAuthor(User author) { + this.author = author; + this.authorId = author != null ? author.getId() : null; + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/bean-instantiations.ymlis excluded by!**/*.yml
📒 Files selected for processing (2)
src/main/java/de/tum/cit/aet/artemis/versioning/domain/ExerciseVersion.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/versioning/service/ExerciseVersionService.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
Files:
src/main/java/de/tum/cit/aet/artemis/versioning/service/ExerciseVersionService.javasrc/main/java/de/tum/cit/aet/artemis/versioning/domain/ExerciseVersion.java
🧬 Code graph analysis (2)
src/main/java/de/tum/cit/aet/artemis/versioning/service/ExerciseVersionService.java (1)
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
Constants(8-526)
src/main/java/de/tum/cit/aet/artemis/versioning/domain/ExerciseVersion.java (1)
src/main/java/de/tum/cit/aet/artemis/exercise/domain/Exercise.java (1)
Entity(75-903)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: Analyse
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: server-tests
- GitHub Check: server-style
- GitHub Check: bean-instantiation-check
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
| // Entity references (for queries, read-only) | ||
| @ManyToOne(fetch = FetchType.LAZY) | ||
| @JoinColumn(name = "exercise_id", insertable = false, updatable = false) | ||
| private Exercise exercise; |
There was a problem hiding this comment.
Do we need the full exercise materialized here? I would assume exerciseId above should be sufficient, right?
(also keep in mind that ManyToOne is always eager and cannot really be lazy)
|
|
||
| @ManyToOne(fetch = FetchType.LAZY) | ||
| @JoinColumn(name = "author_id", insertable = false, updatable = false) | ||
| private User author; |
There was a problem hiding this comment.
Do we need the full author materialized here? I would assume authorId above should be sufficient, right?
(also keep in mind that ManyToOne is always eager and cannot really be lazy)
There was a problem hiding this comment.
will have this removed in the new PR, and save the authorId only. same with exerciseId
|
Closing this PR in favor of explicit exercise version creation on specific endpoints: |
Checklist
General
There are no UI changes in the PR, existing logic does not get interrupted
Server
Motivation and Context
#10967
This change sets up exercise creation/updates to be automatically captured for maintaining a versioning log. This will enable future features such as exercise rollbacks, and iterative reviews on exercises (problem statement + code).
Description
There are no UI changes in the PR, existing logic does not get interrupted
an
EntityListeneris set up forExerciseand a few related entities.ExerciseChangedEventwill be published toApplicationContext.ExerciseVersionService, upon receiving the event, will add it to a queue and batch process it every second.createNewExerciseVersionwill fetch the exercise eagerly with fields used in versioning, creates a snapshot DTO from the exercise, compares with last version's snapshot (if exists), then savesExerciseVersionto database.Steps for Testing
Prerequisites:
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Performance Review
Code Review
Test Coverage
Extensive assertions for server tests are implemented
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests