Skip to content

Support Ordered interface for @ControllerAdvice beans #23163

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dicksonleong opened this issue Jun 20, 2019 · 6 comments
Closed

Support Ordered interface for @ControllerAdvice beans #23163

dicksonleong opened this issue Jun 20, 2019 · 6 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@dicksonleong
Copy link

dicksonleong commented Jun 20, 2019

Affects: 5.1.8


The ControllerAdvice doc states that:

All such beans are sorted via AnnotationAwareOrderComparator, i.e. based on @Order and Ordered, and applied in that order at runtime.

But when I try to order using the Ordered interface, it is not working, although the @Order annotation is working.

Example below:

@ControllerAdvice
//@Order(1)
class TestAdviceFirst : Ordered {
    override fun getOrder(): Int {
        return 1
    }
    @ExceptionHandler
    @ResponseBody
    fun handleException(e: Exception): String {
        return "Handling exception in TestAdviceFirst"
    }
}

@ControllerAdvice
//@Order(2)
class TestAdvice : Ordered {
    override fun getOrder(): Int {
        return 2
    }
    @ExceptionHandler
    @ResponseBody
    fun handleException(e: Exception): String {
        return "Handling exception in TestAdvice"
    }
}

@RestController
class TestController {
    @GetMapping("/exception")
    fun exception(e: Exception) {
        throw Exception()
    }
}

When @Order is commented out, I get the response "Handling exception in TestAdvice". However when I uncomment @Order, then I get the correct order response: "Handling exception in TestAdviceFirst"

There is also a StackOverflow question but it is unanswered: https://stackoverflow.com/questions/51896436/ordered-interface-is-not-taken-into-account-for-controlleradvice-components

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jun 20, 2019
@sbrannen sbrannen added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Jun 20, 2019
@sbrannen sbrannen self-assigned this Jun 20, 2019
@sbrannen sbrannen added status: pending-design-work Needs design work before any code can be developed and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jun 20, 2019
@sbrannen sbrannen added this to the 5.2 RC1 milestone Jun 20, 2019
@sbrannen sbrannen changed the title @ControllerAdvice not following Ordered interface @ControllerAdvice no longers supports Ordered interface Jun 20, 2019
@sbrannen sbrannen changed the title @ControllerAdvice no longers supports Ordered interface @ControllerAdvice no longer supports Ordered interface Jun 20, 2019
@sbrannen sbrannen changed the title @ControllerAdvice no longer supports Ordered interface @ControllerAdvice Javadoc incorrectly states that Ordered is supported Jun 20, 2019
@sbrannen
Copy link
Member

sbrannen commented Jun 20, 2019

After thorough investigation, I have determined that the Ordered interface was never supported for @ControllerAdvice beans.

In conjunction with gh-19993, the Javadoc was incorrectly updated to state that both @Order and Ordered are supported for @ControllerAdvice beans.

I will therefore update the Javadoc to correct that: only @Order is supported for ordering @ControllerAdvice beans.

I will also likely delete or deprecate the unused code paths in ControllerAdviceBean that honor Ordered since that is essentially dead code.

@dicksonleong
Copy link
Author

Is it possible to add support to order @ControllerAdvice beans with Ordered interface? Or is there any technical reason this is not supported?

My use case is that I need to order my @ControllerAdvice beans in a configurable manner, so I can't use @Order since it needs the order to be hardcoded.

@sbrannen
Copy link
Member

My use case is that I need to order my @ControllerAdvice beans in a configurable manner, so I can't use @Order since it needs the order to be hardcoded.

I understand your desire to be able to do that.

Is it possible to add support to order @ControllerAdvice beans with Ordered interface?

It is technically possible, and I have some ideas regarding how to achieve that, but I am missing a key piece of the puzzle (see below).

Or is there any technical reason this is not supported?

It appears to me that there is in fact a technical reason why it is not currently supported. Namely, each @ControllerAdvice bean is lazily retrieved from the ApplicationContext which means that the ordering cannot be based on Ordered, since the entire set of actual bean instances is not available when the ControllerAdviceBean wrappers are sorted.

The missing piece of the puzzle for me is why that is the case. Specifically, I am not sure why Spring doesn't retrieve all such beans from the ApplicationContext simultaneously -- for example, prior to processing the first request.

I'll discuss the background on this behavior with @rstoyanchev, and we will decide how to proceed.

@sbrannen
Copy link
Member

sbrannen commented Jun 21, 2019

Related Issues and Commits

@dicksonleong
Copy link
Author

Thanks for the explanation! Seems like the original idea is not to initialize the @ControllerAdvice bean when creating and sorting the ControllerAdviceBeans, a wild guess on the reason would be to avoid initialize those beans too early and causing issue when resolving bean dependency?

@sbrannen sbrannen added type: enhancement A general enhancement and removed status: pending-design-work Needs design work before any code can be developed labels Jun 21, 2019
@sbrannen sbrannen changed the title @ControllerAdvice Javadoc incorrectly states that Ordered is supported Support Ordered interface for @ControllerAdvice beans Jun 21, 2019
@sbrannen
Copy link
Member

sbrannen commented Jun 21, 2019

Thanks for the explanation!

You're welcome.

Seems like the original idea is not to initialize the @ControllerAdvice bean when creating and sorting the ControllerAdviceBeans

Yes, that's correct.

a wild guess on the reason would be to avoid initialize those beans too early and causing issue when resolving bean dependency?

Yes, it appears that the intention was to follow the pattern used for looking up handler mappings for controllers which may potentially be defined as prototype beans. However, for a @ControllerAdvice bean we do not think users would define them as prototype beans. Even if a @ControllerAdvice is defined as a prototype bean, it is unlikely that the value returned from getOrder() would change per instantiated prototype. So we think it is safe to change the algorithm in order to support Ordered for @ControllerAdvice beans.

I have updated the title of this issue accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants