From 4e3c454dc3308c255fa8a1ae2b3089b394b8b341 Mon Sep 17 00:00:00 2001 From: Lionel Montrieux Date: Wed, 29 Aug 2018 15:54:54 +0200 Subject: [PATCH 01/11] Remove Problem from EventTypeService --- .../nakadi/controller/EventTypeController.java | 8 ++------ .../nakadi/controller/SchemaController.java | 12 +++--------- .../nakadi/service/EventTypeService.java | 17 +++-------------- .../nakadi/controller/SchemaControllerTest.java | 14 +++++++------- 4 files changed, 15 insertions(+), 36 deletions(-) diff --git a/src/main/java/org/zalando/nakadi/controller/EventTypeController.java b/src/main/java/org/zalando/nakadi/controller/EventTypeController.java index 321229d212..fdba2a6176 100644 --- a/src/main/java/org/zalando/nakadi/controller/EventTypeController.java +++ b/src/main/java/org/zalando/nakadi/controller/EventTypeController.java @@ -41,7 +41,6 @@ import org.zalando.nakadi.service.AdminService; import org.zalando.nakadi.service.EventTypeService; import org.zalando.nakadi.service.FeatureToggleService; -import org.zalando.nakadi.service.Result; import org.zalando.problem.Problem; import org.zalando.problem.spring.web.advice.Responses; @@ -146,11 +145,8 @@ public ResponseEntity update( @RequestMapping(value = "/{name:.+}", method = RequestMethod.GET) public ResponseEntity get(@PathVariable final String name, final NativeWebRequest request) { - final Result result = eventTypeService.get(name); - if (!result.isSuccessful()) { - return Responses.create(result.getProblem(), request); - } - return status(HttpStatus.OK).body(result.getValue()); + final EventType eventType = eventTypeService.get(name); + return status(HttpStatus.OK).body(eventType); } private HttpHeaders generateWarningHeaders(final EventTypeBase eventType) { diff --git a/src/main/java/org/zalando/nakadi/controller/SchemaController.java b/src/main/java/org/zalando/nakadi/controller/SchemaController.java index 32caa2c95f..e5c208d83d 100644 --- a/src/main/java/org/zalando/nakadi/controller/SchemaController.java +++ b/src/main/java/org/zalando/nakadi/controller/SchemaController.java @@ -33,10 +33,7 @@ public ResponseEntity getSchemas( @RequestParam(value = "offset", required = false, defaultValue = "0") final int offset, @RequestParam(value = "limit", required = false, defaultValue = "20") final int limit, final NativeWebRequest request) { - final Result eventTypeResult = eventTypeService.get(name); - if (!eventTypeResult.isSuccessful()) { - return Responses.create(eventTypeResult.getProblem(), request); - } + final EventType eventType = eventTypeService.get(name); final Result result = schemaService.getSchemas(name, offset, limit); if (result.isSuccessful()) { @@ -50,12 +47,9 @@ public ResponseEntity getSchemaVersion(@PathVariable("name") final String nam @PathVariable("version") final String version, final NativeWebRequest request) { if (version.equals("latest")) { // latest schema might be cached with the event type - final Result eventTypeResult = eventTypeService.get(name); - if (!eventTypeResult.isSuccessful()) { - return Responses.create(eventTypeResult.getProblem(), request); - } + final EventType eventType = eventTypeService.get(name); - return ResponseEntity.status(HttpStatus.OK).body(eventTypeResult.getValue().getSchema()); + return ResponseEntity.status(HttpStatus.OK).body(eventType.getSchema()); } final Result result = schemaService.getSchemaVersion(name, version); diff --git a/src/main/java/org/zalando/nakadi/service/EventTypeService.java b/src/main/java/org/zalando/nakadi/service/EventTypeService.java index 8da0f8c2af..99670f0f1d 100644 --- a/src/main/java/org/zalando/nakadi/service/EventTypeService.java +++ b/src/main/java/org/zalando/nakadi/service/EventTypeService.java @@ -59,7 +59,6 @@ import org.zalando.nakadi.util.JsonUtils; import org.zalando.nakadi.validation.SchemaEvolutionService; import org.zalando.nakadi.validation.SchemaIncompatibility; -import org.zalando.problem.Problem; import java.io.Closeable; import java.io.IOException; @@ -70,8 +69,6 @@ import java.util.concurrent.TimeoutException; import java.util.stream.Collectors; -import static javax.ws.rs.core.Response.Status.INTERNAL_SERVER_ERROR; -import static javax.ws.rs.core.Response.Status.NOT_FOUND; import static org.zalando.nakadi.service.FeatureToggleService.Feature.DELETE_EVENT_TYPE_WITH_SUBSCRIPTIONS; @Component @@ -439,17 +436,9 @@ private void updateTopicRetentionTime(final String eventTypeName, final Long ret .setRetentionTime(timeline.getTopic(), retentionTime)); } - public Result get(final String eventTypeName) { - try { - final EventType eventType = eventTypeRepository.findByName(eventTypeName); - return Result.ok(eventType); - } catch (final NoSuchEventTypeException e) { - LOG.debug("Could not find EventType: {}", eventTypeName); - return Result.problem(Problem.valueOf(NOT_FOUND, e.getMessage())); - } catch (final InternalNakadiException e) { - LOG.error("Problem loading event type " + eventTypeName, e); - return Result.problem(Problem.valueOf(INTERNAL_SERVER_ERROR, e.getMessage())); - } + public EventType get(final String eventTypeName) throws NoSuchEventTypeException, InternalNakadiException { + final EventType eventType = eventTypeRepository.findByName(eventTypeName); + return eventType; } private Multimap deleteEventType(final String eventTypeName) diff --git a/src/test/java/org/zalando/nakadi/controller/SchemaControllerTest.java b/src/test/java/org/zalando/nakadi/controller/SchemaControllerTest.java index 3054348f45..73752f1448 100644 --- a/src/test/java/org/zalando/nakadi/controller/SchemaControllerTest.java +++ b/src/test/java/org/zalando/nakadi/controller/SchemaControllerTest.java @@ -8,6 +8,7 @@ import org.springframework.http.ResponseEntity; import org.springframework.web.context.request.NativeWebRequest; import org.zalando.nakadi.domain.EventType; +import org.zalando.nakadi.exceptions.runtime.NoSuchEventTypeException; import org.zalando.nakadi.service.EventTypeService; import org.zalando.nakadi.service.Result; import org.zalando.nakadi.service.SchemaService; @@ -34,7 +35,7 @@ public void setUp() { @Test public void testSuccess() { Mockito.when(schemaService.getSchemas("et_test", 0, 1)).thenReturn(Result.ok(null)); - Mockito.when(eventTypeService.get("et_test")).thenReturn(Result.ok(EventTypeTestBuilder.builder().build())); + Mockito.when(eventTypeService.get("et_test")).thenReturn(EventTypeTestBuilder.builder().build()); final ResponseEntity result = new SchemaController(schemaService, eventTypeService) .getSchemas("et_test", 0, 1, nativeWebRequest); @@ -43,7 +44,7 @@ public void testSuccess() { @Test public void testFailure503() { - Mockito.when(eventTypeService.get("et_test")).thenReturn(Result.ok(EventTypeTestBuilder.builder().build())); + Mockito.when(eventTypeService.get("et_test")).thenReturn(EventTypeTestBuilder.builder().build()); Mockito.when(schemaService.getSchemas("et_test", 0, 1)) .thenReturn(Result.problem(Problem.valueOf(Response.Status.SERVICE_UNAVAILABLE))); final ResponseEntity result = @@ -55,7 +56,7 @@ public void testFailure503() { @Test public void testGetLatestSchemaVersionThen200() { final EventType eventType = buildDefaultEventType(); - Mockito.when(eventTypeService.get(eventType.getName())).thenReturn(Result.ok(eventType)); + Mockito.when(eventTypeService.get(eventType.getName())).thenReturn(eventType); final ResponseEntity result = new SchemaController(schemaService, eventTypeService) .getSchemaVersion(eventType.getName(), "latest", nativeWebRequest); @@ -63,15 +64,14 @@ public void testGetLatestSchemaVersionThen200() { Assert.assertEquals(eventType.getSchema().toString(), result.getBody().toString()); } - @Test + @Test(expected = NoSuchEventTypeException.class) public void testGetLatestSchemaVersionWrongEventTypeThen404() { Mockito.when(eventTypeService.get("et_wrong_event")) - .thenReturn(Result.problem(Problem.valueOf(Response.Status.NOT_FOUND))); + .thenThrow(new NoSuchEventTypeException("no event type")); final ResponseEntity result = new SchemaController(schemaService, eventTypeService) .getSchemaVersion("et_wrong_event", "latest", nativeWebRequest); - Assert.assertEquals(HttpStatus.NOT_FOUND, result.getStatusCode()); } @Test @@ -97,7 +97,7 @@ public void testGetIllegalSchemaVersionThen404() { } public void testFailure404() { Mockito.when(eventTypeService.get("et_test")) - .thenReturn(Result.problem(Problem.valueOf(Response.Status.NOT_FOUND))); + .thenThrow(new NoSuchEventTypeException("no event type")); final ResponseEntity result = new SchemaController(schemaService, eventTypeService) .getSchemas("et_test", 0, 1, nativeWebRequest); From 9fa55a72acb6349770d086cadf9ebda9dab0504f Mon Sep 17 00:00:00 2001 From: Lionel Montrieux Date: Wed, 29 Aug 2018 16:56:44 +0200 Subject: [PATCH 02/11] Remove Result from SchemaService --- .../nakadi/controller/ExceptionHandling.java | 19 ++++++++ .../nakadi/controller/SchemaController.java | 21 +++------ .../InvalidVersionNumberException.java | 8 ++++ .../zalando/nakadi/service/SchemaService.java | 33 ++++++------- .../controller/SchemaControllerTest.java | 37 +-------------- .../nakadi/service/SchemaServiceTest.java | 46 ++++++------------- 6 files changed, 65 insertions(+), 99 deletions(-) create mode 100644 src/main/java/org/zalando/nakadi/exceptions/runtime/InvalidVersionNumberException.java diff --git a/src/main/java/org/zalando/nakadi/controller/ExceptionHandling.java b/src/main/java/org/zalando/nakadi/controller/ExceptionHandling.java index 604042b162..2a3aeaed14 100644 --- a/src/main/java/org/zalando/nakadi/controller/ExceptionHandling.java +++ b/src/main/java/org/zalando/nakadi/controller/ExceptionHandling.java @@ -19,7 +19,9 @@ import org.zalando.nakadi.exceptions.runtime.FeatureNotAvailableException; import org.zalando.nakadi.exceptions.runtime.IllegalClientIdException; import org.zalando.nakadi.exceptions.runtime.InternalNakadiException; +import org.zalando.nakadi.exceptions.runtime.InvalidLimitException; import org.zalando.nakadi.exceptions.runtime.InvalidPartitionKeyFieldsException; +import org.zalando.nakadi.exceptions.runtime.InvalidVersionNumberException; import org.zalando.nakadi.exceptions.runtime.LimitReachedException; import org.zalando.nakadi.exceptions.runtime.NakadiBaseException; import org.zalando.nakadi.exceptions.runtime.NakadiRuntimeException; @@ -41,6 +43,7 @@ import javax.ws.rs.core.Response; +import static javax.ws.rs.core.Response.Status.BAD_REQUEST; import static javax.ws.rs.core.Response.Status.CONFLICT; import static javax.ws.rs.core.Response.Status.INTERNAL_SERVER_ERROR; import static javax.ws.rs.core.Response.Status.NOT_FOUND; @@ -259,4 +262,20 @@ public ResponseEntity handleUnprocessableSubscriptionException( LOG.debug(exception.getMessage()); return Responses.create(UNPROCESSABLE_ENTITY, exception.getMessage(), request); } + + @ExceptionHandler(InvalidLimitException.class) + public ResponseEntity handleInvalidLimitException( + final InvalidLimitException exception, + final NativeWebRequest request) { + LOG.debug(exception.getMessage()); + return Responses.create(BAD_REQUEST, exception.getMessage(), request); + } + + @ExceptionHandler(InvalidVersionNumberException.class) + public ResponseEntity handleInvalidVersionNumberException( + final InvalidVersionNumberException exception, + final NativeWebRequest request) { + LOG.debug(exception.getMessage()); + return Responses.create(BAD_REQUEST, exception.getMessage(), request); + } } diff --git a/src/main/java/org/zalando/nakadi/controller/SchemaController.java b/src/main/java/org/zalando/nakadi/controller/SchemaController.java index e5c208d83d..cad9c36d32 100644 --- a/src/main/java/org/zalando/nakadi/controller/SchemaController.java +++ b/src/main/java/org/zalando/nakadi/controller/SchemaController.java @@ -10,10 +10,10 @@ import org.springframework.web.context.request.NativeWebRequest; import org.zalando.nakadi.domain.EventType; import org.zalando.nakadi.domain.EventTypeSchema; +import org.zalando.nakadi.domain.PaginationWrapper; +import org.zalando.nakadi.exceptions.runtime.InvalidLimitException; import org.zalando.nakadi.service.EventTypeService; -import org.zalando.nakadi.service.Result; import org.zalando.nakadi.service.SchemaService; -import org.zalando.problem.spring.web.advice.Responses; @RestController public class SchemaController { @@ -32,14 +32,11 @@ public ResponseEntity getSchemas( @PathVariable("name") final String name, @RequestParam(value = "offset", required = false, defaultValue = "0") final int offset, @RequestParam(value = "limit", required = false, defaultValue = "20") final int limit, - final NativeWebRequest request) { + final NativeWebRequest request) throws InvalidLimitException { final EventType eventType = eventTypeService.get(name); - final Result result = schemaService.getSchemas(name, offset, limit); - if (result.isSuccessful()) { - return ResponseEntity.status(HttpStatus.OK).body(result.getValue()); - } - return Responses.create(result.getProblem(), request); + final PaginationWrapper schemas = schemaService.getSchemas(name, offset, limit); + return ResponseEntity.status(HttpStatus.OK).body(schemas); } @RequestMapping("/event-types/{name}/schemas/{version}") @@ -52,11 +49,7 @@ public ResponseEntity getSchemaVersion(@PathVariable("name") final String nam return ResponseEntity.status(HttpStatus.OK).body(eventType.getSchema()); } - final Result result = schemaService.getSchemaVersion(name, version); - if (!result.isSuccessful()) { - return Responses.create(result.getProblem(), request); - } - - return ResponseEntity.status(HttpStatus.OK).body(result.getValue()); + final EventTypeSchema result = schemaService.getSchemaVersion(name, version); + return ResponseEntity.status(HttpStatus.OK).body(result); } } diff --git a/src/main/java/org/zalando/nakadi/exceptions/runtime/InvalidVersionNumberException.java b/src/main/java/org/zalando/nakadi/exceptions/runtime/InvalidVersionNumberException.java new file mode 100644 index 0000000000..142938fdfe --- /dev/null +++ b/src/main/java/org/zalando/nakadi/exceptions/runtime/InvalidVersionNumberException.java @@ -0,0 +1,8 @@ +package org.zalando.nakadi.exceptions.runtime; + +public class InvalidVersionNumberException extends NakadiBaseException { + + public InvalidVersionNumberException(final String message) { + super(message); + } +} diff --git a/src/main/java/org/zalando/nakadi/service/SchemaService.java b/src/main/java/org/zalando/nakadi/service/SchemaService.java index 544159e1e4..7c86d97af3 100644 --- a/src/main/java/org/zalando/nakadi/service/SchemaService.java +++ b/src/main/java/org/zalando/nakadi/service/SchemaService.java @@ -5,11 +5,12 @@ import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Component; import org.zalando.nakadi.domain.EventTypeSchema; +import org.zalando.nakadi.domain.PaginationWrapper; +import org.zalando.nakadi.exceptions.runtime.InvalidLimitException; +import org.zalando.nakadi.exceptions.runtime.InvalidVersionNumberException; import org.zalando.nakadi.exceptions.runtime.NoSuchSchemaException; import org.zalando.nakadi.repository.db.SchemaRepository; -import org.zalando.problem.Problem; -import javax.ws.rs.core.Response; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -29,35 +30,29 @@ public SchemaService(final SchemaRepository schemaRepository, this.paginationService = paginationService; } - public Result getSchemas(final String name, final int offset, final int limit) { + public PaginationWrapper getSchemas(final String name, final int offset, final int limit) + throws InvalidLimitException { if (limit < 1 || limit > 1000) { - return Result.problem(Problem.valueOf(Response.Status.BAD_REQUEST, - "'limit' parameter should have value from 1 to 1000")); + throw new InvalidLimitException("'limit' parameter sholud have value between 1 and 1000"); } if (offset < 0) { - return Result.problem(Problem.valueOf(Response.Status.BAD_REQUEST, - "'offset' parameter can't be lower than 0")); + throw new InvalidLimitException("'offset' parameter can't be lower than 0"); } - return Result.ok(paginationService + return paginationService .paginate(offset, limit, String.format("/event-types/%s/schemas", name), (o, l) -> schemaRepository.getSchemas(name, o, l), - () -> schemaRepository.getSchemasCount(name))); + () -> schemaRepository.getSchemasCount(name)); } - public Result getSchemaVersion(final String name, final String version) { + public EventTypeSchema getSchemaVersion(final String name, final String version) + throws NoSuchSchemaException, InvalidVersionNumberException { final Matcher versionMatcher = VERSION_PATTERN.matcher(version); if (!versionMatcher.matches()) { - return Result.problem(Problem.valueOf(Response.Status.BAD_REQUEST, "Invalid version number")); - } - - try { - final EventTypeSchema schema = schemaRepository.getSchemaVersion(name, version); - return Result.ok(schema); - } catch (final NoSuchSchemaException e) { - LOG.debug("Could not find EventTypeSchema version: {} for EventType: {}", version, name); - return Result.problem(Problem.valueOf(Response.Status.NOT_FOUND, e.getMessage())); + throw new InvalidVersionNumberException("Invalid version number"); } + final EventTypeSchema schema = schemaRepository.getSchemaVersion(name, version); + return schema; } } diff --git a/src/test/java/org/zalando/nakadi/controller/SchemaControllerTest.java b/src/test/java/org/zalando/nakadi/controller/SchemaControllerTest.java index 73752f1448..5c72f43b88 100644 --- a/src/test/java/org/zalando/nakadi/controller/SchemaControllerTest.java +++ b/src/test/java/org/zalando/nakadi/controller/SchemaControllerTest.java @@ -10,12 +10,8 @@ import org.zalando.nakadi.domain.EventType; import org.zalando.nakadi.exceptions.runtime.NoSuchEventTypeException; import org.zalando.nakadi.service.EventTypeService; -import org.zalando.nakadi.service.Result; import org.zalando.nakadi.service.SchemaService; import org.zalando.nakadi.utils.EventTypeTestBuilder; -import org.zalando.problem.Problem; - -import javax.ws.rs.core.Response; import static org.zalando.nakadi.utils.TestUtils.buildDefaultEventType; @@ -34,7 +30,7 @@ public void setUp() { @Test public void testSuccess() { - Mockito.when(schemaService.getSchemas("et_test", 0, 1)).thenReturn(Result.ok(null)); + Mockito.when(schemaService.getSchemas("et_test", 0, 1)).thenReturn(null); Mockito.when(eventTypeService.get("et_test")).thenReturn(EventTypeTestBuilder.builder().build()); final ResponseEntity result = new SchemaController(schemaService, eventTypeService) @@ -42,17 +38,6 @@ public void testSuccess() { Assert.assertEquals(HttpStatus.OK, result.getStatusCode()); } - @Test - public void testFailure503() { - Mockito.when(eventTypeService.get("et_test")).thenReturn(EventTypeTestBuilder.builder().build()); - Mockito.when(schemaService.getSchemas("et_test", 0, 1)) - .thenReturn(Result.problem(Problem.valueOf(Response.Status.SERVICE_UNAVAILABLE))); - final ResponseEntity result = - new SchemaController(schemaService, eventTypeService) - .getSchemas("et_test", 0, 1, nativeWebRequest); - Assert.assertEquals(HttpStatus.SERVICE_UNAVAILABLE, result.getStatusCode()); - } - @Test public void testGetLatestSchemaVersionThen200() { final EventType eventType = buildDefaultEventType(); @@ -78,29 +63,11 @@ public void testGetLatestSchemaVersionWrongEventTypeThen404() { public void testGetLatestSchemaVersionByNumberThen200() { final EventType eventType = buildDefaultEventType(); Mockito.when(schemaService.getSchemaVersion(eventType.getName(), - eventType.getSchema().getVersion().toString())).thenReturn(Result.ok(eventType.getSchema())); + eventType.getSchema().getVersion().toString())).thenReturn(eventType.getSchema()); final ResponseEntity result = new SchemaController(schemaService, eventTypeService).getSchemaVersion(eventType.getName(), eventType.getSchema().getVersion().toString(), nativeWebRequest); Assert.assertEquals(HttpStatus.OK, result.getStatusCode()); Assert.assertEquals(eventType.getSchema().toString(), result.getBody().toString()); } - - @Test - public void testGetIllegalSchemaVersionThen404() { - Mockito.when(schemaService.getSchemaVersion("et_test_event", "illegal")) - .thenReturn(Result.problem(Problem.valueOf(Response.Status.NOT_FOUND))); - final ResponseEntity result = - new SchemaController(schemaService, eventTypeService) - .getSchemaVersion("et_test_event", "illegal", nativeWebRequest); - Assert.assertEquals(HttpStatus.NOT_FOUND, result.getStatusCode()); - } - public void testFailure404() { - Mockito.when(eventTypeService.get("et_test")) - .thenThrow(new NoSuchEventTypeException("no event type")); - final ResponseEntity result = - new SchemaController(schemaService, eventTypeService) - .getSchemas("et_test", 0, 1, nativeWebRequest); - Assert.assertEquals(HttpStatus.NOT_FOUND, result.getStatusCode()); - } } diff --git a/src/test/java/org/zalando/nakadi/service/SchemaServiceTest.java b/src/test/java/org/zalando/nakadi/service/SchemaServiceTest.java index 5d7fd724fb..bfd1a90fd8 100644 --- a/src/test/java/org/zalando/nakadi/service/SchemaServiceTest.java +++ b/src/test/java/org/zalando/nakadi/service/SchemaServiceTest.java @@ -8,11 +8,10 @@ import org.zalando.nakadi.domain.EventTypeSchema; import org.zalando.nakadi.domain.PaginationWrapper; import org.zalando.nakadi.domain.Version; +import org.zalando.nakadi.exceptions.runtime.InvalidLimitException; import org.zalando.nakadi.exceptions.runtime.NoSuchSchemaException; import org.zalando.nakadi.repository.db.SchemaRepository; -import javax.ws.rs.core.Response; - import static org.zalando.nakadi.utils.TestUtils.buildDefaultEventType; public class SchemaServiceTest { @@ -28,58 +27,45 @@ public void setUp() { schemaService = new SchemaService(schemaRepository, paginationService); } - @Test + @Test(expected = InvalidLimitException.class) public void testOffsetBounds() { - final Result result = schemaService.getSchemas("name", -1, 1); - Assert.assertFalse(result.isSuccessful()); - Assert.assertEquals(Response.Status.BAD_REQUEST, result.getProblem().getStatus()); - Assert.assertEquals("'offset' parameter can't be lower than 0", result.getProblem().getDetail().get()); + schemaService.getSchemas("name", -1, 1); } - @Test + @Test(expected = InvalidLimitException.class) public void testLimitLowerBounds() { - final Result result = schemaService.getSchemas("name", 0, 0); - Assert.assertFalse(result.isSuccessful()); - Assert.assertEquals(Response.Status.BAD_REQUEST, result.getProblem().getStatus()); - Assert.assertEquals("'limit' parameter should have value from 1 to 1000",result.getProblem().getDetail().get()); + schemaService.getSchemas("name", 0, 0); } - @Test + @Test(expected = InvalidLimitException.class) public void testLimitUpperBounds() { - final Result result = schemaService.getSchemas("name", 0, 1001); - Assert.assertFalse(result.isSuccessful()); - Assert.assertEquals(Response.Status.BAD_REQUEST, result.getProblem().getStatus()); - Assert.assertEquals("'limit' parameter should have value from 1 to 1000",result.getProblem().getDetail().get()); + schemaService.getSchemas("name", 0, 1001); } @Test public void testSuccess() { - final Result result = (Result) schemaService.getSchemas("name", 0, 1000); - Assert.assertTrue(result.isSuccessful()); + final PaginationWrapper result = schemaService.getSchemas("name", 0, 1000); + Assert.assertTrue(true); } - @Test + @Test(expected = NoSuchSchemaException.class) public void testIllegalVersionNumber() throws Exception { final EventType eventType = buildDefaultEventType(); Mockito.when(schemaRepository.getSchemaVersion(eventType.getName() + "wrong", eventType.getSchema().getVersion().toString())) .thenThrow(NoSuchSchemaException.class); - final Result result = schemaService.getSchemaVersion(eventType.getName() + "wrong", + final EventTypeSchema result = schemaService.getSchemaVersion(eventType.getName() + "wrong", eventType.getSchema().getVersion().toString()); - Assert.assertFalse(result.isSuccessful()); - Assert.assertEquals(Response.Status.NOT_FOUND, result.getProblem().getStatus()); } - @Test + @Test(expected = NoSuchSchemaException.class) public void testNonExistingVersionNumber() throws Exception { final EventType eventType = buildDefaultEventType(); Mockito.when(schemaRepository.getSchemaVersion(eventType.getName(), eventType.getSchema().getVersion().bump(Version.Level.MINOR).toString())) .thenThrow(NoSuchSchemaException.class); - final Result result = schemaService.getSchemaVersion(eventType.getName(), + schemaService.getSchemaVersion(eventType.getName(), eventType.getSchema().getVersion().bump(Version.Level.MINOR).toString()); - Assert.assertFalse(result.isSuccessful()); - Assert.assertEquals(Response.Status.NOT_FOUND, result.getProblem().getStatus()); } @Test @@ -88,11 +74,9 @@ public void testGetSchemaSuccess() throws Exception { Mockito.when(schemaRepository.getSchemaVersion(eventType.getName(), eventType.getSchema().getVersion().toString())) .thenReturn(eventType.getSchema()); - final Result result = + final EventTypeSchema result = schemaService.getSchemaVersion(eventType.getName(), eventType.getSchema().getVersion().toString()); - Assert.assertTrue(result.isSuccessful()); - Assert.assertEquals(eventType.getSchema().getVersion().toString(), result.getValue().getVersion().toString()); - Assert.assertEquals(eventType.getSchema().getSchema(), result.getValue().getSchema()); + Assert.assertTrue(true); } } \ No newline at end of file From 9ddadeffa806d24204c3acf8b5a976970326909d Mon Sep 17 00:00:00 2001 From: Lionel Montrieux Date: Wed, 29 Aug 2018 18:16:47 +0200 Subject: [PATCH 03/11] Remove Result from StoragesController --- .../nakadi/controller/ExceptionHandling.java | 46 ++++++++++++ .../nakadi/controller/StoragesController.java | 40 +++------- .../runtime/UnprocessableEntityException.java | 8 ++ .../nakadi/service/StorageService.java | 71 +++++++----------- .../controller/StoragesControllerTest.java | 75 ++----------------- .../nakadi/service/StorageServiceTest.java | 26 ++----- 6 files changed, 108 insertions(+), 158 deletions(-) create mode 100644 src/main/java/org/zalando/nakadi/exceptions/runtime/UnprocessableEntityException.java diff --git a/src/main/java/org/zalando/nakadi/controller/ExceptionHandling.java b/src/main/java/org/zalando/nakadi/controller/ExceptionHandling.java index 2a3aeaed14..f47f8d3cd7 100644 --- a/src/main/java/org/zalando/nakadi/controller/ExceptionHandling.java +++ b/src/main/java/org/zalando/nakadi/controller/ExceptionHandling.java @@ -15,6 +15,7 @@ import org.zalando.nakadi.exceptions.runtime.CursorConversionException; import org.zalando.nakadi.exceptions.runtime.CursorsAreEmptyException; import org.zalando.nakadi.exceptions.runtime.DbWriteOperationsBlockedException; +import org.zalando.nakadi.exceptions.runtime.DuplicatedStorageException; import org.zalando.nakadi.exceptions.runtime.EnrichmentException; import org.zalando.nakadi.exceptions.runtime.FeatureNotAvailableException; import org.zalando.nakadi.exceptions.runtime.IllegalClientIdException; @@ -29,12 +30,16 @@ import org.zalando.nakadi.exceptions.runtime.NoSuchEventTypeException; import org.zalando.nakadi.exceptions.runtime.NoSuchPartitionStrategyException; import org.zalando.nakadi.exceptions.runtime.NoSuchSchemaException; +import org.zalando.nakadi.exceptions.runtime.NoSuchStorageException; import org.zalando.nakadi.exceptions.runtime.NoSuchSubscriptionException; import org.zalando.nakadi.exceptions.runtime.PartitioningException; import org.zalando.nakadi.exceptions.runtime.RepositoryProblemException; import org.zalando.nakadi.exceptions.runtime.ServiceTemporarilyUnavailableException; +import org.zalando.nakadi.exceptions.runtime.StorageIsUsedException; import org.zalando.nakadi.exceptions.runtime.TimelineException; import org.zalando.nakadi.exceptions.runtime.TopicCreationException; +import org.zalando.nakadi.exceptions.runtime.UnknownStorageTypeException; +import org.zalando.nakadi.exceptions.runtime.UnprocessableEntityException; import org.zalando.nakadi.exceptions.runtime.UnprocessableSubscriptionException; import org.zalando.problem.MoreStatus; import org.zalando.problem.Problem; @@ -45,6 +50,7 @@ import static javax.ws.rs.core.Response.Status.BAD_REQUEST; import static javax.ws.rs.core.Response.Status.CONFLICT; +import static javax.ws.rs.core.Response.Status.FORBIDDEN; import static javax.ws.rs.core.Response.Status.INTERNAL_SERVER_ERROR; import static javax.ws.rs.core.Response.Status.NOT_FOUND; import static javax.ws.rs.core.Response.Status.NOT_IMPLEMENTED; @@ -278,4 +284,44 @@ public ResponseEntity handleInvalidVersionNumberException( LOG.debug(exception.getMessage()); return Responses.create(BAD_REQUEST, exception.getMessage(), request); } + + @ExceptionHandler(DuplicatedStorageException.class) + public ResponseEntity handleDuplicatedStorageException( + final DuplicatedStorageException exception, + final NativeWebRequest request) { + LOG.debug(exception.getMessage()); + return Responses.create(CONFLICT, exception.getMessage(), request); + } + + @ExceptionHandler(UnknownStorageTypeException.class) + public ResponseEntity handleUnknownStorageTypeException( + final UnknownStorageTypeException exception, + final NativeWebRequest request) { + LOG.debug(exception.getMessage()); + return Responses.create(UNPROCESSABLE_ENTITY, exception.getMessage(), request); + } + + @ExceptionHandler(UnprocessableEntityException.class) + public ResponseEntity handleUnprocessableEntityException( + final UnprocessableEntityException exception, + final NativeWebRequest request) { + LOG.debug(exception.getMessage()); + return Responses.create(UNPROCESSABLE_ENTITY, exception.getMessage(), request); + } + + @ExceptionHandler(NoSuchStorageException.class) + public ResponseEntity handleNoSuchStorageException( + final NoSuchStorageException exception, + final NativeWebRequest request) { + LOG.debug(exception.getMessage()); + return Responses.create(NOT_FOUND, exception.getMessage(), request); + } + + @ExceptionHandler(StorageIsUsedException.class) + public ResponseEntity handleStorageIsUsedException( + final StorageIsUsedException exception, + final NativeWebRequest request) { + LOG.debug(exception.getMessage()); + return Responses.create(FORBIDDEN, exception.getMessage(), request); + } } diff --git a/src/main/java/org/zalando/nakadi/controller/StoragesController.java b/src/main/java/org/zalando/nakadi/controller/StoragesController.java index ac92975b0b..08b7a39539 100644 --- a/src/main/java/org/zalando/nakadi/controller/StoragesController.java +++ b/src/main/java/org/zalando/nakadi/controller/StoragesController.java @@ -11,11 +11,10 @@ import org.springframework.web.context.request.NativeWebRequest; import org.zalando.nakadi.config.SecuritySettings; import org.zalando.nakadi.domain.Storage; +import org.zalando.nakadi.exceptions.runtime.InternalNakadiException; import org.zalando.nakadi.plugin.api.authz.AuthorizationService; import org.zalando.nakadi.service.AdminService; -import org.zalando.nakadi.service.Result; import org.zalando.nakadi.service.StorageService; -import org.zalando.problem.spring.web.advice.Responses; import java.util.List; @@ -41,15 +40,12 @@ public StoragesController(final SecuritySettings securitySettings, final Storage } @RequestMapping(value = "/storages", method = RequestMethod.GET) - public ResponseEntity listStorages(final NativeWebRequest request) { + public ResponseEntity listStorages(final NativeWebRequest request) throws InternalNakadiException { if (!adminService.isAdmin(AuthorizationService.Operation.READ)) { return status(FORBIDDEN).build(); } - final Result> result = storageService.listStorages(); - if (result.isSuccessful()) { - return status(OK).body(result.getValue()); - } - return Responses.create(result.getProblem(), request); + final List storages = storageService.listStorages(); + return status(OK).body(storages); } @RequestMapping(value = "/storages", method = RequestMethod.POST) @@ -58,11 +54,8 @@ public ResponseEntity createStorage(@RequestBody final String storage, if (!adminService.isAdmin(AuthorizationService.Operation.WRITE)) { return status(FORBIDDEN).build(); } - final Result result = storageService.createStorage(new JSONObject(storage)); - if (result.isSuccessful()) { - return status(CREATED).build(); - } - return Responses.create(result.getProblem(), request); + storageService.createStorage(new JSONObject(storage)); + return status(CREATED).build(); } @RequestMapping(value = "/storages/{id}", method = RequestMethod.GET) @@ -70,11 +63,8 @@ public ResponseEntity getStorage(@PathVariable("id") final String id, final N if (!adminService.isAdmin(AuthorizationService.Operation.READ)) { return status(FORBIDDEN).build(); } - final Result result = storageService.getStorage(id); - if (result.isSuccessful()) { - return status(OK).body(result.getValue()); - } - return Responses.create(result.getProblem(), request); + final Storage storage = storageService.getStorage(id); + return status(OK).body(storage); } @RequestMapping(value = "/storages/{id}", method = RequestMethod.DELETE) @@ -82,11 +72,8 @@ public ResponseEntity deleteStorage(@PathVariable("id") final String id, fina if (!adminService.isAdmin(AuthorizationService.Operation.WRITE)) { return status(FORBIDDEN).build(); } - final Result result = storageService.deleteStorage(id); - if (result.isSuccessful()) { - return status(NO_CONTENT).build(); - } - return Responses.create(result.getProblem(), request); + storageService.deleteStorage(id); + return status(NO_CONTENT).build(); } @RequestMapping(value = "/storages/default/{id}", method = RequestMethod.PUT) @@ -94,10 +81,7 @@ public ResponseEntity setDefaultStorage(@PathVariable("id") final String id, if (!adminService.isAdmin(AuthorizationService.Operation.WRITE)) { return status(FORBIDDEN).build(); } - final Result result = storageService.setDefaultStorage(id); - if (result.isSuccessful()) { - return status(OK).body(result.getValue()); - } - return Responses.create(result.getProblem(), request); + final Storage storage = storageService.setDefaultStorage(id); + return status(OK).body(storage); } } diff --git a/src/main/java/org/zalando/nakadi/exceptions/runtime/UnprocessableEntityException.java b/src/main/java/org/zalando/nakadi/exceptions/runtime/UnprocessableEntityException.java new file mode 100644 index 0000000000..5dc2dfb432 --- /dev/null +++ b/src/main/java/org/zalando/nakadi/exceptions/runtime/UnprocessableEntityException.java @@ -0,0 +1,8 @@ +package org.zalando.nakadi.exceptions.runtime; + +public class UnprocessableEntityException extends NakadiBaseException { + + public UnprocessableEntityException(final String message) { + super(message); + } +} diff --git a/src/main/java/org/zalando/nakadi/service/StorageService.java b/src/main/java/org/zalando/nakadi/service/StorageService.java index df3dd48cc7..fca740232e 100644 --- a/src/main/java/org/zalando/nakadi/service/StorageService.java +++ b/src/main/java/org/zalando/nakadi/service/StorageService.java @@ -16,23 +16,20 @@ import org.zalando.nakadi.domain.Storage; import org.zalando.nakadi.exceptions.runtime.DbWriteOperationsBlockedException; import org.zalando.nakadi.exceptions.runtime.DuplicatedStorageException; +import org.zalando.nakadi.exceptions.runtime.InternalNakadiException; import org.zalando.nakadi.exceptions.runtime.NoSuchStorageException; import org.zalando.nakadi.exceptions.runtime.RepositoryProblemException; import org.zalando.nakadi.exceptions.runtime.StorageIsUsedException; +import org.zalando.nakadi.exceptions.runtime.UnknownStorageTypeException; +import org.zalando.nakadi.exceptions.runtime.UnprocessableEntityException; import org.zalando.nakadi.repository.db.StorageDbRepository; import org.zalando.nakadi.repository.zookeeper.ZooKeeperHolder; -import org.zalando.problem.Problem; import javax.annotation.PostConstruct; import java.io.IOException; import java.util.List; import java.util.Optional; -import static javax.ws.rs.core.Response.Status.CONFLICT; -import static javax.ws.rs.core.Response.Status.INTERNAL_SERVER_ERROR; -import static javax.ws.rs.core.Response.Status.NOT_FOUND; -import static org.zalando.problem.MoreStatus.UNPROCESSABLE_ENTITY; - @Service public class StorageService { @@ -63,10 +60,8 @@ private void watchDefaultStorage() { curator.getData().usingWatcher((CuratorWatcher) event -> { final byte[] defaultStorageId = curator.getData().forPath(ZK_TIMELINES_DEFAULT_STORAGE); if (defaultStorageId != null) { - final Result storageResult = getStorage(new String(defaultStorageId)); - if (storageResult.isSuccessful()) { - defaultStorage.setStorage(storageResult.getValue()); - } + final Storage storage = getStorage(new String(defaultStorageId)); + defaultStorage.setStorage(storage); } watchDefaultStorage(); }).forPath(ZK_TIMELINES_DEFAULT_STORAGE); @@ -75,33 +70,35 @@ private void watchDefaultStorage() { } } - public Result> listStorages() { + public List listStorages() { final List storages; try { storages = storageDbRepository.listStorages(); } catch (RepositoryProblemException e) { LOG.error("DB error occurred when listing storages", e); - return Result.problem(Problem.valueOf(INTERNAL_SERVER_ERROR, e.getMessage())); + throw new InternalNakadiException(e.getMessage()); } - return Result.ok(storages); + return storages; } - public Result getStorage(final String id) { + public Storage getStorage(final String id) { final Optional storage; try { storage = storageDbRepository.getStorage(id); } catch (RepositoryProblemException e) { LOG.error("DB error occurred when fetching storage", e); - return Result.problem(Problem.valueOf(INTERNAL_SERVER_ERROR, e.getMessage())); + throw new InternalNakadiException(e.getMessage()); } if (storage.isPresent()) { - return Result.ok(storage.get()); + return storage.get(); } else { - return Result.problem(Problem.valueOf(NOT_FOUND, "No storage with id " + id)); + throw new NoSuchStorageException("No storage with id " + id); } } - public Result createStorage(final JSONObject json) throws DbWriteOperationsBlockedException { + public void createStorage(final JSONObject json) + throws DbWriteOperationsBlockedException, DuplicatedStorageException, InternalNakadiException, + UnknownStorageTypeException { if (featureToggleService.isFeatureEnabled(FeatureToggleService.Feature.DISABLE_DB_WRITE_OPERATIONS)) { throw new DbWriteOperationsBlockedException("Cannot create storage: write operations on DB " + "are blocked by feature flag."); @@ -118,11 +115,10 @@ public Result createStorage(final JSONObject json) throws DbWriteOperation configuration = json.getJSONObject("kafka_configuration"); break; default: - return Result.problem(Problem.valueOf(UNPROCESSABLE_ENTITY, - "Type '" + type + "' is not a valid storage type")); + throw new UnknownStorageTypeException("Type '" + type + "' is not a valid storage type"); } } catch (JSONException e) { - return Result.problem(Problem.valueOf(UNPROCESSABLE_ENTITY, e.getMessage())); + throw new UnprocessableEntityException(e.getMessage()); } final Storage storage = new Storage(); @@ -131,53 +127,44 @@ public Result createStorage(final JSONObject json) throws DbWriteOperation try { storage.parseConfiguration(objectMapper, configuration.toString()); } catch (final IOException e) { - return Result.problem(Problem.valueOf(UNPROCESSABLE_ENTITY, e.getMessage())); + throw new UnprocessableEntityException(e.getMessage()); } try { storageDbRepository.createStorage(storage); } catch (final RepositoryProblemException e) { LOG.error("DB error occurred when creating storage", e); - return Result.problem(Problem.valueOf(INTERNAL_SERVER_ERROR, e.getMessage())); - } catch (final DuplicatedStorageException e) { - return Result.problem(Problem.valueOf(CONFLICT, e.getMessage())); + throw new InternalNakadiException(e.getMessage()); } - return Result.ok(); + return; } - public Result deleteStorage(final String id) throws DbWriteOperationsBlockedException { + public void deleteStorage(final String id) + throws DbWriteOperationsBlockedException, NoSuchStorageException, StorageIsUsedException { if (featureToggleService.isFeatureEnabled(FeatureToggleService.Feature.DISABLE_DB_WRITE_OPERATIONS)) { throw new DbWriteOperationsBlockedException("Cannot delete storage: write operations on DB " + "are blocked by feature flag."); } try { storageDbRepository.deleteStorage(id); - } catch (final NoSuchStorageException e) { - return Result.notFound("No storage with ID " + id); - } catch (final StorageIsUsedException e) { - return Result.forbidden("Storage " + id + " is in use"); } catch (final RepositoryProblemException e) { LOG.error("DB error occurred when deleting storage", e); - return Result.problem(Problem.valueOf(INTERNAL_SERVER_ERROR, e.getMessage())); + throw new InternalNakadiException(e.getMessage()); } catch (final TransactionException e) { LOG.error("Error with transaction handling when deleting storage", e); - return Result.problem(Problem.valueOf(INTERNAL_SERVER_ERROR, - "Transaction error occurred when deleting storage")); + throw new InternalNakadiException("Transaction error occurred when deleting storage"); } - return Result.ok(); + return; } - public Result setDefaultStorage(final String defaultStorageId) { - final Result storageResult = getStorage(defaultStorageId); - if (storageResult.isSuccessful()) { + public Storage setDefaultStorage(final String defaultStorageId) { + final Storage storage = getStorage(defaultStorageId); try { curator.setData().forPath(ZK_TIMELINES_DEFAULT_STORAGE, defaultStorageId.getBytes(Charsets.UTF_8)); } catch (final Exception e) { LOG.error("Error while setting default storage in zk {} ", e.getMessage(), e); - return Result.problem(Problem.valueOf(INTERNAL_SERVER_ERROR, - "Error while setting default storage in zk")); + throw new InternalNakadiException("Error while setting default storage in zk"); } - } - return storageResult; + return storage; } } diff --git a/src/test/java/org/zalando/nakadi/controller/StoragesControllerTest.java b/src/test/java/org/zalando/nakadi/controller/StoragesControllerTest.java index fff98f9f9f..48cae8d611 100644 --- a/src/test/java/org/zalando/nakadi/controller/StoragesControllerTest.java +++ b/src/test/java/org/zalando/nakadi/controller/StoragesControllerTest.java @@ -10,19 +10,15 @@ import org.zalando.nakadi.plugin.api.authz.AuthorizationService; import org.zalando.nakadi.security.ClientResolver; import org.zalando.nakadi.service.AdminService; -import org.zalando.nakadi.service.Result; -import org.zalando.nakadi.service.StorageService; import org.zalando.nakadi.service.FeatureToggleService; +import org.zalando.nakadi.service.StorageService; import org.zalando.nakadi.utils.TestUtils; -import org.zalando.problem.Problem; import java.util.ArrayList; import java.util.List; -import static javax.ws.rs.core.Response.Status.CONFLICT; -import static javax.ws.rs.core.Response.Status.INTERNAL_SERVER_ERROR; -import static javax.ws.rs.core.Response.Status.NOT_FOUND; import static org.mockito.Matchers.any; +import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -34,7 +30,6 @@ import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; import static org.springframework.test.web.servlet.setup.MockMvcBuilders.standaloneSetup; import static org.zalando.nakadi.util.PrincipalMockFactory.mockPrincipal; -import static org.zalando.problem.MoreStatus.UNPROCESSABLE_ENTITY; public class StoragesControllerTest { @@ -60,7 +55,7 @@ public void before() { public void testListStorages() throws Exception { final List storages = createStorageList(); when(storageService.listStorages()) - .thenReturn(Result.ok(storages)); + .thenReturn(storages); when(adminService.isAdmin(AuthorizationService.Operation.READ)).thenReturn(true); mockMvc.perform(get("/storages") .principal(mockPrincipal("nakadi"))) @@ -69,28 +64,17 @@ public void testListStorages() throws Exception { @Test public void testDeleteUnusedStorage() throws Exception { - when(storageService.deleteStorage("s1")) - .thenReturn(Result.ok()); + doNothing().when(storageService).deleteStorage("s1"); when(adminService.isAdmin(AuthorizationService.Operation.WRITE)).thenReturn(true); mockMvc.perform(delete("/storages/s1") .principal(mockPrincipal("nakadi"))) .andExpect(status().isNoContent()); } - @Test - public void testDeleteStorageInUse() throws Exception { - when(storageService.deleteStorage("s1")) - .thenReturn(Result.forbidden("Storage in use")); - when(adminService.isAdmin(AuthorizationService.Operation.WRITE)).thenReturn(true); - mockMvc.perform(delete("/storages/s1") - .principal(mockPrincipal("nakadi"))) - .andExpect(status().isForbidden()); - } - @Test public void testPostStorage() throws Exception { final JSONObject json = createJsonKafkaStorage("test_storage"); - when(storageService.createStorage(any())).thenReturn(Result.ok()); + doNothing().when(storageService).createStorage(any()); when(adminService.isAdmin(AuthorizationService.Operation.WRITE)).thenReturn(true); mockMvc.perform(post("/storages") .contentType(APPLICATION_JSON) @@ -99,34 +83,10 @@ public void testPostStorage() throws Exception { .andExpect(status().isCreated()); } - @Test - public void testPostStorageWithExistingId() throws Exception { - final JSONObject json = createJsonKafkaStorage("test_storage"); - when(storageService.createStorage(any())).thenReturn(Result.problem(Problem.valueOf(CONFLICT))); - when(adminService.isAdmin(AuthorizationService.Operation.WRITE)).thenReturn(true); - mockMvc.perform(post("/storages") - .contentType(APPLICATION_JSON) - .content(json.toString()) - .principal(mockPrincipal("nakadi"))) - .andExpect(status().isConflict()); - } - - @Test - public void testPostStorageWrongFormat() throws Exception { - final JSONObject json = createJsonKafkaStorage("test_storage"); - when(storageService.createStorage(any())).thenReturn(Result.problem(Problem.valueOf(UNPROCESSABLE_ENTITY))); - when(adminService.isAdmin(AuthorizationService.Operation.WRITE)).thenReturn(true); - mockMvc.perform(post("/storages") - .contentType(APPLICATION_JSON) - .content(json.toString()) - .principal(mockPrincipal("nakadi"))) - .andExpect(status().isUnprocessableEntity()); - } - @Test public void testSetDefaultStorageOk() throws Exception { when(storageService.setDefaultStorage("test_storage")) - .thenReturn(Result.ok(createKafkaStorage("test_storage"))); + .thenReturn(createKafkaStorage("test_storage")); when(adminService.isAdmin(AuthorizationService.Operation.WRITE)).thenReturn(true); mockMvc.perform(put("/storages/default/test_storage") .contentType(APPLICATION_JSON) @@ -134,29 +94,6 @@ public void testSetDefaultStorageOk() throws Exception { .andExpect(status().isOk()); } - @Test - public void testSetDefaultStorageNotFound() throws Exception { - when(storageService.setDefaultStorage("test_storage")) - .thenReturn(Result.problem(Problem.valueOf(NOT_FOUND, "No storage with id test_storage"))); - when(adminService.isAdmin(AuthorizationService.Operation.WRITE)).thenReturn(true); - mockMvc.perform(put("/storages/default/test_storage") - .contentType(APPLICATION_JSON) - .principal(mockPrincipal("nakadi"))) - .andExpect(status().isNotFound()); - } - - @Test - public void testSetDefaultStorageInternalServiceError() throws Exception { - when(storageService.setDefaultStorage("test_storage")) - .thenReturn(Result.problem(Problem.valueOf(INTERNAL_SERVER_ERROR, - "Error while setting default storage in zk"))); - when(adminService.isAdmin(AuthorizationService.Operation.WRITE)).thenReturn(true); - mockMvc.perform(put("/storages/default/test_storage") - .contentType(APPLICATION_JSON) - .principal(mockPrincipal("nakadi"))) - .andExpect(status().isInternalServerError()); - } - @Test public void testSetDefaultStorageAccessDenied() throws Exception { when(adminService.isAdmin(AuthorizationService.Operation.WRITE)).thenReturn(false); diff --git a/src/test/java/org/zalando/nakadi/service/StorageServiceTest.java b/src/test/java/org/zalando/nakadi/service/StorageServiceTest.java index 0e10138dfc..3be14501bb 100644 --- a/src/test/java/org/zalando/nakadi/service/StorageServiceTest.java +++ b/src/test/java/org/zalando/nakadi/service/StorageServiceTest.java @@ -11,9 +11,6 @@ import org.zalando.nakadi.repository.zookeeper.ZooKeeperHolder; import org.zalando.nakadi.utils.TestUtils; -import static org.hamcrest.CoreMatchers.equalTo; -import static org.junit.Assert.assertThat; -import static org.junit.Assert.assertTrue; import static org.mockito.Matchers.any; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; @@ -36,41 +33,32 @@ public void setUp() { } @Test - public void testCreateStorage() throws Exception { + public void testCreateStorage() { final Storage dbReply = createTestStorage(); when(storageDbRepository.createStorage(any())).thenReturn(dbReply); final JSONObject storage = createTestStorageJson("s1"); - final Result result = storageService.createStorage(storage); - assertTrue(result.isSuccessful()); + storageService.createStorage(storage); } @Test public void testDeleteUnusedStorage() throws Exception { - assertTrue(storageService.deleteStorage("s3").isSuccessful()); + storageService.deleteStorage("s3"); } - @Test + @Test(expected = StorageIsUsedException.class) public void testDeleteStorageInUse() throws Exception { doThrow(new StorageIsUsedException("", null)).when(storageDbRepository).deleteStorage("s"); - final Result result = storageService.deleteStorage("s"); - - final Result expectedResult = Result.forbidden("Storage s is in use"); - assertThat(result.getProblem().getStatus(), equalTo(expectedResult.getProblem().getStatus())); - assertThat(result.getProblem().getDetail(), equalTo(expectedResult.getProblem().getDetail())); + storageService.deleteStorage("s"); } - @Test + @Test(expected = NoSuchStorageException.class) public void testDeleteNonExistingStorage() throws Exception { doThrow(new NoSuchStorageException("")).when(storageDbRepository).deleteStorage("s"); - final Result result = storageService.deleteStorage("s"); - - final Result expectedResult = Result.notFound("No storage with ID s"); - assertThat(result.getProblem().getStatus(), equalTo(expectedResult.getProblem().getStatus())); - assertThat(result.getProblem().getDetail(), equalTo(expectedResult.getProblem().getDetail())); + storageService.deleteStorage("s"); } private JSONObject createTestStorageJson(final String id) { From f172ca414756359b1b8af7a9a2a9a6c028362d80 Mon Sep 17 00:00:00 2001 From: Lionel Montrieux Date: Wed, 29 Aug 2018 22:46:46 +0200 Subject: [PATCH 04/11] Remove Result from SubscriptionService --- .../controller/SubscriptionController.java | 17 +++--- .../subscription/SubscriptionService.java | 56 +++++++------------ .../SubscriptionControllerTest.java | 2 +- 3 files changed, 30 insertions(+), 45 deletions(-) diff --git a/src/main/java/org/zalando/nakadi/controller/SubscriptionController.java b/src/main/java/org/zalando/nakadi/controller/SubscriptionController.java index c23d7a5bad..784b037694 100644 --- a/src/main/java/org/zalando/nakadi/controller/SubscriptionController.java +++ b/src/main/java/org/zalando/nakadi/controller/SubscriptionController.java @@ -3,7 +3,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.bind.annotation.PathVariable; @@ -22,7 +21,6 @@ import org.zalando.nakadi.exceptions.runtime.ServiceTemporarilyUnavailableException; import org.zalando.nakadi.exceptions.runtime.TimeLagStatsTimeoutException; import org.zalando.nakadi.service.FeatureToggleService; -import org.zalando.nakadi.service.WebResult; import org.zalando.nakadi.service.subscription.SubscriptionService; import org.zalando.nakadi.service.subscription.SubscriptionService.StatsMode; import org.zalando.problem.Problem; @@ -34,6 +32,9 @@ import static javax.ws.rs.core.Response.Status.NOT_IMPLEMENTED; import static javax.ws.rs.core.Response.Status.SERVICE_UNAVAILABLE; +import static org.springframework.http.HttpStatus.NO_CONTENT; +import static org.springframework.http.HttpStatus.OK; +import static org.springframework.http.ResponseEntity.status; import static org.zalando.problem.MoreStatus.UNPROCESSABLE_ENTITY; @@ -62,22 +63,22 @@ public ResponseEntity listSubscriptions( @RequestParam(value = "offset", required = false, defaultValue = "0") final int offset, final NativeWebRequest request) { - return WebResult.wrap( - () -> subscriptionService.listSubscriptions(owningApplication, eventTypes, showStatus, limit, offset), - request); + return status(OK) + .body(subscriptionService + .listSubscriptions(owningApplication, eventTypes, showStatus, limit, offset)); } @RequestMapping(value = "/{id}", method = RequestMethod.GET) public ResponseEntity getSubscription(@PathVariable("id") final String subscriptionId, final NativeWebRequest request) { - return WebResult.wrap(() -> subscriptionService.getSubscription(subscriptionId), request); + return status(OK).body(subscriptionService.getSubscription(subscriptionId)); } @RequestMapping(value = "/{id}", method = RequestMethod.DELETE) public ResponseEntity deleteSubscription(@PathVariable("id") final String subscriptionId, final NativeWebRequest request) { - return WebResult.wrap(() -> subscriptionService.deleteSubscription(subscriptionId), request, - HttpStatus.NO_CONTENT); + subscriptionService.deleteSubscription(subscriptionId); + return status(NO_CONTENT).build(); } @RequestMapping(value = "/{id}/stats", method = RequestMethod.GET) diff --git a/src/main/java/org/zalando/nakadi/service/subscription/SubscriptionService.java b/src/main/java/org/zalando/nakadi/service/subscription/SubscriptionService.java index dcae6188d8..ef4d653f59 100644 --- a/src/main/java/org/zalando/nakadi/service/subscription/SubscriptionService.java +++ b/src/main/java/org/zalando/nakadi/service/subscription/SubscriptionService.java @@ -29,6 +29,7 @@ import org.zalando.nakadi.exceptions.runtime.InternalNakadiException; import org.zalando.nakadi.exceptions.runtime.InvalidCursorException; import org.zalando.nakadi.exceptions.runtime.InvalidCursorOperation; +import org.zalando.nakadi.exceptions.runtime.InvalidLimitException; import org.zalando.nakadi.exceptions.runtime.NoSuchEventTypeException; import org.zalando.nakadi.exceptions.runtime.NoSuchSubscriptionException; import org.zalando.nakadi.exceptions.runtime.RepositoryProblemException; @@ -44,7 +45,6 @@ import org.zalando.nakadi.service.CursorOperationsService; import org.zalando.nakadi.service.FeatureToggleService; import org.zalando.nakadi.service.NakadiKpiPublisher; -import org.zalando.nakadi.service.Result; import org.zalando.nakadi.service.subscription.model.Partition; import org.zalando.nakadi.service.subscription.zk.SubscriptionClientFactory; import org.zalando.nakadi.service.subscription.zk.ZkSubscriptionClient; @@ -52,10 +52,8 @@ import org.zalando.nakadi.service.timeline.TimelineService; import org.zalando.nakadi.util.SubscriptionsUriHelper; import org.zalando.nakadi.view.SubscriptionCursorWithoutToken; -import org.zalando.problem.Problem; import javax.annotation.Nullable; -import javax.ws.rs.core.Response; import java.time.Duration; import java.util.ArrayList; import java.util.Collection; @@ -67,8 +65,6 @@ import java.util.Set; import java.util.stream.Collectors; -import static javax.ws.rs.core.Response.Status.NOT_FOUND; - @Component public class SubscriptionService { @@ -169,18 +165,18 @@ public UriComponents getSubscriptionUri(final Subscription subscription) { return SUBSCRIPTION_PATH.buildAndExpand(subscription.getId()); } - public Result listSubscriptions(@Nullable final String owningApplication, @Nullable final Set eventTypes, - final boolean showStatus, final int limit, final int offset) { + public PaginationWrapper listSubscriptions(@Nullable final String owningApplication, + @Nullable final Set eventTypes, + final boolean showStatus, + final int limit, + final int offset) + throws InvalidLimitException { if (limit < 1 || limit > 1000) { - final Problem problem = Problem.valueOf(Response.Status.BAD_REQUEST, - "'limit' parameter should have value from 1 to 1000"); - return Result.problem(problem); + throw new InvalidLimitException("'limit' parameter should have value between 1 and 1000"); } if (offset < 0) { - final Problem problem = Problem.valueOf(Response.Status.BAD_REQUEST, - "'offset' parameter can't be lower than 0"); - return Result.problem(problem); + throw new InvalidLimitException("'offset' parameter can't be lower than 0"); } try { @@ -196,27 +192,21 @@ public Result listSubscriptions(@Nullable final String owningApplication, @Nulla final List items = paginationWrapper.getItems(); items.forEach(s -> s.setStatus(createSubscriptionStat(s, StatsMode.LIGHT))); } - return Result.ok(paginationWrapper); + return paginationWrapper; } catch (final ServiceTemporarilyUnavailableException e) { LOG.error("Error occurred during listing of subscriptions", e); - return Result.problem(Problem.valueOf(Response.Status.SERVICE_UNAVAILABLE, e.getMessage())); + throw e; } } - public Result getSubscription(final String subscriptionId) { - try { - final Subscription subscription = subscriptionRepository.getSubscription(subscriptionId); - return Result.ok(subscription); - } catch (final NoSuchSubscriptionException e) { - LOG.debug("Failed to find subscription: {}", subscriptionId); - return Result.problem(Problem.valueOf(NOT_FOUND, e.getMessage())); - } catch (final ServiceTemporarilyUnavailableException e) { - LOG.error("Error occurred when trying to get subscription: {}", subscriptionId, e); - return Result.problem(Problem.valueOf(Response.Status.SERVICE_UNAVAILABLE, e.getMessage())); - } + public Subscription getSubscription(final String subscriptionId) + throws NoSuchSubscriptionException, ServiceTemporarilyUnavailableException { + return subscriptionRepository.getSubscription(subscriptionId); } - public Result deleteSubscription(final String subscriptionId) throws DbWriteOperationsBlockedException { + public void deleteSubscription(final String subscriptionId) + throws DbWriteOperationsBlockedException, NoSuchSubscriptionException, NoSuchEventTypeException, + ServiceTemporarilyUnavailableException { if (featureToggleService.isFeatureEnabled(FeatureToggleService.Feature.DISABLE_DB_WRITE_OPERATIONS)) { throw new DbWriteOperationsBlockedException("Cannot delete subscription: write operations on DB " + "are blocked by feature flag."); @@ -235,16 +225,10 @@ public Result deleteSubscription(final String subscriptionId) throws DbWri .put("subscription_id", subscriptionId) .put("status", "deleted")); - return Result.ok(); - } catch (final NoSuchSubscriptionException e) { - LOG.debug("Failed to find subscription: {}", subscriptionId, e); - return Result.problem(Problem.valueOf(NOT_FOUND, e.getMessage())); - } catch (final ServiceTemporarilyUnavailableException e) { - LOG.error("Error occurred when trying to delete subscription: {}", subscriptionId, e); - return Result.problem(Problem.valueOf(Response.Status.SERVICE_UNAVAILABLE, e.getMessage())); - } catch (final NoSuchEventTypeException | InternalNakadiException e) { + return; + } catch (final InternalNakadiException e) { LOG.error("Exception can not occur", e); - return Result.problem(Problem.valueOf(NOT_FOUND, e.getMessage())); + throw e; } } diff --git a/src/test/java/org/zalando/nakadi/controller/SubscriptionControllerTest.java b/src/test/java/org/zalando/nakadi/controller/SubscriptionControllerTest.java index df95d26a89..d31eefb27d 100644 --- a/src/test/java/org/zalando/nakadi/controller/SubscriptionControllerTest.java +++ b/src/test/java/org/zalando/nakadi/controller/SubscriptionControllerTest.java @@ -203,7 +203,7 @@ public void whenListSubscriptionsWithNegativeOffsetThenBadRequest() throws Excep @Test public void whenListSubscriptionsWithIncorrectLimitThenBadRequest() throws Exception { final Problem expectedProblem = Problem.valueOf(BAD_REQUEST, - "'limit' parameter should have value from 1 to 1000"); + "'limit' parameter should have value between 1 and 1000"); checkForProblem(getSubscriptions(ImmutableSet.of("et"), "app", 0, -5), expectedProblem); } From fed5fcb355da1bea94d2bb63910d69966dc555f8 Mon Sep 17 00:00:00 2001 From: Lionel Montrieux Date: Wed, 29 Aug 2018 22:48:23 +0200 Subject: [PATCH 05/11] Remove unused classes --- .../org/zalando/nakadi/service/Result.java | 86 ------------------- .../org/zalando/nakadi/service/WebResult.java | 27 ------ 2 files changed, 113 deletions(-) delete mode 100644 src/main/java/org/zalando/nakadi/service/Result.java delete mode 100644 src/main/java/org/zalando/nakadi/service/WebResult.java diff --git a/src/main/java/org/zalando/nakadi/service/Result.java b/src/main/java/org/zalando/nakadi/service/Result.java deleted file mode 100644 index e87bc52cd6..0000000000 --- a/src/main/java/org/zalando/nakadi/service/Result.java +++ /dev/null @@ -1,86 +0,0 @@ -package org.zalando.nakadi.service; - -import org.zalando.problem.Problem; - -import javax.ws.rs.core.Response; - -public interface Result { - - boolean isSuccessful(); - - T getValue(); - - Problem getProblem(); - - static Result problem(final Problem problem) { - return new Failure<>(problem); - } - - static Result ok() { - return new Success<>(null); - } - - static Result ok(final T value) { - return new Success<>(value); - } - - static Result forbidden(final String message) { - return problem(Problem.valueOf(Response.Status.FORBIDDEN, message)); - } - - static Result notFound(final String message) { - return problem(Problem.valueOf(Response.Status.NOT_FOUND, message)); - } - - static Result conflict(final String message) { - return problem(Problem.valueOf(Response.Status.CONFLICT, message)); - } - - class Success implements Result { - - private final V value; - - private Success(final V value) { - this.value = value; - } - - @Override - public boolean isSuccessful() { - return true; - } - - @Override - public V getValue() { - return value; - } - - @Override - public Problem getProblem() { - throw new IllegalArgumentException("Success.getProblem"); - } - } - - class Failure implements Result { - - private final Problem problem; - - private Failure(final Problem problem) { - this.problem = problem; - } - - @Override - public boolean isSuccessful() { - return false; - } - - @Override - public T getValue() { - throw new IllegalArgumentException("Failure.getValue"); - } - - @Override - public Problem getProblem() { - return problem; - } - } -} diff --git a/src/main/java/org/zalando/nakadi/service/WebResult.java b/src/main/java/org/zalando/nakadi/service/WebResult.java deleted file mode 100644 index d3f0f9db60..0000000000 --- a/src/main/java/org/zalando/nakadi/service/WebResult.java +++ /dev/null @@ -1,27 +0,0 @@ -package org.zalando.nakadi.service; - -import org.springframework.http.HttpStatus; -import org.springframework.http.ResponseEntity; -import org.springframework.web.context.request.NativeWebRequest; -import org.zalando.problem.spring.web.advice.Responses; - -import java.util.function.Supplier; - -import static org.springframework.http.HttpStatus.OK; -import static org.springframework.http.ResponseEntity.status; - -public class WebResult { - - public static ResponseEntity wrap(final Supplier supplier, final NativeWebRequest request) { - return wrap(supplier, request, OK); - } - - public static ResponseEntity wrap(final Supplier supplier, final NativeWebRequest request, - final HttpStatus successCode) { - final Result result = supplier.get(); - if (!result.isSuccessful()) { - return Responses.create(result.getProblem(), request); - } - return status(successCode).body(result.getValue()); - } -} From 7655f2aa46749818b3d39352c60a9c712f91d012 Mon Sep 17 00:00:00 2001 From: Lionel Montrieux Date: Wed, 29 Aug 2018 22:53:15 +0200 Subject: [PATCH 06/11] Remove tests that have been @Ignore for years --- .../HashPartitionStrategyTest.java | 58 ------------------- 1 file changed, 58 deletions(-) diff --git a/src/test/java/org/zalando/nakadi/partitioning/HashPartitionStrategyTest.java b/src/test/java/org/zalando/nakadi/partitioning/HashPartitionStrategyTest.java index db3fda188e..5a5e84596a 100644 --- a/src/test/java/org/zalando/nakadi/partitioning/HashPartitionStrategyTest.java +++ b/src/test/java/org/zalando/nakadi/partitioning/HashPartitionStrategyTest.java @@ -3,7 +3,6 @@ import com.google.common.collect.ImmutableList; import org.apache.commons.lang3.StringUtils; import org.json.JSONObject; -import org.junit.Ignore; import org.junit.Test; import org.zalando.nakadi.domain.EventType; import org.zalando.nakadi.exceptions.Try; @@ -12,7 +11,6 @@ import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; -import java.security.SecureRandom; import java.util.ArrayList; import java.util.LinkedList; import java.util.List; @@ -74,21 +72,6 @@ public void calculatesSamePartitionForSamePartitionKeyFields() throws Exception checkThatEventsWithSameKeysAreInSamePartition(partitions); } - @Test - @Ignore("This might be useful to play around with for future implementations of PartitionStrategies") - public void partitionsAreEvenlyDistributedUsingRandomEvents() { - // This is a probabilistic test. - // The probability that it fails is approx. 0.577% - - fillPartitionsWithRandomEvents(simpleEventType, partitions, 10000); - - final double[] eventDistribution = partitions.stream().map(List::size).mapToDouble(value -> value * 1.0) - .toArray(); - final double variance = calculateVarianceOfUniformDistribution(eventDistribution); - - assertThat(variance, lessThan(1.5)); - } - @Test public void partitionsAreEvenlyDistributed() throws IOException { loadEventSamples(); @@ -109,16 +92,6 @@ private double varianceForEvents(final List events) { return calculateVarianceOfUniformDistribution(eventDistribution); } - @Test - @Ignore("Run this to create a new set of event samples") - public void createSampleSet() { - final List events = generateRandomEvents(10000); - - for (final JSONObject event : events) { - System.out.println(event.toString()); - } - } - @Test public void canHandleComplexKeys() throws Exception { final JSONObject event = new JSONObject(resourceAsString("../complex-event.json", this.getClass())); @@ -131,37 +104,6 @@ public void canHandleComplexKeys() throws Exception { assertThat(partition, isIn(PARTITIONS)); } - @Test - @Ignore("Tests the variance used in the tests here") - public void testVariance() { - final SecureRandom random = new SecureRandom(); - - final int numberOfSamples = 100000; - final int numberOfRuns = 1000; - final double threshold = 1.5; - - double failProbability = 0; - - for (int run = 0; run < numberOfRuns; run++) { - final double[] dist = new double[8]; - for (int i = 0; i < numberOfSamples; i++) { - dist[random.nextInt(dist.length)]++; - } - final double variance = calculateVarianceOfUniformDistribution(dist); - //System.out.println(Arrays.toString(dist) + " = " + variance); - - if (variance > threshold) { - failProbability += (1.0 / numberOfRuns); - } - - if (((run * 100.0) / numberOfRuns) % 1 == 0) { - System.out.println((int) ((run * 100.0) / numberOfRuns) + "%"); - } - } - - System.out.println("probability to fail the test: " + failProbability); - } - @Test public void whenValidateWithHashPartitionStrategyAndDataChangeEventLookupIntoDataField() throws Exception { final EventType eventType = loadEventType( From a59d933f98e3fa0378c6db3b657fb39b9f359d5b Mon Sep 17 00:00:00 2001 From: Lionel Montrieux Date: Thu, 30 Aug 2018 12:46:22 +0200 Subject: [PATCH 07/11] Update CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ffffc21087..37ea114c5c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed - Upgraded dependencies - Refactored exceptions +- Moved Problem creation to controller ## [2.8.3] - 2018-08-01 From b1b0dc94c9f3375d3f0b8c06b9b6307adc91b556 Mon Sep 17 00:00:00 2001 From: Lionel Montrieux Date: Wed, 26 Sep 2018 15:10:27 +0200 Subject: [PATCH 08/11] Explicit throws for runtime exceptions, and remove unnecessary code --- .../controller/EventTypeController.java | 3 +- .../nakadi/controller/SchemaController.java | 11 ++- .../nakadi/controller/StoragesController.java | 23 ++++-- .../controller/SubscriptionController.java | 14 ++-- .../nakadi/service/StorageService.java | 12 ++-- .../subscription/SubscriptionService.java | 70 ++++++++----------- 6 files changed, 74 insertions(+), 59 deletions(-) diff --git a/src/main/java/org/zalando/nakadi/controller/EventTypeController.java b/src/main/java/org/zalando/nakadi/controller/EventTypeController.java index 306513274e..445b3407b8 100644 --- a/src/main/java/org/zalando/nakadi/controller/EventTypeController.java +++ b/src/main/java/org/zalando/nakadi/controller/EventTypeController.java @@ -144,7 +144,8 @@ public ResponseEntity update( } @RequestMapping(value = "/{name:.+}", method = RequestMethod.GET) - public ResponseEntity get(@PathVariable final String name, final NativeWebRequest request) { + public ResponseEntity get(@PathVariable final String name, final NativeWebRequest request) + throws NoSuchEventTypeException, InternalNakadiException{ final EventType eventType = eventTypeService.get(name); return status(HttpStatus.OK).body(eventType); } diff --git a/src/main/java/org/zalando/nakadi/controller/SchemaController.java b/src/main/java/org/zalando/nakadi/controller/SchemaController.java index cad9c36d32..b1403752b0 100644 --- a/src/main/java/org/zalando/nakadi/controller/SchemaController.java +++ b/src/main/java/org/zalando/nakadi/controller/SchemaController.java @@ -11,7 +11,11 @@ import org.zalando.nakadi.domain.EventType; import org.zalando.nakadi.domain.EventTypeSchema; import org.zalando.nakadi.domain.PaginationWrapper; +import org.zalando.nakadi.exceptions.runtime.InternalNakadiException; import org.zalando.nakadi.exceptions.runtime.InvalidLimitException; +import org.zalando.nakadi.exceptions.runtime.InvalidVersionNumberException; +import org.zalando.nakadi.exceptions.runtime.NoSuchEventTypeException; +import org.zalando.nakadi.exceptions.runtime.NoSuchSchemaException; import org.zalando.nakadi.service.EventTypeService; import org.zalando.nakadi.service.SchemaService; @@ -32,7 +36,8 @@ public ResponseEntity getSchemas( @PathVariable("name") final String name, @RequestParam(value = "offset", required = false, defaultValue = "0") final int offset, @RequestParam(value = "limit", required = false, defaultValue = "20") final int limit, - final NativeWebRequest request) throws InvalidLimitException { + final NativeWebRequest request) + throws InvalidLimitException, NoSuchEventTypeException, InternalNakadiException { final EventType eventType = eventTypeService.get(name); final PaginationWrapper schemas = schemaService.getSchemas(name, offset, limit); @@ -42,7 +47,9 @@ public ResponseEntity getSchemas( @RequestMapping("/event-types/{name}/schemas/{version}") public ResponseEntity getSchemaVersion(@PathVariable("name") final String name, @PathVariable("version") final String version, - final NativeWebRequest request) { + final NativeWebRequest request) + throws NoSuchEventTypeException, InternalNakadiException, + NoSuchSchemaException, InvalidVersionNumberException { if (version.equals("latest")) { // latest schema might be cached with the event type final EventType eventType = eventTypeService.get(name); diff --git a/src/main/java/org/zalando/nakadi/controller/StoragesController.java b/src/main/java/org/zalando/nakadi/controller/StoragesController.java index 08b7a39539..46926ceccb 100644 --- a/src/main/java/org/zalando/nakadi/controller/StoragesController.java +++ b/src/main/java/org/zalando/nakadi/controller/StoragesController.java @@ -11,7 +11,13 @@ import org.springframework.web.context.request.NativeWebRequest; import org.zalando.nakadi.config.SecuritySettings; import org.zalando.nakadi.domain.Storage; +import org.zalando.nakadi.exceptions.runtime.DbWriteOperationsBlockedException; +import org.zalando.nakadi.exceptions.runtime.DuplicatedStorageException; import org.zalando.nakadi.exceptions.runtime.InternalNakadiException; +import org.zalando.nakadi.exceptions.runtime.NoSuchStorageException; +import org.zalando.nakadi.exceptions.runtime.StorageIsUsedException; +import org.zalando.nakadi.exceptions.runtime.UnknownStorageTypeException; +import org.zalando.nakadi.plugin.api.PluginException; import org.zalando.nakadi.plugin.api.authz.AuthorizationService; import org.zalando.nakadi.service.AdminService; import org.zalando.nakadi.service.StorageService; @@ -40,7 +46,8 @@ public StoragesController(final SecuritySettings securitySettings, final Storage } @RequestMapping(value = "/storages", method = RequestMethod.GET) - public ResponseEntity listStorages(final NativeWebRequest request) throws InternalNakadiException { + public ResponseEntity listStorages(final NativeWebRequest request) + throws InternalNakadiException, PluginException { if (!adminService.isAdmin(AuthorizationService.Operation.READ)) { return status(FORBIDDEN).build(); } @@ -50,7 +57,9 @@ public ResponseEntity listStorages(final NativeWebRequest request) throws Int @RequestMapping(value = "/storages", method = RequestMethod.POST) public ResponseEntity createStorage(@RequestBody final String storage, - final NativeWebRequest request) { + final NativeWebRequest request) + throws PluginException, DbWriteOperationsBlockedException, + DuplicatedStorageException, InternalNakadiException, UnknownStorageTypeException { if (!adminService.isAdmin(AuthorizationService.Operation.WRITE)) { return status(FORBIDDEN).build(); } @@ -59,7 +68,8 @@ public ResponseEntity createStorage(@RequestBody final String storage, } @RequestMapping(value = "/storages/{id}", method = RequestMethod.GET) - public ResponseEntity getStorage(@PathVariable("id") final String id, final NativeWebRequest request) { + public ResponseEntity getStorage(@PathVariable("id") final String id, final NativeWebRequest request) + throws PluginException, NoSuchStorageException, InternalNakadiException { if (!adminService.isAdmin(AuthorizationService.Operation.READ)) { return status(FORBIDDEN).build(); } @@ -68,7 +78,9 @@ public ResponseEntity getStorage(@PathVariable("id") final String id, final N } @RequestMapping(value = "/storages/{id}", method = RequestMethod.DELETE) - public ResponseEntity deleteStorage(@PathVariable("id") final String id, final NativeWebRequest request) { + public ResponseEntity deleteStorage(@PathVariable("id") final String id, final NativeWebRequest request) + throws PluginException, DbWriteOperationsBlockedException, NoSuchStorageException, + StorageIsUsedException, InternalNakadiException{ if (!adminService.isAdmin(AuthorizationService.Operation.WRITE)) { return status(FORBIDDEN).build(); } @@ -77,7 +89,8 @@ public ResponseEntity deleteStorage(@PathVariable("id") final String id, fina } @RequestMapping(value = "/storages/default/{id}", method = RequestMethod.PUT) - public ResponseEntity setDefaultStorage(@PathVariable("id") final String id, final NativeWebRequest request) { + public ResponseEntity setDefaultStorage(@PathVariable("id") final String id, final NativeWebRequest request) + throws PluginException, NoSuchStorageException, InternalNakadiException { if (!adminService.isAdmin(AuthorizationService.Operation.WRITE)) { return status(FORBIDDEN).build(); } diff --git a/src/main/java/org/zalando/nakadi/controller/SubscriptionController.java b/src/main/java/org/zalando/nakadi/controller/SubscriptionController.java index 784b037694..3cba0a0db0 100644 --- a/src/main/java/org/zalando/nakadi/controller/SubscriptionController.java +++ b/src/main/java/org/zalando/nakadi/controller/SubscriptionController.java @@ -13,9 +13,12 @@ import org.springframework.web.context.request.NativeWebRequest; import org.zalando.nakadi.domain.ItemsWrapper; import org.zalando.nakadi.domain.SubscriptionEventTypeStats; +import org.zalando.nakadi.exceptions.runtime.DbWriteOperationsBlockedException; import org.zalando.nakadi.exceptions.runtime.ErrorGettingCursorTimeLagException; import org.zalando.nakadi.exceptions.runtime.FeatureNotAvailableException; import org.zalando.nakadi.exceptions.runtime.InconsistentStateException; +import org.zalando.nakadi.exceptions.runtime.InternalNakadiException; +import org.zalando.nakadi.exceptions.runtime.InvalidLimitException; import org.zalando.nakadi.exceptions.runtime.NoSuchEventTypeException; import org.zalando.nakadi.exceptions.runtime.NoSuchSubscriptionException; import org.zalando.nakadi.exceptions.runtime.ServiceTemporarilyUnavailableException; @@ -61,8 +64,8 @@ public ResponseEntity listSubscriptions( @RequestParam(value = "show_status", required = false, defaultValue = "false") final boolean showStatus, @RequestParam(value = "limit", required = false, defaultValue = "20") final int limit, @RequestParam(value = "offset", required = false, defaultValue = "0") final int offset, - final NativeWebRequest request) { - + final NativeWebRequest request) + throws InvalidLimitException, ServiceTemporarilyUnavailableException { return status(OK) .body(subscriptionService .listSubscriptions(owningApplication, eventTypes, showStatus, limit, offset)); @@ -70,13 +73,16 @@ public ResponseEntity listSubscriptions( @RequestMapping(value = "/{id}", method = RequestMethod.GET) public ResponseEntity getSubscription(@PathVariable("id") final String subscriptionId, - final NativeWebRequest request) { + final NativeWebRequest request) + throws NoSuchSubscriptionException, ServiceTemporarilyUnavailableException { return status(OK).body(subscriptionService.getSubscription(subscriptionId)); } @RequestMapping(value = "/{id}", method = RequestMethod.DELETE) public ResponseEntity deleteSubscription(@PathVariable("id") final String subscriptionId, - final NativeWebRequest request) { + final NativeWebRequest request) + throws DbWriteOperationsBlockedException, NoSuchSubscriptionException, NoSuchEventTypeException, + ServiceTemporarilyUnavailableException, InternalNakadiException { subscriptionService.deleteSubscription(subscriptionId); return status(NO_CONTENT).build(); } diff --git a/src/main/java/org/zalando/nakadi/service/StorageService.java b/src/main/java/org/zalando/nakadi/service/StorageService.java index fca740232e..59ae711a18 100644 --- a/src/main/java/org/zalando/nakadi/service/StorageService.java +++ b/src/main/java/org/zalando/nakadi/service/StorageService.java @@ -70,7 +70,7 @@ private void watchDefaultStorage() { } } - public List listStorages() { + public List listStorages() throws InternalNakadiException { final List storages; try { storages = storageDbRepository.listStorages(); @@ -81,7 +81,7 @@ public List listStorages() { return storages; } - public Storage getStorage(final String id) { + public Storage getStorage(final String id) throws NoSuchStorageException, InternalNakadiException { final Optional storage; try { storage = storageDbRepository.getStorage(id); @@ -136,11 +136,11 @@ public void createStorage(final JSONObject json) LOG.error("DB error occurred when creating storage", e); throw new InternalNakadiException(e.getMessage()); } - return; } public void deleteStorage(final String id) - throws DbWriteOperationsBlockedException, NoSuchStorageException, StorageIsUsedException { + throws DbWriteOperationsBlockedException, NoSuchStorageException, + StorageIsUsedException, InternalNakadiException { if (featureToggleService.isFeatureEnabled(FeatureToggleService.Feature.DISABLE_DB_WRITE_OPERATIONS)) { throw new DbWriteOperationsBlockedException("Cannot delete storage: write operations on DB " + "are blocked by feature flag."); @@ -154,10 +154,10 @@ public void deleteStorage(final String id) LOG.error("Error with transaction handling when deleting storage", e); throw new InternalNakadiException("Transaction error occurred when deleting storage"); } - return; } - public Storage setDefaultStorage(final String defaultStorageId) { + public Storage setDefaultStorage(final String defaultStorageId) + throws NoSuchStorageException, InternalNakadiException { final Storage storage = getStorage(defaultStorageId); try { curator.setData().forPath(ZK_TIMELINES_DEFAULT_STORAGE, defaultStorageId.getBytes(Charsets.UTF_8)); diff --git a/src/main/java/org/zalando/nakadi/service/subscription/SubscriptionService.java b/src/main/java/org/zalando/nakadi/service/subscription/SubscriptionService.java index ef4d653f59..89b6ae3403 100644 --- a/src/main/java/org/zalando/nakadi/service/subscription/SubscriptionService.java +++ b/src/main/java/org/zalando/nakadi/service/subscription/SubscriptionService.java @@ -84,12 +84,6 @@ public class SubscriptionService { private final SubscriptionTimeLagService subscriptionTimeLagService; private final AuthorizationValidator authorizationValidator; - public enum StatsMode { - LIGHT, - NORMAL, - TIMELAG - } - @Autowired public SubscriptionService(final SubscriptionDbRepository subscriptionRepository, final SubscriptionClientFactory subscriptionClientFactory, @@ -170,7 +164,7 @@ public PaginationWrapper listSubscriptions(@Nullable final String final boolean showStatus, final int limit, final int offset) - throws InvalidLimitException { + throws InvalidLimitException, ServiceTemporarilyUnavailableException { if (limit < 1 || limit > 1000) { throw new InvalidLimitException("'limit' parameter should have value between 1 and 1000"); } @@ -179,24 +173,19 @@ public PaginationWrapper listSubscriptions(@Nullable final String throw new InvalidLimitException("'offset' parameter can't be lower than 0"); } - try { - final Set eventTypesFilter = eventTypes == null ? ImmutableSet.of() : eventTypes; - final Optional owningAppOption = Optional.ofNullable(owningApplication); - final List subscriptions = - subscriptionRepository.listSubscriptions(eventTypesFilter, owningAppOption, offset, limit); - final PaginationLinks paginationLinks = SubscriptionsUriHelper.createSubscriptionPaginationLinks( - owningAppOption, eventTypesFilter, offset, limit, showStatus, subscriptions.size()); - final PaginationWrapper paginationWrapper = - new PaginationWrapper<>(subscriptions, paginationLinks); - if (showStatus) { - final List items = paginationWrapper.getItems(); - items.forEach(s -> s.setStatus(createSubscriptionStat(s, StatsMode.LIGHT))); - } - return paginationWrapper; - } catch (final ServiceTemporarilyUnavailableException e) { - LOG.error("Error occurred during listing of subscriptions", e); - throw e; + final Set eventTypesFilter = eventTypes == null ? ImmutableSet.of() : eventTypes; + final Optional owningAppOption = Optional.ofNullable(owningApplication); + final List subscriptions = + subscriptionRepository.listSubscriptions(eventTypesFilter, owningAppOption, offset, limit); + final PaginationLinks paginationLinks = SubscriptionsUriHelper.createSubscriptionPaginationLinks( + owningAppOption, eventTypesFilter, offset, limit, showStatus, subscriptions.size()); + final PaginationWrapper paginationWrapper = + new PaginationWrapper<>(subscriptions, paginationLinks); + if (showStatus) { + final List items = paginationWrapper.getItems(); + items.forEach(s -> s.setStatus(createSubscriptionStat(s, StatsMode.LIGHT))); } + return paginationWrapper; } public Subscription getSubscription(final String subscriptionId) @@ -206,30 +195,23 @@ public Subscription getSubscription(final String subscriptionId) public void deleteSubscription(final String subscriptionId) throws DbWriteOperationsBlockedException, NoSuchSubscriptionException, NoSuchEventTypeException, - ServiceTemporarilyUnavailableException { + ServiceTemporarilyUnavailableException, InternalNakadiException { if (featureToggleService.isFeatureEnabled(FeatureToggleService.Feature.DISABLE_DB_WRITE_OPERATIONS)) { throw new DbWriteOperationsBlockedException("Cannot delete subscription: write operations on DB " + "are blocked by feature flag."); } - try { - final Subscription subscription = subscriptionRepository.getSubscription(subscriptionId); - - authorizationValidator.authorizeSubscriptionAdmin(subscription); + final Subscription subscription = subscriptionRepository.getSubscription(subscriptionId); - subscriptionRepository.deleteSubscription(subscriptionId); - final ZkSubscriptionClient zkSubscriptionClient = subscriptionClientFactory.createClient( - subscription, LogPathBuilder.build(subscriptionId, "delete_subscription")); - zkSubscriptionClient.deleteSubscription(); + authorizationValidator.authorizeSubscriptionAdmin(subscription); - nakadiKpiPublisher.publish(subLogEventType, () -> new JSONObject() - .put("subscription_id", subscriptionId) - .put("status", "deleted")); + subscriptionRepository.deleteSubscription(subscriptionId); + final ZkSubscriptionClient zkSubscriptionClient = subscriptionClientFactory.createClient( + subscription, LogPathBuilder.build(subscriptionId, "delete_subscription")); + zkSubscriptionClient.deleteSubscription(); - return; - } catch (final InternalNakadiException e) { - LOG.error("Exception can not occur", e); - throw e; - } + nakadiKpiPublisher.publish(subLogEventType, () -> new JSONObject() + .put("subscription_id", subscriptionId) + .put("status", "deleted")); } public ItemsWrapper getSubscriptionStat(final String subscriptionId, @@ -441,4 +423,10 @@ private Collection loadCommittedPositions( } } + public enum StatsMode { + LIGHT, + NORMAL, + TIMELAG + } + } From 8dcd976e0f6e7d6d8aadf044b530022bf7237758 Mon Sep 17 00:00:00 2001 From: Lionel Montrieux Date: Wed, 26 Sep 2018 16:04:25 +0200 Subject: [PATCH 09/11] Move exceptions to their controller if they are specific to one controller --- .../CursorOperationsController.java | 8 ++ .../nakadi/controller/CursorsController.java | 6 + .../controller/EventTypeController.java | 7 ++ .../nakadi/controller/ExceptionHandling.java | 108 ------------------ .../PostSubscriptionController.java | 3 +- .../nakadi/controller/SchemaController.java | 16 +++ .../nakadi/controller/StoragesController.java | 43 +++++++ .../SubscriptionStreamController.java | 7 ++ .../controller/TimelinesController.java | 13 +++ 9 files changed, 102 insertions(+), 109 deletions(-) diff --git a/src/main/java/org/zalando/nakadi/controller/CursorOperationsController.java b/src/main/java/org/zalando/nakadi/controller/CursorOperationsController.java index ab640408df..9c9267fbc6 100644 --- a/src/main/java/org/zalando/nakadi/controller/CursorOperationsController.java +++ b/src/main/java/org/zalando/nakadi/controller/CursorOperationsController.java @@ -43,6 +43,7 @@ import static org.springframework.http.HttpStatus.OK; import static org.springframework.http.ResponseEntity.status; +import static org.zalando.problem.MoreStatus.UNPROCESSABLE_ENTITY; @RestController public class CursorOperationsController { @@ -140,6 +141,13 @@ public ResponseEntity invalidCursorOperation(final InvalidCursorOperation e, clientErrorMessage(e.getReason())), request); } + @ExceptionHandler(CursorConversionException.class) + public ResponseEntity handleCursorConversionException(final CursorConversionException exception, + final NativeWebRequest request) { + LOG.error(exception.getMessage(), exception); + return Responses.create(UNPROCESSABLE_ENTITY, exception.getMessage(), request); + } + private String clientErrorMessage(final InvalidCursorOperation.Reason reason) { switch (reason) { case TIMELINE_NOT_FOUND: return "Timeline not found. It might happen in case the cursor refers to a " + diff --git a/src/main/java/org/zalando/nakadi/controller/CursorsController.java b/src/main/java/org/zalando/nakadi/controller/CursorsController.java index 941f5ab5de..0802be8ad7 100644 --- a/src/main/java/org/zalando/nakadi/controller/CursorsController.java +++ b/src/main/java/org/zalando/nakadi/controller/CursorsController.java @@ -187,4 +187,10 @@ public ResponseEntity handleFeatureNotAllowed(final FeatureNotAvailable return Responses.create(Problem.valueOf(Response.Status.NOT_IMPLEMENTED, "Feature is disabled"), request); } + @ExceptionHandler(CursorsAreEmptyException.class) + public ResponseEntity handleCursorsUnavailableException(final RuntimeException ex, + final NativeWebRequest request) { + LOG.debug(ex.getMessage(), ex); + return Responses.create(MoreStatus.UNPROCESSABLE_ENTITY, ex.getMessage(), request); + } } diff --git a/src/main/java/org/zalando/nakadi/controller/EventTypeController.java b/src/main/java/org/zalando/nakadi/controller/EventTypeController.java index 445b3407b8..2a9bc0d633 100644 --- a/src/main/java/org/zalando/nakadi/controller/EventTypeController.java +++ b/src/main/java/org/zalando/nakadi/controller/EventTypeController.java @@ -224,4 +224,11 @@ public ResponseEntity unableProcess(final EventTypeOptionsValidationExc LOG.debug(exception.getMessage(), exception); return Responses.create(UNPROCESSABLE_ENTITY, exception.getMessage(), request); } + + @ExceptionHandler(TopicCreationException.class) + public ResponseEntity handleTopicCreationException(final TopicCreationException exception, + final NativeWebRequest request) { + LOG.error(exception.getMessage(), exception); + return Responses.create(Response.Status.SERVICE_UNAVAILABLE, exception.getMessage(), request); + } } diff --git a/src/main/java/org/zalando/nakadi/controller/ExceptionHandling.java b/src/main/java/org/zalando/nakadi/controller/ExceptionHandling.java index f47f8d3cd7..9d9f9e2a01 100644 --- a/src/main/java/org/zalando/nakadi/controller/ExceptionHandling.java +++ b/src/main/java/org/zalando/nakadi/controller/ExceptionHandling.java @@ -12,10 +12,7 @@ import org.springframework.web.context.request.NativeWebRequest; import org.zalando.nakadi.exceptions.runtime.AccessDeniedException; import org.zalando.nakadi.exceptions.runtime.CompactionException; -import org.zalando.nakadi.exceptions.runtime.CursorConversionException; -import org.zalando.nakadi.exceptions.runtime.CursorsAreEmptyException; import org.zalando.nakadi.exceptions.runtime.DbWriteOperationsBlockedException; -import org.zalando.nakadi.exceptions.runtime.DuplicatedStorageException; import org.zalando.nakadi.exceptions.runtime.EnrichmentException; import org.zalando.nakadi.exceptions.runtime.FeatureNotAvailableException; import org.zalando.nakadi.exceptions.runtime.IllegalClientIdException; @@ -26,21 +23,12 @@ import org.zalando.nakadi.exceptions.runtime.LimitReachedException; import org.zalando.nakadi.exceptions.runtime.NakadiBaseException; import org.zalando.nakadi.exceptions.runtime.NakadiRuntimeException; -import org.zalando.nakadi.exceptions.runtime.NoStreamingSlotsAvailable; import org.zalando.nakadi.exceptions.runtime.NoSuchEventTypeException; -import org.zalando.nakadi.exceptions.runtime.NoSuchPartitionStrategyException; -import org.zalando.nakadi.exceptions.runtime.NoSuchSchemaException; -import org.zalando.nakadi.exceptions.runtime.NoSuchStorageException; import org.zalando.nakadi.exceptions.runtime.NoSuchSubscriptionException; import org.zalando.nakadi.exceptions.runtime.PartitioningException; import org.zalando.nakadi.exceptions.runtime.RepositoryProblemException; import org.zalando.nakadi.exceptions.runtime.ServiceTemporarilyUnavailableException; -import org.zalando.nakadi.exceptions.runtime.StorageIsUsedException; -import org.zalando.nakadi.exceptions.runtime.TimelineException; -import org.zalando.nakadi.exceptions.runtime.TopicCreationException; -import org.zalando.nakadi.exceptions.runtime.UnknownStorageTypeException; import org.zalando.nakadi.exceptions.runtime.UnprocessableEntityException; -import org.zalando.nakadi.exceptions.runtime.UnprocessableSubscriptionException; import org.zalando.problem.MoreStatus; import org.zalando.problem.Problem; import org.zalando.problem.spring.web.advice.ProblemHandling; @@ -49,8 +37,6 @@ import javax.ws.rs.core.Response; import static javax.ws.rs.core.Response.Status.BAD_REQUEST; -import static javax.ws.rs.core.Response.Status.CONFLICT; -import static javax.ws.rs.core.Response.Status.FORBIDDEN; import static javax.ws.rs.core.Response.Status.INTERNAL_SERVER_ERROR; import static javax.ws.rs.core.Response.Status.NOT_FOUND; import static javax.ws.rs.core.Response.Status.NOT_IMPLEMENTED; @@ -110,13 +96,6 @@ public ResponseEntity handleIllegalClientIdException(final IllegalClien return Responses.create(Response.Status.FORBIDDEN, exception.getMessage(), request); } - @ExceptionHandler(CursorsAreEmptyException.class) - public ResponseEntity handleCursorsUnavailableException(final RuntimeException ex, - final NativeWebRequest request) { - LOG.debug(ex.getMessage(), ex); - return Responses.create(MoreStatus.UNPROCESSABLE_ENTITY, ex.getMessage(), request); - } - @ExceptionHandler public ResponseEntity handleExceptionWrapper(final NakadiRuntimeException exception, final NativeWebRequest request) throws Exception { @@ -141,31 +120,6 @@ public ResponseEntity handleInternalError(final NakadiBaseException exc return Responses.create(Response.Status.INTERNAL_SERVER_ERROR, exception.getMessage(), request); } - @ExceptionHandler(TimelineException.class) - public ResponseEntity handleTimelineException(final TimelineException exception, - final NativeWebRequest request) { - LOG.error(exception.getMessage(), exception); - final Throwable cause = exception.getCause(); - if (cause instanceof InternalNakadiException) { - return Responses.create(Problem.valueOf(INTERNAL_SERVER_ERROR, exception.getMessage()), request); - } - return Responses.create(Response.Status.SERVICE_UNAVAILABLE, exception.getMessage(), request); - } - - @ExceptionHandler(TopicCreationException.class) - public ResponseEntity handleTopicCreationException(final TopicCreationException exception, - final NativeWebRequest request) { - LOG.error(exception.getMessage(), exception); - return Responses.create(Response.Status.SERVICE_UNAVAILABLE, exception.getMessage(), request); - } - - @ExceptionHandler(CursorConversionException.class) - public ResponseEntity handleCursorConversionException(final CursorConversionException exception, - final NativeWebRequest request) { - LOG.error(exception.getMessage(), exception); - return Responses.create(UNPROCESSABLE_ENTITY, exception.getMessage(), request); - } - @ExceptionHandler(ServiceTemporarilyUnavailableException.class) public ResponseEntity handleServiceTemporarilyUnavailableException( final ServiceTemporarilyUnavailableException exception, final NativeWebRequest request) { @@ -210,14 +164,6 @@ public ResponseEntity handleEnrichmentException(final EnrichmentExcepti return Responses.create(UNPROCESSABLE_ENTITY, exception.getMessage(), request); } - @ExceptionHandler(NoSuchPartitionStrategyException.class) - public ResponseEntity handleNoSuchPartitionStrategyException( - final NoSuchPartitionStrategyException exception, - final NativeWebRequest request) { - LOG.debug(exception.getMessage()); - return Responses.create(UNPROCESSABLE_ENTITY, exception.getMessage(), request); - } - @ExceptionHandler(PartitioningException.class) public ResponseEntity handlePartitioningException(final PartitioningException exception, final NativeWebRequest request) { @@ -240,13 +186,6 @@ public ResponseEntity handleNoSuchEventTypeException(final NoSuchEventT return Responses.create(NOT_FOUND, exception.getMessage(), request); } - @ExceptionHandler(NoSuchSchemaException.class) - public ResponseEntity handleNoSuchSchemaException(final NoSuchSchemaException exception, - final NativeWebRequest request) { - LOG.debug(exception.getMessage()); - return Responses.create(NOT_FOUND, exception.getMessage(), request); - } - @ExceptionHandler(NoSuchSubscriptionException.class) public ResponseEntity handleNoSuchSubscriptionException(final NoSuchSubscriptionException exception, final NativeWebRequest request) { @@ -254,21 +193,6 @@ public ResponseEntity handleNoSuchSubscriptionException(final NoSuchSub return Responses.create(NOT_FOUND, exception.getMessage(), request); } - @ExceptionHandler(NoStreamingSlotsAvailable.class) - public ResponseEntity handleNoStreamingSlotsAvailable(final NoStreamingSlotsAvailable exception, - final NativeWebRequest request) { - LOG.debug(exception.getMessage()); - return Responses.create(CONFLICT, exception.getMessage(), request); - } - - @ExceptionHandler(UnprocessableSubscriptionException.class) - public ResponseEntity handleUnprocessableSubscriptionException( - final UnprocessableSubscriptionException exception, - final NativeWebRequest request) { - LOG.debug(exception.getMessage()); - return Responses.create(UNPROCESSABLE_ENTITY, exception.getMessage(), request); - } - @ExceptionHandler(InvalidLimitException.class) public ResponseEntity handleInvalidLimitException( final InvalidLimitException exception, @@ -285,22 +209,6 @@ public ResponseEntity handleInvalidVersionNumberException( return Responses.create(BAD_REQUEST, exception.getMessage(), request); } - @ExceptionHandler(DuplicatedStorageException.class) - public ResponseEntity handleDuplicatedStorageException( - final DuplicatedStorageException exception, - final NativeWebRequest request) { - LOG.debug(exception.getMessage()); - return Responses.create(CONFLICT, exception.getMessage(), request); - } - - @ExceptionHandler(UnknownStorageTypeException.class) - public ResponseEntity handleUnknownStorageTypeException( - final UnknownStorageTypeException exception, - final NativeWebRequest request) { - LOG.debug(exception.getMessage()); - return Responses.create(UNPROCESSABLE_ENTITY, exception.getMessage(), request); - } - @ExceptionHandler(UnprocessableEntityException.class) public ResponseEntity handleUnprocessableEntityException( final UnprocessableEntityException exception, @@ -308,20 +216,4 @@ public ResponseEntity handleUnprocessableEntityException( LOG.debug(exception.getMessage()); return Responses.create(UNPROCESSABLE_ENTITY, exception.getMessage(), request); } - - @ExceptionHandler(NoSuchStorageException.class) - public ResponseEntity handleNoSuchStorageException( - final NoSuchStorageException exception, - final NativeWebRequest request) { - LOG.debug(exception.getMessage()); - return Responses.create(NOT_FOUND, exception.getMessage(), request); - } - - @ExceptionHandler(StorageIsUsedException.class) - public ResponseEntity handleStorageIsUsedException( - final StorageIsUsedException exception, - final NativeWebRequest request) { - LOG.debug(exception.getMessage()); - return Responses.create(FORBIDDEN, exception.getMessage(), request); - } } diff --git a/src/main/java/org/zalando/nakadi/controller/PostSubscriptionController.java b/src/main/java/org/zalando/nakadi/controller/PostSubscriptionController.java index b8d0faa9b8..8a66fb26c8 100644 --- a/src/main/java/org/zalando/nakadi/controller/PostSubscriptionController.java +++ b/src/main/java/org/zalando/nakadi/controller/PostSubscriptionController.java @@ -120,7 +120,8 @@ private ResponseEntity prepareLocationResponse(final Subscription subscriptio @ExceptionHandler({ WrongInitialCursorsException.class, - TooManyPartitionsException.class}) + TooManyPartitionsException.class, + UnprocessableSubscriptionException.class}) public ResponseEntity handleUnprocessableSubscription(final NakadiBaseException exception, final NativeWebRequest request) { LOG.debug("Error occurred when working with subscriptions", exception); diff --git a/src/main/java/org/zalando/nakadi/controller/SchemaController.java b/src/main/java/org/zalando/nakadi/controller/SchemaController.java index b1403752b0..0272f1e480 100644 --- a/src/main/java/org/zalando/nakadi/controller/SchemaController.java +++ b/src/main/java/org/zalando/nakadi/controller/SchemaController.java @@ -1,8 +1,11 @@ package org.zalando.nakadi.controller; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; +import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.bind.annotation.PathVariable; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestParam; @@ -18,10 +21,16 @@ import org.zalando.nakadi.exceptions.runtime.NoSuchSchemaException; import org.zalando.nakadi.service.EventTypeService; import org.zalando.nakadi.service.SchemaService; +import org.zalando.problem.Problem; +import org.zalando.problem.spring.web.advice.Responses; + +import static javax.ws.rs.core.Response.Status.NOT_FOUND; @RestController public class SchemaController { + private static final Logger LOG = LoggerFactory.getLogger(SchemaController.class); + private final SchemaService schemaService; private final EventTypeService eventTypeService; @@ -59,4 +68,11 @@ public ResponseEntity getSchemaVersion(@PathVariable("name") final String nam final EventTypeSchema result = schemaService.getSchemaVersion(name, version); return ResponseEntity.status(HttpStatus.OK).body(result); } + + @ExceptionHandler(NoSuchSchemaException.class) + public ResponseEntity handleNoSuchSchemaException(final NoSuchSchemaException exception, + final NativeWebRequest request) { + LOG.debug(exception.getMessage()); + return Responses.create(NOT_FOUND, exception.getMessage(), request); + } } diff --git a/src/main/java/org/zalando/nakadi/controller/StoragesController.java b/src/main/java/org/zalando/nakadi/controller/StoragesController.java index 46926ceccb..5ea02b3fa9 100644 --- a/src/main/java/org/zalando/nakadi/controller/StoragesController.java +++ b/src/main/java/org/zalando/nakadi/controller/StoragesController.java @@ -1,8 +1,11 @@ package org.zalando.nakadi.controller; import org.json.JSONObject; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.http.ResponseEntity; +import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.bind.annotation.PathVariable; import org.springframework.web.bind.annotation.RequestBody; import org.springframework.web.bind.annotation.RequestMapping; @@ -21,18 +24,26 @@ import org.zalando.nakadi.plugin.api.authz.AuthorizationService; import org.zalando.nakadi.service.AdminService; import org.zalando.nakadi.service.StorageService; +import org.zalando.problem.Problem; +import org.zalando.problem.spring.web.advice.Responses; +import javax.ws.rs.core.Response; import java.util.List; +import static javax.ws.rs.core.Response.Status.CONFLICT; +import static javax.ws.rs.core.Response.Status.NOT_FOUND; import static org.springframework.http.HttpStatus.CREATED; import static org.springframework.http.HttpStatus.FORBIDDEN; import static org.springframework.http.HttpStatus.NO_CONTENT; import static org.springframework.http.HttpStatus.OK; import static org.springframework.http.ResponseEntity.status; +import static org.zalando.problem.MoreStatus.UNPROCESSABLE_ENTITY; @RestController public class StoragesController { + private static final Logger LOG = LoggerFactory.getLogger(StoragesController.class); + private final SecuritySettings securitySettings; private final StorageService storageService; private final AdminService adminService; @@ -97,4 +108,36 @@ public ResponseEntity setDefaultStorage(@PathVariable("id") final String id, final Storage storage = storageService.setDefaultStorage(id); return status(OK).body(storage); } + + @ExceptionHandler(NoSuchStorageException.class) + public ResponseEntity handleNoSuchStorageException( + final NoSuchStorageException exception, + final NativeWebRequest request) { + LOG.debug(exception.getMessage()); + return Responses.create(NOT_FOUND, exception.getMessage(), request); + } + + @ExceptionHandler(StorageIsUsedException.class) + public ResponseEntity handleStorageIsUsedException( + final StorageIsUsedException exception, + final NativeWebRequest request) { + LOG.debug(exception.getMessage()); + return Responses.create(Response.Status.FORBIDDEN, exception.getMessage(), request); + } + + @ExceptionHandler(DuplicatedStorageException.class) + public ResponseEntity handleDuplicatedStorageException( + final DuplicatedStorageException exception, + final NativeWebRequest request) { + LOG.debug(exception.getMessage()); + return Responses.create(CONFLICT, exception.getMessage(), request); + } + + @ExceptionHandler(UnknownStorageTypeException.class) + public ResponseEntity handleUnknownStorageTypeException( + final UnknownStorageTypeException exception, + final NativeWebRequest request) { + LOG.debug(exception.getMessage()); + return Responses.create(UNPROCESSABLE_ENTITY, exception.getMessage(), request); + } } diff --git a/src/main/java/org/zalando/nakadi/controller/SubscriptionStreamController.java b/src/main/java/org/zalando/nakadi/controller/SubscriptionStreamController.java index 5d9b261677..ab8de46013 100644 --- a/src/main/java/org/zalando/nakadi/controller/SubscriptionStreamController.java +++ b/src/main/java/org/zalando/nakadi/controller/SubscriptionStreamController.java @@ -253,4 +253,11 @@ public ResponseEntity invalidEventTypeException(final WrongStreamParame return Responses.create(UNPROCESSABLE_ENTITY, exception.getMessage(), request); } + @ExceptionHandler(NoStreamingSlotsAvailable.class) + public ResponseEntity handleNoStreamingSlotsAvailable(final NoStreamingSlotsAvailable exception, + final NativeWebRequest request) { + LOG.debug(exception.getMessage()); + return Responses.create(CONFLICT, exception.getMessage(), request); + } + } diff --git a/src/main/java/org/zalando/nakadi/controller/TimelinesController.java b/src/main/java/org/zalando/nakadi/controller/TimelinesController.java index c58c3d329f..8bdc559bc0 100644 --- a/src/main/java/org/zalando/nakadi/controller/TimelinesController.java +++ b/src/main/java/org/zalando/nakadi/controller/TimelinesController.java @@ -15,6 +15,7 @@ import org.zalando.nakadi.exceptions.runtime.AccessDeniedException; import org.zalando.nakadi.exceptions.runtime.ConflictException; import org.zalando.nakadi.exceptions.runtime.InconsistentStateException; +import org.zalando.nakadi.exceptions.runtime.InternalNakadiException; import org.zalando.nakadi.exceptions.runtime.NotFoundException; import org.zalando.nakadi.exceptions.runtime.RepositoryProblemException; import org.zalando.nakadi.exceptions.runtime.TimelineException; @@ -32,6 +33,8 @@ import javax.ws.rs.core.Response; import java.util.stream.Collectors; +import static javax.ws.rs.core.Response.Status.INTERNAL_SERVER_ERROR; + @RestController @RequestMapping(value = "/event-types/{name}/timelines", produces = MediaType.APPLICATION_JSON) public class TimelinesController { @@ -81,4 +84,14 @@ public ResponseEntity conflict(final ConflictException ex, final Native return Responses.create(Response.Status.CONFLICT, ex.getMessage(), request); } + @ExceptionHandler(TimelineException.class) + public ResponseEntity handleTimelineException(final TimelineException exception, + final NativeWebRequest request) { + LOG.error(exception.getMessage(), exception); + final Throwable cause = exception.getCause(); + if (cause instanceof InternalNakadiException) { + return Responses.create(Problem.valueOf(INTERNAL_SERVER_ERROR, exception.getMessage()), request); + } + return Responses.create(Response.Status.SERVICE_UNAVAILABLE, exception.getMessage(), request); + } } From 976bfa4c31ac4db602c8a812f3d31464acce2292 Mon Sep 17 00:00:00 2001 From: Lionel Montrieux Date: Wed, 26 Sep 2018 16:29:20 +0200 Subject: [PATCH 10/11] Remove unused exception and clarify usage of some others --- .../controller/EventPublishingController.java | 34 +++++++++++++++++-- .../nakadi/controller/ExceptionHandling.java | 33 ------------------ .../runtime/CompactionException.java | 9 ----- .../partitioning/PartitionResolver.java | 2 +- .../nakadi/service/EventPublisher.java | 23 ++++++------- 5 files changed, 44 insertions(+), 57 deletions(-) delete mode 100644 src/main/java/org/zalando/nakadi/exceptions/runtime/CompactionException.java diff --git a/src/main/java/org/zalando/nakadi/controller/EventPublishingController.java b/src/main/java/org/zalando/nakadi/controller/EventPublishingController.java index f74b47c35d..1bec3318c8 100644 --- a/src/main/java/org/zalando/nakadi/controller/EventPublishingController.java +++ b/src/main/java/org/zalando/nakadi/controller/EventPublishingController.java @@ -9,6 +9,7 @@ import org.springframework.beans.factory.annotation.Value; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; +import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.bind.annotation.PathVariable; import org.springframework.web.bind.annotation.RequestBody; import org.springframework.web.bind.annotation.RequestMapping; @@ -17,9 +18,12 @@ import org.zalando.nakadi.domain.EventPublishResult; import org.zalando.nakadi.domain.EventPublishingStatus; import org.zalando.nakadi.exceptions.runtime.AccessDeniedException; +import org.zalando.nakadi.exceptions.runtime.EnrichmentException; import org.zalando.nakadi.exceptions.runtime.EventTypeTimeoutException; import org.zalando.nakadi.exceptions.runtime.InternalNakadiException; +import org.zalando.nakadi.exceptions.runtime.InvalidPartitionKeyFieldsException; import org.zalando.nakadi.exceptions.runtime.NoSuchEventTypeException; +import org.zalando.nakadi.exceptions.runtime.PartitioningException; import org.zalando.nakadi.exceptions.runtime.ServiceTemporarilyUnavailableException; import org.zalando.nakadi.metrics.EventTypeMetricRegistry; import org.zalando.nakadi.metrics.EventTypeMetrics; @@ -40,6 +44,7 @@ import static javax.ws.rs.core.Response.Status.SERVICE_UNAVAILABLE; import static org.springframework.http.ResponseEntity.status; import static org.springframework.web.bind.annotation.RequestMethod.POST; +import static org.zalando.problem.MoreStatus.UNPROCESSABLE_ENTITY; import static org.zalando.problem.spring.web.advice.Responses.create; @RestController @@ -71,7 +76,9 @@ public EventPublishingController(final EventPublisher publisher, public ResponseEntity postEvent(@PathVariable final String eventTypeName, @RequestBody final String eventsAsString, final NativeWebRequest request, - final Client client) throws AccessDeniedException { + final Client client) + throws AccessDeniedException, EnrichmentException, + PartitioningException, ServiceTemporarilyUnavailableException { LOG.trace("Received event {} for event type {}", eventsAsString, eventTypeName); final EventTypeMetrics eventTypeMetrics = eventTypeMetricRegistry.metricsFor(eventTypeName); @@ -91,12 +98,35 @@ public ResponseEntity postEvent(@PathVariable final String eventTypeName, } } + @ExceptionHandler(EnrichmentException.class) + public ResponseEntity handleEnrichmentException(final EnrichmentException exception, + final NativeWebRequest request) { + LOG.debug(exception.getMessage()); + return Responses.create(UNPROCESSABLE_ENTITY, exception.getMessage(), request); + } + + @ExceptionHandler(PartitioningException.class) + public ResponseEntity handlePartitioningException(final PartitioningException exception, + final NativeWebRequest request) { + LOG.debug(exception.getMessage()); + return Responses.create(UNPROCESSABLE_ENTITY, exception.getMessage(), request); + } + + @ExceptionHandler(InvalidPartitionKeyFieldsException.class) + public ResponseEntity handleInvalidPartitionKeyFieldsException( + final InvalidPartitionKeyFieldsException exception, + final NativeWebRequest request) { + LOG.debug(exception.getMessage()); + return Responses.create(UNPROCESSABLE_ENTITY, exception.getMessage(), request); + } + private ResponseEntity postEventInternal(final String eventTypeName, final String eventsAsString, final NativeWebRequest nativeWebRequest, final EventTypeMetrics eventTypeMetrics, final Client client) - throws AccessDeniedException, ServiceTemporarilyUnavailableException { + throws AccessDeniedException, EnrichmentException, PartitioningException, + ServiceTemporarilyUnavailableException { final long startingNanos = System.nanoTime(); try { final EventPublishResult result = publisher.publish(eventsAsString, eventTypeName); diff --git a/src/main/java/org/zalando/nakadi/controller/ExceptionHandling.java b/src/main/java/org/zalando/nakadi/controller/ExceptionHandling.java index 9d9f9e2a01..2798f17b9e 100644 --- a/src/main/java/org/zalando/nakadi/controller/ExceptionHandling.java +++ b/src/main/java/org/zalando/nakadi/controller/ExceptionHandling.java @@ -11,21 +11,17 @@ import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.context.request.NativeWebRequest; import org.zalando.nakadi.exceptions.runtime.AccessDeniedException; -import org.zalando.nakadi.exceptions.runtime.CompactionException; import org.zalando.nakadi.exceptions.runtime.DbWriteOperationsBlockedException; -import org.zalando.nakadi.exceptions.runtime.EnrichmentException; import org.zalando.nakadi.exceptions.runtime.FeatureNotAvailableException; import org.zalando.nakadi.exceptions.runtime.IllegalClientIdException; import org.zalando.nakadi.exceptions.runtime.InternalNakadiException; import org.zalando.nakadi.exceptions.runtime.InvalidLimitException; -import org.zalando.nakadi.exceptions.runtime.InvalidPartitionKeyFieldsException; import org.zalando.nakadi.exceptions.runtime.InvalidVersionNumberException; import org.zalando.nakadi.exceptions.runtime.LimitReachedException; import org.zalando.nakadi.exceptions.runtime.NakadiBaseException; import org.zalando.nakadi.exceptions.runtime.NakadiRuntimeException; import org.zalando.nakadi.exceptions.runtime.NoSuchEventTypeException; import org.zalando.nakadi.exceptions.runtime.NoSuchSubscriptionException; -import org.zalando.nakadi.exceptions.runtime.PartitioningException; import org.zalando.nakadi.exceptions.runtime.RepositoryProblemException; import org.zalando.nakadi.exceptions.runtime.ServiceTemporarilyUnavailableException; import org.zalando.nakadi.exceptions.runtime.UnprocessableEntityException; @@ -150,35 +146,6 @@ public ResponseEntity handleFeatureNotAvailable( return Responses.create(Problem.valueOf(NOT_IMPLEMENTED, ex.getMessage()), request); } - @ExceptionHandler(CompactionException.class) - public ResponseEntity handleCompactionException(final CompactionException exception, - final NativeWebRequest request) { - LOG.debug(exception.getMessage()); - return Responses.create(UNPROCESSABLE_ENTITY, exception.getMessage(), request); - } - - @ExceptionHandler(EnrichmentException.class) - public ResponseEntity handleEnrichmentException(final EnrichmentException exception, - final NativeWebRequest request) { - LOG.debug(exception.getMessage()); - return Responses.create(UNPROCESSABLE_ENTITY, exception.getMessage(), request); - } - - @ExceptionHandler(PartitioningException.class) - public ResponseEntity handlePartitioningException(final PartitioningException exception, - final NativeWebRequest request) { - LOG.debug(exception.getMessage()); - return Responses.create(UNPROCESSABLE_ENTITY, exception.getMessage(), request); - } - - @ExceptionHandler(InvalidPartitionKeyFieldsException.class) - public ResponseEntity handleInvalidPartitionKeyFieldsException( - final InvalidPartitionKeyFieldsException exception, - final NativeWebRequest request) { - LOG.debug(exception.getMessage()); - return Responses.create(UNPROCESSABLE_ENTITY, exception.getMessage(), request); - } - @ExceptionHandler(NoSuchEventTypeException.class) public ResponseEntity handleNoSuchEventTypeException(final NoSuchEventTypeException exception, final NativeWebRequest request) { diff --git a/src/main/java/org/zalando/nakadi/exceptions/runtime/CompactionException.java b/src/main/java/org/zalando/nakadi/exceptions/runtime/CompactionException.java deleted file mode 100644 index 0555c78036..0000000000 --- a/src/main/java/org/zalando/nakadi/exceptions/runtime/CompactionException.java +++ /dev/null @@ -1,9 +0,0 @@ -package org.zalando.nakadi.exceptions.runtime; - -public class CompactionException extends NakadiBaseException { - - public CompactionException(final String message) { - super(message); - } - -} diff --git a/src/main/java/org/zalando/nakadi/partitioning/PartitionResolver.java b/src/main/java/org/zalando/nakadi/partitioning/PartitionResolver.java index c55e31a09c..b51d595ab5 100644 --- a/src/main/java/org/zalando/nakadi/partitioning/PartitionResolver.java +++ b/src/main/java/org/zalando/nakadi/partitioning/PartitionResolver.java @@ -7,9 +7,9 @@ import org.springframework.stereotype.Component; import org.zalando.nakadi.domain.EventType; import org.zalando.nakadi.domain.EventTypeBase; +import org.zalando.nakadi.exceptions.runtime.InvalidEventTypeException; import org.zalando.nakadi.exceptions.runtime.NoSuchPartitionStrategyException; import org.zalando.nakadi.exceptions.runtime.PartitioningException; -import org.zalando.nakadi.exceptions.runtime.InvalidEventTypeException; import org.zalando.nakadi.service.timeline.TimelineService; import java.util.List; diff --git a/src/main/java/org/zalando/nakadi/service/EventPublisher.java b/src/main/java/org/zalando/nakadi/service/EventPublisher.java index 6a7a87f5ae..1e3bbdd4ce 100644 --- a/src/main/java/org/zalando/nakadi/service/EventPublisher.java +++ b/src/main/java/org/zalando/nakadi/service/EventPublisher.java @@ -16,15 +16,14 @@ import org.zalando.nakadi.domain.EventType; import org.zalando.nakadi.domain.Timeline; import org.zalando.nakadi.enrichment.Enrichment; -import org.zalando.nakadi.exceptions.runtime.CompactionException; -import org.zalando.nakadi.exceptions.runtime.EnrichmentException; -import org.zalando.nakadi.exceptions.runtime.InternalNakadiException; -import org.zalando.nakadi.exceptions.runtime.NoSuchEventTypeException; -import org.zalando.nakadi.exceptions.runtime.PartitioningException; import org.zalando.nakadi.exceptions.runtime.AccessDeniedException; +import org.zalando.nakadi.exceptions.runtime.EnrichmentException; import org.zalando.nakadi.exceptions.runtime.EventPublishingException; import org.zalando.nakadi.exceptions.runtime.EventTypeTimeoutException; import org.zalando.nakadi.exceptions.runtime.EventValidationException; +import org.zalando.nakadi.exceptions.runtime.InternalNakadiException; +import org.zalando.nakadi.exceptions.runtime.NoSuchEventTypeException; +import org.zalando.nakadi.exceptions.runtime.PartitioningException; import org.zalando.nakadi.exceptions.runtime.ServiceTemporarilyUnavailableException; import org.zalando.nakadi.partitioning.PartitionResolver; import org.zalando.nakadi.repository.db.EventTypeCache; @@ -74,9 +73,11 @@ public EventPublisher(final TimelineService timelineService, public EventPublishResult publish(final String events, final String eventTypeName) throws NoSuchEventTypeException, InternalNakadiException, + EnrichmentException, EventTypeTimeoutException, AccessDeniedException, - ServiceTemporarilyUnavailableException { + ServiceTemporarilyUnavailableException, + PartitioningException{ return publishInternal(events, eventTypeName, true); } @@ -84,7 +85,7 @@ EventPublishResult publishInternal(final String events, final String eventTypeName, final boolean useAuthz) throws NoSuchEventTypeException, InternalNakadiException, EventTypeTimeoutException, - AccessDeniedException, ServiceTemporarilyUnavailableException { + AccessDeniedException, ServiceTemporarilyUnavailableException, EnrichmentException, PartitioningException { Closeable publishingCloser = null; final List batch = BatchFactory.from(events); @@ -112,9 +113,6 @@ EventPublishResult publishInternal(final String events, } catch (final PartitioningException e) { LOG.debug("Event partition error: {}", e.getMessage()); return aborted(EventPublishingStep.PARTITIONING, batch); - } catch (final CompactionException e) { - LOG.debug("Event compaction error: {}", e.getMessage()); - return aborted(EventPublishingStep.PARTITIONING, batch); } catch (final EnrichmentException e) { LOG.debug("Event enrichment error: {}", e.getMessage()); return aborted(EventPublishingStep.ENRICHING, batch); @@ -157,7 +155,8 @@ private List responses(final List batch) { .collect(Collectors.toList()); } - private void partition(final List batch, final EventType eventType) throws PartitioningException { + private void partition(final List batch, final EventType eventType) + throws PartitioningException { for (final BatchItem item : batch) { item.setStep(EventPublishingStep.PARTITIONING); try { @@ -170,7 +169,7 @@ private void partition(final List batch, final EventType eventType) t } } - private void compact(final List batch, final EventType eventType) throws CompactionException { + private void compact(final List batch, final EventType eventType) { if (eventType.getCleanupPolicy() == CleanupPolicy.COMPACT) { for (final BatchItem item : batch) { final String compactionKey = item.getEvent() From c61ab82c5dd60ca599018b51dfb42df06d5945f7 Mon Sep 17 00:00:00 2001 From: Lionel Montrieux Date: Thu, 4 Oct 2018 13:06:57 +0200 Subject: [PATCH 11/11] Group exception handlers when appropriate --- .../controller/EventPublishingController.java | 24 ++----- .../controller/EventTypeController.java | 64 +++++-------------- .../nakadi/controller/ExceptionHandling.java | 64 ++++++------------- .../controller/SubscriptionController.java | 22 ++----- .../EventTypeAuthorizationTest.java | 2 +- .../controller/ExceptionHandlingTest.java | 2 +- 6 files changed, 52 insertions(+), 126 deletions(-) diff --git a/src/main/java/org/zalando/nakadi/controller/EventPublishingController.java b/src/main/java/org/zalando/nakadi/controller/EventPublishingController.java index 1bec3318c8..ac6ef46382 100644 --- a/src/main/java/org/zalando/nakadi/controller/EventPublishingController.java +++ b/src/main/java/org/zalando/nakadi/controller/EventPublishingController.java @@ -22,6 +22,7 @@ import org.zalando.nakadi.exceptions.runtime.EventTypeTimeoutException; import org.zalando.nakadi.exceptions.runtime.InternalNakadiException; import org.zalando.nakadi.exceptions.runtime.InvalidPartitionKeyFieldsException; +import org.zalando.nakadi.exceptions.runtime.NakadiBaseException; import org.zalando.nakadi.exceptions.runtime.NoSuchEventTypeException; import org.zalando.nakadi.exceptions.runtime.PartitioningException; import org.zalando.nakadi.exceptions.runtime.ServiceTemporarilyUnavailableException; @@ -98,24 +99,11 @@ public ResponseEntity postEvent(@PathVariable final String eventTypeName, } } - @ExceptionHandler(EnrichmentException.class) - public ResponseEntity handleEnrichmentException(final EnrichmentException exception, - final NativeWebRequest request) { - LOG.debug(exception.getMessage()); - return Responses.create(UNPROCESSABLE_ENTITY, exception.getMessage(), request); - } - - @ExceptionHandler(PartitioningException.class) - public ResponseEntity handlePartitioningException(final PartitioningException exception, - final NativeWebRequest request) { - LOG.debug(exception.getMessage()); - return Responses.create(UNPROCESSABLE_ENTITY, exception.getMessage(), request); - } - - @ExceptionHandler(InvalidPartitionKeyFieldsException.class) - public ResponseEntity handleInvalidPartitionKeyFieldsException( - final InvalidPartitionKeyFieldsException exception, - final NativeWebRequest request) { + @ExceptionHandler({EnrichmentException.class, + PartitioningException.class, + InvalidPartitionKeyFieldsException.class}) + public ResponseEntity handleUnprocessableEntityResponses(final NakadiBaseException exception, + final NativeWebRequest request) { LOG.debug(exception.getMessage()); return Responses.create(UNPROCESSABLE_ENTITY, exception.getMessage(), request); } diff --git a/src/main/java/org/zalando/nakadi/controller/EventTypeController.java b/src/main/java/org/zalando/nakadi/controller/EventTypeController.java index 2a9bc0d633..155de667b1 100644 --- a/src/main/java/org/zalando/nakadi/controller/EventTypeController.java +++ b/src/main/java/org/zalando/nakadi/controller/EventTypeController.java @@ -28,6 +28,7 @@ import org.zalando.nakadi.exceptions.runtime.InconsistentStateException; import org.zalando.nakadi.exceptions.runtime.InternalNakadiException; import org.zalando.nakadi.exceptions.runtime.InvalidEventTypeException; +import org.zalando.nakadi.exceptions.runtime.NakadiBaseException; import org.zalando.nakadi.exceptions.runtime.NakadiRuntimeException; import org.zalando.nakadi.exceptions.runtime.NoSuchEventTypeException; import org.zalando.nakadi.exceptions.runtime.NoSuchPartitionStrategyException; @@ -170,6 +171,16 @@ private HttpHeaders generateWarningHeaders(final EventTypeBase eventType) { return headers; } + @ExceptionHandler({UnableProcessException.class, + NoSuchPartitionStrategyException.class, + InvalidEventTypeException.class, + EventTypeOptionsValidationException.class}) + public ResponseEntity handleUnprocessableEntityResponses(final NakadiBaseException exception, + final NativeWebRequest request) { + LOG.debug(exception.getMessage(), exception); + return Responses.create(UNPROCESSABLE_ENTITY, exception.getMessage(), request); + } + @ExceptionHandler(EventTypeDeletionException.class) public ResponseEntity deletion(final EventTypeDeletionException exception, final NativeWebRequest request) { @@ -177,58 +188,17 @@ public ResponseEntity deletion(final EventTypeDeletionException excepti return Responses.create(Response.Status.INTERNAL_SERVER_ERROR, exception.getMessage(), request); } - @ExceptionHandler(UnableProcessException.class) - public ResponseEntity unableProcess(final UnableProcessException exception, - final NativeWebRequest request) { - LOG.debug(exception.getMessage(), exception); - return Responses.create(UNPROCESSABLE_ENTITY, exception.getMessage(), request); - } - - @ExceptionHandler(ConflictException.class) - public ResponseEntity conflict(final ConflictException exception, final NativeWebRequest request) { + @ExceptionHandler({ConflictException.class, DuplicatedEventTypeNameException.class}) + public ResponseEntity handleConflictResponses(final NakadiBaseException exception, + final NativeWebRequest request) { LOG.debug(exception.getMessage(), exception); return Responses.create(Response.Status.CONFLICT, exception.getMessage(), request); } - @ExceptionHandler(EventTypeUnavailableException.class) - public ResponseEntity eventTypeUnavailable(final EventTypeUnavailableException exception, - final NativeWebRequest request) { - LOG.debug(exception.getMessage(), exception); - return Responses.create(Response.Status.SERVICE_UNAVAILABLE, exception.getMessage(), request); - } - - @ExceptionHandler(NoSuchPartitionStrategyException.class) - public ResponseEntity noSuchPartitionStrategyException(final NoSuchPartitionStrategyException exception, - final NativeWebRequest request) { + @ExceptionHandler({EventTypeUnavailableException.class, TopicCreationException.class}) + public ResponseEntity handleServiceUnavailableResponses(final NakadiBaseException exception, + final NativeWebRequest request) { LOG.debug(exception.getMessage(), exception); - return Responses.create(Problem.valueOf(UNPROCESSABLE_ENTITY, exception.getMessage()), request); - } - - @ExceptionHandler(DuplicatedEventTypeNameException.class) - public ResponseEntity duplicatedEventTypeNameException(final DuplicatedEventTypeNameException exception, - final NativeWebRequest request) { - LOG.debug(exception.getMessage(), exception); - return Responses.create(Problem.valueOf(Response.Status.CONFLICT, exception.getMessage()), request); - } - - @ExceptionHandler(InvalidEventTypeException.class) - public ResponseEntity invalidEventTypeException(final InvalidEventTypeException exception, - final NativeWebRequest request) { - LOG.debug(exception.getMessage(), exception); - return Responses.create(Problem.valueOf(UNPROCESSABLE_ENTITY, exception.getMessage()), request); - } - - @ExceptionHandler(EventTypeOptionsValidationException.class) - public ResponseEntity unableProcess(final EventTypeOptionsValidationException exception, - final NativeWebRequest request) { - LOG.debug(exception.getMessage(), exception); - return Responses.create(UNPROCESSABLE_ENTITY, exception.getMessage(), request); - } - - @ExceptionHandler(TopicCreationException.class) - public ResponseEntity handleTopicCreationException(final TopicCreationException exception, - final NativeWebRequest request) { - LOG.error(exception.getMessage(), exception); return Responses.create(Response.Status.SERVICE_UNAVAILABLE, exception.getMessage(), request); } } diff --git a/src/main/java/org/zalando/nakadi/controller/ExceptionHandling.java b/src/main/java/org/zalando/nakadi/controller/ExceptionHandling.java index 2798f17b9e..da288f2ff9 100644 --- a/src/main/java/org/zalando/nakadi/controller/ExceptionHandling.java +++ b/src/main/java/org/zalando/nakadi/controller/ExceptionHandling.java @@ -33,9 +33,11 @@ import javax.ws.rs.core.Response; import static javax.ws.rs.core.Response.Status.BAD_REQUEST; +import static javax.ws.rs.core.Response.Status.FORBIDDEN; import static javax.ws.rs.core.Response.Status.INTERNAL_SERVER_ERROR; import static javax.ws.rs.core.Response.Status.NOT_FOUND; import static javax.ws.rs.core.Response.Status.NOT_IMPLEMENTED; +import static javax.ws.rs.core.Response.Status.SERVICE_UNAVAILABLE; import static org.zalando.problem.MoreStatus.UNPROCESSABLE_ENTITY; @@ -81,15 +83,21 @@ class and stacktrace like information. } @ExceptionHandler(AccessDeniedException.class) - public ResponseEntity accessDeniedException(final AccessDeniedException exception, - final NativeWebRequest request) { - return Responses.create(Response.Status.FORBIDDEN, exception.explain(), request); + public ResponseEntity handleAccessDeniedException(final AccessDeniedException exception, + final NativeWebRequest request) { + return Responses.create(FORBIDDEN, exception.explain(), request); } - @ExceptionHandler(IllegalClientIdException.class) - public ResponseEntity handleIllegalClientIdException(final IllegalClientIdException exception, - final NativeWebRequest request) { - return Responses.create(Response.Status.FORBIDDEN, exception.getMessage(), request); + public ResponseEntity handleForbiddenRequests(final NakadiBaseException exception, + final NativeWebRequest request) { + return Responses.create(FORBIDDEN, exception.getMessage(), request); + } + + @ExceptionHandler({RepositoryProblemException.class, ServiceTemporarilyUnavailableException.class}) + public ResponseEntity handleServiceUnavailableResponses(final NakadiBaseException exception, + final NativeWebRequest request) { + LOG.error(exception.getMessage(), exception); + return Responses.create(SERVICE_UNAVAILABLE, exception.getMessage(), request); } @ExceptionHandler @@ -102,13 +110,6 @@ public ResponseEntity handleExceptionWrapper(final NakadiRuntimeExcepti throw exception.getException(); } - @ExceptionHandler(RepositoryProblemException.class) - public ResponseEntity handleRepositoryProblem(final RepositoryProblemException exception, - final NativeWebRequest request) { - LOG.error("Repository problem occurred", exception); - return Responses.create(Response.Status.SERVICE_UNAVAILABLE, exception.getMessage(), request); - } - @ExceptionHandler(NakadiBaseException.class) public ResponseEntity handleInternalError(final NakadiBaseException exception, final NativeWebRequest request) { @@ -116,13 +117,6 @@ public ResponseEntity handleInternalError(final NakadiBaseException exc return Responses.create(Response.Status.INTERNAL_SERVER_ERROR, exception.getMessage(), request); } - @ExceptionHandler(ServiceTemporarilyUnavailableException.class) - public ResponseEntity handleServiceTemporarilyUnavailableException( - final ServiceTemporarilyUnavailableException exception, final NativeWebRequest request) { - LOG.error(exception.getMessage(), exception); - return Responses.create(Response.Status.SERVICE_UNAVAILABLE, exception.getMessage(), request); - } - @ExceptionHandler(LimitReachedException.class) public ResponseEntity handleLimitReachedException( final ServiceTemporarilyUnavailableException exception, final NativeWebRequest request) { @@ -146,32 +140,16 @@ public ResponseEntity handleFeatureNotAvailable( return Responses.create(Problem.valueOf(NOT_IMPLEMENTED, ex.getMessage()), request); } - @ExceptionHandler(NoSuchEventTypeException.class) - public ResponseEntity handleNoSuchEventTypeException(final NoSuchEventTypeException exception, - final NativeWebRequest request) { - LOG.debug(exception.getMessage()); - return Responses.create(NOT_FOUND, exception.getMessage(), request); - } - - @ExceptionHandler(NoSuchSubscriptionException.class) - public ResponseEntity handleNoSuchSubscriptionException(final NoSuchSubscriptionException exception, - final NativeWebRequest request) { + @ExceptionHandler({NoSuchEventTypeException.class, NoSuchSubscriptionException.class}) + public ResponseEntity handleNotFoundRequests(final NakadiBaseException exception, + final NativeWebRequest request) { LOG.debug(exception.getMessage()); return Responses.create(NOT_FOUND, exception.getMessage(), request); } - @ExceptionHandler(InvalidLimitException.class) - public ResponseEntity handleInvalidLimitException( - final InvalidLimitException exception, - final NativeWebRequest request) { - LOG.debug(exception.getMessage()); - return Responses.create(BAD_REQUEST, exception.getMessage(), request); - } - - @ExceptionHandler(InvalidVersionNumberException.class) - public ResponseEntity handleInvalidVersionNumberException( - final InvalidVersionNumberException exception, - final NativeWebRequest request) { + @ExceptionHandler({InvalidLimitException.class, InvalidVersionNumberException.class}) + public ResponseEntity handleBadRequests(final NakadiBaseException exception, + final NativeWebRequest request) { LOG.debug(exception.getMessage()); return Responses.create(BAD_REQUEST, exception.getMessage(), request); } diff --git a/src/main/java/org/zalando/nakadi/controller/SubscriptionController.java b/src/main/java/org/zalando/nakadi/controller/SubscriptionController.java index 3cba0a0db0..4e87708420 100644 --- a/src/main/java/org/zalando/nakadi/controller/SubscriptionController.java +++ b/src/main/java/org/zalando/nakadi/controller/SubscriptionController.java @@ -19,6 +19,7 @@ import org.zalando.nakadi.exceptions.runtime.InconsistentStateException; import org.zalando.nakadi.exceptions.runtime.InternalNakadiException; import org.zalando.nakadi.exceptions.runtime.InvalidLimitException; +import org.zalando.nakadi.exceptions.runtime.NakadiBaseException; import org.zalando.nakadi.exceptions.runtime.NoSuchEventTypeException; import org.zalando.nakadi.exceptions.runtime.NoSuchSubscriptionException; import org.zalando.nakadi.exceptions.runtime.ServiceTemporarilyUnavailableException; @@ -111,25 +112,14 @@ public ResponseEntity handleTimeLagException(final ErrorGettingCursorTi return Responses.create(Problem.valueOf(UNPROCESSABLE_ENTITY, ex.getMessage()), request); } - @ExceptionHandler(InconsistentStateException.class) - public ResponseEntity handleInconsistentState(final InconsistentStateException ex, - final NativeWebRequest request) { - LOG.debug(ex.getMessage(), ex); - return Responses.create( - Problem.valueOf( - SERVICE_UNAVAILABLE, - ex.getMessage()), - request); - } - - @ExceptionHandler(ServiceTemporarilyUnavailableException.class) - public ResponseEntity handleServiceTemporarilyUnavailable(final ServiceTemporarilyUnavailableException ex, - final NativeWebRequest request) { - LOG.debug(ex.getMessage(), ex); + @ExceptionHandler({InconsistentStateException.class, ServiceTemporarilyUnavailableException.class}) + public ResponseEntity handleServiceUnavailableResponses(final NakadiBaseException exception, + final NativeWebRequest request) { + LOG.debug(exception.getMessage(), exception); return Responses.create( Problem.valueOf( SERVICE_UNAVAILABLE, - ex.getMessage()), + exception.getMessage()), request); } diff --git a/src/test/java/org/zalando/nakadi/controller/EventTypeAuthorizationTest.java b/src/test/java/org/zalando/nakadi/controller/EventTypeAuthorizationTest.java index 00a10cec0b..7c964714c1 100644 --- a/src/test/java/org/zalando/nakadi/controller/EventTypeAuthorizationTest.java +++ b/src/test/java/org/zalando/nakadi/controller/EventTypeAuthorizationTest.java @@ -95,7 +95,7 @@ public void whenPUTNullAuthorizationForExistingAuthorization() throws Exception } @Test - public void whenDELETENotAuthorized200() throws Exception { + public void whenDELETENotAuthorizedThen403() throws Exception { final EventType eventType = EventTypeTestBuilder.builder().build(); final Resource resource = eventType.asResource(); diff --git a/src/test/java/org/zalando/nakadi/controller/ExceptionHandlingTest.java b/src/test/java/org/zalando/nakadi/controller/ExceptionHandlingTest.java index bcc9c9169f..7ea059c9b1 100644 --- a/src/test/java/org/zalando/nakadi/controller/ExceptionHandlingTest.java +++ b/src/test/java/org/zalando/nakadi/controller/ExceptionHandlingTest.java @@ -20,7 +20,7 @@ public void testIllegalClientIdException() { final NativeWebRequest mockedRequest = Mockito.mock(NativeWebRequest.class); Mockito.when(mockedRequest.getHeader(Matchers.any())).thenReturn(""); - final ResponseEntity problemResponseEntity = exceptionHandling.handleIllegalClientIdException( + final ResponseEntity problemResponseEntity = exceptionHandling.handleForbiddenRequests( new IllegalClientIdException("You don't have access to this event type"), mockedRequest); Assert.assertEquals(problemResponseEntity.getStatusCode(), HttpStatus.FORBIDDEN);