From c749001758bab9ed246714a338c7891a26df0771 Mon Sep 17 00:00:00 2001 From: Andrea Lamparelli Date: Mon, 24 Feb 2025 11:55:11 +0100 Subject: [PATCH] Split add and update test apis Signed-off-by: Andrea Lamparelli --- docs/site/content/en/openapi/openapi.yaml | 18 +++++ .../horreum/api/services/TestService.java | 5 ++ .../tools/horreum/svc/TestServiceImpl.java | 74 ++++++++++++------- .../horreum/svc/TestServiceNoRestTest.java | 28 ++++++- 4 files changed, 97 insertions(+), 28 deletions(-) diff --git a/docs/site/content/en/openapi/openapi.yaml b/docs/site/content/en/openapi/openapi.yaml index cda16ca0c..57d52a1c1 100644 --- a/docs/site/content/en/openapi/openapi.yaml +++ b/docs/site/content/en/openapi/openapi.yaml @@ -1730,6 +1730,24 @@ paths: application/json: schema: $ref: "#/components/schemas/TestQueryResult" + put: + tags: + - Test + description: Update an existing test + operationId: update + requestBody: + content: + application/json: + schema: + $ref: "#/components/schemas/Test" + required: true + responses: + "200": + description: OK + content: + application/json: + schema: + $ref: "#/components/schemas/Test" post: tags: - Test diff --git a/horreum-api/src/main/java/io/hyperfoil/tools/horreum/api/services/TestService.java b/horreum-api/src/main/java/io/hyperfoil/tools/horreum/api/services/TestService.java index 06cf3d733..c98abf0a1 100644 --- a/horreum-api/src/main/java/io/hyperfoil/tools/horreum/api/services/TestService.java +++ b/horreum-api/src/main/java/io/hyperfoil/tools/horreum/api/services/TestService.java @@ -9,6 +9,7 @@ import jakarta.ws.rs.DefaultValue; import jakarta.ws.rs.GET; import jakarta.ws.rs.POST; +import jakarta.ws.rs.PUT; import jakarta.ws.rs.Path; import jakarta.ws.rs.PathParam; import jakarta.ws.rs.Produces; @@ -70,6 +71,10 @@ public interface TestService { @Operation(description = "Create a new test") Test add(@RequestBody(required = true) Test test); + @PUT + @Operation(description = "Update an existing test") + Test update(@RequestBody(required = true) Test test); + @GET @Operation(description = "Retrieve a paginated list of Tests with available count") @Parameters(value = { diff --git a/horreum-backend/src/main/java/io/hyperfoil/tools/horreum/svc/TestServiceImpl.java b/horreum-backend/src/main/java/io/hyperfoil/tools/horreum/svc/TestServiceImpl.java index 671ce507e..deb0158c7 100644 --- a/horreum-backend/src/main/java/io/hyperfoil/tools/horreum/svc/TestServiceImpl.java +++ b/horreum-backend/src/main/java/io/hyperfoil/tools/horreum/svc/TestServiceImpl.java @@ -198,16 +198,38 @@ public Test add(Test dto) { if (!identity.hasRole(dto.owner)) { throw ServiceException.forbidden("This user does not have the " + dto.owner + " role!"); } - if (dto.name == null || dto.name.isBlank()) - throw ServiceException.badRequest("Test name can not be empty"); + + // we are adding a new test, ensure no IDs are present + dto.clearIds(); + log.debugf("Creating new test: %s", dto.toString()); - return TestMapper.from(addAuthenticated(dto)); + return TestMapper.from(addOrUpdateAuthenticated(dto)); + } + + @Override + @RolesAllowed(Roles.TESTER) + @WithRoles + @Transactional + public Test update(Test dto) { + if (!identity.hasRole(dto.owner)) { + throw ServiceException.forbidden("This user does not have the " + dto.owner + " role!"); + } + + if (dto.id == null || TestDAO.findById(dto.id) == null) { + throw ServiceException.notFound("Missing test id or test with id " + dto.id + " does not exist"); + } + + log.debugf("Updating test (%d): %s", dto.id, dto.toString()); + return TestMapper.from(addOrUpdateAuthenticated(dto)); } - TestDAO addAuthenticated(Test dto) { - TestDAO existing = TestDAO.find("id", dto.id).firstResult(); - if (existing == null) - dto.clearIds(); + TestDAO addOrUpdateAuthenticated(Test dto) { + // early data validation + if (dto.name == null || dto.name.isBlank()) { + throw ServiceException.badRequest("Test name can not be empty"); + } + + TestDAO existing = dto.id != null ? TestDAO.findById(dto.id) : null; TestDAO test = TestMapper.to(dto); if (test.notificationsEnabled == null) { test.notificationsEnabled = true; @@ -222,14 +244,12 @@ TestDAO addAuthenticated(Test dto) { throw ServiceException.forbidden("This user does not have the " + existing.owner + " role!"); } // We're not updating views using this method - boolean shouldRecalculateLables = false; - if (!Objects.equals(test.fingerprintFilter, existing.fingerprintFilter) || - !Objects.equals(test.fingerprintLabels, existing.fingerprintLabels)) - shouldRecalculateLables = true; + boolean shouldRecalculateLabels = !Objects.equals(test.fingerprintFilter, existing.fingerprintFilter) || + !Objects.equals(test.fingerprintLabels, existing.fingerprintLabels); test.views = existing.views; test = em.merge(test); - if (shouldRecalculateLables) + if (shouldRecalculateLabels) mediator.updateFingerprints(test.id); } else { // We need to persist the test before view in order for RLS to work @@ -720,35 +740,35 @@ public TestExport export(int testId) { @Override public void importTest(ObjectNode node) { - TestExport newTest; + TestExport testExport; try { - newTest = mapper.convertValue(node, TestExport.class); + testExport = mapper.convertValue(node, TestExport.class); } catch (IllegalArgumentException e) { throw ServiceException.badRequest("Failed to parse Test definition: " + e.getMessage()); } // if the datastore does NOT exist in our db then create it - if (newTest.datastore != null - && (newTest.datastore.id == null || (DatastoreConfigDAO.findById(newTest.datastore.id) == null))) { + if (testExport.datastore != null + && (testExport.datastore.id == null || (DatastoreConfigDAO.findById(testExport.datastore.id) == null))) { // reset the datastore to be sure we are creating a new one - newTest.datastore.id = null; - DatastoreConfigDAO datastore = DatasourceMapper.to(newTest.datastore); + testExport.datastore.id = null; + DatastoreConfigDAO datastore = DatasourceMapper.to(testExport.datastore); datastore.persist(); - newTest.datastore.id = datastore.id; - newTest.datastoreId = newTest.datastore.id; - } else if (newTest.datastore != null && newTest.datastore.id != null) { + testExport.datastore.id = datastore.id; + testExport.datastoreId = testExport.datastore.id; + } else if (testExport.datastore != null && testExport.datastore.id != null) { // if the datastore already exists in the db simply link its id to the datastoreId field - newTest.datastoreId = newTest.datastore.id; + testExport.datastoreId = testExport.datastore.id; } - Test t = add(newTest); + Test t = (testExport.id == null || TestDAO.findById(testExport.id) == null) ? add(testExport) : update(testExport); - if (!Objects.equals(t.id, newTest.id)) { - newTest.id = t.id; - newTest.updateRefs(); + if (!Objects.equals(t.id, testExport.id)) { + testExport.id = t.id; + testExport.updateRefs(); } - mediator.importTestToAll(newTest); + mediator.importTestToAll(testExport); } protected TestDAO getTestForUpdate(int testId) { diff --git a/horreum-backend/src/test/java/io/hyperfoil/tools/horreum/svc/TestServiceNoRestTest.java b/horreum-backend/src/test/java/io/hyperfoil/tools/horreum/svc/TestServiceNoRestTest.java index 17c5d63a6..615f5560b 100644 --- a/horreum-backend/src/test/java/io/hyperfoil/tools/horreum/svc/TestServiceNoRestTest.java +++ b/horreum-backend/src/test/java/io/hyperfoil/tools/horreum/svc/TestServiceNoRestTest.java @@ -200,7 +200,7 @@ void testUpdateTest() { created.name = "differentName"; created.notificationsEnabled = false; - Test updated = testService.add(created); + Test updated = testService.update(created); assertEquals(1, TestDAO.count()); test = TestDAO.findById(updated.id); assertEquals(created.name, test.name); @@ -209,6 +209,32 @@ void testUpdateTest() { assertEquals(false, test.notificationsEnabled); } + @org.junit.jupiter.api.Test + void testUpdateNotExistingTest() { + Test t = createSampleTest("test", null, null, null); + + Test created = testService.add(t); + assertNotNull(created.id); + assertEquals(1, TestDAO.count()); + TestDAO test = TestDAO.findById(created.id); + assertEquals(t.name, test.name); + assertEquals(FOO_TEAM, test.owner); + assertEquals(Access.PUBLIC, test.access); + assertEquals(true, test.notificationsEnabled); + + // change to a non-existing id + created.id = 999; + ServiceException thrown = assertThrows(ServiceException.class, () -> testService.update(created)); + assertEquals("Missing test id or test with id 999 does not exist", thrown.getMessage()); + assertEquals(Response.Status.NOT_FOUND.getStatusCode(), thrown.getResponse().getStatus()); + + // change to a null id + created.id = null; + thrown = assertThrows(ServiceException.class, () -> testService.update(created)); + assertEquals("Missing test id or test with id null does not exist", thrown.getMessage()); + assertEquals(Response.Status.NOT_FOUND.getStatusCode(), thrown.getResponse().getStatus()); + } + @org.junit.jupiter.api.Test void testGetTestByName() { String testName = "MyTest1";