Reusing the Error Codes

[wimdeblauwe]

…es with a List.

This allows for duplicates of the error codes to be present. This is important to
support validation errors that might have the same error code on multiple fields.

The refactoring introduced the ErrorWithArguments class which combines the previous
errorCodes field with the arguments field. This refactoring avoids the potential
mismatch between what is the errorCodes field and the map's keys.

[wimdeblauwe]

This PR is still a work in progress, but I would like to get early feedback if this is appropriate for solving #102

@alimate I would love your thoughts on this. If this is ok to go down this route, I can clean up the patch further and fix all TODO's in the unit tests.

[codecov-io]

Codecov Report

Merging #105 into master will decrease coverage by 1.97%.
The diff coverage is 84.94%.

@@             Coverage Diff              @@
##             master     #105      +/-   ##
============================================
- Coverage     93.58%   91.60%   -1.98%     
- Complexity      296      298       +2     
============================================
  Files            34       35       +1     
  Lines           701      715      +14     
  Branches         85       87       +2     
============================================
- Hits            656      655       -1     
- Misses           25       37      +12     
- Partials         20       23       +3     

Impacted Files	Coverage Δ	Complexity Δ
.../main/java/me/alidg/errors/ErrorWithArguments.java	62.50% <62.50%> (ø)	7.00 <7.00> (?)
...rc/main/java/me/alidg/errors/HandledException.java	70.00% <72.72%> (-30.00%)	10.00 <6.00> (ø)
...s/handlers/ConstraintViolationWebErrorHandler.java	92.30% <83.33%> (-7.70%)	11.00 <3.00> (-1.00)
.../alidg/errors/handlers/ServletWebErrorHandler.java	90.62% <90.90%> (-1.05%)	18.00 <0.00> (ø)
...rc/main/java/me/alidg/errors/WebErrorHandlers.java	90.47% <100.00%> (-0.15%)	20.00 <1.00> (-1.00)
...lidg/errors/handlers/AnnotatedWebErrorHandler.java	93.02% <100.00%> (ø)	23.00 <0.00> (ø)
...idg/errors/handlers/LastResortWebErrorHandler.java	100.00% <100.00%> (ø)	3.00 <1.00> (ø)
...dlers/MissingRequestParametersWebErrorHandler.java	90.90% <100.00%> (ø)	9.00 <0.00> (ø)
...lidg/errors/handlers/MultipartWebErrorHandler.java	100.00% <100.00%> (ø)	4.00 <0.00> (ø)
...errors/handlers/ResponseStatusWebErrorHandler.java	94.36% <100.00%> (-1.41%)	29.00 <0.00> (-1.00)
... and 6 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 41d2b04...dcffddc. Read the comment docs.

[alimate]

@wimdeblauwe Thank you. I will look at them in the upcoming days.

[wimdeblauwe]

@alimate Have you been able to look at this?

[alimate]

@wimdeblauwe I could not manage to find a time.
@zarebski-m May you took over me here and review this PR. That’s, naturally, if you have the time and are interested - if not, that’s absolutely fine.

[zarebski-m]

@wimdeblauwe This looks really nice, I'd say that we can go this way.

My only concern are changes to public API of HandledException which are exposed via library's extension point (WebErrorHandler). If possible, please leave old API of this class unchanged and deprecated, but don't remove anything as this will possibly break client code.

FYI @alimate

[wimdeblauwe]

@zarebski-m I have re-instantiated the public methods of HandledException and also updated the unit tests.

There are only 2 unit tests left: ConstraintViolationWebErrorHandlerTest and ServletWebErrorHandlerTest. They give me failures and I don't really see why, will have to investigate further.

On a side note: I find that the test with the parameters are a bit annoying if you need to debug since there is no easy way to run a single test with a specific set of parameters that fails.

[zarebski-m]

@wimdeblauwe It's better, but I'd like to mention that constructors are also part of public API. ;)

It's especially important here because WebErrorHandler is effectively a HandledException provider which means that its implementations must directly instantiate handled exceptions.

[wimdeblauwe]

Constructors are back :)

For ConstraintViolationWebErrorHandlerTest, this test case is failing:

            p(
                v(new Person("", 19)),
                setOf("username.blank", "username.size"),
                singletonMap("username.size", asList(
                    arg("max", 10),
                    arg("min", 6),
                    arg("invalid", ""),
                    arg("property", "username")))
            ),

I get:

Expecting:
  <[[invalid=, property=username], [max=10, min=6, invalid=, property=username]]>
to contain exactly in any order:
  <[[max=10, min=6, invalid=, property=username]]>
but the following elements were unexpected:
  <[[invalid=, property=username]]>

Is the test really valid as it was before? Because there where more keys in the errorCodes then there are keys in the map with the errorCode -> arguments. With this new setup, this mismatch is no longer possible.

The validation annotations on Person look like this:

        @NotBlank(message = "{username.blank}")
        @Size(min = 6, max = 10, message = "username.size")
        private String username;

So for me it is normal that there is a 2nd list of arguments. Can I change the test in that direction?

[wimdeblauwe]

To make the tests better fit with the updated design of ErrorWithArguments, I think it would be better to do something like this:

package me.alidg.errors.handlers;

import junitparams.JUnitParamsRunner;
import junitparams.Parameters;
import me.alidg.errors.Argument;
import me.alidg.errors.ErrorWithArguments;
import me.alidg.errors.HandledException;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.springframework.http.HttpInputMessage;
import org.springframework.http.HttpStatus;
import org.springframework.http.MediaType;
import org.springframework.http.converter.HttpMessageNotReadableException;
import org.springframework.web.HttpMediaTypeNotAcceptableException;
import org.springframework.web.HttpMediaTypeNotSupportedException;
import org.springframework.web.HttpRequestMethodNotSupportedException;
import org.springframework.web.bind.MissingServletRequestParameterException;
import org.springframework.web.multipart.support.MissingServletRequestPartException;
import org.springframework.web.servlet.NoHandlerFoundException;

import javax.servlet.ServletException;
import java.util.HashSet;
import java.util.List;

import static java.util.Arrays.asList;
import static java.util.Collections.*;
import static me.alidg.Params.p;
import static me.alidg.errors.Argument.arg;
import static me.alidg.errors.handlers.ServletWebErrorHandler.*;
import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Mockito.mock;
import static org.springframework.http.HttpStatus.*;
import static org.springframework.http.MediaType.*;

/**
 * Unit tests for {@link ServletWebErrorHandler} handler.
 *
 * @author Ali Dehghani
 */
@RunWith(JUnitParamsRunner.class)
public class ServletWebErrorHandlerTest {

    /**
     * Subject under test.
     */
    private final ServletWebErrorHandler handler = new ServletWebErrorHandler();

    @Test
    @Parameters(method = "provideParamsForCanHandle")
    public void canHandle_ShouldReturnTrueForSpringMvcSpecificErrors(Throwable exception, boolean expected) {
        assertThat(handler.canHandle(exception))
            .isEqualTo(expected);
    }

    @Test
    @Parameters(method = "provideParamsForHandle")
    public void handle_ShouldHandleSpringMvcErrorsProperly(Throwable exception,
                                                           HttpStatus expectedStatus,
                                                           List<ErrorWithArguments> expectedErrors) {
        HandledException handledException = handler.handle(exception);

        assertThat(handledException.getStatusCode()).isEqualTo(expectedStatus);

        List<ErrorWithArguments> errors = handledException.getErrors();
        assertThat(errors).containsExactlyInAnyOrderElementsOf(expectedErrors);

    }

    private Object[] provideParamsForCanHandle() {
        return p(
            p(null, false),
            p(new RuntimeException(), false),
            p(new NoHandlerFoundException(null, null, null), true),
            p(new HttpMessageNotReadableException("", mock(HttpInputMessage.class)), true),
            p(new MissingServletRequestParameterException("name", "String"), true),
            p(new HttpMediaTypeNotAcceptableException(""), true),
            p(new HttpMediaTypeNotSupportedException(""), true),
            p(new HttpRequestMethodNotSupportedException(""), true),
            p(new MissingServletRequestPartException("file"), true)
        );
    }

    private Object[] provideParamsForHandle() {
        return p(
            p(new HttpMessageNotReadableException("", mock(HttpInputMessage.class)), 
              BAD_REQUEST,
              singletonList(ErrorWithArguments.noArgumentError(INVALID_OR_MISSING_BODY))),
            p(
                new HttpMediaTypeNotAcceptableException(asList(APPLICATION_JSON, MediaType.APPLICATION_PDF)),
                HttpStatus.NOT_ACCEPTABLE,
                singletonList(new ErrorWithArguments(ServletWebErrorHandler.NOT_ACCEPTABLE,
                                                     singletonList(arg("types", new HashSet<>(asList(APPLICATION_JSON_VALUE, APPLICATION_PDF_VALUE))))))
            ),
            p(
                new HttpMediaTypeNotSupportedException(APPLICATION_JSON, emptyList()),
                UNSUPPORTED_MEDIA_TYPE,
                singletonList(new ErrorWithArguments(NOT_SUPPORTED, singletonList(arg("type", APPLICATION_JSON_VALUE))))
            ),
            p(
                new HttpRequestMethodNotSupportedException("POST"),
                HttpStatus.METHOD_NOT_ALLOWED,
                singletonList(new ErrorWithArguments(ServletWebErrorHandler.METHOD_NOT_ALLOWED, singletonList(arg("method", "POST"))))
            ),
            p(
                new MissingServletRequestParameterException("name", "String"),
                BAD_REQUEST,
                singletonList(new ErrorWithArguments(MISSING_PARAMETER, asList(arg("name", "name"), arg("expected", "String"))))
            ),
            p(
                new MissingServletRequestPartException("file"),
                BAD_REQUEST,
                singletonList(new ErrorWithArguments(MISSING_PART, singletonList(arg("name", "file"))))
            ),
            p(
                new NoHandlerFoundException("POST", "/test", null),
                NOT_FOUND,
                singletonList(new ErrorWithArguments(NO_HANDLER, singletonList(arg("path", "/test"))))
            ),
            p(new ServletException(),
              INTERNAL_SERVER_ERROR,
              singletonList(ErrorWithArguments.noArgumentError("unknown_error")))
        );
    }
}

What I did is replace the arguments of handle_ShouldHandleSpringMvcErrorsProperly with passing in a List<ErrorWithArguments> instead of the separate Set<String> and Map<String, List<Arguments>>

IMO it makes it more readable and you can't have a mismatch between the Set and the Map.

Do you like this? Do you want me to change all tests like that? Or do you want this on a separate branch?

[zarebski-m]

@wimdeblauwe Thanks for your changes, I really like them.

Regarding test cases: your changes definitely make sense to me and indeed are more readable. I'm not sure about rewriting all the tests, though; not in this PR at least. I'd suggest to make minimal effort to keep current tests working as they are, and possibly rewrite them in another PR.

Changes in business logic and overhaul of tests in one go can unintentionally introduce regressions.

[wimdeblauwe]

@zarebski-m Seems like the best to do it in a separate PR indeed to me as well.

I think I only need an answer to #105 (comment) in order to finish the PR?

[zarebski-m]

@wimdeblauwe It seems to me that this test case could indeed be wrong. There should be two sets of arguments, one for each violated constraint.

[wimdeblauwe]

Ok, final test also fixed. This is now ready to be fully reviewed (and hopefully merged :-) )

[wimdeblauwe]

Can we progress on this? Is there anything else that is needed?

[zarebski-m]

@alimate Could you have a look at this PR?

[wimdeblauwe]

Friendly reminder @alimate

[alimate]

@wimdeblauwe I'm terribly sorry about these recurring delays. I was swamped for the last 5 months!
Anyway, I started to review these changes. I will get back to you when I'm done. Hopefully until Friday

Cheers!

[wimdeblauwe]

@alimate Looking forward to it :-)

[wimdeblauwe]

Any update on this?

[alimate]

Honestly, I'm terribly sorry for these sort of delays. I completely understand that you invest your valuable time on this and you have every right to follow up.
Even though I really like to continue contributing to this project, I may not find the time for that.
Mainly because I'm living in a country with 1000+ percent inflation rate and maintaining an open source project, doesn't seem to support the cost of living here.
At the same time, this project is being used in multiple projects in production and I should publish new versions with extreme care.

Again, I'm sorry that I can't find the time to contribute more.

[wimdeblauwe]

Are you saying that it would be better for me to fork the project if I want to get my changes in a published version? Or is it possible to share the maintenance load of the project with somebody else? Maybe @zarebski-m ?

[alimate]

For now, forking it seems to be a better option.

[wimdeblauwe]

Ok, no problem. I totally understand. This has lead me to think hard about what I want from error handling and there are quite a few things I'd like differently. Due to that, I started my own project at https://github.com/wimdeblauwe/error-handling-spring-boot-starter It is certainly not as feature-rich as yours, but it is more closely aligned with my needs. So you can close this PR as I will not further work on this, as I now have my own project to take care of. :) Good luck with your project!