ErrorCode on BadRequest on client side

[d80harri]

Hey guys,

I just noticed that katharsis is dead and switched to crnk. Refactoring was really easy so far, thanks for that.

However, I experience problems when it comes to mapping exceptions. On the server side I throw an "ImportException" which extends RuntimeException. I wrote an ExceptionMapper that maps to a bad RequestException and sets the ErrorData (error code inclusive) on the ErrorResponse. The mapper is executed on the server, the response looks fine but on the client side it seems that exception mapper is not registered (or maybe I miss something here). Therefore, in my testcases, I receive a BadRequestException but with no ErrorData.code.

Do I need to register the exception mapper on the client or will it be auto-discovered? I also tried to register a new module that adds the exception mapper -> no result.

With katharsis I had a similar problem. It seemed that the cause was katharsis-project/katharsis-framework#448 but I am not sure if this still holds for crnk.

build.gradle

plugins {
	...
	id 'org.springframework.boot' version '1.5.7.RELEASE'
	... and more ...
}

dependencies {
	...
	compile("org.springframework.boot:spring-boot-starter-web")
	testCompile("org.springframework.boot:spring-boot-starter-test")
	compile("org.springframework:spring-orm")
	compile("org.springframework.boot:spring-boot-devtools")
	compile("org.springframework:spring-tx")
	
	compile group: 'io.crnk', name: 'crnk-setup-spring-boot1', version: '2.6.20180522184741'
	testCompile group: 'io.crnk', name: 'crnk-client', version: '2.6.20180522184741'
         
         .... and more ...
}

** SpringBoot Application class **

 @Bean
    public CrnkClient crnkClient(@Value("${crnk.domainName}") String domainName, @Value("${crnk.path-prefix}") String pathPrefix) {
        CrnkClient client = new CrnkClient(domainName + pathPrefix);
        client.addModule(new Module() {
            @Override
            public String getModuleName() {
                return null;
            }

            @Override
            public void setupModule(ModuleContext context) {
                context.addExceptionMapper(new ImportExceptionMapper());
            }
        });
        return client;
    }

Exception thrown by a repository

public static class ImportException extends RuntimeException {
        public ImportException() {
        }

        public ImportException(String message) {
            super(message);
        }

        public ImportException(String message, Throwable cause) {
            super(message, cause);
        }

        public ImportException(Throwable cause) {
            super(cause);
        }

        public ImportException(String message, Throwable cause, boolean enableSuppression, boolean writableStackTrace) {
            super(message, cause, enableSuppression, writableStackTrace);
        }
    }

public class ImportExceptionMapper implements io.crnk.core.engine.error.ExceptionMapper<AbstractImportEntryHandler.ImportException> {
    private final int STATUS_CODE = io.crnk.core.engine.http.HttpStatus.BAD_REQUEST_400;

    @Override
    public io.crnk.core.engine.error.ErrorResponse toErrorResponse(AbstractImportEntryHandler.ImportException exception) {
        io.crnk.core.engine.document.ErrorDataBuilder builder = io.crnk.core.engine.document.ErrorData.builder();
        builder = builder.setStatus(String.valueOf(STATUS_CODE));
        builder = builder.setCode("DUPLICATE_ID"); 
        builder = builder.setTitle(exception.getLocalizedMessage());
        builder = builder.setDetail("An id does already exist");
        io.crnk.core.engine.document.ErrorData error = builder.build();

        return io.crnk.core.engine.error.ErrorResponse.builder().setStatus(STATUS_CODE).setSingleErrorData(error).build();
    }

    @Override
    public AbstractImportEntryHandler.ImportException fromErrorResponse(io.crnk.core.engine.error.ErrorResponse errorResponse) {
        return new AbstractImportEntryHandler.ImportException(); // TODO
    }

    @Override
    public boolean accepts(io.crnk.core.engine.error.ErrorResponse errorResponse) {
        return true; // TODO ?
    }
}

Response from server.. This looks great

{
    "errors": [
        {
            "status": "400",
            "code": "DUPLICATE_ID",
            "title": "Cannot import entry. Duplicate key RelativeId{value='firstPosition'}",
            "detail": "An id does already exist"
        }
    ]
}

@Test
    public void testImportWithDuplicateId() throws Throwable {
        ImportEntryCollection importEntries = new ImportEntryCollection();
        List<AbstractImportEntry> entries = new ArrayList<>();
        importEntries.setEntries(entries);

        entries.add(new PositionImportEntry(new RelativeId("duplicate"), "firstPosition"));
        entries.add(new PositionImportEntry(new RelativeId("duplicate"), "firstPosition"));

        //testClient.create(importEntries);
        MoroAssertions.assertThatThrownBy(() -> testClient.create(importEntries)).hasSingleErrorCode("DUPLICATE_ID");
    }

// within AssertJ-assertion class
public CrnkExceptionAssert hasSingleErrorCode(String errorCode) {
            ErrorData errorResponse = this.actual.getErrorData();

            assertThat(errorResponse).hasSingleErrorCode(errorCode);
            return this;
        }

Expected behaviour: Test passes
Actual behaviour:
org.junit.ComparisonFailure:
Expected :"DUPLICATE_ID"
Actual :null

[remmeier]

that utf8 thing will dissappear. Just today a ticket was created.

I suspect the BadRequestException has a higher priority. You should inherit from that one. Then it probably works. Need to clear that up in the docs.

[remmeier]

(in other areas I started introducing priorities)

[d80harri]

I guess priorities for exception mappers would be a good idea. I just noticed that ExceptionMapperFactory prefers mappers that handle exception that are deeper in the type hierarchy which does not really make sense for me.

Extending from BadRequestException will probably solve my problem, but by doing so I dont even need to implement an exception mapper. This is OK for my usecase but what if third party libraries throw exceptions (that do not extend from Crnk exceptions) or you simple do not want to extend from one of the crnk exceptions because the class that throws the exception does not reside in your rest layer?

Furthermore: Is there a reaseon why CrnkExceptionMapper does not pass the ErrorData object to the exceptions he instantiates? And why does e.g. BadRequestException offer constructors that accept http status codes?

It seems like the exception handling mechanism still needs some tuning. Do you agree? Can I help you out on those issues?

[remmeier]

yes, the constructors need stream-lining, that did not really change this past year.

I considered a number of times touching it. So far it is unmodified from Katharsis behavior I think. There is already a Prioritizable interface that could be used. There are some first places where it is applied, but it should be applied everywhere.

if you have time, PRs would be great. We can also discuss changes further.

[mack1070101]

@remmeier I've recently run across the same/similar issue, and would love to contibute to crnk by fixing this. Do you have any recommendations of where/how to get started on that?