ConversionContext.with(argument) does not take reject(value, exception) into account (fallback to default)

[fabmars]

I want to be able to inject a given header into some controller argument (String) using MyAnnocation. But I also want to be able to give the Value type to that argument and convert the String to that Value using ConversionService.

To reproduce you need to wire some converters into your ConversionService, like so

    @Bean @Singleton
    public TypeConverterRegistrar converterRegistrar(MyConverter myConverter) {
        return conversionService -> conversionService.addConverter(String.class, Value.class, myConverter);
    }

    public ConversionService conversionService(MyConverter myConverter) {
        DefaultConversionService conversionService = new DefaultConversionService();
        return conversionService.addConverter(String.class, Value.class, myConverter);
    }

    @Bean @Primary @Singleton
    public RequestBinderRegistry binderRegistry(ConversionService conversionService,
                                                MyAnnotatedArgumentBinder myAnnotatedArgumentBinder) {
        return new DefaultRequestBinderRegistry(conversionService, myAnnotatedArgumentBinder);
    }

And like so:

@Singleton
public class ValueConverter implements TypeConverter<String, Value> {

    @Override
    public Optional<Value> convert(String value, Class<Value> targetType, ConversionContext context) {
        try {
            return Optional.of(/*conversion occuring here*/);
        }
        catch(IllegalArgumentException e) {
            context.reject(value, e);
            return Optional.empty();
        }
    }
}

See the context.reject(value, e) ? That's where the problem is.

I used an annotation to tell Micronaut it has a request argument to process (like @Body for instance)

@Target({ElementType.FIELD, ElementType.PARAMETER, ElementType.METHOD})
@Retention(RetentionPolicy.RUNTIME)
@Bindable
@Documented
public @interface MyAnnotation {
}

So far so good. Like I wrote earlier, for reasons specific to my project, I must be able to bind 2 different types (say: String and Value). Since the annotation-driven binders of the RequestBinderRegistry are stored in a map whose key is the actual annotation, I can only have 1 one-size-fits-all AnnotatedRequestArgumentBinder and write something like:

@Singleton
public class MyAnnotatedArgumentBinder implements AnnotatedRequestArgumentBinder<MyAnnotation, Object> {

    private final ConversionService<?> conversionService;

    public MyAnnotatedArgumentBinder(ConversionService<?> conversionService) {
        this.conversionService = conversionService;
    }

    @Override
    public BindingResult<Object> bind(ArgumentConversionContext<Object> context, HttpRequest<?> request) {
        Optional<String> stringValue = // get someheader from the request
        if(stringValue.isPresent()) {
            Argument<Object> argument = context.getArgument();
            Class<Object> type = argument.getType();
            if(Value.class.isAssignableFrom(type)) {
                return () -> conversionService.convert(stringValue.get(), context);
            }
            else if(type.equals(String.class)) {
                // simulating Value arg to trigger conversion (string -> string wouldn't do anything)
                DefaultArgument<Object> valueArgument = new DefaultArgument<>(Value.class, argument.getName(), context.getAnnotationMetadata());
                conversionService.convert(stringValue.get(), context.with(valueArgument));
                if(context.hasErrors()) {
                    return BindingResult.UNSATISFIED;
                }
                return () -> Optional.of(stringValue.get());
            } else {
                throw new IllegalArgumentException("Value cannot be assigned to type " + type.getSimpleName());
            }
        } else {
            return BindingResult.EMPTY;
        }
    }

    @Override
    public Class<MyAnnotation> getAnnotationType() {
        return MyAnnotation.class;
    }
}

As you can see I'm using context.with(argument) to "force" the conversionService to do something in one of the 2 types I'm targeting. However, in THAT specific case, context.reject(Objecft value, Exception e) on my ValueConverter will be "swallowed" into the void because the DefaultArgumentConversionContext instance created by context.with(argument) does not override void reject(Object value, Exception exception), hence it's NOT cascaded to the original context. In turn void reject(Exception exception) is overridden and does cascade (but I'm losing the original value information in the process).

Expected Behaviour

HTTP 400 "Failed to convert argument [value] for value [xxxxxxxx] due to: \<exception message>"

Actual Behaviour

In this specific case using reject(Object value, Exception exception), the conversion isn't rejected and the value may be bound (to null if @Nullable) and the overall response's HTTP status isn't 400.

Workaround

Use void reject(Exception exception) but the original value information is lost.

Environment Information

• Operating System: Ubuntu
• Micronaut Version: 1.3.7 (but the code is the same in 2.0.0)
• JDK Version: 1.8

Example Application

The fix being 3 lines, maybe we can skip this.

Fix

Trivial, add this

            @Override
            public void reject(Object value, Exception exception) {
                thisContext.reject(value, exception);
            }

to ConversionContext#with(Argument<T> argument)

This needs to be applied in 1.3.x and 2.0.x

[fabmars]

PR on 1.3.x https://github.com/micronaut-projects/micronaut-core/pull/3729 But it should also be in 2.0.0 (master), awaiting feedback as of what to do there.

[jameskleeh]

Note that this is most definitely not the way to add a type converter:

   @Bean @Primary
    public ConversionService conversionService(MyConverter myConverter) {
        DefaultConversionService conversionService = new DefaultConversionService();
        return conversionService.addConverter(String.class, Value.class, myConverter);
    }

Create a bean that implements TypeConverterRegistrar and use that to register the converter

[fabmars]

Ah yeah I'm new to Micronaut (3 days and counting) and I must have missed the part in the documentation detailing that. Thanks for clarifying. I updated my code example at the top.