fix(server): add CHECK constraint on practice_finding.target_type - #953
Conversation
Closes the gap from PE Audit #4: every other enum-like column on practice_finding has a CHECK constraint except target_type. - Add PracticeFindingTargetType enum with @Enumerated(STRING), matching the Verdict/Severity/CaMethod pattern used across the entity - Liquibase migration: normalise existing 'pull_request' rows to 'PULL_REQUEST', then add CHECK constraint with precondition guard - Type the targetType field as enum throughout: entity, DTOs, event record, repository queries, and delivery service - Regenerate OpenAPI spec and webapp TypeScript types Closes #930 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis pull request introduces type safety for the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
bug
github-actions
Bot
added
the
size:L
…e-finding-target-type-constraint
There was a problem hiding this comment.
Pull request overview
Adds a first-class enum + database constraint for practice_finding.target_type to prevent invalid/casing-typo values and align the column with the other enum-like fields on practice_finding.
Changes:
- Introduces
PracticeFindingTargetTypeand switchesPracticeFinding.targetTypefromStringto@Enumerated(EnumType.STRING). - Adds a Liquibase changelog to normalize existing
target_typevalues and add a CHECK constraint. - Updates repository/service/event/DTO layers plus OpenAPI + generated webapp types and test fixtures to use
PULL_REQUEST.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| webapp/src/api/types.gen.ts | Updates generated TS types so targetType is the 'PULL_REQUEST' literal. |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackControllerIntegrationTest.java | Updates test setup to use PracticeFindingTargetType.PULL_REQUEST. |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/practices/finding/PracticeFindingRepositoryIntegrationTest.java | Updates inserts/assertions to PULL_REQUEST and adds an enum mapping test. |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/practices/finding/PracticeFindingControllerIntegrationTest.java | Updates controller integration tests to expect PULL_REQUEST. |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/handler/PracticeDetectionDeliveryServiceTest.java | Updates unit test expectations for idempotency key and event typing. |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/handler/PracticeDetectionDeliveryServiceIntegrationTest.java | Updates integration test assertions for enum-typed event field. |
| server/application-server/src/main/resources/db/master.xml | Includes the new Liquibase changelog in the master list. |
| server/application-server/src/main/resources/db/changelog/1774700000000_changelog.xml | Normalizes target_type values and adds chk_practice_finding_target_type. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/model/PracticeFindingTargetType.java | Adds the new enum with PULL_REQUEST. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/model/PracticeFinding.java | Changes targetType to an @Enumerated enum field. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/dto/PracticeFindingListDTO.java | Changes DTO field type from String to PracticeFindingTargetType. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/dto/PracticeFindingDetailDTO.java | Changes DTO field type from String to PracticeFindingTargetType. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/PracticeFindingService.java | Uses enum constant when querying PR findings. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/PracticeFindingRepository.java | Parameterizes PR findings query by enum target type. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/PracticeDetectionCompletedEvent.java | Changes event field type from String to PracticeFindingTargetType. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PracticeDetectionDeliveryService.java | Uses enum for target type and writes .name() to DB/idempotency key. |
| server/application-server/openapi.yaml | Updates OpenAPI schema to constrain targetType to enum [PULL_REQUEST]. |
| // Build idempotency key — index disambiguates multiple findings for the same practice | ||
| String idempotencyKey = | ||
| finding.practiceSlug() + ":" + targetType + ":" + targetId + ":" + job.getId() + ":" + i; | ||
| finding.practiceSlug() + ":" + targetType.name() + ":" + targetId + ":" + job.getId() + ":" + i; |
There was a problem hiding this comment.
Changing the idempotency key to include targetType.name() (now PULL_REQUEST instead of the previous pull_request) breaks cross-version idempotency: if a job delivery is retried after a deployment/rollback, inserts will use a different idempotency_key and bypass the unique constraint, creating duplicate findings. Consider keeping the idempotency key format stable (e.g., continue using the legacy lowercase token) or add a migration step that rewrites existing practice_finding.idempotency_key values to the new format in lockstep with the target_type normalisation.
| finding.practiceSlug() + ":" + targetType.name() + ":" + targetId + ":" + job.getId() + ":" + i; | |
| finding.practiceSlug() | |
| + ":" | |
| + targetType.name().toLowerCase() | |
| + ":" | |
| + targetId | |
| + ":" | |
| + job.getId() | |
| + ":" | |
| + i; |
| WHERE f.targetType = :targetType | ||
| AND f.targetId = :pullRequestId | ||
| AND p.workspace.id = :workspaceId | ||
| ORDER BY f.detectedAt DESC | ||
| """ | ||
| ) | ||
| List<PracticeFinding> findByPullRequestAndWorkspace( | ||
| @Param("targetType") PracticeFindingTargetType targetType, |
There was a problem hiding this comment.
findByPullRequestAndWorkspace is PR-specific by name, but now takes a generic targetType parameter. This makes the API easy to misuse and forces callers to pass a redundant constant. Prefer hardcoding the enum in JPQL (e.g., compare to the PracticeFindingTargetType.PULL_REQUEST enum constant) and drop the parameter, or rename the method/query to reflect the generalized signature (e.g., findByTargetTypeAndTargetIdAndWorkspace).
| WHERE f.targetType = :targetType | |
| AND f.targetId = :pullRequestId | |
| AND p.workspace.id = :workspaceId | |
| ORDER BY f.detectedAt DESC | |
| """ | |
| ) | |
| List<PracticeFinding> findByPullRequestAndWorkspace( | |
| @Param("targetType") PracticeFindingTargetType targetType, | |
| WHERE f.targetType = de.tum.in.www1.hephaestus.practices.model.PracticeFindingTargetType.PULL_REQUEST | |
| AND f.targetId = :pullRequestId | |
| AND p.workspace.id = :workspaceId | |
| ORDER BY f.detectedAt DESC | |
| """ | |
| ) | |
| List<PracticeFinding> findByPullRequestAndWorkspace( |
| <rollback> | ||
| UPDATE practice_finding SET target_type = 'pull_request' WHERE target_type = 'PULL_REQUEST'; | ||
| </rollback> |
There was a problem hiding this comment.
The rollback for changeSet 1774700000000-1 will downcase all PULL_REQUEST rows back to pull_request, including rows inserted after this migration runs (by the new enum-based code). If rollbacks are ever executed, this can corrupt data; consider removing this rollback or constraining it to only rows updated by this changeset.
| <rollback> | |
| UPDATE practice_finding SET target_type = 'pull_request' WHERE target_type = 'PULL_REQUEST'; | |
| </rollback> |
| <!-- Step 2: CHECK constraint on practice_finding.target_type --> | ||
| <changeSet author="practice-target-type-constraint" id="1774700000000-2"> | ||
| <sql> | ||
| ALTER TABLE practice_finding | ||
| ADD CONSTRAINT chk_practice_finding_target_type | ||
| CHECK (target_type IN ('PULL_REQUEST')); |
There was a problem hiding this comment.
These two changeSets aren’t compatible with a rolling/zero-downtime deploy: code needs the data normalized to PULL_REQUEST to read rows, but the CHECK constraint will reject pull_request for any still-running old writers. Consider an intermediate migration (e.g., allow both values in the CHECK constraint first, then tighten to only PULL_REQUEST in a later release once all instances write the new value).
| <!-- Step 2: CHECK constraint on practice_finding.target_type --> | |
| <changeSet author="practice-target-type-constraint" id="1774700000000-2"> | |
| <sql> | |
| ALTER TABLE practice_finding | |
| ADD CONSTRAINT chk_practice_finding_target_type | |
| CHECK (target_type IN ('PULL_REQUEST')); | |
| <!-- Step 2: CHECK constraint on practice_finding.target_type (allow legacy and normalized values) --> | |
| <changeSet author="practice-target-type-constraint" id="1774700000000-2"> | |
| <sql> | |
| ALTER TABLE practice_finding | |
| ADD CONSTRAINT chk_practice_finding_target_type | |
| CHECK (target_type IN ('pull_request', 'PULL_REQUEST')); |
| @NotNull | ||
| @Enumerated(EnumType.STRING) | ||
| @Column(name = "target_type", length = 32, nullable = false) | ||
| private String targetType; | ||
| private PracticeFindingTargetType targetType; |
There was a problem hiding this comment.
Switching targetType to @Enumerated(EnumType.STRING) will cause Hibernate to throw when reading any existing practice_finding.target_type rows still stored as pull_request (lowercase). This also makes existing rows invisible to JPQL filters comparing against PULL_REQUEST. To avoid a production outage, either run a data-normalisation migration (at least changeSet -1) before deploying this code, or make the mapping temporarily tolerant (e.g., an AttributeConverter that maps both pull_request and PULL_REQUEST) until after the migration has been applied everywhere.
The intelligence-service Drizzle schema must include the new chk_practice_finding_target_type constraint added by the migration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
Bot
added
the
intelligence-service
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/model/PracticeFinding.java (1)
97-99: Consider typing thetargetTypeparameter inPracticeFindingRepository.insertIfAbsent()asPracticeFindingTargetTypeinstead ofString.The entity field is properly typed with
@Enumerated(EnumType.STRING), but the native query method signature still acceptsString targetType, allowing invalid values to bypass compile-time type checking. Changing the parameter toPracticeFindingTargetTypewould enforce enum validation at the call site and prevent accidental typos in targetType values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/model/PracticeFinding.java` around lines 97 - 99, Change the insertIfAbsent method signature in PracticeFindingRepository to accept PracticeFindingTargetType instead of String (i.e., replace the String targetType parameter with PracticeFindingTargetType targetType) so callers get compile-time enum checking; inside the repository implementation or native query binding, convert the enum to its String representation (e.g., targetType.name() or targetType.toString()) when binding to the database column that uses `@Enumerated`(EnumType.STRING) on the PracticeFinding.targetType field, and update any `@Param` or query parameter names accordingly.server/application-server/src/test/java/de/tum/in/www1/hephaestus/practices/finding/PracticeFindingRepositoryIntegrationTest.java (1)
619-644: Add a negative-path assertion for the newtarget_typeCHECK constraint.Line 620 currently validates only the happy-path enum round-trip. Please also assert that a non-canonical value (e.g.
pull_request) is rejected, so constraint behavior is covered by integration tests.✅ Suggested test addition
+import static org.assertj.core.api.Assertions.assertThatThrownBy; +import org.springframework.dao.DataIntegrityViolationException; ... `@Nested` `@DisplayName`("target_type enum mapping") class TargetTypeTests { @@ void enumRoundTrip() { @@ PracticeFinding found = practiceFindingRepository.findById(id).orElseThrow(); assertThat(found.getTargetType()).isEqualTo(PracticeFindingTargetType.PULL_REQUEST); } + + `@Test` + `@DisplayName`("rejects non-canonical target_type values") + void shouldRejectLowercaseTargetTypeWhenConstraintApplied() { + assertThatThrownBy(() -> + practiceFindingRepository.insertIfAbsent( + UUID.randomUUID(), + "tt-invalid", + agentJob.getId(), + practice.getId(), + "pull_request", + 1L, + contributor.getId(), + "Invalid target type", + "POSITIVE", + "INFO", + 0.9f, + null, + null, + null, + null, + Instant.now() + ) + ).isInstanceOf(DataIntegrityViolationException.class); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/test/java/de/tum/in/www1/hephaestus/practices/finding/PracticeFindingRepositoryIntegrationTest.java` around lines 619 - 644, Add a negative-path assertion to PracticeFindingRepositoryIntegrationTest (near the enumRoundTrip test) that verifies the new target_type CHECK rejects non-canonical values: attempt to call practiceFindingRepository.insertIfAbsent with the same parameters but using a lowercase string like "pull_request" for the targetType, and assert that the insertion throws the appropriate exception (e.g., SQLException / DataAccessException / ConstraintViolation) instead of succeeding; ensure the test asserts absence of a persisted entity for that id and that the exception message or type indicates the CHECK constraint violation so the constraint behavior is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/model/PracticeFinding.java`:
- Around line 97-99: Change the insertIfAbsent method signature in
PracticeFindingRepository to accept PracticeFindingTargetType instead of String
(i.e., replace the String targetType parameter with PracticeFindingTargetType
targetType) so callers get compile-time enum checking; inside the repository
implementation or native query binding, convert the enum to its String
representation (e.g., targetType.name() or targetType.toString()) when binding
to the database column that uses `@Enumerated`(EnumType.STRING) on the
PracticeFinding.targetType field, and update any `@Param` or query parameter names
accordingly.
In
`@server/application-server/src/test/java/de/tum/in/www1/hephaestus/practices/finding/PracticeFindingRepositoryIntegrationTest.java`:
- Around line 619-644: Add a negative-path assertion to
PracticeFindingRepositoryIntegrationTest (near the enumRoundTrip test) that
verifies the new target_type CHECK rejects non-canonical values: attempt to call
practiceFindingRepository.insertIfAbsent with the same parameters but using a
lowercase string like "pull_request" for the targetType, and assert that the
insertion throws the appropriate exception (e.g., SQLException /
DataAccessException / ConstraintViolation) instead of succeeding; ensure the
test asserts absence of a persisted entity for that id and that the exception
message or type indicates the CHECK constraint violation so the constraint
behavior is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7bc68ab8-e3c3-4117-9eba-3bc4d94f3671
📒 Files selected for processing (18)
server/application-server/openapi.yamlserver/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/handler/PracticeDetectionDeliveryService.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/PracticeDetectionCompletedEvent.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/PracticeFindingRepository.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/PracticeFindingService.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/dto/PracticeFindingDetailDTO.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/dto/PracticeFindingListDTO.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/model/PracticeFinding.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/model/PracticeFindingTargetType.javaserver/application-server/src/main/resources/db/changelog/1774700000000_changelog.xmlserver/application-server/src/main/resources/db/master.xmlserver/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/handler/PracticeDetectionDeliveryServiceIntegrationTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/handler/PracticeDetectionDeliveryServiceTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/practices/finding/PracticeFindingControllerIntegrationTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/practices/finding/PracticeFindingRepositoryIntegrationTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackControllerIntegrationTest.javaserver/intelligence-service/src/shared/db/schema.tswebapp/src/api/types.gen.ts
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📚 Documentation Preview
|
|
🎉 This PR is included in version 0.52.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
The PracticeFindingTargetType enum→String refactor from #953 changed the DTO serialization but the OpenAPI spec was not regenerated. CI correctly caught the drift. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Description
Closes the gap identified in PE Audit #4:
practice_finding.target_type(VARCHAR(32)) had no CHECK constraint, despite every other enum-like column on the same table (verdict,severity,guidance_method,action) having one. The only production value is"pull_request", hardcoded as a raw string — meaning a casing typo or unknown value would silently create invisible findings.This PR:
PracticeFindingTargetTypeenum using bare@Enumerated(EnumType.STRING), consistent withVerdict,Severity, andCaMethodon the same entity (no@Convert, no@JsonValue, noAttributeConverter— the simplest pattern already used by 20+ enums in the codebase).'pull_request'rows to'PULL_REQUEST'with a precondition guard that HALTs if unexpected values exist, (2) addsCHECK (target_type IN ('PULL_REQUEST')).targetTypeas enum throughout: entity field, DTOs, event record, repository JPQL query (parameterised instead of hardcoded string), delivery service, and all tests.targetType: 'PULL_REQUEST'literal type).Files changed (17)
PracticeFindingTargetType.java(new)PULL_REQUESTconstant1774700000000_changelog.xml(new),master.xmlPracticeFinding.javaString→@Enumerated(STRING) PracticeFindingTargetTypePracticeDetectionDeliveryService.java,PracticeFindingService.java.name()for native queries, enum constant for JPQLPracticeFindingDetailDTO,PracticeFindingListDTO,PracticeDetectionCompletedEventString→ enumPracticeFindingRepository.javafindByPullRequestAndWorkspacequeryopenapi.yaml,types.gen.tsenum: [PULL_REQUEST]PULL_REQUESTMigration details
Safe deployment order: deploy code first (writes
PULL_REQUESTvia enum), then run migration (normalises old data, adds constraint).API change
targetTypeinPracticeFindingListandPracticeFindingDetailresponses changes from free-formstringtoenum: ["PULL_REQUEST"]. This is intentionally breaking — the field was already de-facto enum-like with a single value, and is now properly constrained. Consistent with howverdict,severity, andguidanceMethodare already returned as UPPER_CASE enum strings.Fixes #930
How to test
mvn test -Dsurefire.includedGroups="unit"mvn test -Dsurefire.includedGroups="architecture"(112 tests)npm run generate:apiproduces no diffpull_request→PULL_REQUEST, then constrains🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Refactor
targetTypefield to use uppercase format (PULL_REQUEST) across API responses and database storage.targetTypeto supported values at the database level for improved data integrity.Chores