katharsis-project / katharsis-framework

Katharsis adds powerful layer for RESTful endpoints providing implementenation of JSON:API standard
http://katharsis.io
Apache License 2.0
135 stars 65 forks source link

Can't create ExceptionMapper subclasses #155

Open Ramblurr opened 8 years ago

Ramblurr commented 8 years ago

We have some code that is shared by all ExceptionMappers. I implemented this in an abstract class:

public abstract class BaseExceptionMapper<E extends Throwable> implements ExceptionMapper<E> { 
...

Then extended the class in the actual exception mappers:

@ExceptionMapperProvider
public class FooExceptionMapper extends BaseExceptionMapper<APIException>
{

This causes NPEs at runtime because ExceptionMapperRegistryBuilder.getGenericType() returns null:

// in ExceptionMapperRegistryBuilder.java
   private Class<? extends Throwable> getGenericType(Class<? extends JsonApiExceptionMapper> mapper) {
        Type[] types = mapper.getGenericInterfaces();
        for (Type type : types) {
            if (type instanceof ParameterizedType && (((ParameterizedType)type).getRawType() == JsonApiExceptionMapper.class || ((ParameterizedType)type).getRawType() == ExceptionMapper.class)) {
                //noinspection unchecked
                return (Class<? extends Throwable>)((ParameterizedType)type).getActualTypeArguments()[0];
            }
        }
        //Won't get in here      <------ it did get here
        return null;
    }

Would be happy to submit a patch here, but what is the correct behavior?

  1. Should this method walk the inheritance tree to find the generic exception type?
  2. Should a custom exception mapper registry be used?

At least I think this method should throw an exception rather than returning null, as it will definitely NPE later on.

chb0github commented 8 years ago

This code shouldn't return a 'null' but a Throwable.class (but even that isn't truly determinable due to erasure, just assumed). The slickest (and probably correct) thing would be to walk the tree. Not too tough. I have code sitting around that does this very thing for an almost identical purpose.

If you're suggesting a pull request, I will review it.

On Mon, Oct 10, 2016 at 7:33 AM Casey Link notifications@github.com wrote:

We have some code that is shared by all ExceptionMappers. I implemented this in an abstract class:

public abstract class BaseExceptionMapper implements ExceptionMapper { ...

Then extended the class in the actual exception mappers:

@ExceptionMapperProviderpublic class FooExceptionMapper extends BaseExceptionMapper {

This causes NPEs at runtime because ExceptionMapperRegistryBuilder.getGenericType() returns null:

// in ExceptionMapperRegistryBuilder.java private Class<? extends Throwable> getGenericType(Class<? extends JsonApiExceptionMapper> mapper) { Type[] types = mapper.getGenericInterfaces(); for (Type type : types) { if (type instanceof ParameterizedType && (((ParameterizedType)type).getRawType() == JsonApiExceptionMapper.class || ((ParameterizedType)type).getRawType() == ExceptionMapper.class)) { //noinspection unchecked return (Class<? extends Throwable>)((ParameterizedType)type).getActualTypeArguments()[0]; } } //Won't get in here <------ it did get here return null; }

Would be happy to submit a patch here, but what is the correct behavior?

  1. Should this method walk the inheritance tree to find the generic exception type?
  2. Should a custom exception mapper registry be used?

At least I think this method should throw an exception rather than returning null, as it will definitely NPE later on.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/katharsis-project/katharsis-framework/issues/155, or mute the thread https://github.com/notifications/unsubscribe-auth/ABaI0EMv3yj4J1ZO6Ywdq4Orposqwohbks5qykzQgaJpZM4KSoYS .