Split add and update test apis

Signed-off-by: Andrea Lamparelli <[email protected]>

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

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 = {

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) {

horreum-backend/src/test/java/io/hyperfoil/tools/horreum/svc/AlertingServiceTest.java

@@ -260,7 +260,7 @@ public void testFingerprintLabelsChange(TestInfo info) throws Exception {
         test.fingerprintLabels = ((ArrayNode) test.fingerprintLabels).add("bar");
         // We'll change the filter here but we do NOT expect to be applied to existing datapoints
         test.fingerprintFilter = "value => false";
-        test = createTest(test); // this is update
+        test = updateTest(test); // this is update
         // the fingerprint should be updated within the same transaction as test update
         em.clear();
         List<FingerprintDAO> fingerprintsAfter = FingerprintDAO.listAll();

horreum-backend/src/test/java/io/hyperfoil/tools/horreum/svc/BaseServiceTest.java

@@ -438,12 +438,19 @@ protected Test createTest(Test test) {
         return test;
     }
 
-    protected void createViews(List<View> views) {
-        jsonRequest()
-                .body(views)
-                .post("/api/ui/views")
+    protected Test updateTest(Test test) {
+        log.debugf("Updating test via /api/test: %s", test.toString());
+
+        test = jsonRequest()
+                .body(test)
+                .put("/api/test")
                 .then()
-                .statusCode(204);
+                .statusCode(200)
+                .extract().body().as(Test.class);
+
+        log.debugf("Test updated via /api/test: %s", test.toString());
+
+        return test;
     }
 
     protected View createView(View view) {

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";

horreum-web/src/api.tsx

@@ -245,9 +245,12 @@ export function removeUserOrTeam(id: number, userOrTeam: string, alerting: Alert
     return apiCall(subscriptionsApi.removeUserOrTeam(id, userOrTeam), alerting, "REMOVE_SUBSCRIPTION", "Failed to remove test subscriptions");
 }
 
-export function sendTest(test: Test, alerting: AlertContextType): Promise<Test> {
+export function addTest(test: Test, alerting: AlertContextType): Promise<Test> {
     return apiCall(testApi.add(test), alerting, "SEND_TEST", "Failed to send test");
+}
 
+export function updateTest(test: Test, alerting: AlertContextType): Promise<Test> {
+    return apiCall(testApi.update(test), alerting, "UPDATE_TEST", "Failed to update test");
 }
 
 

horreum-web/src/domain/tests/Test.tsx

@@ -1,5 +1,5 @@
 import { useState, useEffect, useRef, useContext } from "react"
-import { useParams } from "react-router-dom"
+import { useNavigate, useParams } from "react-router-dom"
 
 import { useSelector } from "react-redux"
 
@@ -39,6 +39,7 @@ import RunList from "../runs/RunList";
 import ExportButton from "../../components/ExportButton";
 
 export default function TestView() {
+    const navigate = useNavigate()
     const {testId} = useParams<any>()
     const [testIdVal, setTestIdVal] = useState(testId === "_new" ? 0 : parseInt(testId ?? "-1"))
     const [test, setTest] = useState<Test | undefined>()
@@ -76,6 +77,13 @@ export default function TestView() {
         document.title = (testIdVal === 0 ? "New test" : test && test.name ? test.name : "Loading test...") + " | Horreum"
     }, [test, testIdVal])
 
+    useEffect(() => {
+        // something has changed, i.e., new test created
+        if (testIdVal > 0 && testIdVal !== test?.id) {
+            navigate("/test/" + testIdVal, {replace: false})
+        }
+    }, [testIdVal])
+
     //TODO:: replace redux
     const isTester = useTester(test ? test.owner : undefined)
 

horreum-web/src/domain/tests/TestSettings.tsx

@@ -18,7 +18,7 @@ import {
 import FolderSelect from "../../components/FolderSelect"
 import { TabFunctionsRef } from "../../components/SavedTabs"
 
-import { Test, Access, sendTest, configApi, Datastore, apiCall } from "../../api"
+import { Test, Access, addTest, configApi, Datastore, apiCall, updateTest } from "../../api"
 import { useTester, defaultTeamSelector, teamToName } from "../../auth"
 import { AppContext } from "../../context/appContext";
 import { AppContextType } from "../../context/@types/appContextTypes";
@@ -105,7 +105,9 @@ export default function TestSettings({ test, onTestIdChange, onModified, funcsRe
                 access: access,
                 transformers: test?.transformers || [],
             }
-            const response = await sendTest(newTest, alerting)
+            const response = (test?.id && test?.id > 0)
+                ? await updateTest(newTest, alerting)
+                : await addTest(newTest, alerting)
             if (response.id !== test?.id) {
                 return onTestIdChange(response.id)
             }