Affects: \5.1.9.RELEASE

given:

public abstract class AbstractestController {
    @ModelAttribute("checkC")
    public void checkC() {
        int a = 0;
    }
}

public abstract class AbstracterController extends AbstractestController {
    @ModelAttribute("checkB")
    public void checkB(@ModelAttribute("checkC") Void o) {
        int a = 0;
    }
}

public abstract class AbstractController extends AbstracterController {
    @ModelAttribute("checkA")
    public void checkA(@ModelAttribute("checkB") Void checkB) {
        int a = 0;
    }
}

@RestController
public class TestController extends AbstractController {
    @PostMapping("/test")
    public void test() {
    }
}

When POSTing to /test I expect all three check methods to be called. However checkB is not called.

Workaround:

public abstract class AbstractestController {
    @ModelAttribute("checkC")
    public Object checkC() {
        int a = 0;
        return new Object();
    }
}

public abstract class AbstracterController extends AbstractestController {
    @ModelAttribute("checkB")
    public Object checkB(@ModelAttribute("checkC") Object o) {
        int a = 0;
        return o;
    }
}

public abstract class AbstractController extends AbstracterController {
    @ModelAttribute("checkA")
    public Object checkA(@ModelAttribute("checkB") Object o) {
        int a = 0;
        return o;
    }
}

@RestController
public class TestController extends AbstractController {
    @PostMapping("/test")
    public void test() {
    }
}

Comment From: mdeinum

That isn't a workaround, that is how @ModelAttribute is supposed to work. Adding @ModelAttribute on an void method (and not passing in a Model as method argument) doesn't really make sense. So your workaround is the actual way it is supposed to work.

What is the use case for adding @ModelAttribute on void methods? It feels wrong, have an @ModelAttribute method but it doesn't contribute anything to the model.

Comment From: ravenblackdusk

The use case is running some code that checks some stuff and throws an exception if requirements are not met before each @RequestMapping in certain @Controllers. Other options for implementing this are not as clean(for example interceptors or filters). I'm calling this inconsistent behavior since in my example checkC and checkA methods are called in the expected order but checkB is left out and I'm calling it a workaround since the new Object() in my example is just a dummy object(unnecessary garbage) to make the methods get called in the expected order. Two solutions come to mind: 1. Fully support Void type for @ModelAttributes. 2. Do not support Void type for @ModelAttributes and document this and throw a runtime error during startup(checkB method was silently being ignored in my project for a long time) and perhaps create another annotation for such use cases.

Comment From: mdeinum

void methods are supported but require a Model or the likes as a method attribute, to add something to the model. You are abusing it for something it isn't designed for (validation). If it is cross-cutting it should really be in an interceptor or validator (but not sure what you are trying to achieve).

Looks more like a question for stack overflow to determine the right component to work with then a bug (maybe an enhancement or documentation ticket).

Comment From: ravenblackdusk

I was talking about @ModelAttribute Void o as a method argument rather than methods with void return type when I said they should be fully supported or explicitly unsupported; right now they are working in an undefined manner(its not understood why checkC and checkA are called in the expected order but checkB is not) which I think is the main problem rather than the main problem being them not working the way I want. Determining what better options I have rather than the workaround that I'm using now is a separate issue. I think the solution that is to be chosen dictates the type of the issue. if @ModelAttribute java.lang.Void o is to be fully supported then perhaps this is a bug.

Comment From: bclozel

The use case you've chosen are not supported nor advised. As @mdeinum said, the behavior of @ModelAttribute methods and arguments is defined in the reference documentation.

The documentation also states that:

You can use the @ModelAttribute annotation on a method argument to access an attribute from the model or have it be instantiated if not present.

While the documentation doesn't clearly states that Void is not supported, it explains the goal of this feature. Since you can't instantiate Void objects in the Java programming language, Spring Framework is being lenient here but might enforce stricter rules in the future.

Depending on your use case, I'd advise interceptors, validation or other extension points that Spring is providing.

Comment From: rstoyanchev

Keep in mind this probably isn't doing what you think it is:

    @ModelAttribute("checkB")
    public void checkB(@ModelAttribute("checkC") Void o) {
        int a = 0;
    }

The annotation means, invoke this method and add the return value as a model attribute named '"checkB". The method however returns no value and consequently nothing is added to the model. We could flag this as an error, but I don't see what else we could do. Adding Void as model attribute at that point would be much stranger than raising an error.

Comment From: ravenblackdusk

@rstoyanchev throwing an error is way better than the current behavior which is silently failing in undefined cases and working in other cases. Though I'm not sure if Void model attributes are that strange. If you think of it in kotlin terms a void method in java is either a kotlin.Unit or kotlin.Nothing and kotlin.Unit somewhat makes sense as it is an object after all. You can store it in a variable. But since it can only have one value your logic would not depend on the value hence there is no need to store it in a variable since the variable will be unused but these are simplifications that we make and are not mandatory. For example you can have a method that returns kotlin.Any and sometimes it might return Unit, other times some other object. @bclozel if by interceptors you mean writing stuff like:

if (requestURI.startsWith("/foo-controller") || requestURI.startsWith("/bar-controller") || ...) {
    doSomething();
}
if (requestURI.startsWith("/baz-controller") || requestURI.startsWith("/bar-controller") || ...) {
    doSomethingElse();
}

This is a poor choice for multiple obvious reasons. And by validation do you mean stuff like spring hibernate validation? I want to validate that the request has some headers and/or params with complex database dependent logic and I don't want to put extra method arguments for every request mapping in those controllers.

Comment From: bclozel

I don't get your point about Any and Unit - in all cases, Spring Framework won't be able to instantiate a Void type, while the goal of this feature is to instantiate something to be added to the model.

You could take a look at MappedInterceptor and still reconsider that solution, as you seem to describe something that crosses controller boundaries and might even apply to non-controller handlers.

Comment From: ravenblackdusk

@bclozel Spring Framework is already instantiating the Void type; in case of void checkA(@ModelAttribute("checkB") Void checkB) the method argument is instantiated by using org.springframework.beans.BeanUtils#instantiateClass(java.lang.reflect.Constructor<T>, java.lang.Object...) although I'd rather cache the Void instance. That's my main objection to this behavior that it kind of supports Void type but not completely. Someone like me might see that one of the methods is being called and assume that all are being called. In my use case I don't have any non-controller handlers. Consider this example which is much closer to my real use case:

public class AbstractAuthenticatedGatewayController {
    @Autowired
    CurrentGateway currentGateway;

    @ModelAttribute(GATEWAY_CHECK)
    public Object checkGatewayAndConnector() {
        ofNullable(currentGateway.get())
                .orElseThrow(() -> new HttpClientErrorException(BAD_GATEWAY, "gateway not registered"));
        return new Object();
    }
}

public abstract class AbstractAuthenticatedTenantController extends AbstractAuthenticatedGatewayController{
    @Autowired
    CurrentGateway currentGateway;
    @Autowired
    CurrentTenant currentTenant;

    @ModelAttribute(TENANT_CHECK)
    public Object checkTenant(@ModelAttribute(GATEWAY_CHECK) Object o) throws TenantNotFoundException {
        ofNullable(currentTenant.get()).orElseThrow(TenantNotFoundException::new);
        return o;
    }
}

Some controllers extend AbstractAuthenticatedTenantController, some extend both and others extend none. MappedInterceptor still has the following short-commings:

  1. If I have two different controllers with @RequestMapping("/foo") should I include "/foo" and manually exclude all sub mappings of the other controller that doesn't extend the super classes?
  2. You still end up using if statements if you need to match request methods.
  3. @ModelAttributes are defined where they should. The behavior of the controller is defined by its type. Interceptors can be defined anywhere.

Comment From: rstoyanchev

Switching to 5.3 to minimize unnecessary noise for existing applications. If a developed actually wanted to have a Void attribute added in this way, they would discovered long ago it doesn't have the desired effect. No need to raise alarms for something that isn't an issue in a maintenance release.