Split add and update test apis

Signed-off-by: Andrea Lamparelli <[email protected]>
Hyperfoil · Feb 24, 2025 · c749001 · c749001
1 parent 2ae7ba4
commit c749001
Show file tree

Hide file tree

Showing 4 changed files with 97 additions and 28 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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";