From 7af164e002211f8a0e80b389d86ae021f524fe40 Mon Sep 17 00:00:00 2001 From: Darin Kelkhoff Date: Tue, 9 May 2023 10:09:29 -0500 Subject: [PATCH] More error handling from customizers --- .../AbstractPreUpdateCustomizer.java | 3 +- .../model/metadata/tables/QTableMetaData.java | 17 ++- .../qqq/api/actions/ApiImplementation.java | 42 ++++-- .../java/com/kingsrook/qqq/api/TestUtils.java | 61 +++++++- .../api/javalin/QJavalinApiHandlerTest.java | 134 ++++++++++++++++-- 5 files changed, 237 insertions(+), 20 deletions(-) diff --git a/qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/actions/customizers/AbstractPreUpdateCustomizer.java b/qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/actions/customizers/AbstractPreUpdateCustomizer.java index b166d414..a0c927c4 100644 --- a/qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/actions/customizers/AbstractPreUpdateCustomizer.java +++ b/qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/actions/customizers/AbstractPreUpdateCustomizer.java @@ -26,6 +26,7 @@ import java.io.Serializable; import java.util.HashMap; import java.util.List; import java.util.Map; +import com.kingsrook.qqq.backend.core.exceptions.QException; import com.kingsrook.qqq.backend.core.model.actions.tables.update.UpdateInput; import com.kingsrook.qqq.backend.core.model.data.QRecord; @@ -59,7 +60,7 @@ public abstract class AbstractPreUpdateCustomizer /******************************************************************************* ** *******************************************************************************/ - public abstract List apply(List records); + public abstract List apply(List records) throws QException; diff --git a/qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/model/metadata/tables/QTableMetaData.java b/qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/model/metadata/tables/QTableMetaData.java index 23c467d8..cba66e16 100644 --- a/qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/model/metadata/tables/QTableMetaData.java +++ b/qqq-backend-core/src/main/java/com/kingsrook/qqq/backend/core/model/metadata/tables/QTableMetaData.java @@ -32,6 +32,7 @@ import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; +import com.kingsrook.qqq.backend.core.actions.customizers.TableCustomizers; import com.kingsrook.qqq.backend.core.exceptions.QException; import com.kingsrook.qqq.backend.core.model.data.QRecordEntity; import com.kingsrook.qqq.backend.core.model.data.QRecordEntityField; @@ -517,13 +518,27 @@ public class QTableMetaData implements QAppChildMetaData, Serializable, MetaData { this.customizers = new HashMap<>(); } - // todo - check for dupes? + + if(this.customizers.containsKey(role)) + { + throw (new IllegalArgumentException("Attempt to add a second customizer with role [" + role + "] to table [" + name + "].")); + } this.customizers.put(role, customizer); return (this); } + /******************************************************************************* + ** + *******************************************************************************/ + public QTableMetaData withCustomizer(TableCustomizers tableCustomizer, QCodeReference customizer) + { + return (withCustomizer(tableCustomizer.getRole(), customizer)); + } + + + /******************************************************************************* ** *******************************************************************************/ diff --git a/qqq-middleware-api/src/main/java/com/kingsrook/qqq/api/actions/ApiImplementation.java b/qqq-middleware-api/src/main/java/com/kingsrook/qqq/api/actions/ApiImplementation.java index 603f5e1e..a9224655 100644 --- a/qqq-middleware-api/src/main/java/com/kingsrook/qqq/api/actions/ApiImplementation.java +++ b/qqq-middleware-api/src/main/java/com/kingsrook/qqq/api/actions/ApiImplementation.java @@ -473,8 +473,16 @@ public class ApiImplementation List warnings = record.getWarnings(); if(CollectionUtils.nullSafeHasContents(errors)) { - outputRecord.put("statusCode", HttpStatus.Code.BAD_REQUEST.getCode()); - outputRecord.put("statusText", HttpStatus.Code.BAD_REQUEST.getMessage()); + if(areAnyErrorsBadRequest(errors)) + { + outputRecord.put("statusCode", HttpStatus.Code.BAD_REQUEST.getCode()); + outputRecord.put("statusText", HttpStatus.Code.BAD_REQUEST.getMessage()); + } + else + { + outputRecord.put("statusCode", HttpStatus.Code.INTERNAL_SERVER_ERROR.getCode()); + outputRecord.put("statusText", HttpStatus.Code.INTERNAL_SERVER_ERROR.getMessage()); + } outputRecord.put("error", "Error inserting " + table.getLabel() + ": " + joinErrorsWithCommasAndAnd(errors)); } else if(CollectionUtils.nullSafeHasContents(warnings)) @@ -577,6 +585,7 @@ public class ApiImplementation UpdateOutput updateOutput = updateAction.execute(updateInput); List errors = updateOutput.getRecords().get(0).getErrors(); + // todo - do we want, if there were warnings, to return a 200 w/ a body w/ the warnings? maybe... if(CollectionUtils.nullSafeHasContents(errors)) { if(areAnyErrorsNotFound(errors)) @@ -686,26 +695,38 @@ public class ApiImplementation } List errors = record.getErrors(); + + HttpStatus.Code statusCode; if(CollectionUtils.nullSafeHasContents(errors)) { outputRecord.put("error", "Error updating " + table.getLabel() + ": " + joinErrorsWithCommasAndAnd(errors)); if(areAnyErrorsNotFound(errors)) { - outputRecord.put("statusCode", HttpStatus.Code.NOT_FOUND.getCode()); - outputRecord.put("statusText", HttpStatus.Code.NOT_FOUND.getMessage()); + statusCode = HttpStatus.Code.NOT_FOUND; + } + else if(areAnyErrorsBadRequest(errors)) + { + statusCode = HttpStatus.Code.BAD_REQUEST; } else { - outputRecord.put("statusCode", HttpStatus.Code.BAD_REQUEST.getCode()); - outputRecord.put("statusText", HttpStatus.Code.BAD_REQUEST.getMessage()); + statusCode = HttpStatus.Code.INTERNAL_SERVER_ERROR; } } else { - outputRecord.put("statusCode", HttpStatus.Code.NO_CONTENT.getCode()); - outputRecord.put("statusText", HttpStatus.Code.NO_CONTENT.getMessage()); + statusCode = HttpStatus.Code.NO_CONTENT; + + List warnings = record.getWarnings(); + if(CollectionUtils.nullSafeHasContents(warnings)) + { + outputRecord.put("warning", "Warning updating " + table.getLabel() + ": " + joinErrorsWithCommasAndAnd(warnings)); + } } + outputRecord.put("statusCode", statusCode.getCode()); + outputRecord.put("statusText", statusCode.getMessage()); + i++; } @@ -740,6 +761,11 @@ public class ApiImplementation { throw (new QNotFoundException("Could not find " + table.getLabel() + " with " + table.getFields().get(table.getPrimaryKeyField()).getLabel() + " of " + primaryKey)); } + else if(areAnyErrorsBadRequest(errors)) + { + throw (new QBadRequestException("Error deleting " + table.getLabel() + ": " + joinErrorsWithCommasAndAnd(errors))); + } + // todo - do we want, if there were warnings, to return a 200 w/ a body w/ the warnings? maybe... else { throw (new QException("Error deleting " + table.getLabel() + ": " + joinErrorsWithCommasAndAnd(errors))); diff --git a/qqq-middleware-api/src/test/java/com/kingsrook/qqq/api/TestUtils.java b/qqq-middleware-api/src/test/java/com/kingsrook/qqq/api/TestUtils.java index 4926b5a1..52b68f51 100644 --- a/qqq-middleware-api/src/test/java/com/kingsrook/qqq/api/TestUtils.java +++ b/qqq-middleware-api/src/test/java/com/kingsrook/qqq/api/TestUtils.java @@ -31,7 +31,9 @@ import com.kingsrook.qqq.api.model.metadata.fields.ApiFieldMetaData; import com.kingsrook.qqq.api.model.metadata.fields.ApiFieldMetaDataContainer; import com.kingsrook.qqq.api.model.metadata.tables.ApiTableMetaData; import com.kingsrook.qqq.api.model.metadata.tables.ApiTableMetaDataContainer; +import com.kingsrook.qqq.backend.core.actions.customizers.AbstractPreDeleteCustomizer; import com.kingsrook.qqq.backend.core.actions.customizers.AbstractPreInsertCustomizer; +import com.kingsrook.qqq.backend.core.actions.customizers.AbstractPreUpdateCustomizer; import com.kingsrook.qqq.backend.core.actions.customizers.TableCustomizers; import com.kingsrook.qqq.backend.core.actions.tables.InsertAction; import com.kingsrook.qqq.backend.core.exceptions.QException; @@ -54,6 +56,7 @@ import com.kingsrook.qqq.backend.core.model.metadata.tables.QTableMetaData; import com.kingsrook.qqq.backend.core.model.metadata.tables.UniqueKey; import com.kingsrook.qqq.backend.core.model.statusmessages.BadInputStatusMessage; import com.kingsrook.qqq.backend.core.model.statusmessages.QWarningMessage; +import com.kingsrook.qqq.backend.core.model.statusmessages.SystemErrorStatusMessage; import com.kingsrook.qqq.backend.core.modules.backend.implementations.memory.MemoryBackendModule; import com.kingsrook.qqq.backend.core.utils.CollectionUtils; @@ -156,7 +159,7 @@ public class TestUtils { for(QRecord record : CollectionUtils.nonNullList(records)) { - if(!record.getValueString("firstName").matches(".*[a-z].*")) + if(record.getValueString("firstName") != null && !record.getValueString("firstName").matches(".*[a-z].*")) { record.addWarning(new QWarningMessage("First name does not contain any letters...")); } @@ -180,6 +183,7 @@ public class TestUtils .withBackendName(MEMORY_BACKEND_NAME) .withPrimaryKeyField("id") .withUniqueKey(new UniqueKey("email")) + .withCustomizer(TableCustomizers.PRE_DELETE_RECORD, new QCodeReference(PersonPreDeleteCustomizer.class)) .withField(new QFieldMetaData("id", QFieldType.INTEGER).withIsEditable(false)) .withField(new QFieldMetaData("createDate", QFieldType.DATE_TIME).withIsEditable(false)) .withField(new QFieldMetaData("modifyDate", QFieldType.DATE_TIME).withIsEditable(false)) @@ -241,6 +245,7 @@ public class TestUtils return new QTableMetaData() .withName(TABLE_NAME_ORDER) .withCustomizer(TableCustomizers.PRE_INSERT_RECORD.getRole(), new QCodeReference(OrderPreInsertCustomizer.class)) + .withCustomizer(TableCustomizers.PRE_UPDATE_RECORD.getRole(), new QCodeReference(OrderPreUpdateCustomizer.class)) .withBackendName(MEMORY_BACKEND_NAME) .withMiddlewareMetaData(new ApiTableMetaDataContainer().withApiTableMetaData(TestUtils.API_NAME, new ApiTableMetaData().withInitialVersion(V2022_Q4))) .withPrimaryKeyField("id") @@ -427,11 +432,63 @@ public class TestUtils if("throw".equals(record.getValueString("orderNo"))) { - throw (new QException("Throwing error, as requested...")); + record.addError(new SystemErrorStatusMessage("Throwing error, as requested...")); } } return (records); } } + + + + /******************************************************************************* + ** + *******************************************************************************/ + public static class OrderPreUpdateCustomizer extends AbstractPreUpdateCustomizer + { + @Override + public List apply(List records) throws QException + { + ///////////////////////////////////////////// + // use same logic as pre-insert customizer // + ///////////////////////////////////////////// + return new OrderPreInsertCustomizer().apply(records); + } + } + + + + /******************************************************************************* + ** + *******************************************************************************/ + public static class PersonPreDeleteCustomizer extends AbstractPreDeleteCustomizer + { + public static final Integer DELETE_ERROR_ID = 9999; + public static final Integer DELETE_WARN_ID = 9998; + + + + /******************************************************************************* + ** + *******************************************************************************/ + @Override + public List apply(List records) + { + for(QRecord record : records) + { + if(DELETE_ERROR_ID.equals(record.getValue("id"))) + { + record.addError(new BadInputStatusMessage("You may not delete this person")); + } + else if(DELETE_WARN_ID.equals(record.getValue("id"))) + { + record.addWarning(new QWarningMessage("It was bad that you deleted this person")); + } + } + + return (records); + } + } + } diff --git a/qqq-middleware-api/src/test/java/com/kingsrook/qqq/api/javalin/QJavalinApiHandlerTest.java b/qqq-middleware-api/src/test/java/com/kingsrook/qqq/api/javalin/QJavalinApiHandlerTest.java index 2f43f7dc..97226c39 100644 --- a/qqq-middleware-api/src/test/java/com/kingsrook/qqq/api/javalin/QJavalinApiHandlerTest.java +++ b/qqq-middleware-api/src/test/java/com/kingsrook/qqq/api/javalin/QJavalinApiHandlerTest.java @@ -128,7 +128,6 @@ class QJavalinApiHandlerTest extends BaseTest void testSpec() { HttpResponse response = Unirest.get(BASE_URL + "/api/" + VERSION + "/openapi.yaml").asString(); - System.out.println(response.getBody()); assertThat(response.getBody()) .contains(""" title: "Test API" @@ -217,8 +216,7 @@ class QJavalinApiHandlerTest extends BaseTest HttpResponse response = Unirest.get(BASE_URL + "/api/" + VERSION + "/order/1").asString(); assertEquals(HttpStatus.OK_200, response.getStatus()); JSONObject jsonObject = new JSONObject(response.getBody()); - System.out.println(jsonObject.toString(3)); - JSONArray orderLines = jsonObject.getJSONArray("orderLines"); + JSONArray orderLines = jsonObject.getJSONArray("orderLines"); assertEquals(3, orderLines.length()); JSONObject orderLine0 = orderLines.getJSONObject(0); JSONArray lineExtrinsics = orderLine0.getJSONArray("extrinsics"); @@ -522,7 +520,6 @@ class QJavalinApiHandlerTest extends BaseTest HttpResponse response = Unirest.get(BASE_URL + "/api/" + VERSION + "/order/query?id=1").asString(); assertEquals(HttpStatus.OK_200, response.getStatus()); JSONObject jsonObject = new JSONObject(response.getBody()); - System.out.println(jsonObject.toString(3)); JSONObject order0 = jsonObject.getJSONArray("records").getJSONObject(0); JSONArray orderLines = order0.getJSONArray("orderLines"); assertEquals(3, orderLines.length()); @@ -595,7 +592,6 @@ class QJavalinApiHandlerTest extends BaseTest } """) .asString(); - System.out.println(response.getBody()); assertEquals(HttpStatus.CREATED_201, response.getStatus()); JSONObject jsonObject = new JSONObject(response.getBody()); assertEquals(1, jsonObject.getInt("id")); @@ -699,7 +695,6 @@ class QJavalinApiHandlerTest extends BaseTest } """) .asString(); - System.out.println(response.getBody()); assertErrorResponse(HttpStatus.BAD_REQUEST_400, "Quantity may not be less than 0", response); response = Unirest.post(BASE_URL + "/api/" + VERSION + "/order/") @@ -707,8 +702,30 @@ class QJavalinApiHandlerTest extends BaseTest {"orderNo": "throw", "storeId": 47} """) .asString(); - System.out.println(response.getBody()); assertErrorResponse(HttpStatus.INTERNAL_SERVER_ERROR_500, "Throwing error, as requested", response); + + response = Unirest.post(BASE_URL + "/api/" + VERSION + "/order/bulk") + .body(""" + [ + {"orderNo": "ORD123", "storeId": 47, "orderLines": + [ + {"lineNumber": 1, "sku": "BASIC1", "quantity": 0}, + ] + }, + {"orderNo": "throw", "storeId": 47} + ] + """) + .asString(); + assertEquals(HttpStatus.MULTI_STATUS_207, response.getStatus()); + JSONArray jsonArray = new JSONArray(response.getBody()); + assertEquals(2, jsonArray.length()); + + assertEquals(HttpStatus.BAD_REQUEST_400, jsonArray.getJSONObject(0).getInt("statusCode")); + assertThat(jsonArray.getJSONObject(0).getString("error")).contains("Quantity may not be less than 0"); + + assertEquals(HttpStatus.INTERNAL_SERVER_ERROR_500, jsonArray.getJSONObject(1).getInt("statusCode")); + assertThat(jsonArray.getJSONObject(1).getString("error")).contains("Throwing error, as requested"); + } @@ -921,6 +938,57 @@ class QJavalinApiHandlerTest extends BaseTest + /******************************************************************************* + ** + *******************************************************************************/ + @Test + void testUpdateErrorsFromCustomizer() throws QException + { + insert1Order3Lines4LineExtrinsicsAnd1OrderExtrinsic(); + + HttpResponse response = Unirest.patch(BASE_URL + "/api/" + VERSION + "/order/1") + .body(""" + {"orderNo": "ORD123", "storeId": 47, "orderLines": + [ + {"lineNumber": 1, "sku": "BASIC1", "quantity": 0}, + ] + } + """) + .asString(); + assertErrorResponse(HttpStatus.BAD_REQUEST_400, "Quantity may not be less than 0", response); + + response = Unirest.patch(BASE_URL + "/api/" + VERSION + "/order/1") + .body(""" + {"orderNo": "throw", "storeId": 47} + """) + .asString(); + assertErrorResponse(HttpStatus.INTERNAL_SERVER_ERROR_500, "Throwing error, as requested", response); + + response = Unirest.patch(BASE_URL + "/api/" + VERSION + "/order/bulk") + .body(""" + [ + {"id": 1, "orderNo": "ORD123", "storeId": 47, "orderLines": + [ + {"lineNumber": 1, "sku": "BASIC1", "quantity": 0}, + ] + }, + {"id": 1, "orderNo": "throw", "storeId": 47} + ] + """) + .asString(); + assertEquals(HttpStatus.MULTI_STATUS_207, response.getStatus()); + JSONArray jsonArray = new JSONArray(response.getBody()); + assertEquals(2, jsonArray.length()); + + assertEquals(HttpStatus.BAD_REQUEST_400, jsonArray.getJSONObject(0).getInt("statusCode")); + assertThat(jsonArray.getJSONObject(0).getString("error")).contains("Quantity may not be less than 0"); + + assertEquals(HttpStatus.INTERNAL_SERVER_ERROR_500, jsonArray.getJSONObject(1).getInt("statusCode")); + assertThat(jsonArray.getJSONObject(1).getString("error")).contains("Throwing error, as requested"); + } + + + /******************************************************************************* ** *******************************************************************************/ @@ -941,7 +1009,6 @@ class QJavalinApiHandlerTest extends BaseTest .asString(); assertEquals(HttpStatus.MULTI_STATUS_207, response.getStatus()); JSONArray jsonArray = new JSONArray(response.getBody()); - System.out.println(jsonArray.toString(3)); assertEquals(4, jsonArray.length()); assertEquals(HttpStatus.NO_CONTENT_204, jsonArray.getJSONObject(0).getInt("statusCode")); @@ -1155,6 +1222,57 @@ class QJavalinApiHandlerTest extends BaseTest + /******************************************************************************* + ** + *******************************************************************************/ + @Test + void testDeleteErrorsFromCustomizer() throws QException + { + InsertInput insertInput = new InsertInput(); + insertInput.setTableName(TestUtils.TABLE_NAME_PERSON); + insertInput.setRecords(List.of( + new QRecord().withValue("id", TestUtils.PersonPreDeleteCustomizer.DELETE_ERROR_ID), + new QRecord().withValue("id", TestUtils.PersonPreDeleteCustomizer.DELETE_WARN_ID))); + new InsertAction().execute(insertInput); + + HttpResponse response = Unirest.delete(BASE_URL + "/api/" + VERSION + "/person/" + TestUtils.PersonPreDeleteCustomizer.DELETE_ERROR_ID).asString(); + assertErrorResponse(HttpStatus.BAD_REQUEST_400, "You may not delete this person", response); + + response = Unirest.delete(BASE_URL + "/api/" + VERSION + "/person/" + TestUtils.PersonPreDeleteCustomizer.DELETE_WARN_ID).asString(); + assertErrorResponse(HttpStatus.NO_CONTENT_204, "It was bad that you deleted this person", response); + } + + + + /******************************************************************************* + ** + *******************************************************************************/ + @Test + void testDeleteBulkErrorsFromCustomizer() throws QException + { + InsertInput insertInput = new InsertInput(); + insertInput.setTableName(TestUtils.TABLE_NAME_PERSON); + insertInput.setRecords(List.of( + new QRecord().withValue("id", TestUtils.PersonPreDeleteCustomizer.DELETE_ERROR_ID), + new QRecord().withValue("id", TestUtils.PersonPreDeleteCustomizer.DELETE_WARN_ID))); + new InsertAction().execute(insertInput); + + HttpResponse response = Unirest.delete(BASE_URL + "/api/" + VERSION + "/person/bulk") + .body("[" + TestUtils.PersonPreDeleteCustomizer.DELETE_ERROR_ID + "," + TestUtils.PersonPreDeleteCustomizer.DELETE_WARN_ID + "]") + .asString(); + assertEquals(HttpStatus.MULTI_STATUS_207, response.getStatus()); + JSONArray jsonArray = new JSONArray(response.getBody()); + assertEquals(2, jsonArray.length()); + + assertEquals(HttpStatus.BAD_REQUEST_400, jsonArray.getJSONObject(0).getInt("statusCode")); + assertThat(jsonArray.getJSONObject(0).getString("error")).contains("Error deleting Person: You may not delete this person"); + + assertEquals(HttpStatus.NO_CONTENT_204, jsonArray.getJSONObject(1).getInt("statusCode")); + assertFalse(jsonArray.getJSONObject(1).has("error")); + } + + + /******************************************************************************* ** *******************************************************************************/