From a8f544e49edad3c09e76644882b5886ebc29c4fb Mon Sep 17 00:00:00 2001 From: Andrea Lamparelli Date: Mon, 24 Feb 2025 11:55:11 +0100 Subject: [PATCH 1/2] 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/AlertingServiceTest.java | 2 +- .../tools/horreum/svc/BaseServiceTest.java | 17 +++-- .../horreum/svc/TestServiceNoRestTest.java | 28 ++++++- horreum-web/src/api.tsx | 5 +- horreum-web/src/domain/tests/Test.tsx | 10 ++- horreum-web/src/domain/tests/TestSettings.tsx | 6 +- 9 files changed, 127 insertions(+), 38 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/AlertingServiceTest.java b/horreum-backend/src/test/java/io/hyperfoil/tools/horreum/svc/AlertingServiceTest.java index d6a38aede..79dc22862 100644 --- a/horreum-backend/src/test/java/io/hyperfoil/tools/horreum/svc/AlertingServiceTest.java +++ b/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 fingerprintsAfter = FingerprintDAO.listAll(); diff --git a/horreum-backend/src/test/java/io/hyperfoil/tools/horreum/svc/BaseServiceTest.java b/horreum-backend/src/test/java/io/hyperfoil/tools/horreum/svc/BaseServiceTest.java index 37a8dcf39..91f0a247b 100644 --- a/horreum-backend/src/test/java/io/hyperfoil/tools/horreum/svc/BaseServiceTest.java +++ b/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 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) { 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"; diff --git a/horreum-web/src/api.tsx b/horreum-web/src/api.tsx index 8afa644cc..c7c4d6b7b 100644 --- a/horreum-web/src/api.tsx +++ b/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 { +export function addTest(test: Test, alerting: AlertContextType): Promise { return apiCall(testApi.add(test), alerting, "SEND_TEST", "Failed to send test"); +} +export function updateTest(test: Test, alerting: AlertContextType): Promise { + return apiCall(testApi.update(test), alerting, "UPDATE_TEST", "Failed to update test"); } diff --git a/horreum-web/src/domain/tests/Test.tsx b/horreum-web/src/domain/tests/Test.tsx index f0296e090..32a4035dd 100644 --- a/horreum-web/src/domain/tests/Test.tsx +++ b/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() const [testIdVal, setTestIdVal] = useState(testId === "_new" ? 0 : parseInt(testId ?? "-1")) const [test, setTest] = useState() @@ -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) diff --git a/horreum-web/src/domain/tests/TestSettings.tsx b/horreum-web/src/domain/tests/TestSettings.tsx index 9f4b0df33..a2c0166df 100644 --- a/horreum-web/src/domain/tests/TestSettings.tsx +++ b/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) } From 0ec59a3814933b8f01b27cb4762cc5a648fc4b7e Mon Sep 17 00:00:00 2001 From: Andrea Lamparelli Date: Thu, 6 Mar 2025 11:20:37 +0100 Subject: [PATCH 2/2] Make test creation to return 201 Signed-off-by: Andrea Lamparelli --- docs/site/content/en/openapi/openapi.yaml | 6 +++--- .../hyperfoil/tools/horreum/api/services/TestService.java | 8 ++++++++ .../io/hyperfoil/tools/horreum/svc/BaseServiceTest.java | 2 +- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/docs/site/content/en/openapi/openapi.yaml b/docs/site/content/en/openapi/openapi.yaml index 57d52a1c1..4775ee15f 100644 --- a/docs/site/content/en/openapi/openapi.yaml +++ b/docs/site/content/en/openapi/openapi.yaml @@ -1743,7 +1743,7 @@ paths: required: true responses: "200": - description: OK + description: Test updated successfully content: application/json: schema: @@ -1760,8 +1760,8 @@ paths: $ref: "#/components/schemas/Test" required: true responses: - "200": - description: OK + "201": + description: New test created successfully content: application/json: schema: 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 c98abf0a1..8e4cbcd9d 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 @@ -30,6 +30,7 @@ import org.eclipse.microprofile.openapi.annotations.responses.APIResponseSchema; import org.eclipse.microprofile.openapi.annotations.responses.APIResponses; import org.eclipse.microprofile.openapi.annotations.tags.Tag; +import org.jboss.resteasy.reactive.ResponseStatus; import org.jboss.resteasy.reactive.Separator; import com.fasterxml.jackson.annotation.JsonProperty; @@ -69,10 +70,17 @@ public interface TestService { @POST @Operation(description = "Create a new test") + @ResponseStatus(201) + @APIResponses(value = { + @APIResponse(responseCode = "201", description = "New test created successfully", content = @Content(mediaType = MediaType.APPLICATION_JSON, schema = @Schema(implementation = Test.class))) + }) Test add(@RequestBody(required = true) Test test); @PUT @Operation(description = "Update an existing test") + @APIResponses(value = { + @APIResponse(responseCode = "200", description = "Test updated successfully", content = @Content(mediaType = MediaType.APPLICATION_JSON, schema = @Schema(implementation = Test.class))) + }) Test update(@RequestBody(required = true) Test test); @GET diff --git a/horreum-backend/src/test/java/io/hyperfoil/tools/horreum/svc/BaseServiceTest.java b/horreum-backend/src/test/java/io/hyperfoil/tools/horreum/svc/BaseServiceTest.java index 91f0a247b..77f3b55c4 100644 --- a/horreum-backend/src/test/java/io/hyperfoil/tools/horreum/svc/BaseServiceTest.java +++ b/horreum-backend/src/test/java/io/hyperfoil/tools/horreum/svc/BaseServiceTest.java @@ -430,7 +430,7 @@ protected Test createTest(Test test) { .body(test) .post("/api/test") .then() - .statusCode(200) + .statusCode(201) .extract().body().as(Test.class); log.debugf("New test created via /api/test: %s", test.toString());