Bump zalando problem libs #946

lmontrieux · 2018-08-30T15:49:39Z

Upgrade zalando problems libraries

Zalando ticket : ARUHA-1695

Description

This PR:

upgrades all zalando-problem libraries to the latest version
upgrades gradle to 4.10.2
removes anything deprecated from build.gradle
refactors exception handlers

Review

Tests
Documentation
CHANGELOG

Update the following libraries: - jackson-datatype-problem to 0.21.0 - problem to 0.21.0 - problem-spring-web to 0.23.0 Code fixes and refactoring of exception handling

codecov-io · 2018-08-30T16:18:22Z

Codecov Report

Merging #946 into master will increase coverage by 0.05%.
The diff coverage is 52.17%.

@@             Coverage Diff              @@
##             master     #946      +/-   ##
============================================
+ Coverage     54.13%   54.18%   +0.05%     
- Complexity     1760     1775      +15     
============================================
  Files           318      335      +17     
  Lines          9589     9572      -17     
  Branches        872      874       +2     
============================================
- Hits           5191     5187       -4     
+ Misses         4083     4067      -16     
- Partials        315      318       +3

Impacted Files	Coverage Δ	Complexity Δ
...lando/nakadi/controller/HealthCheckController.java	`100% <ø> (ø)`	`2 <0> (ø)`	⬇️
.../nakadi/controller/CursorOperationsController.java	`12.06% <ø> (+2.06%)`	`2 <0> (ø)`	⬇️
...rg/zalando/nakadi/controller/SchemaController.java	`100% <ø> (+13.33%)`	`5 <0> (ø)`	⬇️
...g/zalando/nakadi/controller/VersionController.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...zalando/nakadi/controller/TimelinesController.java	`100% <ø> (+55%)`	`3 <0> (-1)`	⬇️
...akadi/controller/SubscriptionStreamController.java	`12.5% <ø> (-0.41%)`	`2 <0> (ø)`
...org/zalando/nakadi/util/GzipBodyRequestFilter.java	`13.95% <0%> (ø)`	`4 <0> (ø)`	⬇️
.../zalando/nakadi/controller/SettingsController.java	`15.62% <0%> (-3.3%)`	`1 <0> (-1)`
...lando/nakadi/controller/EventStreamController.java	`85.71% <0%> (ø)`	`20 <0> (ø)`	⬇️
...do/nakadi/exceptions/runtime/BlockedException.java	`0% <0%> (ø)`	`0 <0> (?)`
... and 45 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f65fd8...3921ea7. Read the comment docs.

# Conflicts: # src/main/java/org/zalando/nakadi/config/SecurityConfiguration.java

adyach

Good work @lmontrieux, thank you! It makes sense for us to fully understand Zalando problem approach

adyach · 2018-09-26T13:56:08Z

src/main/java/org/zalando/nakadi/controller/SubscriptionController.java

@@ -89,48 +72,4 @@ public SubscriptionController(final FeatureToggleService featureToggleService,
        final StatsMode statsMode = showTimeLag ? StatsMode.TIMELAG : StatsMode.NORMAL;
        return subscriptionService.getSubscriptionStat(subscriptionId, statsMode);
    }
-
-    @ExceptionHandler(FeatureNotAvailableException.class)


The same as for previous PR, maybe we should exceptions related to this particular controller in the controller

adyach · 2018-09-26T13:57:40Z

src/main/java/org/zalando/nakadi/controller/PartitionsController.java


 @RestController
-public class PartitionsController {
+public class PartitionsController extends NakadiProblemControllerAdvice {


why does every controller extend NakadiProblemControllerAdvice? I thought once NakadiProblemControllerAdvice is @ControllerAdvice it should work transparently.

@adyach I think @lmontrieux extended this class because it defines the method create used to generate the Problem response.

We would have to write some "catch/rethrow" here if we are not extending such class. What do you think? I prefer catch/rethrow instead of inheritance.

I think spring wants you to use runtime exceptions everywhere, keep controller code linear without error handling and catch all in the controller advisor.

Now I have a global @ControllerAdvice for the handlers that relate to all controllers, and a few @ControllerAdvice with higher priority, for those controller-specific cases. Once we merge the previous PR on this work (moving problem creation to controller), some more controller-specific handlers will also more to the specific @ControllerAdvice classes (such as StorageNotFoundException)

adyach · 2018-09-26T13:58:21Z

src/main/java/org/zalando/nakadi/controller/NakadiProblemHandling.java

+import static org.zalando.problem.Status.UNPROCESSABLE_ENTITY;
+
+
+public interface NakadiProblemHandling extends ProblemHandling {


why do we have this interface? it could be moved to NakadiProblemControllerAdvice

why should we extend ProblemHandling? do we use everything from there? for example I see validation trait there, I remember ValidationProblem.java in EventTypeController.create(), which we create by ourselves, but in case we have validation trait I expect to be created for us.

It's a good point. I changed the approach, I think it is cleaner and more spring boot-like now.

adyach

Good job, you have made controllers super lean !

adyach · 2018-10-10T06:56:24Z

src/main/java/org/zalando/nakadi/config/SecurityConfiguration.java

+                return status;
+            }
+        }
+        return null;


probably this is an exceptional situation

Can we use 418 I'm a teapot? :)

I mean, it should never happen that the status code is not a real status code.

I would through runtime exception here, and if it happens we will have 500 at the end, which should be correct.

adyach · 2018-10-10T07:49:54Z

src/main/java/org/zalando/nakadi/controller/EventPublishingController.java

@@ -110,16 +110,13 @@ private ResponseEntity postEventInternal(final String eventTypeName,
            return response(result);
        } catch (final JSONException e) {


I see that catch blocks here are only for logging purposes, we do logging in top level controller, do you think we can remove them?

adyach · 2018-10-10T07:52:33Z

src/main/java/org/zalando/nakadi/controller/EventTypeController.java

        if (featureToggleService.isFeatureEnabled(DISABLE_EVENT_TYPE_CREATION)) {
            return new ResponseEntity<>(HttpStatus.NOT_IMPLEMENTED);
        }

        if (errors.hasErrors()) {
-            return Responses.create(new ValidationProblem(errors), request);
+            throw new ValidationException(errors);


I suggest to follow common approach (no matter which one you choose), because a couple of lines above I see you return response entity in exceptional situation, and here you throw an exception

it looks like it does not make much sense to throw exception from where you can already form proper response

Well, actually I was trying to avoid creating Problem in the controllers, and delegate that to ControllerAdvice classes instead. I agree with the first comment however, that the strategy should be consistent.

it is not necessarily to create Problem here, you can create ResponseEntity
?

I think we should create Problems for every error

adyach · 2018-10-10T07:56:14Z

src/main/java/org/zalando/nakadi/controller/PostSubscriptionController.java

        if (errors.hasErrors()) {
-            return Responses.create(new ValidationProblem(errors), request);
+            throw new ValidationException(errors);


adyach · 2018-10-10T07:56:57Z

src/main/java/org/zalando/nakadi/controller/SettingsController.java

        if (!adminService.isAdmin(AuthorizationService.Operation.ADMIN)) {
            return ResponseEntity.status(HttpStatus.FORBIDDEN).build();
        }
+        if (errors.hasErrors()) {
+            throw new ValidationException(errors);


adyach · 2018-10-10T07:59:02Z

src/main/java/org/zalando/nakadi/controller/advice/CursorOperationsHandler.java

+
+@Priority(10)
+@ControllerAdvice(assignableTypes = CursorOperationsController.class)
+public class CursorOperationsHandler implements AdviceTrait {


why does it implement AdviceTrait?

to use AdviceTrait.create(..)

# Conflicts: # src/main/java/org/zalando/nakadi/controller/CursorOperationsController.java # src/main/java/org/zalando/nakadi/controller/CursorsController.java # src/main/java/org/zalando/nakadi/controller/EventPublishingController.java # src/main/java/org/zalando/nakadi/controller/EventTypeController.java # src/main/java/org/zalando/nakadi/controller/ExceptionHandling.java # src/main/java/org/zalando/nakadi/controller/PostSubscriptionController.java # src/main/java/org/zalando/nakadi/controller/StoragesController.java # src/main/java/org/zalando/nakadi/controller/SubscriptionController.java # src/main/java/org/zalando/nakadi/controller/SubscriptionStreamController.java # src/main/java/org/zalando/nakadi/controller/TimelinesController.java # src/test/java/org/zalando/nakadi/controller/NakadiProblemHandlingTest.java

when appropriate

adyach

LGTM, though I would call *Handlers as *ExceptionHandlers

adyach · 2018-10-12T10:59:17Z

build.gradle

@@ -188,8 +190,9 @@ dependencies {
 // end::dependencies[]

 // tag::wrapper[]
-task wrapper(type: Wrapper) {
-    gradleVersion = '2.3'
+wrapper {


do we really need this section ? we have defined it in gradle-wrapper.properties

adyach · 2018-10-12T11:10:15Z

src/main/java/org/zalando/nakadi/controller/advice/NakadiProblemHandling.java

    }

-    private String generateErrorTraceId() {
+    public String generateErrorTraceId() {


adyach · 2018-10-12T11:11:14Z

src/main/java/org/zalando/nakadi/controller/advice/PartitionsHandler.java

+
+    @ExceptionHandler(NotFoundException.class)
+    public ResponseEntity<Problem> notFound(final NotFoundException ex, final NativeWebRequest request) {
+        LOG.error(ex.getMessage(), ex);


adyach · 2018-10-12T11:11:46Z

src/main/java/org/zalando/nakadi/controller/advice/SettingsHandler.java

+    @ExceptionHandler(UnableProcessException.class)
+    public ResponseEntity<Problem> handleUnableProcessException(final RuntimeException exception,
+                                                                final NativeWebRequest request) {
+        LOG.error(exception.getMessage(), exception);


lmontrieux · 2018-10-12T12:21:54Z

@adyach I've addressed all comments. As we discussed offline, I went through all exception handlers, with the following guideline:

5xx -> log.error and log exception (sometimes it's just log.warn when it makes more sense)
4xx -> log.debug and only log exception.getMessage()

adyach · 2018-10-12T13:14:21Z

👍

lmontrieux · 2018-10-12T13:26:41Z

deploy validation please

lmontrieux · 2018-10-12T13:26:54Z

👍

lmontrieux · 2018-10-15T10:47:56Z

👍

# Conflicts: # src/main/java/org/zalando/nakadi/controller/SubscriptionController.java # src/main/java/org/zalando/nakadi/controller/advice/NakadiProblemExceptionHandler.java

lmontrieux · 2018-10-18T09:27:53Z

👍

adyach · 2018-10-18T09:30:31Z

👍

Lionel Montrieux added 5 commits August 30, 2018 16:08

Update Zalando-problem libraries

a4b5e4d

Update the following libraries: - jackson-datatype-problem to 0.21.0 - problem to 0.21.0 - problem-spring-web to 0.23.0 Code fixes and refactoring of exception handling

Clean up controllers

6a76eda

gradle 4.10

e0991d0

Build file ready for Gradle 5.0

7466426

Fix tests

48d3919

lmontrieux requested a review from a team as a code owner August 30, 2018 15:49

Lionel Montrieux added 2 commits September 4, 2018 15:37

Merge branch 'master' into bump-zalando-problem-libs

053c474

# Conflicts: # src/main/java/org/zalando/nakadi/config/SecurityConfiguration.java

Post-merge fix

7e93e91

adyach suggested changes Sep 26, 2018

View reviewed changes

Lionel Montrieux added 2 commits October 8, 2018 16:55

Refactor

a52724c

Fix tests

6091c2c

adyach suggested changes Oct 10, 2018

View reviewed changes

Lionel Montrieux added 11 commits October 10, 2018 16:38

Fixes post review

cdeeed1

Remove unused field

935e4ad

Move exception handlers to controller-specific ControllerAdvices

2ea0ba3

when appropriate

Group identical handlers together

1bdfdae

bump problem version

16ff08c

Review fix

8dcd105

Bump gradle version

b1c85fb

Add OWASP dependency checker

38b20ea

Bump spring boot version

65228ba

Revert spring boot upgrade

6db5c16

adyach suggested changes Oct 12, 2018

View reviewed changes

Lionel Montrieux added 2 commits October 12, 2018 14:09

Fixes after review

e86d256

Rename controller advices

6131ff2

adyach approved these changes Oct 12, 2018

View reviewed changes

Lionel Montrieux added 2 commits October 12, 2018 16:53

Handle 403 consistently

3e2256d

Fix failing test

85420d7

Lionel Montrieux added 2 commits October 16, 2018 13:29

Merge branch 'master' into bump-zalando-problem-libs

9ac673c

# Conflicts: # src/main/java/org/zalando/nakadi/controller/SubscriptionController.java # src/main/java/org/zalando/nakadi/controller/advice/NakadiProblemExceptionHandler.java

Fix post merge

3921ea7

lmontrieux merged commit f3d01f6 into master Oct 18, 2018

lmontrieux deleted the bump-zalando-problem-libs branch October 18, 2018 11:39

		import static org.zalando.problem.Status.UNPROCESSABLE_ENTITY;


		public interface NakadiProblemHandling extends ProblemHandling {

		@@ -110,16 +110,13 @@ private ResponseEntity postEventInternal(final String eventTypeName,
		return response(result);
		} catch (final JSONException e) {

Bump zalando problem libs #946

Bump zalando problem libs #946

Conversation

lmontrieux commented Aug 30, 2018 • edited Loading

Upgrade zalando problems libraries

Description

Review

codecov-io commented Aug 30, 2018 • edited Loading

Codecov Report

adyach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adyach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adyach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmontrieux commented Oct 12, 2018

adyach commented Oct 12, 2018

lmontrieux commented Oct 12, 2018

lmontrieux commented Oct 12, 2018

lmontrieux commented Oct 15, 2018

lmontrieux commented Oct 18, 2018

adyach commented Oct 18, 2018

lmontrieux commented Aug 30, 2018 •

edited

Loading

codecov-io commented Aug 30, 2018 •

edited

Loading