From 9190141fb110b5f79a5849b41fd441caa18d574b Mon Sep 17 00:00:00 2001 From: amckenzie Date: Tue, 17 Dec 2019 15:18:53 +0000 Subject: [PATCH] Make stream_id and event_number in event_error_log nullable --- CHANGELOG.md | 2 +- .../errors/DefaultSystemErrorService.java | 24 ++--- .../errors/MissingStreamIdException.java | 8 ++ .../errors/DefaultSystemErrorServiceTest.java | 74 ++-------------- .../004-add-event-error-log-table.xml | 12 +-- .../services/system/domain/EventError.java | 47 +++++----- .../persistence/EventErrorLogRepository.java | 58 +++++++++---- .../EventErrorLogRepositoryIT.java | 87 +++++++++++++++++-- 8 files changed, 173 insertions(+), 139 deletions(-) create mode 100644 framework-system/framework-system-errors/src/main/java/uk/gov/justice/services/framework/system/errors/MissingStreamIdException.java diff --git a/CHANGELOG.md b/CHANGELOG.md index b1e98f9d8..fcce3d857 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ on [Keep a CHANGELOG](http://keepachangelog.com/). This project adheres to ## [Unreleased] -## [6.5.0-M4] - 2019-12-17 +## [6.5.0-M5] - 2019-12-17 - Update framework-api to 4.2.1 - Add table in system database 'event_error_log' for storing errors with events that failed to process - New SystemErrorService for reporting system errors diff --git a/framework-system/framework-system-errors/src/main/java/uk/gov/justice/services/framework/system/errors/DefaultSystemErrorService.java b/framework-system/framework-system-errors/src/main/java/uk/gov/justice/services/framework/system/errors/DefaultSystemErrorService.java index 591dd75bf..44b46f105 100644 --- a/framework-system/framework-system-errors/src/main/java/uk/gov/justice/services/framework/system/errors/DefaultSystemErrorService.java +++ b/framework-system/framework-system-errors/src/main/java/uk/gov/justice/services/framework/system/errors/DefaultSystemErrorService.java @@ -1,17 +1,14 @@ package uk.gov.justice.services.framework.system.errors; -import static java.lang.String.format; import static javax.transaction.Transactional.TxType.NOT_SUPPORTED; import uk.gov.justice.services.common.util.UtcClock; -import uk.gov.justice.services.framework.system.errors.SystemErrorService; import uk.gov.justice.services.framework.utilities.exceptions.StackTraceProvider; import uk.gov.justice.services.messaging.JsonEnvelope; import uk.gov.justice.services.messaging.Metadata; import uk.gov.justice.services.system.domain.EventError; import uk.gov.justice.services.system.persistence.EventErrorLogRepository; -import java.time.Clock; import java.util.Optional; import java.util.UUID; @@ -20,9 +17,6 @@ public class DefaultSystemErrorService implements SystemErrorService { - private static final Long MISSING_EVENT_NUMBER = -1L; - private static final String NO_COMMENT = ""; - @Inject private EventErrorLogRepository eventErrorLogRepository; @@ -44,30 +38,22 @@ public void reportError( final UUID eventId = metadata.id(); final String eventName = metadata.name(); final Optional eventNumber = metadata.eventNumber(); + final Optional streamId = metadata.streamId(); final EventError eventError = new EventError( messageId, componentName, - eventId, eventName, - eventNumber.orElse(MISSING_EVENT_NUMBER), + eventId, + streamId, + eventNumber, metadata.asJsonObject().toString(), jsonEnvelope.payload().toString(), exception.getMessage(), stackTraceProvider.getStackTrace(exception), - clock.now(), - getComment(eventNumber) + clock.now() ); eventErrorLogRepository.save(eventError); } - - private String getComment(final Optional eventNumberOptional) { - - if (eventNumberOptional.isPresent()) { - return NO_COMMENT; - } - - return format("Event number is missing from event. Setting to %d instead", MISSING_EVENT_NUMBER); - } } diff --git a/framework-system/framework-system-errors/src/main/java/uk/gov/justice/services/framework/system/errors/MissingStreamIdException.java b/framework-system/framework-system-errors/src/main/java/uk/gov/justice/services/framework/system/errors/MissingStreamIdException.java new file mode 100644 index 000000000..513dde859 --- /dev/null +++ b/framework-system/framework-system-errors/src/main/java/uk/gov/justice/services/framework/system/errors/MissingStreamIdException.java @@ -0,0 +1,8 @@ +package uk.gov.justice.services.framework.system.errors; + +public class MissingStreamIdException extends RuntimeException { + + public MissingStreamIdException(final String message) { + super(message); + } +} diff --git a/framework-system/framework-system-errors/src/test/java/uk/gov/justice/services/framework/system/errors/DefaultSystemErrorServiceTest.java b/framework-system/framework-system-errors/src/test/java/uk/gov/justice/services/framework/system/errors/DefaultSystemErrorServiceTest.java index 88fb62454..bd6162444 100644 --- a/framework-system/framework-system-errors/src/test/java/uk/gov/justice/services/framework/system/errors/DefaultSystemErrorServiceTest.java +++ b/framework-system/framework-system-errors/src/test/java/uk/gov/justice/services/framework/system/errors/DefaultSystemErrorServiceTest.java @@ -1,10 +1,8 @@ package uk.gov.justice.services.framework.system.errors; -import static java.util.Optional.empty; import static java.util.Optional.of; import static java.util.UUID.randomUUID; import static org.hamcrest.CoreMatchers.is; -import static org.hamcrest.CoreMatchers.nullValue; import static org.junit.Assert.assertThat; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; @@ -18,6 +16,7 @@ import uk.gov.justice.services.system.persistence.EventErrorLogRepository; import java.time.ZonedDateTime; +import java.util.Optional; import java.util.UUID; import javax.json.JsonObject; @@ -34,8 +33,6 @@ @RunWith(MockitoJUnitRunner.class) public class DefaultSystemErrorServiceTest { - private static final String AN_EMPTY_STRING = ""; - @Mock private EventErrorLogRepository eventErrorLogRepository; @@ -59,8 +56,9 @@ public void shouldCreateEventErrorAndPersist() throws Exception { final String eventName = "context.events.an.event"; final String errorMessage = "Help help we're all going to die"; final Throwable exception = new NullPointerException(errorMessage); - final Long eventNumber = 239874L; + final Optional eventNumber = of(239874L); final UUID eventId = randomUUID(); + final Optional streamId = of(randomUUID()); final String metadataJson = "{metadata: stuff}"; final String payloadJson = "{payload: stuff}"; final String stackTrace = "the stacktrace"; @@ -72,66 +70,10 @@ public void shouldCreateEventErrorAndPersist() throws Exception { final JsonValue payload = mock(JsonValue.class); when(jsonEnvelope.metadata()).thenReturn(metadata); - when(metadata.id()).thenReturn(eventId); when(metadata.name()).thenReturn(eventName); - when(metadata.eventNumber()).thenReturn(of(eventNumber)); - - when(metadata.asJsonObject()).thenReturn(metadataJsonObject); - when(jsonEnvelope.payload()).thenReturn(payload); - - when(metadataJsonObject.toString()).thenReturn(metadataJson); - when(payload.toString()).thenReturn(payloadJson); - when(stackTraceProvider.getStackTrace(exception)).thenReturn(stackTrace); - - when(clock.now()).thenReturn(erroredAt); - - defaultSystemErrorService.reportError( - messageId, - componentName, - jsonEnvelope, - exception - ); - - verify(eventErrorLogRepository).save(eventErrorCaptor.capture()); - - final EventError eventError = eventErrorCaptor.getValue(); - - assertThat(eventError.getMessageId(), is(messageId)); - assertThat(eventError.getComponent(), is(componentName)); - assertThat(eventError.getEventId(), is(eventId)); - assertThat(eventError.getEventName(), is(eventName)); - assertThat(eventError.getEventNumber(), is(eventNumber)); - assertThat(eventError.getMetadata(), is(metadataJson)); - assertThat(eventError.getPayload(), is(payloadJson)); - assertThat(eventError.getErrorMessage(), is(errorMessage)); - assertThat(eventError.getStacktrace(), is(stackTrace)); - assertThat(eventError.getErroredAt(), is(erroredAt)); - assertThat(eventError.getComments(), is(AN_EMPTY_STRING)); - } - - @Test - public void shouldHandleMissingEventNumber() throws Exception { - - final String messageId = "message id"; - final String componentName = "EVENT_LISTENER"; - final String eventName = "context.events.an.event"; - final String errorMessage = "Help help we're all going to die"; - final Throwable exception = new NullPointerException(errorMessage); - final UUID eventId = randomUUID(); - final String metadataJson = "{metadata: stuff}"; - final String payloadJson = "{payload: stuff}"; - final String stackTrace = "the stacktrace"; - final ZonedDateTime erroredAt = new UtcClock().now(); - - final JsonEnvelope jsonEnvelope = mock(JsonEnvelope.class); - final Metadata metadata = mock(Metadata.class); - final JsonObject metadataJsonObject = mock(JsonObject.class); - final JsonValue payload = mock(JsonValue.class); - - when(jsonEnvelope.metadata()).thenReturn(metadata); when(metadata.id()).thenReturn(eventId); - when(metadata.name()).thenReturn(eventName); - when(metadata.eventNumber()).thenReturn(empty()); + when(metadata.streamId()).thenReturn(streamId); + when(metadata.eventNumber()).thenReturn(eventNumber); when(metadata.asJsonObject()).thenReturn(metadataJsonObject); when(jsonEnvelope.payload()).thenReturn(payload); @@ -156,13 +98,13 @@ public void shouldHandleMissingEventNumber() throws Exception { assertThat(eventError.getMessageId(), is(messageId)); assertThat(eventError.getComponent(), is(componentName)); assertThat(eventError.getEventName(), is(eventName)); - assertThat(eventError.getErrorMessage(), is(errorMessage)); - assertThat(eventError.getEventNumber(), is(-1L)); assertThat(eventError.getEventId(), is(eventId)); + assertThat(eventError.getStreamId(), is(streamId)); + assertThat(eventError.getEventNumber(), is(eventNumber)); assertThat(eventError.getMetadata(), is(metadataJson)); assertThat(eventError.getPayload(), is(payloadJson)); + assertThat(eventError.getErrorMessage(), is(errorMessage)); assertThat(eventError.getStacktrace(), is(stackTrace)); assertThat(eventError.getErroredAt(), is(erroredAt)); - assertThat(eventError.getComments(), is("Event number is missing from event. Setting to -1 instead")); } } diff --git a/framework-system/framework-system-liquibase/src/main/resources/liquibase/framework-system-changesets/004-add-event-error-log-table.xml b/framework-system/framework-system-liquibase/src/main/resources/liquibase/framework-system-changesets/004-add-event-error-log-table.xml index 642cc212c..83e004d3b 100644 --- a/framework-system/framework-system-liquibase/src/main/resources/liquibase/framework-system-changesets/004-add-event-error-log-table.xml +++ b/framework-system/framework-system-liquibase/src/main/resources/liquibase/framework-system-changesets/004-add-event-error-log-table.xml @@ -16,14 +16,17 @@ - + - + + + + - + @@ -40,9 +43,6 @@ - - - diff --git a/framework-system/framework-system-persistence/src/main/java/uk/gov/justice/services/system/domain/EventError.java b/framework-system/framework-system-persistence/src/main/java/uk/gov/justice/services/system/domain/EventError.java index 7b20f73fd..ee303659e 100644 --- a/framework-system/framework-system-persistence/src/main/java/uk/gov/justice/services/system/domain/EventError.java +++ b/framework-system/framework-system-persistence/src/main/java/uk/gov/justice/services/system/domain/EventError.java @@ -2,45 +2,46 @@ import java.time.ZonedDateTime; import java.util.Objects; +import java.util.Optional; import java.util.UUID; public class EventError { private final String messageId; private final String component; - private final UUID eventId; private final String eventName; - private final Long eventNumber; + private final UUID eventId; + private final Optional streamId; + private final Optional eventNumber; private final String metadata; private final String payload; private final String errorMessage; private final String stacktrace; private final ZonedDateTime erroredAt; - private final String comments; public EventError( final String messageId, final String component, - final UUID eventId, final String eventName, - final Long eventNumber, + final UUID eventId, + final Optional streamId, + final Optional eventNumber, final String metadata, final String payload, final String errorMessage, final String stacktrace, - final ZonedDateTime erroredAt, - final String comments) { + final ZonedDateTime erroredAt) { this.messageId = messageId; this.component = component; - this.eventId = eventId; this.eventName = eventName; + this.eventId = eventId; + this.streamId = streamId; this.eventNumber = eventNumber; this.metadata = metadata; this.payload = payload; this.errorMessage = errorMessage; this.stacktrace = stacktrace; this.erroredAt = erroredAt; - this.comments = comments; } public String getMessageId() { @@ -51,15 +52,19 @@ public String getComponent() { return component; } + public String getEventName() { + return eventName; + } + public UUID getEventId() { return eventId; } - public String getEventName() { - return eventName; + public Optional getStreamId() { + return streamId; } - public Long getEventNumber() { + public Optional getEventNumber() { return eventNumber; } @@ -83,10 +88,6 @@ public ZonedDateTime getErroredAt() { return erroredAt; } - public String getComments() { - return comments; - } - @Override public boolean equals(final Object o) { if (this == o) return true; @@ -94,19 +95,20 @@ public boolean equals(final Object o) { final EventError that = (EventError) o; return Objects.equals(messageId, that.messageId) && Objects.equals(component, that.component) && - Objects.equals(eventId, that.eventId) && Objects.equals(eventName, that.eventName) && + Objects.equals(eventId, that.eventId) && + Objects.equals(streamId, that.streamId) && Objects.equals(eventNumber, that.eventNumber) && Objects.equals(metadata, that.metadata) && Objects.equals(payload, that.payload) && Objects.equals(errorMessage, that.errorMessage) && - Objects.equals(erroredAt, that.erroredAt) && - Objects.equals(comments, that.comments); + Objects.equals(stacktrace, that.stacktrace) && + Objects.equals(erroredAt, that.erroredAt); } @Override public int hashCode() { - return Objects.hash(messageId, component, eventId, eventName, eventNumber, metadata, payload, errorMessage, stacktrace, erroredAt, comments); + return Objects.hash(messageId, component, eventName, eventId, streamId, eventNumber, metadata, payload, errorMessage, stacktrace, erroredAt); } @Override @@ -114,14 +116,13 @@ public String toString() { return "EventError{" + "messageId='" + messageId + '\'' + ", component='" + component + '\'' + - ", eventId=" + eventId + ", eventName='" + eventName + '\'' + + ", eventId=" + eventId + + ", streamId=" + streamId + ", eventNumber=" + eventNumber + ", metadata='" + metadata + '\'' + - ", payload='" + payload + '\'' + ", errorMessage='" + errorMessage + '\'' + ", erroredAt=" + erroredAt + - ", comments='" + comments + '\'' + '}'; } } diff --git a/framework-system/framework-system-persistence/src/main/java/uk/gov/justice/services/system/persistence/EventErrorLogRepository.java b/framework-system/framework-system-persistence/src/main/java/uk/gov/justice/services/system/persistence/EventErrorLogRepository.java index 4ab7b1f61..0fe964d8f 100644 --- a/framework-system/framework-system-persistence/src/main/java/uk/gov/justice/services/system/persistence/EventErrorLogRepository.java +++ b/framework-system/framework-system-persistence/src/main/java/uk/gov/justice/services/system/persistence/EventErrorLogRepository.java @@ -1,5 +1,10 @@ package uk.gov.justice.services.system.persistence; +import static java.sql.Types.BIGINT; +import static java.sql.Types.OTHER; +import static java.util.Optional.empty; +import static java.util.Optional.of; +import static java.util.Optional.ofNullable; import static javax.transaction.Transactional.TxType.REQUIRES_NEW; import static uk.gov.justice.services.common.converter.ZonedDateTimes.fromSqlTimestamp; import static uk.gov.justice.services.common.converter.ZonedDateTimes.toSqlTimestamp; @@ -11,9 +16,11 @@ import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.SQLException; +import java.sql.Types; import java.time.ZonedDateTime; import java.util.ArrayList; import java.util.List; +import java.util.Optional; import java.util.UUID; import javax.inject.Inject; @@ -23,13 +30,13 @@ public class EventErrorLogRepository { private static final String INSERT_EVENT_ERROR_LOG_SQL = "INSERT INTO event_error_log " + - "(message_id, component, event_name, event_id, event_number, " + - "metadata, payload, error_message, stacktrace, errored_at, comments) " + + "(message_id, component, event_name, event_id, stream_id, event_number, " + + "metadata, payload, error_message, stacktrace, errored_at) " + "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)"; private static final String FIND_ALL_EVENT_ERROR_LOG_SQL = "SELECT " + - "message_id, component, event_name, event_id, event_number, " + - "metadata, payload, error_message, stacktrace, errored_at, comments " + + "message_id, component, event_name, event_id, stream_id, event_number, " + + "metadata, payload, error_message, stacktrace, errored_at " + "FROM event_error_log"; private static final String TRUNCATE_EVENT_ERROR_LOG_SQL = "TRUNCATE event_error_log CASCADE"; @@ -49,13 +56,25 @@ public void save(final EventError eventError) { preparedStatement.setString(2, eventError.getComponent()); preparedStatement.setString(3, eventError.getEventName()); preparedStatement.setObject(4, eventError.getEventId()); - preparedStatement.setLong(5, eventError.getEventNumber()); - preparedStatement.setString(6, eventError.getMetadata()); - preparedStatement.setString(7, eventError.getPayload()); - preparedStatement.setString(8, eventError.getErrorMessage()); - preparedStatement.setString(9, eventError.getStacktrace()); - preparedStatement.setTimestamp(10, toSqlTimestamp(eventError.getErroredAt())); - preparedStatement.setString(11, eventError.getComments()); + final Optional streamId = eventError.getStreamId(); + + if (streamId.isPresent()) { + preparedStatement.setObject(5, streamId.get()); + } else { + preparedStatement.setNull(5, OTHER); + } + final Optional eventNumber = eventError.getEventNumber(); + + if (eventNumber.isPresent()) { + preparedStatement.setLong(6, eventNumber.get()); + } else { + preparedStatement.setNull(6, BIGINT); + } + preparedStatement.setString(7, eventError.getMetadata()); + preparedStatement.setString(8, eventError.getPayload()); + preparedStatement.setString(9, eventError.getErrorMessage()); + preparedStatement.setString(10, eventError.getStacktrace()); + preparedStatement.setTimestamp(11, toSqlTimestamp(eventError.getErroredAt())); preparedStatement.executeUpdate(); } catch (final SQLException e) { throw new SystemPersistenceException("Failed to insert into event_error_log", e); @@ -76,26 +95,33 @@ public List findAll() { final String component = resultSet.getString("component"); final String eventName = resultSet.getString("event_name"); final UUID eventId = (UUID) resultSet.getObject("event_id"); + final UUID streamId = (UUID) resultSet.getObject("stream_id"); final long eventNumber = resultSet.getLong("event_number"); final String metadata = resultSet.getString("metadata"); final String payload = resultSet.getString("payload"); final String errorMessage = resultSet.getString("error_message"); final String stacktrace = resultSet.getString("stacktrace"); final ZonedDateTime erroredAt = fromSqlTimestamp(resultSet.getTimestamp("errored_at")); - final String comment = resultSet.getString("comments"); + + final Optional eventNumberOptional; + if (eventNumber == 0) { + eventNumberOptional = empty(); + } else { + eventNumberOptional = of(eventNumber); + } final EventError eventError = new EventError( messageId, component, - eventId, eventName, - eventNumber, + eventId, + ofNullable(streamId), + eventNumberOptional, metadata, payload, errorMessage, stacktrace, - erroredAt, - comment + erroredAt ); eventErrors.add(eventError); diff --git a/framework-system/framework-system-persistence/src/test/java/uk/gov/justice/services/system/persistence/EventErrorLogRepositoryIT.java b/framework-system/framework-system-persistence/src/test/java/uk/gov/justice/services/system/persistence/EventErrorLogRepositoryIT.java index be1899f3b..767c1cdb2 100644 --- a/framework-system/framework-system-persistence/src/test/java/uk/gov/justice/services/system/persistence/EventErrorLogRepositoryIT.java +++ b/framework-system/framework-system-persistence/src/test/java/uk/gov/justice/services/system/persistence/EventErrorLogRepositoryIT.java @@ -1,5 +1,7 @@ package uk.gov.justice.services.system.persistence; +import static java.util.Optional.empty; +import static java.util.Optional.of; import static java.util.UUID.randomUUID; import static org.hamcrest.CoreMatchers.is; import static org.junit.Assert.assertThat; @@ -12,6 +14,7 @@ import java.time.ZonedDateTime; import java.util.List; +import java.util.Optional; import javax.sql.DataSource; @@ -44,28 +47,28 @@ public void shouldStoreEventErrorLog() throws Exception { final EventError eventError_1 = new EventError( "messageId_1", "component_1", - randomUUID(), "eventName_1", - 1L, + randomUUID(), + of(randomUUID()), + of(1L), "metadata_1", "payload_1", "errorMessage_1", "stacktrace_1", - now, - "comment_1" + now ); final EventError eventError_2 = new EventError( "messageId_2", "component_2", - randomUUID(), "eventName_2", - 2L, + randomUUID(), + of(randomUUID()), + of(2L), "metadata_2", "payload_2", "errorMessage_2", "stacktrace_2", - then, - "comment_2" + then ); eventErrorLogRepository.save(eventError_1); @@ -78,4 +81,72 @@ public void shouldStoreEventErrorLog() throws Exception { assertThat(eventErrors.get(0), is(eventError_1)); assertThat(eventErrors.get(1), is(eventError_2)); } + + @Test + public void shouldHandleMissingEventNumber() throws Exception { + + final DataSource systemDataSource = new TestJdbcDataSourceProvider().getSystemDataSource("framework"); + when(systemJdbcDataSourceProvider.getDataSource()).thenReturn(systemDataSource); + + eventErrorLogRepository.deleteAll(); + + final ZonedDateTime now = new UtcClock().now(); + + final EventError eventError = new EventError( + "messageId", + "component", + "eventName", + randomUUID(), + of(randomUUID()), + empty(), + "metadata", + "payload", + "errorMessage", + "stacktrace", + now + ); + + eventErrorLogRepository.save(eventError); + + final List eventErrors = eventErrorLogRepository.findAll(); + + assertThat(eventErrors.size(), is(1)); + + assertThat(eventErrors.get(0), is(eventError)); + assertThat(eventErrors.get(0).getEventNumber(), is(empty())); + } + + @Test + public void shouldHandleMissingStreamId() throws Exception { + + final DataSource systemDataSource = new TestJdbcDataSourceProvider().getSystemDataSource("framework"); + when(systemJdbcDataSourceProvider.getDataSource()).thenReturn(systemDataSource); + + eventErrorLogRepository.deleteAll(); + + final ZonedDateTime now = new UtcClock().now(); + + final EventError eventError = new EventError( + "messageId", + "component", + "eventName", + randomUUID(), + empty(), + of(1L), + "metadata", + "payload", + "errorMessage", + "stacktrace", + now + ); + + eventErrorLogRepository.save(eventError); + + final List eventErrors = eventErrorLogRepository.findAll(); + + assertThat(eventErrors.size(), is(1)); + + assertThat(eventErrors.get(0), is(eventError)); + assertThat(eventErrors.get(0).getStreamId(), is(empty())); + } }