From 869c622675af3c70ae59f6d2d9b686b62a7d1dcf Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 15 Oct 2026 11:49:31 +0000 Subject: [PATCH 1/7] Fix project API bugs carried over from the Java 8 code - POST ignores client-supplied id/createdDate/lastModifiedDate instead of silently overwriting an existing project - PATCH no longer reports a conflict against the project being updated (e.g. re-sending its current externalId) - PATCH with "sdlcSystem": null returns 400 instead of a 500 NPE - A unique-constraint violation from a concurrent duplicate returns 409 instead of 500 - POST/PATCH responses carry the stored SDLC system instead of the client's id-only stub - Enable JPA auditing so @CreatedDate/@LastModifiedDate are populated - Stop enabling the H2 console in every deployment; spring-boot-h2console still turns it on automatically when devtools is active - Map NotFoundException to 404 in one handler and log it at INFO Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012MLyPXGHeCc1fvTeB8JnT2 --- .../resourceapi/config/JpaAuditingConfig.java | 13 +++ .../controllers/ProjectRestController.java | 100 ++++++++++-------- .../company/resourceapi/entities/Project.java | 6 ++ src/main/resources/application.properties | 2 - .../ProjectRestControllerConflictTest.java | 50 +++++++++ .../ProjectRestControllerTest.java | 68 ++++++++++++ .../SdlcSystemRepositoryTest.java | 31 ++++++ 7 files changed, 224 insertions(+), 46 deletions(-) create mode 100644 src/main/java/com/company/resourceapi/config/JpaAuditingConfig.java create mode 100644 src/test/java/com/company/resourceapi/controllers/ProjectRestControllerConflictTest.java create mode 100644 src/test/java/com/company/resourceapi/repositories/SdlcSystemRepositoryTest.java diff --git a/src/main/java/com/company/resourceapi/config/JpaAuditingConfig.java b/src/main/java/com/company/resourceapi/config/JpaAuditingConfig.java new file mode 100644 index 0000000..3052630 --- /dev/null +++ b/src/main/java/com/company/resourceapi/config/JpaAuditingConfig.java @@ -0,0 +1,13 @@ +package com.company.resourceapi.config; + +import org.springframework.context.annotation.Configuration; +import org.springframework.data.jpa.repository.config.EnableJpaAuditing; + +/** + * Populates the {@code @CreatedDate}/{@code @LastModifiedDate} fields of the entities. + * Kept out of the application class so web slice tests don't need a JPA context. + */ +@Configuration +@EnableJpaAuditing +public class JpaAuditingConfig { +} diff --git a/src/main/java/com/company/resourceapi/controllers/ProjectRestController.java b/src/main/java/com/company/resourceapi/controllers/ProjectRestController.java index e2ffbc9..98f9e9f 100755 --- a/src/main/java/com/company/resourceapi/controllers/ProjectRestController.java +++ b/src/main/java/com/company/resourceapi/controllers/ProjectRestController.java @@ -3,7 +3,6 @@ import static org.springframework.http.HttpStatus.BAD_REQUEST; import static org.springframework.http.HttpStatus.CONFLICT; import static org.springframework.http.HttpStatus.CREATED; -import static org.springframework.http.HttpStatus.OK; import static org.springframework.http.HttpStatus.NOT_FOUND; import com.company.resourceapi.entities.Project; @@ -15,12 +14,15 @@ import io.swagger.v3.oas.annotations.Parameter; import io.swagger.v3.oas.annotations.tags.Tag; import jakarta.validation.Valid; +import jakarta.validation.constraints.AssertTrue; import java.net.URI; import lombok.Getter; import lombok.NoArgsConstructor; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; +import org.springframework.dao.DataIntegrityViolationException; import org.springframework.http.ResponseEntity; +import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.PatchMapping; import org.springframework.web.bind.annotation.PathVariable; @@ -50,29 +52,22 @@ public class ProjectRestController { @GetMapping(ENDPOINT_ID) public ResponseEntity getProject( @Parameter(name = PATH_VARIABLE_ID, description = API_PARAM_ID, required = true) - @PathVariable(PATH_VARIABLE_ID) final long projectId) { - try { - return ResponseEntity.status(OK).body(projectService.getProject(projectId)); - } catch (NotFoundException e) { - log.debug(e.getMessage()); - return ResponseEntity.status(NOT_FOUND).build(); - } + @PathVariable(PATH_VARIABLE_ID) final long projectId) throws NotFoundException { + return ResponseEntity.ok(projectService.getProject(projectId)); } @Operation(summary = "Create a Project") @PostMapping public ResponseEntity createProject( @Valid - @RequestBody Project project) { - long systemId = project.getSdlcSystem().getId(); + @RequestBody Project project) throws NotFoundException { + var sdlcSystem = findSdlcSystem(project.getSdlcSystem().getId()); - if (sdlcSystemService.getSdlcSystem(systemId).isEmpty()) { - return ResponseEntity.status(NOT_FOUND).build(); - } - if (projectService.getProject(project.getExternalId(), systemId).isPresent()) { + if (projectService.getProject(project.getExternalId(), sdlcSystem.getId()).isPresent()) { return ResponseEntity.status(CONFLICT).build(); } + project.setSdlcSystem(sdlcSystem); var createdProject = projectService.createProject(project); return ResponseEntity.status(CREATED) .location(URI.create(LOCATION_HEADER.formatted(createdProject.getId()))) @@ -84,42 +79,53 @@ public ResponseEntity createProject( public ResponseEntity updateProject( @Parameter(name = PATH_VARIABLE_ID, description = API_PARAM_ID, required = true) @PathVariable(PATH_VARIABLE_ID) long projectId, - @Valid @RequestBody PatchedProject patchedProject) { + @Valid @RequestBody PatchedProject patchedProject) throws NotFoundException { + var project = projectService.getProject(projectId); - try { - var project = projectService.getProject(projectId); + if (patchedProject.isNameDirty()) { + project.setName(patchedProject.getName()); + } - if (patchedProject.isNameDirty()) { - project.setName(patchedProject.getName()); - } + if (patchedProject.isExternalIdDirty()) { + project.setExternalId(patchedProject.getExternalId()); + } - if (patchedProject.isExternalIdDirty()) { - project.setExternalId(patchedProject.getExternalId()); - } + if (patchedProject.isSdlcSystemDirty()) { + project.setSdlcSystem(findSdlcSystem(patchedProject.getSdlcSystem().getId())); + } - if (patchedProject.isSdlcSystemDirty()) { - long systemId = patchedProject.getSdlcSystem().getId(); - if (sdlcSystemService.getSdlcSystem(systemId).isPresent()) { - project.setSdlcSystem(patchedProject.getSdlcSystem()); - } else { - throw new NotFoundException(SdlcSystem.class, systemId); - } - } + if ((patchedProject.isExternalIdDirty() || patchedProject.isSdlcSystemDirty()) + && projectService + .getProject(project.getExternalId(), project.getSdlcSystem().getId()) + .filter(existingProject -> existingProject.getId() != projectId) + .isPresent()) { + return ResponseEntity.status(CONFLICT).build(); + } - if ((patchedProject.isExternalIdDirty() || patchedProject.isSdlcSystemDirty()) - && projectService - .getProject(project.getExternalId(), project.getSdlcSystem().getId()) - .isPresent()) { - return ResponseEntity.status(CONFLICT).build(); - } + return projectService.updateProject(project) + .map(ResponseEntity::ok) + .orElseGet(() -> ResponseEntity.status(BAD_REQUEST).build()); + } - return projectService.updateProject(project) - .map(resultantProject -> ResponseEntity.status(OK).body(resultantProject)) - .orElseGet(() -> ResponseEntity.status(BAD_REQUEST).build()); - } catch (NotFoundException e) { - log.debug(e.getMessage()); - return ResponseEntity.status(NOT_FOUND).build(); - } + @ExceptionHandler(NotFoundException.class) + ResponseEntity handleNotFound(NotFoundException e) { + log.info(e.getMessage()); + return ResponseEntity.status(NOT_FOUND).build(); + } + + /** + * The (sdlcSystem, externalId) pair is unique, so a concurrent request can still insert a + * duplicate after the conflict check above has passed. + */ + @ExceptionHandler(DataIntegrityViolationException.class) + ResponseEntity handleDataIntegrityViolation(DataIntegrityViolationException e) { + log.info("Rejected conflicting project: {}", e.getMostSpecificCause().getMessage()); + return ResponseEntity.status(CONFLICT).build(); + } + + private SdlcSystem findSdlcSystem(long systemId) throws NotFoundException { + return sdlcSystemService.getSdlcSystem(systemId) + .orElseThrow(() -> new NotFoundException(SdlcSystem.class, systemId)); } @Getter @@ -148,5 +154,11 @@ public void setSdlcSystem(SdlcSystem sdlcSystem) { this.sdlcSystem = sdlcSystem; isSdlcSystemDirty = true; } + + /** A project always belongs to an SDLC system, so it can be replaced but not removed. */ + @AssertTrue(message = "sdlcSystem must not be null") + boolean isSdlcSystemPresentIfPatched() { + return !isSdlcSystemDirty || sdlcSystem != null; + } } } diff --git a/src/main/java/com/company/resourceapi/entities/Project.java b/src/main/java/com/company/resourceapi/entities/Project.java index 74d782f..ecfd836 100755 --- a/src/main/java/com/company/resourceapi/entities/Project.java +++ b/src/main/java/com/company/resourceapi/entities/Project.java @@ -1,5 +1,8 @@ package com.company.resourceapi.entities; +import static com.fasterxml.jackson.annotation.JsonProperty.Access.READ_ONLY; + +import com.fasterxml.jackson.annotation.JsonProperty; import jakarta.persistence.Column; import jakarta.persistence.Entity; import jakarta.persistence.EntityListeners; @@ -27,6 +30,7 @@ public class Project { @Id @GeneratedValue(strategy = GenerationType.IDENTITY) + @JsonProperty(access = READ_ONLY) private long id; @Column(name = "external_id", nullable = false) @@ -43,9 +47,11 @@ public class Project { @Column(name = "created_date", nullable = false) @CreatedDate + @JsonProperty(access = READ_ONLY) private Instant createdDate = Instant.now(); @Column(name = "last_modified_date", nullable = false) @LastModifiedDate + @JsonProperty(access = READ_ONLY) private Instant lastModifiedDate = Instant.now(); } diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index 51bc616..377abda 100755 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -1,5 +1,3 @@ -# h2 -spring.h2.console.enabled=true # datasource spring.datasource.url=jdbc:h2:mem:test;MODE=MySQL;DATABASE_TO_LOWER=TRUE spring.sql.init.platform=h2 diff --git a/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerConflictTest.java b/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerConflictTest.java new file mode 100644 index 0000000..b90e299 --- /dev/null +++ b/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerConflictTest.java @@ -0,0 +1,50 @@ +package com.company.resourceapi.controllers; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.when; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import com.company.resourceapi.entities.SdlcSystem; +import com.company.resourceapi.services.ProjectService; +import com.company.resourceapi.services.SdlcSystemService; +import java.util.Optional; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.webmvc.test.autoconfigure.WebMvcTest; +import org.springframework.dao.DataIntegrityViolationException; +import org.springframework.http.MediaType; +import org.springframework.test.context.bean.override.mockito.MockitoBean; +import org.springframework.test.web.servlet.MockMvc; + +@WebMvcTest(ProjectRestController.class) +class ProjectRestControllerConflictTest { + + @Autowired + private MockMvc mockMvc; + + @MockitoBean + private ProjectService projectService; + + @MockitoBean + private SdlcSystemService sdlcSystemService; + + @Test + void givenConcurrentDuplicateWhenPostThenReturnStatusConflict() throws Exception { + // given: the conflict check passes, but a concurrent request inserted the same project first + var sdlcSystem = new SdlcSystem(); + sdlcSystem.setId(1); + when(sdlcSystemService.getSdlcSystem(1)).thenReturn(Optional.of(sdlcSystem)); + when(projectService.getProject(anyString(), anyLong())).thenReturn(Optional.empty()); + when(projectService.createProject(any())) + .thenThrow(new DataIntegrityViolationException("Unique index or primary key violation")); + + // when / then + mockMvc.perform(post(ProjectRestController.ENDPOINT) + .content("{\"externalId\": \"DUPLICATE\", \"sdlcSystem\": {\"id\": 1}}") + .contentType(MediaType.APPLICATION_JSON)) + .andExpect(status().isConflict()); + } +} diff --git a/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerTest.java b/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerTest.java index 546c224..5b9840d 100755 --- a/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerTest.java +++ b/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerTest.java @@ -1,6 +1,7 @@ package com.company.resourceapi.controllers; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.not; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.patch; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; @@ -174,4 +175,71 @@ void givenIdAndPatchedProjectWhenPatchedThenReturnStatusConflict() throws Except // then .andExpect(status().isConflict()); } + + @Test + void givenProjectJsonWithIdWhenPostThenCreateNewProjectInsteadOfOverwriting() throws Exception { + final long EXISTING_PROJECT_ID = 1; + final String jsonContent = "{\"id\": " + EXISTING_PROJECT_ID + "," + + String.format(PROJECT_JSON_TEMPLATE, "NEWPROJECT", "New Project", 3).substring(1); + + mockMvc.perform(post(ProjectRestController.ENDPOINT) + .content(jsonContent) + .contentType(MediaType.APPLICATION_JSON)) + .andExpect(status().isCreated()) + .andExpect(jsonPath("$.id", not(equalTo((int) EXISTING_PROJECT_ID)))) + .andExpect(jsonPath(EXTERNALID_JPATH, equalTo("NEWPROJECT"))); + + mockMvc.perform(get(String.format(ENDPOINT_WITH_ID, EXISTING_PROJECT_ID))) + .andExpect(status().isOk()) + .andExpect(jsonPath(EXTERNALID_JPATH, equalTo("SAMPLEPROJECT"))); + } + + @Test + void givenProjectJsonWhenPostThenReturnFullSdlcSystem() throws Exception { + final String jsonContent = String.format(PROJECT_JSON_TEMPLATE, "FULLSYSTEM", "Name", 2); + + mockMvc.perform(post(ProjectRestController.ENDPOINT) + .content(jsonContent) + .contentType(MediaType.APPLICATION_JSON)) + .andExpect(status().isCreated()) + .andExpect(jsonPath("$.sdlcSystem.baseUrl", equalTo("http://bugzilla.company.com"))); + } + + @Test + void givenUnchangedExternalIdWhenPatchedThenReturnStatusOk() throws Exception { + final long PROJECT_ID = 6; + final String UNCHANGED_EXTERNALID = "PROJECTONE"; + String requestUri = String.format(ENDPOINT_WITH_ID, PROJECT_ID); + final String jsonContent = String.format("{\"externalId\": \"%s\"}", UNCHANGED_EXTERNALID); + + mockMvc.perform(patch(requestUri) + .contentType(MediaType.APPLICATION_JSON) + .content(jsonContent)) + .andExpect(status().isOk()) + .andExpect(jsonPath(EXTERNALID_JPATH, equalTo(UNCHANGED_EXTERNALID))); + } + + @Test + void givenNullSdlcSystemWhenPatchedThenReturnStatusBadRequest() throws Exception { + final long PROJECT_ID = 1; + String requestUri = String.format(ENDPOINT_WITH_ID, PROJECT_ID); + + mockMvc.perform(patch(requestUri) + .contentType(MediaType.APPLICATION_JSON) + .content("{\"sdlcSystem\": null}")) + .andExpect(status().isBadRequest()); + } + + @Test + void givenNewSdlcSystemWhenPatchedThenReturnFullSdlcSystem() throws Exception { + final long PROJECT_ID = 7; + String requestUri = String.format(ENDPOINT_WITH_ID, PROJECT_ID); + + mockMvc.perform(patch(requestUri) + .contentType(MediaType.APPLICATION_JSON) + .content(String.format(SDLC_SYSTEM_JSON_TEMPLATE2, 2))) + .andExpect(status().isOk()) + .andExpect(jsonPath(SYSTEM_ID_JPATH, equalTo(2))) + .andExpect(jsonPath("$.sdlcSystem.baseUrl", equalTo("http://bugzilla.company.com"))); + } } diff --git a/src/test/java/com/company/resourceapi/repositories/SdlcSystemRepositoryTest.java b/src/test/java/com/company/resourceapi/repositories/SdlcSystemRepositoryTest.java new file mode 100644 index 0000000..9d57894 --- /dev/null +++ b/src/test/java/com/company/resourceapi/repositories/SdlcSystemRepositoryTest.java @@ -0,0 +1,31 @@ +package com.company.resourceapi.repositories; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.company.resourceapi.entities.SdlcSystem; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.transaction.annotation.Transactional; + +@SpringBootTest +@Transactional +class SdlcSystemRepositoryTest { + + @Autowired + private SdlcSystemRepository sdlcSystemRepository; + + @Test + void givenNewSdlcSystemWhenSavedThenAuditDatesArePopulated() { + // given + var sdlcSystem = new SdlcSystem(); + sdlcSystem.setBaseUrl("http://youtrack.company.com"); + + // when + var saved = sdlcSystemRepository.saveAndFlush(sdlcSystem); + + // then + assertThat(saved.getCreatedDate()).isNotNull(); + assertThat(saved.getLastModifiedDate()).isNotNull(); + } +} From 78975907f01e1171b401278193b88f9402d25e67 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 15 Oct 2026 11:57:03 +0000 Subject: [PATCH 2/7] Address review: validation, unique-only 409, drop Data REST exposure - Reject externalId/name longer than the 255-char columns and blank externalId on PATCH with 400 instead of 409/500 - Map only unique-constraint violations to 409; other integrity errors keep the default handling - Remove spring-boot-starter-data-rest, which exported the repositories at /projects and /sdlcSystems and bypassed every controller check - Rely on JPA auditing for timestamps: drop the manual lastModifiedDate update and field initializers; updateProject now returns Project, which removes an unreachable 400 branch - Log routine 404/409s at DEBUG - Make the unchanged-externalId test independent of test order - Document that the H2 console is a devtools-only feature Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012MLyPXGHeCc1fvTeB8JnT2 --- README.md | 2 + pom.xml | 4 -- .../controllers/ProjectRestController.java | 27 +++++++--- .../company/resourceapi/entities/Project.java | 10 +++- .../resourceapi/services/ProjectService.java | 2 +- .../services/impl/ProjectServiceImpl.java | 6 +-- .../ProjectRestControllerConflictTest.java | 8 ++- .../ProjectRestControllerTest.java | 41 +++++++++++++++- .../UniqueConstraintViolationTest.java | 49 +++++++++++++++++++ .../repositories/ProjectRepositoryTest.java | 31 ++++++++++++ .../services/impl/ProjectServiceImplTest.java | 8 +-- 11 files changed, 162 insertions(+), 26 deletions(-) create mode 100644 src/test/java/com/company/resourceapi/controllers/UniqueConstraintViolationTest.java create mode 100644 src/test/java/com/company/resourceapi/repositories/ProjectRepositoryTest.java diff --git a/README.md b/README.md index 8a62a2d..5532035 100644 --- a/README.md +++ b/README.md @@ -10,5 +10,7 @@ Java 25+ (Spring Boot 4) ## Database Database uses in-memory DB H2. It will automatically build and export the required tables and data. +The H2 console (`/h2-console`, JDBC URL `jdbc:h2:mem:test`) is available while running with devtools, e.g. `./mvnw spring-boot:run`. It is not served by the packaged jar. + ## Testing There are scripts to test the Rest API in Postman. \ No newline at end of file diff --git a/pom.xml b/pom.xml index d8d9d9e..129e48d 100755 --- a/pom.xml +++ b/pom.xml @@ -26,10 +26,6 @@ org.springframework.boot spring-boot-starter-data-jpa - - org.springframework.boot - spring-boot-starter-data-rest - org.springframework.boot spring-boot-starter-webmvc diff --git a/src/main/java/com/company/resourceapi/controllers/ProjectRestController.java b/src/main/java/com/company/resourceapi/controllers/ProjectRestController.java index 98f9e9f..a736c71 100755 --- a/src/main/java/com/company/resourceapi/controllers/ProjectRestController.java +++ b/src/main/java/com/company/resourceapi/controllers/ProjectRestController.java @@ -1,6 +1,5 @@ package com.company.resourceapi.controllers; -import static org.springframework.http.HttpStatus.BAD_REQUEST; import static org.springframework.http.HttpStatus.CONFLICT; import static org.springframework.http.HttpStatus.CREATED; import static org.springframework.http.HttpStatus.NOT_FOUND; @@ -15,11 +14,15 @@ import io.swagger.v3.oas.annotations.tags.Tag; import jakarta.validation.Valid; import jakarta.validation.constraints.AssertTrue; +import jakarta.validation.constraints.Pattern; +import jakarta.validation.constraints.Size; import java.net.URI; import lombok.Getter; import lombok.NoArgsConstructor; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; +import org.hibernate.exception.ConstraintViolationException; +import org.hibernate.exception.ConstraintViolationException.ConstraintKind; import org.springframework.dao.DataIntegrityViolationException; import org.springframework.http.ResponseEntity; import org.springframework.web.bind.annotation.ExceptionHandler; @@ -102,27 +105,34 @@ public ResponseEntity updateProject( return ResponseEntity.status(CONFLICT).build(); } - return projectService.updateProject(project) - .map(ResponseEntity::ok) - .orElseGet(() -> ResponseEntity.status(BAD_REQUEST).build()); + return ResponseEntity.ok(projectService.updateProject(project)); } @ExceptionHandler(NotFoundException.class) ResponseEntity handleNotFound(NotFoundException e) { - log.info(e.getMessage()); + log.debug(e.getMessage()); return ResponseEntity.status(NOT_FOUND).build(); } /** * The (sdlcSystem, externalId) pair is unique, so a concurrent request can still insert a - * duplicate after the conflict check above has passed. + * duplicate after the conflict check above has passed. Any other integrity violation is a + * server-side problem and is left to the default error handling. */ @ExceptionHandler(DataIntegrityViolationException.class) ResponseEntity handleDataIntegrityViolation(DataIntegrityViolationException e) { - log.info("Rejected conflicting project: {}", e.getMostSpecificCause().getMessage()); + if (!isUniqueConstraintViolation(e)) { + throw e; + } + log.debug("Rejected duplicate project: {}", e.getMostSpecificCause().getMessage()); return ResponseEntity.status(CONFLICT).build(); } + static boolean isUniqueConstraintViolation(DataIntegrityViolationException e) { + return e.getCause() instanceof ConstraintViolationException violation + && violation.getKind() == ConstraintKind.UNIQUE; + } + private SdlcSystem findSdlcSystem(long systemId) throws NotFoundException { return sdlcSystemService.getSdlcSystem(systemId) .orElseThrow(() -> new NotFoundException(SdlcSystem.class, systemId)); @@ -131,7 +141,10 @@ private SdlcSystem findSdlcSystem(long systemId) throws NotFoundException { @Getter @NoArgsConstructor static class PatchedProject { + @Size(max = Project.MAX_TEXT_LENGTH) + @Pattern(regexp = "(?s).*\\S.*", message = "must not be blank") private String externalId; + @Size(max = Project.MAX_TEXT_LENGTH) private String name; private SdlcSystem sdlcSystem; private boolean isNameDirty = false; diff --git a/src/main/java/com/company/resourceapi/entities/Project.java b/src/main/java/com/company/resourceapi/entities/Project.java index ecfd836..cc8b2b0 100755 --- a/src/main/java/com/company/resourceapi/entities/Project.java +++ b/src/main/java/com/company/resourceapi/entities/Project.java @@ -14,6 +14,7 @@ import jakarta.persistence.Table; import jakarta.validation.constraints.NotBlank; import jakarta.validation.constraints.NotNull; +import jakarta.validation.constraints.Size; import java.time.Instant; import lombok.Data; import lombok.NoArgsConstructor; @@ -28,6 +29,9 @@ @NoArgsConstructor public class Project { + /** Length of the varchar columns in schema.sql. */ + public static final int MAX_TEXT_LENGTH = 255; + @Id @GeneratedValue(strategy = GenerationType.IDENTITY) @JsonProperty(access = READ_ONLY) @@ -35,9 +39,11 @@ public class Project { @Column(name = "external_id", nullable = false) @NotBlank + @Size(max = MAX_TEXT_LENGTH) private String externalId; @Column(name = "name") + @Size(max = MAX_TEXT_LENGTH) private String name; @ManyToOne @@ -48,10 +54,10 @@ public class Project { @Column(name = "created_date", nullable = false) @CreatedDate @JsonProperty(access = READ_ONLY) - private Instant createdDate = Instant.now(); + private Instant createdDate; @Column(name = "last_modified_date", nullable = false) @LastModifiedDate @JsonProperty(access = READ_ONLY) - private Instant lastModifiedDate = Instant.now(); + private Instant lastModifiedDate; } diff --git a/src/main/java/com/company/resourceapi/services/ProjectService.java b/src/main/java/com/company/resourceapi/services/ProjectService.java index 8301db9..054e56f 100755 --- a/src/main/java/com/company/resourceapi/services/ProjectService.java +++ b/src/main/java/com/company/resourceapi/services/ProjectService.java @@ -10,5 +10,5 @@ public interface ProjectService { Project createProject(Project project); Optional getProject(String externalId, long sdlcSystemId); Optional getProjectStream(long id); - Optional updateProject(Project updatedProject); + Project updateProject(Project updatedProject); } diff --git a/src/main/java/com/company/resourceapi/services/impl/ProjectServiceImpl.java b/src/main/java/com/company/resourceapi/services/impl/ProjectServiceImpl.java index b6e39bc..ffd625f 100755 --- a/src/main/java/com/company/resourceapi/services/impl/ProjectServiceImpl.java +++ b/src/main/java/com/company/resourceapi/services/impl/ProjectServiceImpl.java @@ -4,7 +4,6 @@ import com.company.resourceapi.exceptions.NotFoundException; import com.company.resourceapi.repositories.ProjectRepository; import com.company.resourceapi.services.ProjectService; -import java.time.Instant; import java.util.Optional; import lombok.RequiredArgsConstructor; import org.springframework.stereotype.Service; @@ -31,8 +30,7 @@ public Optional getProjectStream(long id) { return projectRepository.findById(id); } - public Optional updateProject(Project updatedProject) { - updatedProject.setLastModifiedDate(Instant.now()); - return Optional.of(projectRepository.save(updatedProject)); + public Project updateProject(Project updatedProject) { + return projectRepository.save(updatedProject); } } diff --git a/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerConflictTest.java b/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerConflictTest.java index b90e299..c14c220 100644 --- a/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerConflictTest.java +++ b/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerConflictTest.java @@ -10,7 +10,10 @@ import com.company.resourceapi.entities.SdlcSystem; import com.company.resourceapi.services.ProjectService; import com.company.resourceapi.services.SdlcSystemService; +import java.sql.SQLException; import java.util.Optional; +import org.hibernate.exception.ConstraintViolationException; +import org.hibernate.exception.ConstraintViolationException.ConstraintKind; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.webmvc.test.autoconfigure.WebMvcTest; @@ -38,8 +41,9 @@ void givenConcurrentDuplicateWhenPostThenReturnStatusConflict() throws Exception sdlcSystem.setId(1); when(sdlcSystemService.getSdlcSystem(1)).thenReturn(Optional.of(sdlcSystem)); when(projectService.getProject(anyString(), anyLong())).thenReturn(Optional.empty()); - when(projectService.createProject(any())) - .thenThrow(new DataIntegrityViolationException("Unique index or primary key violation")); + when(projectService.createProject(any())).thenThrow(new DataIntegrityViolationException( + "Unique index or primary key violation", new ConstraintViolationException( + "duplicate", new SQLException(), ConstraintKind.UNIQUE, "system_project"))); // when / then mockMvc.perform(post(ProjectRestController.ENDPOINT) diff --git a/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerTest.java b/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerTest.java index 5b9840d..eaa26af 100755 --- a/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerTest.java +++ b/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerTest.java @@ -207,8 +207,8 @@ void givenProjectJsonWhenPostThenReturnFullSdlcSystem() throws Exception { @Test void givenUnchangedExternalIdWhenPatchedThenReturnStatusOk() throws Exception { - final long PROJECT_ID = 6; - final String UNCHANGED_EXTERNALID = "PROJECTONE"; + final long PROJECT_ID = 8; + final String UNCHANGED_EXTERNALID = "PROJECTTHREE"; String requestUri = String.format(ENDPOINT_WITH_ID, PROJECT_ID); final String jsonContent = String.format("{\"externalId\": \"%s\"}", UNCHANGED_EXTERNALID); @@ -242,4 +242,41 @@ void givenNewSdlcSystemWhenPatchedThenReturnFullSdlcSystem() throws Exception { .andExpect(jsonPath(SYSTEM_ID_JPATH, equalTo(2))) .andExpect(jsonPath("$.sdlcSystem.baseUrl", equalTo("http://bugzilla.company.com"))); } + + @Test + void givenTooLongExternalIdWhenPostThenReturnStatusBadRequest() throws Exception { + final String jsonContent = String.format(PROJECT_JSON_TEMPLATE, "X".repeat(256), "Name", 1); + + mockMvc.perform(post(ProjectRestController.ENDPOINT) + .content(jsonContent) + .contentType(MediaType.APPLICATION_JSON)) + .andExpect(status().isBadRequest()); + } + + @Test + void givenTooLongNameWhenPatchedThenReturnStatusBadRequest() throws Exception { + String requestUri = String.format(ENDPOINT_WITH_ID, 1); + + mockMvc.perform(patch(requestUri) + .contentType(MediaType.APPLICATION_JSON) + .content(String.format("{\"name\": \"%s\"}", "N".repeat(256)))) + .andExpect(status().isBadRequest()); + } + + @Test + void givenBlankExternalIdWhenPatchedThenReturnStatusBadRequest() throws Exception { + String requestUri = String.format(ENDPOINT_WITH_ID, 1); + + mockMvc.perform(patch(requestUri) + .contentType(MediaType.APPLICATION_JSON) + .content("{\"externalId\": \" \"}")) + .andExpect(status().isBadRequest()); + } + + @Test + void givenRepositoryPathWhenGetThenReturnStatusNotFound() throws Exception { + // Repositories must only be reachable through the validated controller endpoints + mockMvc.perform(get("/projects")) + .andExpect(status().isNotFound()); + } } diff --git a/src/test/java/com/company/resourceapi/controllers/UniqueConstraintViolationTest.java b/src/test/java/com/company/resourceapi/controllers/UniqueConstraintViolationTest.java new file mode 100644 index 0000000..71a3153 --- /dev/null +++ b/src/test/java/com/company/resourceapi/controllers/UniqueConstraintViolationTest.java @@ -0,0 +1,49 @@ +package com.company.resourceapi.controllers; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.catchThrowableOfType; + +import com.company.resourceapi.entities.Project; +import com.company.resourceapi.repositories.ProjectRepository; +import com.company.resourceapi.repositories.SdlcSystemRepository; +import java.sql.SQLException; +import org.hibernate.exception.DataException; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.dao.DataIntegrityViolationException; +import org.springframework.transaction.annotation.Transactional; + +@SpringBootTest +@Transactional +class UniqueConstraintViolationTest { + + @Autowired + private ProjectRepository projectRepository; + + @Autowired + private SdlcSystemRepository sdlcSystemRepository; + + @Test + void givenDuplicateProjectWhenSavedThenRecognisedAsUniqueConstraintViolation() { + // given: (SAMPLEPROJECT, 1) already exists in data.sql + var duplicate = new Project(); + duplicate.setExternalId("SAMPLEPROJECT"); + duplicate.setSdlcSystem(sdlcSystemRepository.findById(1L).orElseThrow()); + + // when + var e = catchThrowableOfType(DataIntegrityViolationException.class, + () -> projectRepository.saveAndFlush(duplicate)); + + // then + assertThat(ProjectRestController.isUniqueConstraintViolation(e)).isTrue(); + } + + @Test + void givenOtherIntegrityViolationThenNotRecognisedAsUniqueConstraintViolation() { + var e = new DataIntegrityViolationException("value too long", + new DataException("value too long", new SQLException())); + + assertThat(ProjectRestController.isUniqueConstraintViolation(e)).isFalse(); + } +} diff --git a/src/test/java/com/company/resourceapi/repositories/ProjectRepositoryTest.java b/src/test/java/com/company/resourceapi/repositories/ProjectRepositoryTest.java new file mode 100644 index 0000000..53250f8 --- /dev/null +++ b/src/test/java/com/company/resourceapi/repositories/ProjectRepositoryTest.java @@ -0,0 +1,31 @@ +package com.company.resourceapi.repositories; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.time.Instant; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.transaction.annotation.Transactional; + +@SpringBootTest +@Transactional +class ProjectRepositoryTest { + + @Autowired + private ProjectRepository projectRepository; + + @Test + void givenChangedProjectWhenSavedThenLastModifiedDateIsUpdated() { + // given + var project = projectRepository.findById(2L).orElseThrow(); + project.setLastModifiedDate(Instant.EPOCH); + project.setName("Renamed"); + + // when + var saved = projectRepository.saveAndFlush(project); + + // then + assertThat(saved.getLastModifiedDate()).isAfter(Instant.EPOCH); + } +} diff --git a/src/test/java/com/company/resourceapi/services/impl/ProjectServiceImplTest.java b/src/test/java/com/company/resourceapi/services/impl/ProjectServiceImplTest.java index 863a9c3..4aa13c0 100755 --- a/src/test/java/com/company/resourceapi/services/impl/ProjectServiceImplTest.java +++ b/src/test/java/com/company/resourceapi/services/impl/ProjectServiceImplTest.java @@ -1,6 +1,6 @@ package com.company.resourceapi.services.impl; -import static org.mockito.ArgumentMatchers.any; +import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -27,17 +27,17 @@ void setup() { } @Test - void givenProjectWhenUpdateProjectThenSetModifiedLastDate() { + void givenProjectWhenUpdateProjectThenReturnSavedProject() { // given Project updatedProject = mock(Project.class); Project resultantProject = mock(Project.class); when(projectRepository.save(updatedProject)).thenReturn(resultantProject); // when - projectService.updateProject(updatedProject); + Project result = projectService.updateProject(updatedProject); // then - verify(updatedProject).setLastModifiedDate(any()); + assertThat(result).isSameAs(resultantProject); verify(projectRepository).save(updatedProject); } } From 8757af9a45451112fec6dd924f5157146654406d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 15 Oct 2026 12:02:14 +0000 Subject: [PATCH 3/7] Address review round 2: consistent externalId rules, second-precision audit dates - PATCH externalId uses the same blank rule as @NotBlank on POST, and an explicit null is rejected with 400 instead of being silently ignored - Audit timestamps are truncated to seconds to match the datetime columns, so responses show what is stored - Unique-violation detection walks the whole cause chain - SdlcSystemService.getSdlcSystem throws NotFoundException, like ProjectService.getProject, removing the controller-side lookup helper Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012MLyPXGHeCc1fvTeB8JnT2 --- .../resourceapi/config/JpaAuditingConfig.java | 13 +++++++- .../controllers/ProjectRestController.java | 32 +++++++++--------- .../services/SdlcSystemService.java | 4 +-- .../services/impl/SdlcSystemServiceImpl.java | 7 ++-- .../ProjectRestControllerConflictTest.java | 2 +- .../ProjectRestControllerTest.java | 33 +++++++++++++++++++ .../UniqueConstraintViolationTest.java | 13 ++++++++ 7 files changed, 82 insertions(+), 22 deletions(-) diff --git a/src/main/java/com/company/resourceapi/config/JpaAuditingConfig.java b/src/main/java/com/company/resourceapi/config/JpaAuditingConfig.java index 3052630..aa13020 100644 --- a/src/main/java/com/company/resourceapi/config/JpaAuditingConfig.java +++ b/src/main/java/com/company/resourceapi/config/JpaAuditingConfig.java @@ -1,6 +1,11 @@ package com.company.resourceapi.config; +import java.time.Instant; +import java.time.temporal.ChronoUnit; +import java.util.Optional; +import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; +import org.springframework.data.auditing.DateTimeProvider; import org.springframework.data.jpa.repository.config.EnableJpaAuditing; /** @@ -8,6 +13,12 @@ * Kept out of the application class so web slice tests don't need a JPA context. */ @Configuration -@EnableJpaAuditing +@EnableJpaAuditing(dateTimeProviderRef = "auditingDateTimeProvider") public class JpaAuditingConfig { + + /** The audit columns are {@code datetime}, so keep responses consistent with what is stored. */ + @Bean + DateTimeProvider auditingDateTimeProvider() { + return () -> Optional.of(Instant.now().truncatedTo(ChronoUnit.SECONDS)); + } } diff --git a/src/main/java/com/company/resourceapi/controllers/ProjectRestController.java b/src/main/java/com/company/resourceapi/controllers/ProjectRestController.java index a736c71..1cc5cb4 100755 --- a/src/main/java/com/company/resourceapi/controllers/ProjectRestController.java +++ b/src/main/java/com/company/resourceapi/controllers/ProjectRestController.java @@ -14,7 +14,6 @@ import io.swagger.v3.oas.annotations.tags.Tag; import jakarta.validation.Valid; import jakarta.validation.constraints.AssertTrue; -import jakarta.validation.constraints.Pattern; import jakarta.validation.constraints.Size; import java.net.URI; import lombok.Getter; @@ -64,7 +63,7 @@ public ResponseEntity getProject( public ResponseEntity createProject( @Valid @RequestBody Project project) throws NotFoundException { - var sdlcSystem = findSdlcSystem(project.getSdlcSystem().getId()); + var sdlcSystem = sdlcSystemService.getSdlcSystem(project.getSdlcSystem().getId()); if (projectService.getProject(project.getExternalId(), sdlcSystem.getId()).isPresent()) { return ResponseEntity.status(CONFLICT).build(); @@ -94,7 +93,8 @@ public ResponseEntity updateProject( } if (patchedProject.isSdlcSystemDirty()) { - project.setSdlcSystem(findSdlcSystem(patchedProject.getSdlcSystem().getId())); + project.setSdlcSystem( + sdlcSystemService.getSdlcSystem(patchedProject.getSdlcSystem().getId())); } if ((patchedProject.isExternalIdDirty() || patchedProject.isSdlcSystemDirty()) @@ -129,20 +129,18 @@ ResponseEntity handleDataIntegrityViolation(DataIntegrityViolationExceptio } static boolean isUniqueConstraintViolation(DataIntegrityViolationException e) { - return e.getCause() instanceof ConstraintViolationException violation - && violation.getKind() == ConstraintKind.UNIQUE; - } - - private SdlcSystem findSdlcSystem(long systemId) throws NotFoundException { - return sdlcSystemService.getSdlcSystem(systemId) - .orElseThrow(() -> new NotFoundException(SdlcSystem.class, systemId)); + for (Throwable cause = e.getCause(); cause != null; cause = cause.getCause()) { + if (cause instanceof ConstraintViolationException violation) { + return violation.getKind() == ConstraintKind.UNIQUE; + } + } + return false; } @Getter @NoArgsConstructor static class PatchedProject { @Size(max = Project.MAX_TEXT_LENGTH) - @Pattern(regexp = "(?s).*\\S.*", message = "must not be blank") private String externalId; @Size(max = Project.MAX_TEXT_LENGTH) private String name; @@ -152,10 +150,8 @@ static class PatchedProject { private boolean isSdlcSystemDirty = false; public void setExternalId(String externalId) { - if (externalId != null) { - this.externalId = externalId; - isExternalIdDirty = true; - } + this.externalId = externalId; + isExternalIdDirty = true; } public void setName(String name) { @@ -168,6 +164,12 @@ public void setSdlcSystem(SdlcSystem sdlcSystem) { isSdlcSystemDirty = true; } + /** Same rule as {@code @NotBlank} on {@link Project#getExternalId()}, applied only if patched. */ + @AssertTrue(message = "externalId must not be blank") + boolean isExternalIdPresentIfPatched() { + return !isExternalIdDirty || (externalId != null && !externalId.trim().isEmpty()); + } + /** A project always belongs to an SDLC system, so it can be replaced but not removed. */ @AssertTrue(message = "sdlcSystem must not be null") boolean isSdlcSystemPresentIfPatched() { diff --git a/src/main/java/com/company/resourceapi/services/SdlcSystemService.java b/src/main/java/com/company/resourceapi/services/SdlcSystemService.java index 4dac0ab..a200c1c 100755 --- a/src/main/java/com/company/resourceapi/services/SdlcSystemService.java +++ b/src/main/java/com/company/resourceapi/services/SdlcSystemService.java @@ -1,8 +1,8 @@ package com.company.resourceapi.services; import com.company.resourceapi.entities.SdlcSystem; -import java.util.Optional; +import com.company.resourceapi.exceptions.NotFoundException; public interface SdlcSystemService { - Optional getSdlcSystem(long id); + SdlcSystem getSdlcSystem(long id) throws NotFoundException; } diff --git a/src/main/java/com/company/resourceapi/services/impl/SdlcSystemServiceImpl.java b/src/main/java/com/company/resourceapi/services/impl/SdlcSystemServiceImpl.java index 8082fd8..a9b60ec 100755 --- a/src/main/java/com/company/resourceapi/services/impl/SdlcSystemServiceImpl.java +++ b/src/main/java/com/company/resourceapi/services/impl/SdlcSystemServiceImpl.java @@ -1,9 +1,9 @@ package com.company.resourceapi.services.impl; import com.company.resourceapi.entities.SdlcSystem; +import com.company.resourceapi.exceptions.NotFoundException; import com.company.resourceapi.repositories.SdlcSystemRepository; import com.company.resourceapi.services.SdlcSystemService; -import java.util.Optional; import lombok.RequiredArgsConstructor; import org.springframework.stereotype.Service; @@ -13,7 +13,8 @@ public class SdlcSystemServiceImpl implements SdlcSystemService { private final SdlcSystemRepository sdlcSystemRepository; - public Optional getSdlcSystem(long id) { - return sdlcSystemRepository.findById(id); + public SdlcSystem getSdlcSystem(long id) throws NotFoundException { + return sdlcSystemRepository.findById(id) + .orElseThrow(() -> new NotFoundException(SdlcSystem.class, id)); } } diff --git a/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerConflictTest.java b/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerConflictTest.java index c14c220..ff4c17b 100644 --- a/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerConflictTest.java +++ b/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerConflictTest.java @@ -39,7 +39,7 @@ void givenConcurrentDuplicateWhenPostThenReturnStatusConflict() throws Exception // given: the conflict check passes, but a concurrent request inserted the same project first var sdlcSystem = new SdlcSystem(); sdlcSystem.setId(1); - when(sdlcSystemService.getSdlcSystem(1)).thenReturn(Optional.of(sdlcSystem)); + when(sdlcSystemService.getSdlcSystem(1)).thenReturn(sdlcSystem); when(projectService.getProject(anyString(), anyLong())).thenReturn(Optional.empty()); when(projectService.createProject(any())).thenThrow(new DataIntegrityViolationException( "Unique index or primary key violation", new ConstraintViolationException( diff --git a/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerTest.java b/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerTest.java index eaa26af..7b9fb64 100755 --- a/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerTest.java +++ b/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerTest.java @@ -1,6 +1,7 @@ package com.company.resourceapi.controllers; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.matchesPattern; import static org.hamcrest.Matchers.not; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.patch; @@ -279,4 +280,36 @@ void givenRepositoryPathWhenGetThenReturnStatusNotFound() throws Exception { mockMvc.perform(get("/projects")) .andExpect(status().isNotFound()); } + + @Test + void givenNullExternalIdWhenPatchedThenReturnStatusBadRequest() throws Exception { + String requestUri = String.format(ENDPOINT_WITH_ID, 1); + + mockMvc.perform(patch(requestUri) + .contentType(MediaType.APPLICATION_JSON) + .content("{\"externalId\": null}")) + .andExpect(status().isBadRequest()); + } + + @Test + void givenControlCharacterExternalIdWhenPatchedThenReturnStatusBadRequest() throws Exception { + String requestUri = String.format(ENDPOINT_WITH_ID, 1); + + mockMvc.perform(patch(requestUri) + .contentType(MediaType.APPLICATION_JSON) + .content("{\"externalId\": \"\\u0001\"}")) + .andExpect(status().isBadRequest()); + } + + @Test + void givenProjectJsonWhenPostThenTimestampsMatchStoredPrecision() throws Exception { + final String jsonContent = String.format(PROJECT_JSON_TEMPLATE, "SECONDS", "Name", 1); + + mockMvc.perform(post(ProjectRestController.ENDPOINT) + .content(jsonContent) + .contentType(MediaType.APPLICATION_JSON)) + .andExpect(status().isCreated()) + .andExpect(jsonPath("$.createdDate", matchesPattern(".*T\\d{2}:\\d{2}:\\d{2}Z"))) + .andExpect(jsonPath("$.lastModifiedDate", matchesPattern(".*T\\d{2}:\\d{2}:\\d{2}Z"))); + } } diff --git a/src/test/java/com/company/resourceapi/controllers/UniqueConstraintViolationTest.java b/src/test/java/com/company/resourceapi/controllers/UniqueConstraintViolationTest.java index 71a3153..244e793 100644 --- a/src/test/java/com/company/resourceapi/controllers/UniqueConstraintViolationTest.java +++ b/src/test/java/com/company/resourceapi/controllers/UniqueConstraintViolationTest.java @@ -6,7 +6,10 @@ import com.company.resourceapi.entities.Project; import com.company.resourceapi.repositories.ProjectRepository; import com.company.resourceapi.repositories.SdlcSystemRepository; +import jakarta.persistence.PersistenceException; import java.sql.SQLException; +import org.hibernate.exception.ConstraintViolationException; +import org.hibernate.exception.ConstraintViolationException.ConstraintKind; import org.hibernate.exception.DataException; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; @@ -46,4 +49,14 @@ void givenOtherIntegrityViolationThenNotRecognisedAsUniqueConstraintViolation() assertThat(ProjectRestController.isUniqueConstraintViolation(e)).isFalse(); } + + @Test + void givenWrappedUniqueViolationThenRecognisedAsUniqueConstraintViolation() { + // e.g. a commit-time failure wrapped by the persistence provider + var e = new DataIntegrityViolationException("could not commit", + new PersistenceException(new ConstraintViolationException( + "duplicate", new SQLException(), ConstraintKind.UNIQUE, "system_project"))); + + assertThat(ProjectRestController.isUniqueConstraintViolation(e)).isTrue(); + } } From a2e03f454fcbfa91d2631cc4901f4194fbdb53d4 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 15 Oct 2026 12:06:20 +0000 Subject: [PATCH 4/7] Address review round 3: one validation rule for POST and PATCH - PATCH applies the changes to the loaded project and validates it with the entity's own constraints (@NotBlank, @Size, @NotNull), replacing the hand-copied @AssertTrue/@Size rules that had drifted from POST - Turn off open-in-view so the project PATCH loads is detached and a rejected change can never be flushed later in the request - Comment why the duplicate pre-check stays alongside the unique-key handler - Tests: POST and PATCH agree on unusual externalIds; a rejected PATCH leaves the project unchanged Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012MLyPXGHeCc1fvTeB8JnT2 --- .../controllers/ProjectRestController.java | 38 ++++++++++--------- src/main/resources/application.properties | 1 + .../ProjectRestControllerTest.java | 32 ++++++++++++++-- 3 files changed, 49 insertions(+), 22 deletions(-) diff --git a/src/main/java/com/company/resourceapi/controllers/ProjectRestController.java b/src/main/java/com/company/resourceapi/controllers/ProjectRestController.java index 1cc5cb4..5a75e0e 100755 --- a/src/main/java/com/company/resourceapi/controllers/ProjectRestController.java +++ b/src/main/java/com/company/resourceapi/controllers/ProjectRestController.java @@ -1,5 +1,6 @@ package com.company.resourceapi.controllers; +import static org.springframework.http.HttpStatus.BAD_REQUEST; import static org.springframework.http.HttpStatus.CONFLICT; import static org.springframework.http.HttpStatus.CREATED; import static org.springframework.http.HttpStatus.NOT_FOUND; @@ -13,8 +14,7 @@ import io.swagger.v3.oas.annotations.Parameter; import io.swagger.v3.oas.annotations.tags.Tag; import jakarta.validation.Valid; -import jakarta.validation.constraints.AssertTrue; -import jakarta.validation.constraints.Size; +import jakarta.validation.Validator; import java.net.URI; import lombok.Getter; import lombok.NoArgsConstructor; @@ -49,6 +49,7 @@ public class ProjectRestController { private final ProjectService projectService; private final SdlcSystemService sdlcSystemService; + private final Validator validator; @Operation(summary = "Get a Project") @GetMapping(ENDPOINT_ID) @@ -65,6 +66,7 @@ public ResponseEntity createProject( @RequestBody Project project) throws NotFoundException { var sdlcSystem = sdlcSystemService.getSdlcSystem(project.getSdlcSystem().getId()); + // Checked up front for a clean 409; the unique key still guards against concurrent requests if (projectService.getProject(project.getExternalId(), sdlcSystem.getId()).isPresent()) { return ResponseEntity.status(CONFLICT).build(); } @@ -82,6 +84,7 @@ public ResponseEntity updateProject( @Parameter(name = PATH_VARIABLE_ID, description = API_PARAM_ID, required = true) @PathVariable(PATH_VARIABLE_ID) long projectId, @Valid @RequestBody PatchedProject patchedProject) throws NotFoundException { + // Detached (open-in-view is off), so rejected changes below are never flushed var project = projectService.getProject(projectId); if (patchedProject.isNameDirty()) { @@ -93,8 +96,15 @@ public ResponseEntity updateProject( } if (patchedProject.isSdlcSystemDirty()) { - project.setSdlcSystem( - sdlcSystemService.getSdlcSystem(patchedProject.getSdlcSystem().getId())); + var patchedSystem = patchedProject.getSdlcSystem(); + project.setSdlcSystem(patchedSystem == null + ? null : sdlcSystemService.getSdlcSystem(patchedSystem.getId())); + } + + // Validate the patched project against the same constraints as POST + var violations = validator.validate(project); + if (!violations.isEmpty()) { + throw new jakarta.validation.ConstraintViolationException(violations); } if ((patchedProject.isExternalIdDirty() || patchedProject.isSdlcSystemDirty()) @@ -108,6 +118,12 @@ public ResponseEntity updateProject( return ResponseEntity.ok(projectService.updateProject(project)); } + @ExceptionHandler(jakarta.validation.ConstraintViolationException.class) + ResponseEntity handleInvalidPatch(jakarta.validation.ConstraintViolationException e) { + log.debug(e.getMessage()); + return ResponseEntity.status(BAD_REQUEST).build(); + } + @ExceptionHandler(NotFoundException.class) ResponseEntity handleNotFound(NotFoundException e) { log.debug(e.getMessage()); @@ -140,9 +156,7 @@ static boolean isUniqueConstraintViolation(DataIntegrityViolationException e) { @Getter @NoArgsConstructor static class PatchedProject { - @Size(max = Project.MAX_TEXT_LENGTH) private String externalId; - @Size(max = Project.MAX_TEXT_LENGTH) private String name; private SdlcSystem sdlcSystem; private boolean isNameDirty = false; @@ -163,17 +177,5 @@ public void setSdlcSystem(SdlcSystem sdlcSystem) { this.sdlcSystem = sdlcSystem; isSdlcSystemDirty = true; } - - /** Same rule as {@code @NotBlank} on {@link Project#getExternalId()}, applied only if patched. */ - @AssertTrue(message = "externalId must not be blank") - boolean isExternalIdPresentIfPatched() { - return !isExternalIdDirty || (externalId != null && !externalId.trim().isEmpty()); - } - - /** A project always belongs to an SDLC system, so it can be replaced but not removed. */ - @AssertTrue(message = "sdlcSystem must not be null") - boolean isSdlcSystemPresentIfPatched() { - return !isSdlcSystemDirty || sdlcSystem != null; - } } } diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index 377abda..154ee08 100755 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -4,4 +4,5 @@ spring.sql.init.platform=h2 spring.datasource.driver-class-name=org.h2.Driver # jpa spring.jpa.show-sql=true +spring.jpa.open-in-view=false spring.jpa.hibernate.ddl-auto=none diff --git a/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerTest.java b/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerTest.java index 7b9fb64..227ad23 100755 --- a/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerTest.java +++ b/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerTest.java @@ -1,5 +1,6 @@ package com.company.resourceapi.controllers; +import static org.assertj.core.api.Assertions.assertThat; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.matchesPattern; import static org.hamcrest.Matchers.not; @@ -11,6 +12,8 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.webmvc.test.autoconfigure.AutoConfigureMockMvc; import org.springframework.boot.test.context.SpringBootTest; @@ -291,14 +294,35 @@ void givenNullExternalIdWhenPatchedThenReturnStatusBadRequest() throws Exception .andExpect(status().isBadRequest()); } + @ParameterizedTest + @ValueSource(strings = {"\\u0001", "\\u3000", "\\t"}) + void givenUnusualExternalIdThenPostAndPatchApplySameRule(String externalId) throws Exception { + int postStatus = mockMvc.perform(post(ProjectRestController.ENDPOINT) + .content(String.format(PROJECT_JSON_TEMPLATE, externalId, "Name", 2)) + .contentType(MediaType.APPLICATION_JSON)) + .andReturn().getResponse().getStatus(); + int patchStatus = mockMvc.perform(patch(String.format(ENDPOINT_WITH_ID, 3)) + .content(String.format("{\"externalId\": \"%s\"}", externalId)) + .contentType(MediaType.APPLICATION_JSON)) + .andReturn().getResponse().getStatus(); + + assertThat(postStatus == 400).isEqualTo(patchStatus == 400); + } + @Test - void givenControlCharacterExternalIdWhenPatchedThenReturnStatusBadRequest() throws Exception { - String requestUri = String.format(ENDPOINT_WITH_ID, 1); + void givenRejectedPatchThenProjectIsUnchanged() throws Exception { + final long PROJECT_ID = 2; + String requestUri = String.format(ENDPOINT_WITH_ID, PROJECT_ID); + // valid externalId but unknown SDLC system: 404 after the externalId was applied mockMvc.perform(patch(requestUri) .contentType(MediaType.APPLICATION_JSON) - .content("{\"externalId\": \"\\u0001\"}")) - .andExpect(status().isBadRequest()); + .content("{\"externalId\": \"REJECTED\", \"sdlcSystem\": {\"id\": 999}}")) + .andExpect(status().isNotFound()); + + mockMvc.perform(get(requestUri)) + .andExpect(status().isOk()) + .andExpect(jsonPath(EXTERNALID_JPATH, equalTo("PROJECTX"))); } @Test From 7f3dc7e6793320a37fe7327b3e4bbf9abb98913e Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 15 Oct 2026 12:14:19 +0000 Subject: [PATCH 5/7] Address review round 4: transactional PATCH without lost updates - ProjectService.updateProject loads, applies, validates and conflict-checks the project in one transaction, rolling back on any rejection (including the checked NotFoundException); correctness no longer depends on the open-in-view setting, and the extra SELECT from merging a detached copy is gone - @DynamicUpdate writes only changed columns, so concurrent PATCHes of different fields no longer overwrite each other - The conflict query doesn't flush pending changes first, so the explicit 409 check runs against the stored state - An sdlcSystem reference without an id is a 400, not "SdlcSystem 0 not found" - Drop the no-op @Valid on the PATCH body; clearer handler naming - Tests: concurrent field updates are both kept, rejected PATCH rolls back (each verified to fail without its fix), service unit tests Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012MLyPXGHeCc1fvTeB8JnT2 --- .../controllers/ProjectRestController.java | 74 +++++++++---------- .../company/resourceapi/entities/Project.java | 2 + .../exceptions/ConflictException.java | 8 ++ .../repositories/ProjectRepository.java | 6 ++ .../resourceapi/services/ProjectService.java | 14 +++- .../services/impl/ProjectServiceImpl.java | 27 ++++++- .../ProjectRestControllerTest.java | 16 ++++ .../ProjectServiceImplConcurrencyTest.java | 57 ++++++++++++++ .../services/impl/ProjectServiceImplTest.java | 57 +++++++++++--- 9 files changed, 210 insertions(+), 51 deletions(-) create mode 100644 src/main/java/com/company/resourceapi/exceptions/ConflictException.java create mode 100644 src/test/java/com/company/resourceapi/services/impl/ProjectServiceImplConcurrencyTest.java diff --git a/src/main/java/com/company/resourceapi/controllers/ProjectRestController.java b/src/main/java/com/company/resourceapi/controllers/ProjectRestController.java index 5a75e0e..08d1028 100755 --- a/src/main/java/com/company/resourceapi/controllers/ProjectRestController.java +++ b/src/main/java/com/company/resourceapi/controllers/ProjectRestController.java @@ -7,20 +7,20 @@ import com.company.resourceapi.entities.Project; import com.company.resourceapi.entities.SdlcSystem; +import com.company.resourceapi.exceptions.ConflictException; import com.company.resourceapi.exceptions.NotFoundException; import com.company.resourceapi.services.ProjectService; import com.company.resourceapi.services.SdlcSystemService; import io.swagger.v3.oas.annotations.Operation; import io.swagger.v3.oas.annotations.Parameter; import io.swagger.v3.oas.annotations.tags.Tag; +import jakarta.validation.ConstraintViolationException; import jakarta.validation.Valid; -import jakarta.validation.Validator; import java.net.URI; import lombok.Getter; import lombok.NoArgsConstructor; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; -import org.hibernate.exception.ConstraintViolationException; import org.hibernate.exception.ConstraintViolationException.ConstraintKind; import org.springframework.dao.DataIntegrityViolationException; import org.springframework.http.ResponseEntity; @@ -32,6 +32,7 @@ import org.springframework.web.bind.annotation.RequestBody; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RestController; +import org.springframework.web.server.ResponseStatusException; @RestController @RequestMapping(ProjectRestController.ENDPOINT) @@ -49,7 +50,6 @@ public class ProjectRestController { private final ProjectService projectService; private final SdlcSystemService sdlcSystemService; - private final Validator validator; @Operation(summary = "Get a Project") @GetMapping(ENDPOINT_ID) @@ -64,7 +64,7 @@ public ResponseEntity getProject( public ResponseEntity createProject( @Valid @RequestBody Project project) throws NotFoundException { - var sdlcSystem = sdlcSystemService.getSdlcSystem(project.getSdlcSystem().getId()); + var sdlcSystem = findSdlcSystem(project.getSdlcSystem()); // Checked up front for a clean 409; the unique key still guards against concurrent requests if (projectService.getProject(project.getExternalId(), sdlcSystem.getId()).isPresent()) { @@ -83,43 +83,27 @@ public ResponseEntity createProject( public ResponseEntity updateProject( @Parameter(name = PATH_VARIABLE_ID, description = API_PARAM_ID, required = true) @PathVariable(PATH_VARIABLE_ID) long projectId, - @Valid @RequestBody PatchedProject patchedProject) throws NotFoundException { - // Detached (open-in-view is off), so rejected changes below are never flushed - var project = projectService.getProject(projectId); - - if (patchedProject.isNameDirty()) { - project.setName(patchedProject.getName()); - } - - if (patchedProject.isExternalIdDirty()) { - project.setExternalId(patchedProject.getExternalId()); - } - - if (patchedProject.isSdlcSystemDirty()) { - var patchedSystem = patchedProject.getSdlcSystem(); - project.setSdlcSystem(patchedSystem == null - ? null : sdlcSystemService.getSdlcSystem(patchedSystem.getId())); - } + @RequestBody PatchedProject patchedProject) throws NotFoundException { + var updatedProject = projectService.updateProject(projectId, project -> { + if (patchedProject.isNameDirty()) { + project.setName(patchedProject.getName()); + } - // Validate the patched project against the same constraints as POST - var violations = validator.validate(project); - if (!violations.isEmpty()) { - throw new jakarta.validation.ConstraintViolationException(violations); - } + if (patchedProject.isExternalIdDirty()) { + project.setExternalId(patchedProject.getExternalId()); + } - if ((patchedProject.isExternalIdDirty() || patchedProject.isSdlcSystemDirty()) - && projectService - .getProject(project.getExternalId(), project.getSdlcSystem().getId()) - .filter(existingProject -> existingProject.getId() != projectId) - .isPresent()) { - return ResponseEntity.status(CONFLICT).build(); - } + if (patchedProject.isSdlcSystemDirty()) { + var patchedSystem = patchedProject.getSdlcSystem(); + project.setSdlcSystem(patchedSystem == null ? null : findSdlcSystem(patchedSystem)); + } + }); - return ResponseEntity.ok(projectService.updateProject(project)); + return ResponseEntity.ok(updatedProject); } - @ExceptionHandler(jakarta.validation.ConstraintViolationException.class) - ResponseEntity handleInvalidPatch(jakarta.validation.ConstraintViolationException e) { + @ExceptionHandler(ConstraintViolationException.class) + ResponseEntity handleConstraintViolation(ConstraintViolationException e) { log.debug(e.getMessage()); return ResponseEntity.status(BAD_REQUEST).build(); } @@ -130,9 +114,15 @@ ResponseEntity handleNotFound(NotFoundException e) { return ResponseEntity.status(NOT_FOUND).build(); } + @ExceptionHandler(ConflictException.class) + ResponseEntity handleConflict(ConflictException e) { + log.debug(e.getMessage()); + return ResponseEntity.status(CONFLICT).build(); + } + /** * The (sdlcSystem, externalId) pair is unique, so a concurrent request can still insert a - * duplicate after the conflict check above has passed. Any other integrity violation is a + * duplicate after the conflict checks have passed. Any other integrity violation is a * server-side problem and is left to the default error handling. */ @ExceptionHandler(DataIntegrityViolationException.class) @@ -146,13 +136,21 @@ ResponseEntity handleDataIntegrityViolation(DataIntegrityViolationExceptio static boolean isUniqueConstraintViolation(DataIntegrityViolationException e) { for (Throwable cause = e.getCause(); cause != null; cause = cause.getCause()) { - if (cause instanceof ConstraintViolationException violation) { + if (cause instanceof org.hibernate.exception.ConstraintViolationException violation) { return violation.getKind() == ConstraintKind.UNIQUE; } } return false; } + /** Resolves the SDLC system a request refers to; database ids start at 1, so 0 means "missing". */ + private SdlcSystem findSdlcSystem(SdlcSystem reference) throws NotFoundException { + if (reference.getId() <= 0) { + throw new ResponseStatusException(BAD_REQUEST, "sdlcSystem.id is required"); + } + return sdlcSystemService.getSdlcSystem(reference.getId()); + } + @Getter @NoArgsConstructor static class PatchedProject { diff --git a/src/main/java/com/company/resourceapi/entities/Project.java b/src/main/java/com/company/resourceapi/entities/Project.java index cc8b2b0..3ed07c4 100755 --- a/src/main/java/com/company/resourceapi/entities/Project.java +++ b/src/main/java/com/company/resourceapi/entities/Project.java @@ -18,6 +18,7 @@ import java.time.Instant; import lombok.Data; import lombok.NoArgsConstructor; +import org.hibernate.annotations.DynamicUpdate; import org.springframework.data.annotation.CreatedDate; import org.springframework.data.annotation.LastModifiedDate; import org.springframework.data.jpa.domain.support.AuditingEntityListener; @@ -27,6 +28,7 @@ @EntityListeners(AuditingEntityListener.class) @Data @NoArgsConstructor +@DynamicUpdate public class Project { /** Length of the varchar columns in schema.sql. */ diff --git a/src/main/java/com/company/resourceapi/exceptions/ConflictException.java b/src/main/java/com/company/resourceapi/exceptions/ConflictException.java new file mode 100644 index 0000000..b9a139f --- /dev/null +++ b/src/main/java/com/company/resourceapi/exceptions/ConflictException.java @@ -0,0 +1,8 @@ +package com.company.resourceapi.exceptions; + +public class ConflictException extends RuntimeException { + + public ConflictException(String message) { + super(message); + } +} diff --git a/src/main/java/com/company/resourceapi/repositories/ProjectRepository.java b/src/main/java/com/company/resourceapi/repositories/ProjectRepository.java index dc873c1..91e2070 100755 --- a/src/main/java/com/company/resourceapi/repositories/ProjectRepository.java +++ b/src/main/java/com/company/resourceapi/repositories/ProjectRepository.java @@ -1,13 +1,19 @@ package com.company.resourceapi.repositories; import com.company.resourceapi.entities.Project; +import jakarta.persistence.QueryHint; import java.util.Optional; +import org.hibernate.jpa.HibernateHints; import org.springframework.data.jpa.repository.JpaRepository; +import org.springframework.data.jpa.repository.QueryHints; import org.springframework.stereotype.Repository; @Repository public interface ProjectRepository extends JpaRepository{ Optional findBySdlcSystemIdAndId(long sdlcSystemId, long projectId); + + /** Doesn't flush pending changes first, so it can check a modified project before it is saved. */ + @QueryHints(@QueryHint(name = HibernateHints.HINT_FLUSH_MODE, value = "COMMIT")) Optional findByExternalIdAndSdlcSystemId(String externalId, long sdlcSystemId); } diff --git a/src/main/java/com/company/resourceapi/services/ProjectService.java b/src/main/java/com/company/resourceapi/services/ProjectService.java index 054e56f..c4d86c9 100755 --- a/src/main/java/com/company/resourceapi/services/ProjectService.java +++ b/src/main/java/com/company/resourceapi/services/ProjectService.java @@ -10,5 +10,17 @@ public interface ProjectService { Project createProject(Project project); Optional getProject(String externalId, long sdlcSystemId); Optional getProjectStream(long id); - Project updateProject(Project updatedProject); + + /** + * Applies {@code update} to the stored project and saves it in one transaction. The change is + * rolled back if the update throws, the result violates the entity's constraints + * ({@link jakarta.validation.ConstraintViolationException}) or another project in the same + * SDLC system already has the external ID ({@link com.company.resourceapi.exceptions.ConflictException}). + */ + Project updateProject(long id, ProjectUpdate update) throws NotFoundException; + + @FunctionalInterface + interface ProjectUpdate { + void applyTo(Project project) throws NotFoundException; + } } diff --git a/src/main/java/com/company/resourceapi/services/impl/ProjectServiceImpl.java b/src/main/java/com/company/resourceapi/services/impl/ProjectServiceImpl.java index ffd625f..e0cf471 100755 --- a/src/main/java/com/company/resourceapi/services/impl/ProjectServiceImpl.java +++ b/src/main/java/com/company/resourceapi/services/impl/ProjectServiceImpl.java @@ -1,17 +1,22 @@ package com.company.resourceapi.services.impl; import com.company.resourceapi.entities.Project; +import com.company.resourceapi.exceptions.ConflictException; import com.company.resourceapi.exceptions.NotFoundException; import com.company.resourceapi.repositories.ProjectRepository; import com.company.resourceapi.services.ProjectService; +import jakarta.validation.ConstraintViolationException; +import jakarta.validation.Validator; import java.util.Optional; import lombok.RequiredArgsConstructor; import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Transactional; @Service @RequiredArgsConstructor public class ProjectServiceImpl implements ProjectService { private final ProjectRepository projectRepository; + private final Validator validator; public Project getProject(long id) throws NotFoundException { return getProjectStream(id).orElseThrow(() -> new NotFoundException(Project.class, id)); @@ -30,7 +35,25 @@ public Optional getProjectStream(long id) { return projectRepository.findById(id); } - public Project updateProject(Project updatedProject) { - return projectRepository.save(updatedProject); + @Transactional(rollbackFor = NotFoundException.class) + public Project updateProject(long id, ProjectUpdate update) throws NotFoundException { + var project = getProject(id); + update.applyTo(project); + + var violations = validator.validate(project); + if (!violations.isEmpty()) { + throw new ConstraintViolationException(violations); + } + + if (projectRepository + .findByExternalIdAndSdlcSystemId(project.getExternalId(), project.getSdlcSystem().getId()) + .filter(existingProject -> existingProject.getId() != id) + .isPresent()) { + throw new ConflictException("Project %s already exists in SdlcSystem %d." + .formatted(project.getExternalId(), project.getSdlcSystem().getId())); + } + + // Managed entity: flushed on commit, only the changed columns (@DynamicUpdate) + return project; } } diff --git a/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerTest.java b/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerTest.java index 227ad23..8df6bbf 100755 --- a/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerTest.java +++ b/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerTest.java @@ -336,4 +336,20 @@ void givenProjectJsonWhenPostThenTimestampsMatchStoredPrecision() throws Excepti .andExpect(jsonPath("$.createdDate", matchesPattern(".*T\\d{2}:\\d{2}:\\d{2}Z"))) .andExpect(jsonPath("$.lastModifiedDate", matchesPattern(".*T\\d{2}:\\d{2}:\\d{2}Z"))); } + + @Test + void givenSdlcSystemWithoutIdWhenPostThenReturnStatusBadRequest() throws Exception { + mockMvc.perform(post(ProjectRestController.ENDPOINT) + .content("{\"externalId\": \"NOSYSTEMID\", \"sdlcSystem\": {}}") + .contentType(MediaType.APPLICATION_JSON)) + .andExpect(status().isBadRequest()); + } + + @Test + void givenSdlcSystemWithoutIdWhenPatchedThenReturnStatusBadRequest() throws Exception { + mockMvc.perform(patch(String.format(ENDPOINT_WITH_ID, 1)) + .content("{\"sdlcSystem\": {}}") + .contentType(MediaType.APPLICATION_JSON)) + .andExpect(status().isBadRequest()); + } } diff --git a/src/test/java/com/company/resourceapi/services/impl/ProjectServiceImplConcurrencyTest.java b/src/test/java/com/company/resourceapi/services/impl/ProjectServiceImplConcurrencyTest.java new file mode 100644 index 0000000..136740b --- /dev/null +++ b/src/test/java/com/company/resourceapi/services/impl/ProjectServiceImplConcurrencyTest.java @@ -0,0 +1,57 @@ +package com.company.resourceapi.services.impl; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.company.resourceapi.entities.Project; +import com.company.resourceapi.repositories.ProjectRepository; +import com.company.resourceapi.repositories.SdlcSystemRepository; +import com.company.resourceapi.services.ProjectService; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.transaction.PlatformTransactionManager; +import org.springframework.transaction.TransactionDefinition; +import org.springframework.transaction.support.TransactionTemplate; + +@SpringBootTest +class ProjectServiceImplConcurrencyTest { + + @Autowired + private ProjectService projectService; + + @Autowired + private ProjectRepository projectRepository; + + @Autowired + private SdlcSystemRepository sdlcSystemRepository; + + @Autowired + private PlatformTransactionManager transactionManager; + + @Test + void givenConcurrentUpdatesOfDifferentFieldsThenBothChangesAreKept() throws Exception { + // given + var project = new Project(); + project.setExternalId("CONCURRENT"); + project.setName("Original"); + project.setSdlcSystem(sdlcSystemRepository.findById(3L).orElseThrow()); + long projectId = projectRepository.save(project).getId(); + + var otherRequest = new TransactionTemplate(transactionManager); + otherRequest.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRES_NEW); + + // when: another request renames the project after this update has loaded it + projectService.updateProject(projectId, loaded -> { + otherRequest.executeWithoutResult(status -> { + var concurrent = projectRepository.findById(projectId).orElseThrow(); + concurrent.setName("Renamed concurrently"); + }); + loaded.setExternalId("CONCURRENT-EDITED"); + }); + + // then + var stored = projectRepository.findById(projectId).orElseThrow(); + assertThat(stored.getExternalId()).isEqualTo("CONCURRENT-EDITED"); + assertThat(stored.getName()).isEqualTo("Renamed concurrently"); + } +} diff --git a/src/test/java/com/company/resourceapi/services/impl/ProjectServiceImplTest.java b/src/test/java/com/company/resourceapi/services/impl/ProjectServiceImplTest.java index 4aa13c0..9684fa4 100755 --- a/src/test/java/com/company/resourceapi/services/impl/ProjectServiceImplTest.java +++ b/src/test/java/com/company/resourceapi/services/impl/ProjectServiceImplTest.java @@ -1,12 +1,18 @@ package com.company.resourceapi.services.impl; import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.when; import com.company.resourceapi.entities.Project; +import com.company.resourceapi.entities.SdlcSystem; +import com.company.resourceapi.exceptions.ConflictException; import com.company.resourceapi.repositories.ProjectRepository; +import jakarta.validation.ConstraintViolationException; +import jakarta.validation.Validation; +import java.util.Optional; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -16,28 +22,59 @@ @ExtendWith(MockitoExtension.class) public class ProjectServiceImplTest { + private static final long PROJECT_ID = 1; + @Mock private ProjectRepository projectRepository; private ProjectServiceImpl projectService; + private Project storedProject; + @BeforeEach void setup() { - projectService = new ProjectServiceImpl(projectRepository); + projectService = new ProjectServiceImpl(projectRepository, + Validation.buildDefaultValidatorFactory().getValidator()); + + var sdlcSystem = new SdlcSystem(); + sdlcSystem.setId(1); + storedProject = new Project(); + storedProject.setId(PROJECT_ID); + storedProject.setExternalId("SAMPLEPROJECT"); + storedProject.setSdlcSystem(sdlcSystem); + when(projectRepository.findById(PROJECT_ID)).thenReturn(Optional.of(storedProject)); } @Test - void givenProjectWhenUpdateProjectThenReturnSavedProject() { + void givenValidUpdateWhenUpdateProjectThenReturnUpdatedProject() throws Exception { // given - Project updatedProject = mock(Project.class); - Project resultantProject = mock(Project.class); - when(projectRepository.save(updatedProject)).thenReturn(resultantProject); + when(projectRepository.findByExternalIdAndSdlcSystemId(anyString(), anyLong())) + .thenReturn(Optional.of(storedProject)); // when - Project result = projectService.updateProject(updatedProject); + Project result = projectService.updateProject(PROJECT_ID, project -> project.setName("Renamed")); // then - assertThat(result).isSameAs(resultantProject); - verify(projectRepository).save(updatedProject); + assertThat(result).isSameAs(storedProject); + assertThat(result.getName()).isEqualTo("Renamed"); + } + + @Test + void givenInvalidUpdateWhenUpdateProjectThenThrowConstraintViolation() { + assertThatThrownBy(() -> projectService.updateProject(PROJECT_ID, project -> project.setExternalId(" "))) + .isInstanceOf(ConstraintViolationException.class); + } + + @Test + void givenDuplicateExternalIdWhenUpdateProjectThenThrowConflict() { + // given + var otherProject = new Project(); + otherProject.setId(2); + when(projectRepository.findByExternalIdAndSdlcSystemId("PROJECTX", 1)) + .thenReturn(Optional.of(otherProject)); + + // when / then + assertThatThrownBy(() -> projectService.updateProject(PROJECT_ID, project -> project.setExternalId("PROJECTX"))) + .isInstanceOf(ConflictException.class); } } From 196391ac716441a45a805dd2c26ff20c2cad949d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 15 Oct 2026 12:17:31 +0000 Subject: [PATCH 6/7] Address review round 5: skip needless duplicate query, test commit-time 409 - Only query for a duplicate when the PATCH changes externalId or the SDLC system - Test that a unique violation raised at commit is recognised as one (the path concurrent PATCHes take) - Concurrency test removes the project it creates from the shared database Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012MLyPXGHeCc1fvTeB8JnT2 --- .../services/impl/ProjectServiceImpl.java | 6 ++++- .../UniqueConstraintViolationTest.java | 21 +++++++++++++++ .../ProjectServiceImplConcurrencyTest.java | 27 +++++++++++-------- .../services/impl/ProjectServiceImplTest.java | 9 +++---- 4 files changed, 46 insertions(+), 17 deletions(-) diff --git a/src/main/java/com/company/resourceapi/services/impl/ProjectServiceImpl.java b/src/main/java/com/company/resourceapi/services/impl/ProjectServiceImpl.java index e0cf471..334e5d6 100755 --- a/src/main/java/com/company/resourceapi/services/impl/ProjectServiceImpl.java +++ b/src/main/java/com/company/resourceapi/services/impl/ProjectServiceImpl.java @@ -38,6 +38,8 @@ public Optional getProjectStream(long id) { @Transactional(rollbackFor = NotFoundException.class) public Project updateProject(long id, ProjectUpdate update) throws NotFoundException { var project = getProject(id); + var externalIdBefore = project.getExternalId(); + var sdlcSystemBefore = project.getSdlcSystem(); update.applyTo(project); var violations = validator.validate(project); @@ -45,7 +47,9 @@ public Project updateProject(long id, ProjectUpdate update) throws NotFoundExcep throw new ConstraintViolationException(violations); } - if (projectRepository + boolean keyChanged = !project.getExternalId().equals(externalIdBefore) + || project.getSdlcSystem().getId() != sdlcSystemBefore.getId(); + if (keyChanged && projectRepository .findByExternalIdAndSdlcSystemId(project.getExternalId(), project.getSdlcSystem().getId()) .filter(existingProject -> existingProject.getId() != id) .isPresent()) { diff --git a/src/test/java/com/company/resourceapi/controllers/UniqueConstraintViolationTest.java b/src/test/java/com/company/resourceapi/controllers/UniqueConstraintViolationTest.java index 244e793..45650cb 100644 --- a/src/test/java/com/company/resourceapi/controllers/UniqueConstraintViolationTest.java +++ b/src/test/java/com/company/resourceapi/controllers/UniqueConstraintViolationTest.java @@ -15,7 +15,10 @@ import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.dao.DataIntegrityViolationException; +import org.springframework.transaction.PlatformTransactionManager; +import org.springframework.transaction.TransactionDefinition; import org.springframework.transaction.annotation.Transactional; +import org.springframework.transaction.support.TransactionTemplate; @SpringBootTest @Transactional @@ -27,6 +30,9 @@ class UniqueConstraintViolationTest { @Autowired private SdlcSystemRepository sdlcSystemRepository; + @Autowired + private PlatformTransactionManager transactionManager; + @Test void givenDuplicateProjectWhenSavedThenRecognisedAsUniqueConstraintViolation() { // given: (SAMPLEPROJECT, 1) already exists in data.sql @@ -59,4 +65,19 @@ void givenWrappedUniqueViolationThenRecognisedAsUniqueConstraintViolation() { assertThat(ProjectRestController.isUniqueConstraintViolation(e)).isTrue(); } + + @Test + void givenDuplicateDetectedAtCommitThenRecognisedAsUniqueConstraintViolation() { + // given: a separate transaction whose change only hits the unique key when it commits + var concurrentRequest = new TransactionTemplate(transactionManager); + concurrentRequest.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRES_NEW); + + // when: project 2 (PROJECTX, 2) is renamed to SAMPLEPROJECT, which project 3 already has in system 2 + var e = catchThrowableOfType(DataIntegrityViolationException.class, + () -> concurrentRequest.executeWithoutResult(status -> + projectRepository.findById(2L).orElseThrow().setExternalId("SAMPLEPROJECT"))); + + // then + assertThat(ProjectRestController.isUniqueConstraintViolation(e)).isTrue(); + } } diff --git a/src/test/java/com/company/resourceapi/services/impl/ProjectServiceImplConcurrencyTest.java b/src/test/java/com/company/resourceapi/services/impl/ProjectServiceImplConcurrencyTest.java index 136740b..7b597f8 100644 --- a/src/test/java/com/company/resourceapi/services/impl/ProjectServiceImplConcurrencyTest.java +++ b/src/test/java/com/company/resourceapi/services/impl/ProjectServiceImplConcurrencyTest.java @@ -40,18 +40,23 @@ void givenConcurrentUpdatesOfDifferentFieldsThenBothChangesAreKept() throws Exce var otherRequest = new TransactionTemplate(transactionManager); otherRequest.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRES_NEW); - // when: another request renames the project after this update has loaded it - projectService.updateProject(projectId, loaded -> { - otherRequest.executeWithoutResult(status -> { - var concurrent = projectRepository.findById(projectId).orElseThrow(); - concurrent.setName("Renamed concurrently"); + try { + // when: another request renames the project after this update has loaded it + projectService.updateProject(projectId, loaded -> { + otherRequest.executeWithoutResult(status -> { + var concurrent = projectRepository.findById(projectId).orElseThrow(); + concurrent.setName("Renamed concurrently"); + }); + loaded.setExternalId("CONCURRENT-EDITED"); }); - loaded.setExternalId("CONCURRENT-EDITED"); - }); - // then - var stored = projectRepository.findById(projectId).orElseThrow(); - assertThat(stored.getExternalId()).isEqualTo("CONCURRENT-EDITED"); - assertThat(stored.getName()).isEqualTo("Renamed concurrently"); + // then + var stored = projectRepository.findById(projectId).orElseThrow(); + assertThat(stored.getExternalId()).isEqualTo("CONCURRENT-EDITED"); + assertThat(stored.getName()).isEqualTo("Renamed concurrently"); + } finally { + // the context (and its in-memory database) is shared with other test classes + projectRepository.deleteById(projectId); + } } } diff --git a/src/test/java/com/company/resourceapi/services/impl/ProjectServiceImplTest.java b/src/test/java/com/company/resourceapi/services/impl/ProjectServiceImplTest.java index 9684fa4..d15418f 100755 --- a/src/test/java/com/company/resourceapi/services/impl/ProjectServiceImplTest.java +++ b/src/test/java/com/company/resourceapi/services/impl/ProjectServiceImplTest.java @@ -4,6 +4,8 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import com.company.resourceapi.entities.Project; @@ -46,17 +48,14 @@ void setup() { } @Test - void givenValidUpdateWhenUpdateProjectThenReturnUpdatedProject() throws Exception { - // given - when(projectRepository.findByExternalIdAndSdlcSystemId(anyString(), anyLong())) - .thenReturn(Optional.of(storedProject)); - + void givenNameOnlyUpdateWhenUpdateProjectThenSkipDuplicateCheck() throws Exception { // when Project result = projectService.updateProject(PROJECT_ID, project -> project.setName("Renamed")); // then assertThat(result).isSameAs(storedProject); assertThat(result.getName()).isEqualTo("Renamed"); + verify(projectRepository, never()).findByExternalIdAndSdlcSystemId(anyString(), anyLong()); } @Test From 1dbad2fdd4388f90c1c37e5f7af6d889cae62017 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 15 Oct 2026 12:22:13 +0000 Subject: [PATCH 7/7] Address review round 6: scope no-flush query to PATCH, 404 for negative ids - The FlushMode COMMIT hint moves to a dedicated existsByExternalIdAndSdlcSystemIdAndIdNot query used only by updateProject, so other callers of the shared finder keep auto-flush - Only an absent sdlcSystem id (0) is a 400; a negative id is looked up and reported as 404 like any other unknown system Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_012MLyPXGHeCc1fvTeB8JnT2 --- .../resourceapi/controllers/ProjectRestController.java | 4 ++-- .../resourceapi/repositories/ProjectRepository.java | 8 ++++++-- .../resourceapi/services/impl/ProjectServiceImpl.java | 6 ++---- .../controllers/ProjectRestControllerTest.java | 8 ++++++++ .../services/impl/ProjectServiceImplTest.java | 9 ++++----- 5 files changed, 22 insertions(+), 13 deletions(-) diff --git a/src/main/java/com/company/resourceapi/controllers/ProjectRestController.java b/src/main/java/com/company/resourceapi/controllers/ProjectRestController.java index 08d1028..f6d0cbf 100755 --- a/src/main/java/com/company/resourceapi/controllers/ProjectRestController.java +++ b/src/main/java/com/company/resourceapi/controllers/ProjectRestController.java @@ -143,9 +143,9 @@ static boolean isUniqueConstraintViolation(DataIntegrityViolationException e) { return false; } - /** Resolves the SDLC system a request refers to; database ids start at 1, so 0 means "missing". */ + /** Resolves the SDLC system a request refers to; an absent id deserializes to 0. */ private SdlcSystem findSdlcSystem(SdlcSystem reference) throws NotFoundException { - if (reference.getId() <= 0) { + if (reference.getId() == 0) { throw new ResponseStatusException(BAD_REQUEST, "sdlcSystem.id is required"); } return sdlcSystemService.getSdlcSystem(reference.getId()); diff --git a/src/main/java/com/company/resourceapi/repositories/ProjectRepository.java b/src/main/java/com/company/resourceapi/repositories/ProjectRepository.java index 91e2070..d1cc2df 100755 --- a/src/main/java/com/company/resourceapi/repositories/ProjectRepository.java +++ b/src/main/java/com/company/resourceapi/repositories/ProjectRepository.java @@ -12,8 +12,12 @@ public interface ProjectRepository extends JpaRepository{ Optional findBySdlcSystemIdAndId(long sdlcSystemId, long projectId); + Optional findByExternalIdAndSdlcSystemId(String externalId, long sdlcSystemId); - /** Doesn't flush pending changes first, so it can check a modified project before it is saved. */ + /** + * Finds another project with the same key. Doesn't flush pending changes first, so it can + * check a modified project before that change is written. + */ @QueryHints(@QueryHint(name = HibernateHints.HINT_FLUSH_MODE, value = "COMMIT")) - Optional findByExternalIdAndSdlcSystemId(String externalId, long sdlcSystemId); + boolean existsByExternalIdAndSdlcSystemIdAndIdNot(String externalId, long sdlcSystemId, long id); } diff --git a/src/main/java/com/company/resourceapi/services/impl/ProjectServiceImpl.java b/src/main/java/com/company/resourceapi/services/impl/ProjectServiceImpl.java index 334e5d6..8aef1bf 100755 --- a/src/main/java/com/company/resourceapi/services/impl/ProjectServiceImpl.java +++ b/src/main/java/com/company/resourceapi/services/impl/ProjectServiceImpl.java @@ -49,10 +49,8 @@ public Project updateProject(long id, ProjectUpdate update) throws NotFoundExcep boolean keyChanged = !project.getExternalId().equals(externalIdBefore) || project.getSdlcSystem().getId() != sdlcSystemBefore.getId(); - if (keyChanged && projectRepository - .findByExternalIdAndSdlcSystemId(project.getExternalId(), project.getSdlcSystem().getId()) - .filter(existingProject -> existingProject.getId() != id) - .isPresent()) { + if (keyChanged && projectRepository.existsByExternalIdAndSdlcSystemIdAndIdNot( + project.getExternalId(), project.getSdlcSystem().getId(), id)) { throw new ConflictException("Project %s already exists in SdlcSystem %d." .formatted(project.getExternalId(), project.getSdlcSystem().getId())); } diff --git a/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerTest.java b/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerTest.java index 8df6bbf..e9bcc28 100755 --- a/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerTest.java +++ b/src/test/java/com/company/resourceapi/controllers/ProjectRestControllerTest.java @@ -352,4 +352,12 @@ void givenSdlcSystemWithoutIdWhenPatchedThenReturnStatusBadRequest() throws Exce .contentType(MediaType.APPLICATION_JSON)) .andExpect(status().isBadRequest()); } + + @Test + void givenNegativeSdlcSystemIdWhenPostThenReturnStatusNotFound() throws Exception { + mockMvc.perform(post(ProjectRestController.ENDPOINT) + .content(String.format(PROJECT_JSON_TEMPLATE, "NEGATIVE", "Name", -5)) + .contentType(MediaType.APPLICATION_JSON)) + .andExpect(status().isNotFound()); + } } diff --git a/src/test/java/com/company/resourceapi/services/impl/ProjectServiceImplTest.java b/src/test/java/com/company/resourceapi/services/impl/ProjectServiceImplTest.java index d15418f..85716a7 100755 --- a/src/test/java/com/company/resourceapi/services/impl/ProjectServiceImplTest.java +++ b/src/test/java/com/company/resourceapi/services/impl/ProjectServiceImplTest.java @@ -55,7 +55,8 @@ void givenNameOnlyUpdateWhenUpdateProjectThenSkipDuplicateCheck() throws Excepti // then assertThat(result).isSameAs(storedProject); assertThat(result.getName()).isEqualTo("Renamed"); - verify(projectRepository, never()).findByExternalIdAndSdlcSystemId(anyString(), anyLong()); + verify(projectRepository, never()) + .existsByExternalIdAndSdlcSystemIdAndIdNot(anyString(), anyLong(), anyLong()); } @Test @@ -67,10 +68,8 @@ void givenInvalidUpdateWhenUpdateProjectThenThrowConstraintViolation() { @Test void givenDuplicateExternalIdWhenUpdateProjectThenThrowConflict() { // given - var otherProject = new Project(); - otherProject.setId(2); - when(projectRepository.findByExternalIdAndSdlcSystemId("PROJECTX", 1)) - .thenReturn(Optional.of(otherProject)); + when(projectRepository.existsByExternalIdAndSdlcSystemIdAndIdNot("PROJECTX", 1, PROJECT_ID)) + .thenReturn(true); // when / then assertThatThrownBy(() -> projectService.updateProject(PROJECT_ID, project -> project.setExternalId("PROJECTX")))