From 226a495087ebaa580a71270a807e4bce8ba80b89 Mon Sep 17 00:00:00 2001 From: tillias Date: Tue, 27 Oct 2020 20:21:11 +0100 Subject: [PATCH] Custom exception handling for graph dependency validation errors #54 HTTP 422, see https://softwareengineering.stackexchange.com/questions/329229/should-i-return-an-http-400-bad-request-status-if-a-parameter-is-syntactically --- pom.xml | 9 ++ .../service/custom/DependencyService.java | 8 +- .../custom/ReleasePathCustomService.java | 3 - .../exceptions/SelfCircularException.java | 12 +- .../web/rest/errors/ExceptionTranslator.java | 3 +- .../errors/custom/ReleasePathAdviceTrait.java | 52 +++++++++ src/main/webapp/app/core/core.module.ts | 2 +- .../create-dependency-dialog.component.ts | 2 +- .../dependency-dashboard.component.html | 4 + .../dependency-dashboard.component.ts | 7 +- .../app/layouts/main/main.component.html | 7 ++ .../app/shared/alert/alert-error.component.ts | 4 + .../service/custom/DependencyServiceTest.java | 109 ++++++++---------- .../custom/GraphLoaderServiceTest.java | 9 +- 14 files changed, 153 insertions(+), 78 deletions(-) create mode 100644 src/main/java/com/github/microcatalog/web/rest/errors/custom/ReleasePathAdviceTrait.java diff --git a/pom.xml b/pom.xml index 1f486e3..8d7b588 100644 --- a/pom.xml +++ b/pom.xml @@ -80,6 +80,7 @@ 1.0.0 3.7.0.1746 1.5.0 + 3.18.0 ${project.build.directory}/jacoco/test ${jacoco.utReportFolder}/test.exec ${project.build.directory}/jacoco/integrationTest @@ -114,6 +115,14 @@ jgrapht-io ${jgrapht.version} + + org.assertj + assertj-core + ${assertj.version} + test + + + io.github.jhipster jhipster-framework diff --git a/src/main/java/com/github/microcatalog/service/custom/DependencyService.java b/src/main/java/com/github/microcatalog/service/custom/DependencyService.java index 5916342..d787b75 100644 --- a/src/main/java/com/github/microcatalog/service/custom/DependencyService.java +++ b/src/main/java/com/github/microcatalog/service/custom/DependencyService.java @@ -75,9 +75,9 @@ private void validateSelfCycle(final Dependency dependency) { return; } - final Long sourceId = dependency.getSource().getId(); - if (Objects.equals(sourceId, dependency.getTarget().getId())) { - throw new SelfCircularException(String.format("Source id is the same as target id: %s", sourceId)); + final Microservice source = dependency.getSource(); + if (Objects.equals(source.getId(), dependency.getTarget().getId())) { + throw new SelfCircularException("Source of dependency can't be the same as target", source); } } @@ -109,7 +109,7 @@ private void checkCycles(final Graph graph) { final Set cycles = cycleDetector.findCycles(); log.debug("Cycles: {}", cycles); - throw new CircularDependenciesException("Circular dependency will be introduced.", cycles); + throw new CircularDependenciesException("Circular dependency will be introduced", cycles); } } } diff --git a/src/main/java/com/github/microcatalog/service/custom/ReleasePathCustomService.java b/src/main/java/com/github/microcatalog/service/custom/ReleasePathCustomService.java index b60ae43..6d96e9b 100644 --- a/src/main/java/com/github/microcatalog/service/custom/ReleasePathCustomService.java +++ b/src/main/java/com/github/microcatalog/service/custom/ReleasePathCustomService.java @@ -10,7 +10,6 @@ import org.jgrapht.graph.AsSubgraph; import org.jgrapht.graph.DefaultEdge; import org.jgrapht.graph.EdgeReversedGraph; -import org.jgrapht.nio.dot.DOTExporter; import org.jgrapht.traverse.DepthFirstIterator; import org.jgrapht.traverse.GraphIterator; import org.slf4j.Logger; @@ -18,8 +17,6 @@ import org.springframework.stereotype.Service; import javax.transaction.Transactional; -import java.io.StringWriter; -import java.io.Writer; import java.time.Instant; import java.util.*; import java.util.stream.Collectors; diff --git a/src/main/java/com/github/microcatalog/service/custom/exceptions/SelfCircularException.java b/src/main/java/com/github/microcatalog/service/custom/exceptions/SelfCircularException.java index 621011f..e827e24 100644 --- a/src/main/java/com/github/microcatalog/service/custom/exceptions/SelfCircularException.java +++ b/src/main/java/com/github/microcatalog/service/custom/exceptions/SelfCircularException.java @@ -1,10 +1,20 @@ package com.github.microcatalog.service.custom.exceptions; +import com.github.microcatalog.domain.Microservice; + /** * Occurs when there is attempt creating dependency with source and target pointing to the same microservice */ public class SelfCircularException extends RuntimeException { - public SelfCircularException(String message) { + private final Microservice source; + + public SelfCircularException(String message, Microservice source) { super(message); + + this.source = source; + } + + public Microservice getSource() { + return source; } } diff --git a/src/main/java/com/github/microcatalog/web/rest/errors/ExceptionTranslator.java b/src/main/java/com/github/microcatalog/web/rest/errors/ExceptionTranslator.java index 78eaaa8..763b74f 100644 --- a/src/main/java/com/github/microcatalog/web/rest/errors/ExceptionTranslator.java +++ b/src/main/java/com/github/microcatalog/web/rest/errors/ExceptionTranslator.java @@ -1,5 +1,6 @@ package com.github.microcatalog.web.rest.errors; +import com.github.microcatalog.web.rest.errors.custom.ReleasePathAdviceTrait; import io.github.jhipster.config.JHipsterConstants; import io.github.jhipster.web.util.HeaderUtil; @@ -39,7 +40,7 @@ * The error response follows RFC7807 - Problem Details for HTTP APIs (https://tools.ietf.org/html/rfc7807). */ @ControllerAdvice -public class ExceptionTranslator implements ProblemHandling, SecurityAdviceTrait { +public class ExceptionTranslator implements ProblemHandling, SecurityAdviceTrait, ReleasePathAdviceTrait { private static final String FIELD_ERRORS_KEY = "fieldErrors"; private static final String MESSAGE_KEY = "message"; diff --git a/src/main/java/com/github/microcatalog/web/rest/errors/custom/ReleasePathAdviceTrait.java b/src/main/java/com/github/microcatalog/web/rest/errors/custom/ReleasePathAdviceTrait.java new file mode 100644 index 0000000..0a02790 --- /dev/null +++ b/src/main/java/com/github/microcatalog/web/rest/errors/custom/ReleasePathAdviceTrait.java @@ -0,0 +1,52 @@ +package com.github.microcatalog.web.rest.errors.custom; + +import com.github.microcatalog.domain.Microservice; +import com.github.microcatalog.service.custom.exceptions.CircularDependenciesException; +import com.github.microcatalog.service.custom.exceptions.SelfCircularException; +import org.springframework.http.ResponseEntity; +import org.springframework.web.bind.annotation.ExceptionHandler; +import org.springframework.web.context.request.NativeWebRequest; +import org.zalando.problem.Problem; +import org.zalando.problem.spring.web.advice.AdviceTrait; + +import java.util.List; +import java.util.stream.Collectors; + +import static org.zalando.problem.Status.UNPROCESSABLE_ENTITY; + +/** + * Using 422 because of https://softwareengineering.stackexchange.com/questions/329229/should-i-return-an-http-400-bad-request-status-if-a-parameter-is-syntactically + */ +public interface ReleasePathAdviceTrait extends AdviceTrait { + + String HEADER_KEY = "BusinessException"; + String PAYLOAD_KEY = "BusinessPayload"; + String MESSAGE_KEY = "BusinessMessage"; + + @ExceptionHandler + default ResponseEntity handleSelfCircularException(final SelfCircularException exception, final NativeWebRequest request) { + final Microservice source = exception.getSource(); + final Problem problem = Problem.builder() + .withStatus(UNPROCESSABLE_ENTITY) + .with(HEADER_KEY, SelfCircularException.class.getName()) + .with(PAYLOAD_KEY, String.format("id = %s, name = %s", source.getId(), source.getName())) + .with(MESSAGE_KEY, exception.getMessage()) + .build(); + + return create(exception, problem, request); + } + + @ExceptionHandler + default ResponseEntity handleCircularDependenciesException(final CircularDependenciesException exception, final NativeWebRequest request) { + final List microserviceIds = exception.getCycles().stream().map(m -> String.valueOf(m.getId())).collect(Collectors.toList()); + + final Problem problem = Problem.builder() + .withStatus(UNPROCESSABLE_ENTITY) + .with(HEADER_KEY, CircularDependenciesException.class.getName()) + .with(PAYLOAD_KEY, String.join(",", microserviceIds)) + .with(MESSAGE_KEY, exception.getMessage()) + .build(); + + return create(exception, problem, request); + } +} diff --git a/src/main/webapp/app/core/core.module.ts b/src/main/webapp/app/core/core.module.ts index fe2d57f..a213d4c 100644 --- a/src/main/webapp/app/core/core.module.ts +++ b/src/main/webapp/app/core/core.module.ts @@ -26,7 +26,7 @@ import { fontAwesomeIcons } from './icons/font-awesome-icons'; NgxWebstorageModule.forRoot({ prefix: 'jhi', separator: '-' }), NgJhipsterModule.forRoot({ // set below to true to make alerts look like toast - alertAsToast: false, + alertAsToast: true, alertTimeout: 5000, i18nEnabled: true, defaultI18nLang: 'en', diff --git a/src/main/webapp/app/dashboard/dependency-dashboard/create-dependency-dialog/create-dependency-dialog.component.ts b/src/main/webapp/app/dashboard/dependency-dashboard/create-dependency-dialog/create-dependency-dialog.component.ts index 92de8f7..0304aa5 100644 --- a/src/main/webapp/app/dashboard/dependency-dashboard/create-dependency-dialog/create-dependency-dialog.component.ts +++ b/src/main/webapp/app/dashboard/dependency-dashboard/create-dependency-dialog/create-dependency-dialog.component.ts @@ -17,7 +17,7 @@ export class CreateDependencyDialogComponent implements OnInit { name: string; isSaving = false; - constructor(public eventManager: JhiEventManager, public activeModal: NgbActiveModal, public dependencyService: DependencyService) { + constructor(protected eventManager: JhiEventManager, protected activeModal: NgbActiveModal, public dependencyService: DependencyService) { this.name = 'Please specify source & target'; } diff --git a/src/main/webapp/app/dashboard/dependency-dashboard/dependency-dashboard.component.html b/src/main/webapp/app/dashboard/dependency-dashboard/dependency-dashboard.component.html index aa788a5..fbd8cac 100644 --- a/src/main/webapp/app/dashboard/dependency-dashboard/dependency-dashboard.component.html +++ b/src/main/webapp/app/dashboard/dependency-dashboard/dependency-dashboard.component.html @@ -56,6 +56,10 @@ [disabled]="!nodeSelection">Build release path +
+ +
diff --git a/src/main/webapp/app/dashboard/dependency-dashboard/dependency-dashboard.component.ts b/src/main/webapp/app/dashboard/dependency-dashboard/dependency-dashboard.component.ts index 9db3eb4..d5fd550 100644 --- a/src/main/webapp/app/dashboard/dependency-dashboard/dependency-dashboard.component.ts +++ b/src/main/webapp/app/dashboard/dependency-dashboard/dependency-dashboard.component.ts @@ -3,7 +3,7 @@ import { DependencyService } from '../../entities/dependency/dependency.service' import { IMicroservice } from '../../shared/model/microservice.model'; import { EXPERIMENTAL_FEATURE } from '../../app.constants'; import { CreateDependencyDialogService } from './create-dependency-dialog/create-dependency-dialog.service'; -import { JhiEventManager } from 'ng-jhipster'; +import { JhiAlertService, JhiEventManager } from 'ng-jhipster'; import { Subscription } from 'rxjs'; import { MicroserviceService } from '../../entities/microservice/microservice.service'; import { map } from 'rxjs/operators'; @@ -34,6 +34,7 @@ export class DependencyDashboardComponent implements OnInit, AfterViewInit, OnDe constructor( protected eventManager: JhiEventManager, + protected alertService: JhiAlertService, protected dependencyService: DependencyService, protected microserviceService: MicroserviceService, protected releasePathService: ReleasePathCustomService, @@ -118,7 +119,9 @@ export class DependencyDashboardComponent implements OnInit, AfterViewInit, OnDe this.refreshGraph(); } - buildReleasePath(): void {} + buildReleasePath(): void { + this.alertService.success('Alert'); + } selectedNodeId(): number { if (this.nodeSelection && this.nodeSelection.hasNodes()) { diff --git a/src/main/webapp/app/layouts/main/main.component.html b/src/main/webapp/app/layouts/main/main.component.html index d8bcc22..211ba1b 100644 --- a/src/main/webapp/app/layouts/main/main.component.html +++ b/src/main/webapp/app/layouts/main/main.component.html @@ -1,9 +1,14 @@ +
+
+ +
+
@@ -11,3 +16,5 @@
+ + diff --git a/src/main/webapp/app/shared/alert/alert-error.component.ts b/src/main/webapp/app/shared/alert/alert-error.component.ts index a540625..d82eae0 100644 --- a/src/main/webapp/app/shared/alert/alert-error.component.ts +++ b/src/main/webapp/app/shared/alert/alert-error.component.ts @@ -72,6 +72,10 @@ export class AlertErrorComponent implements OnDestroy { this.addErrorAlert('Not found', 'error.url.not.found'); break; + case 422: + this.addErrorAlert(httpErrorResponse.error.BusinessMessage); + break; + default: if (httpErrorResponse.error !== '' && httpErrorResponse.error.message) { this.addErrorAlert(httpErrorResponse.error.message); diff --git a/src/test/java/com/github/microcatalog/service/custom/DependencyServiceTest.java b/src/test/java/com/github/microcatalog/service/custom/DependencyServiceTest.java index 7b4e4a7..aa9f91b 100644 --- a/src/test/java/com/github/microcatalog/service/custom/DependencyServiceTest.java +++ b/src/test/java/com/github/microcatalog/service/custom/DependencyServiceTest.java @@ -6,7 +6,7 @@ import com.github.microcatalog.service.custom.exceptions.CircularDependenciesException; import com.github.microcatalog.service.custom.exceptions.SelfCircularException; import com.github.microcatalog.utils.DependencyBuilder; -import com.github.microcatalog.web.rest.errors.BadRequestAlertException; +import org.assertj.core.api.InstanceOfAssertFactories; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.context.SpringBootTest; @@ -43,9 +43,9 @@ void findAll() { @Test void findById() { - final long id = 3L; + final Long id = 3L; - given(repository.findById(id)).willReturn(Optional.of(new DependencyBuilder().withId(id).build())); + given(repository.findById(id)).willReturn(Optional.of(dependency(id.intValue(), null, null))); Optional maybeDependency = service.findById(id); assertThat(maybeDependency).isPresent(); @@ -66,20 +66,17 @@ void deleteById() { @Test void create_WithId_ExceptionIsThrown() { assertThatIllegalArgumentException() - .isThrownBy(() ->service.create(new DependencyBuilder().withId(1L).build())) + .isThrownBy(() -> service.create(dependency(1, null, null))) .withMessageStartingWith("A new dependency can not already have an id"); } @Test void create_SelfCycle_ExceptionIsThrown() { - assertThatThrownBy(() -> service.create( - new DependencyBuilder() - .withSource(2L) - .withTarget(2L) - .build() - )) + final Dependency dependency = dependency(null, 2, 2); + + assertThatThrownBy(() -> service.create(dependency)) .isInstanceOf(SelfCircularException.class) - .hasMessageStartingWith("Source id is the same as target id"); + .hasMessageStartingWith("Source of dependency can't be the same as target"); } @Test @@ -96,14 +93,14 @@ void create_WillIntroduceCircularDependencies_ExceptionIsThrown() { ) ); - assertThatThrownBy(() -> service.create( - new DependencyBuilder() - .withSource(4L) - .withTarget(1L) - .build() - )) + final Dependency dependency = dependency(null, 4, 1); + + assertThatThrownBy(() -> service.create(dependency)) .isInstanceOf(CircularDependenciesException.class) - .hasMessageStartingWith("Circular dependency will be introduced."); + .hasMessageStartingWith("Circular dependency will be introduced") + .extracting("cycles", InstanceOfAssertFactories.ITERABLE) + .extracting("id") + .containsExactlyInAnyOrder(1L, 2L, 3L, 4L); } @Test @@ -119,19 +116,11 @@ void create_Success() { ) ); - final Dependency expected = new DependencyBuilder() - .withId(1L) - .withSource(3L) - .withTarget(4L) - .build(); + final Dependency expected = dependency(1, 3, 4); given(repository.save(any(Dependency.class))).willReturn(expected); - Dependency persistent = service.create( - new DependencyBuilder() - .withSource(3L) - .withTarget(4L) - .build()); + Dependency persistent = service.create(dependency(null, 3, 4)); assertThat(persistent).isEqualTo(expected); @@ -142,21 +131,17 @@ void create_Success() { void update_NoId_ExceptionIsThrown() { assertThatIllegalArgumentException() .isThrownBy(() -> - service.update(new DependencyBuilder().withSource(1L).withTarget(2L).build())) + service.update(dependency(null, 1, 2))) .withMessageStartingWith("Updating non-persistent entity without id"); } @Test void update_SelfCycle_ExceptionIsThrown() { - assertThatThrownBy(() -> service.update( - new DependencyBuilder() - .withId(1L) - .withSource(2L) - .withTarget(2L) - .build() - )) + final Dependency dependency = dependency(1, 2, 2); + + assertThatThrownBy(() -> service.update(dependency)) .isInstanceOf(SelfCircularException.class) - .hasMessageStartingWith("Source id is the same as target id"); + .hasMessageStartingWith("Source of dependency can't be the same as target"); } @Test @@ -174,17 +159,16 @@ void update_WillIntroduceCircularDependencies_ExceptionIsThrown() { ); given(repository.findById(3L)) - .willReturn(Optional.of(new DependencyBuilder().withId(3L).withSource(3L).withTarget(4L).build())); - - assertThatThrownBy(() -> service.update( - new DependencyBuilder() - .withId(3L) - .withSource(3L) - .withTarget(1L) - .build() - )) + .willReturn(Optional.of(dependency(3, 3, 4))); + + final Dependency dependency = dependency(3, 3, 1); + + assertThatThrownBy(() -> service.update(dependency)) .isInstanceOf(CircularDependenciesException.class) - .hasMessageStartingWith("Circular dependency will be introduced."); + .hasMessageStartingWith("Circular dependency will be introduced") + .extracting("cycles", InstanceOfAssertFactories.ITERABLE) + .extracting("id") + .containsExactlyInAnyOrder(1L, 2L, 3L); } @Test @@ -202,25 +186,32 @@ void update_Success() { ); given(repository.findById(2L)) - .willReturn(Optional.of(new DependencyBuilder().withId(2L).withSource(2L).withTarget(3L).build())); + .willReturn(Optional.of(dependency(2, 2, 3))); - final Dependency expected = new DependencyBuilder() - .withId(2L) - .withSource(2L) - .withTarget(4L) - .build(); + final Dependency expected = dependency(2, 2, 4); given(repository.save(any(Dependency.class))).willReturn(expected); - Dependency persistent = service.update( - new DependencyBuilder() - .withId(2L) - .withSource(2L) - .withTarget(4L) - .build()); + Dependency persistent = service.update(dependency(2, 2, 4)); assertThat(persistent).isEqualTo(expected); then(repository).should().save(any(Dependency.class)); } + + private Dependency dependency(final Integer id, final Integer sourceId, final Integer targetId) { + DependencyBuilder builder = new DependencyBuilder(); + + if (sourceId != null) { + builder.withSource(Long.valueOf(sourceId)); + } + if (targetId != null) { + builder.withTarget(Long.valueOf(targetId)); + } + if (id != null) { + builder.withId(Long.valueOf(id)); + } + + return builder.build(); + } } diff --git a/src/test/java/com/github/microcatalog/service/custom/GraphLoaderServiceTest.java b/src/test/java/com/github/microcatalog/service/custom/GraphLoaderServiceTest.java index ea75043..6493d4f 100644 --- a/src/test/java/com/github/microcatalog/service/custom/GraphLoaderServiceTest.java +++ b/src/test/java/com/github/microcatalog/service/custom/GraphLoaderServiceTest.java @@ -8,14 +8,11 @@ import com.github.microcatalog.utils.MicroserviceBuilder; import org.jgrapht.Graph; import org.jgrapht.graph.DefaultEdge; -import org.jgrapht.nio.dot.DOTExporter; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.test.mock.mockito.MockBean; -import java.io.StringWriter; -import java.io.Writer; import java.util.*; import java.util.stream.Collectors; import java.util.stream.LongStream; @@ -25,7 +22,7 @@ import static org.mockito.BDDMockito.then; @SpringBootTest(classes = GraphLoaderService.class) -public class GraphLoaderServiceTest { +class GraphLoaderServiceTest { @MockBean private MicroserviceRepository microserviceRepository; @@ -43,8 +40,8 @@ void loadEmptyGraph() { Graph graph = sut.loadGraph(); assertThat(graph).isNotNull(); - assertThat(graph.vertexSet()).hasSize(0); - assertThat(graph.edgeSet()).hasSize(0); + assertThat(graph.vertexSet()).isEmpty(); + assertThat(graph.edgeSet()).isEmpty(); then(microserviceRepository).should().findAll(); then(dependencyRepository).should().findAll();