Skip to content
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

Document ControllerAdviceBean as internal usage #32776

Open
sinanoezdemir opened this issue May 7, 2024 · 9 comments
Open

Document ControllerAdviceBean as internal usage #32776

sinanoezdemir opened this issue May 7, 2024 · 9 comments
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: documentation A documentation task
Milestone

Comments

@sinanoezdemir
Copy link

sinanoezdemir commented May 7, 2024

Enhancement/ Misleading description in javadoc.

In the Javadoc of ControllerAdviceBean you can read

Encapsulates information about an @ControllerAdvice Spring-managed bean without necessarily requiring it to be instantiated.
The findAnnotatedBeans(ApplicationContext) method can be used to discover such beans. However, a ControllerAdviceBean may be created from any object, including ones without an @ControllerAdvice annotation.

ControllerAdviceBean

Reading this I assume, that it should be possible to create ControllerAdviceBeans and register them programmatically (without using the @ControllerAdvice annotation).

As far as I understood the ExceptionHandlerExceptionResolver is the class keeping the ControllerAdviceBeans to apply matching ExceptionHandlers when an exception occurs.

However, when initialising the exceptionHandlerAdviceCache it uses ControllerAdviceBean.findAnnotatedBeans which scans the application context for beans annotated with @ControllerAdvice.

The exceptionHandlerAdviceCache is otherwise not modifiable. So there is actually no other way to add ControllerAdviseBeans besides using the annotation.

I think that's why the Javadoc is misleading.

However, it would be great to have the ability to define ControllerAdviseBeans without using the @ControllerAdvice annotation.

  • Context: I am developing in a micro service environment and we have multiple micro services using the same ControllerAdvise which we extracted into a Library. I do not want libraries to contain spring component annotations. I rather would like to instantiate a bean in the configurations of each micro service. With the current implementation it's not possible to "just" remove the annotation and instantiate that bean in a configuration.

I assumed something similar to this should work.

@Configuration
public class SomeConfiguration {

  @Bean
  public ControllerAdviceBean myControllerAdvice() {
    return new ControllerAdviceBean(new MyControllerAdvice());
  }

}

//Example
public  class MyControllerAdvise {

    @ExceptionHandler({Exception.class})
    public ResponseEntity<Object> handleRemainingExceptions(Exception exception, WebRequest request) {
      // does something
      return ResponseEntity.internalServerError().body("Something went wrong");
    }
    
 }

Please excuse me if I misunderstood a concept here. Just trying to suggest an improvement.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label May 7, 2024
@rishiraj88
Copy link

I agree with you, @sinanoezdemir , about your note on the Javadoc of ControllerAdviceBean (in the beginning of your comment). I, too, find the exact match between your statements and the Javadoc statements. That should be altered.

However, pardon me for not diving deep into your usecase and context of micro services.

@snicoll snicoll added the in: web Issues in web modules (web, webmvc, webflux, websocket) label May 8, 2024
@bclozel
Copy link
Member

bclozel commented May 21, 2024

Hello @sinanoezdemir , thanks for bringing this up. We have discussed this today as a team and I'll summarize here.

We're not sure if you don't want your shared library to be scanned for components, or if it's scanned by micro-services but you would like to make this bit optional.

  1. If you don't want your library to be scanned by mistake, this is a quite common concer. Spring Boot is contributed numerous configuration classes that should never be scanned. If an application scans the org.springframework.boot namespace, this is considered as an invalid setup. There might be ways to explicitly reject such situations and tell the user.
  2. If your library is scanned but you would like to optionally contribute this component, there are many ways to express that with Spring: profiles, conditions, functional registration of beans...

Are your applications using Spring Boot? If so, you could contribute your own auto-configurations and never rely on scanning for your library components. Your classes would still be annotated with @ControllerAdvice but could be conditionally contributed to the application context.

Now depending on the outcome of this discussion, we might revisit the ControllerAdviceBean javadoc and part of its implementation: this is a public type that needs to be shared for Spring MVC and WebFlux, but it's not meant for public use. As you've found out, non-annotated @ControllerAdvice instances are not supported at the moment and we could streamline the current implementation to reflect that as it's only partially there.

@bclozel bclozel added the status: waiting-for-feedback We need additional information before we can continue label May 21, 2024
@sinanoezdemir
Copy link
Author

sinanoezdemir commented May 24, 2024

@bclozel thanks for your response.

I just mentioned this issue because of what you sad in your last sentence

As you've found out, non-annotated @ControllerAdvice instances are not supported at the moment

In our team we try to avoid relying on component scanning for libraries. Instead we want to manually configure the beans within the services and use the libraries like a blue print for these kind of things. In this case exception handling.

I have a working workaround but I think it would be great if it would be possible to register ControllerAdvices by annotation and programmatically. Off course I understand that this is probably easier sad then done.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels May 24, 2024
@bclozel
Copy link
Member

bclozel commented May 24, 2024

In our team we try to avoid relying on component scanning for libraries. Instead we want to manually configure the beans within the services and use the libraries like a blue print for these kind of things. In this case exception handling.

Thanks for your response.

I have a working workaround but I think it would be great if it would be possible to register ControllerAdvices by annotation and programmatically. Off course I understand that this is probably easier sad then done.

I don't think that pointing to a class would really qualify as programmatic registration. This class would still contain @ExceptionHandler method and would still be annotated-driven. You could look at how we handle programmatic registration of mappings for controllers in Spring MVC. As you can see, there is no annotation involved here anymore.

Are your applications using Spring Boot? If so, you could contribute your own auto-configurations and never rely on scanning for your library components. Your classes would still be annotated with @ControllerAdvice but could be conditionally contributed to the application context.

The auto-configuration approach would be more aligned with your vision of programmatic registration in this case. Have you considered this?

@bclozel bclozel added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels May 24, 2024
@qzmer1104
Copy link

qzmer1104 commented May 24, 2024

I have the same concerns。

1、common-spring.jar
@RestControllerAdvice(basePackages="com.a") class GlobalExceptionHandler {}
org.springframework.boot.autoconfigure.AutoConfiguration.imports
xxx.xxx.GlobalExceptionHandler

2、app-1
app package
com.a.xxxxConotroller
pom.xml
maven import common-spring.jar

GlobalExceptionHandler is useful

2、app-2
app package
com.c.controller
pom.xml
maven import common-spring.jar
GlobalExceptionHandler is useless

We may no need to package a new common-spring.jar just only modify the basePackages

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels May 24, 2024
@bclozel
Copy link
Member

bclozel commented May 24, 2024

@qzmer1104 so you are looking for something like this, but for exception handling?

Have you considered custom auto-configurations?

@bclozel bclozel added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels May 24, 2024
@sinanoezdemir
Copy link
Author

Thank you, @bclozel, for your responses.

I agree that @ExceptionHandler and @configuration would still be necessary, but that is acceptable. My main goal was to avoid relying on component scanning. With the configuration class, you have to register the bean manually, and the library containing the exception handlers does not create beans by itself. We try to avoid the creation of beans within libraries whenever possible.

We also considered auto-configuration, and that approach would work. There are ways to implement a workaround, so that is not the issue. I just thought it would be beneficial if the framework supported this method of declaring controller advices.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels May 28, 2024
@bclozel
Copy link
Member

bclozel commented May 28, 2024

We have discussed this as a team and think that conditional configurations or Spring Boot auto-configurations are the preferred way of handling this use case.

We'll repurpose this issue to update the ControllerAdviceBean Javadoc to make it obvious that this is for internal use (it's really public because it must be used from different packages). In the process, we should review its constructors and code paths to check whether some of those partially implement this "programmatic registration" feature that we don't intend to support. If we find any, we should deprecate/remove those code paths.

@bclozel bclozel added type: documentation A documentation task and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels May 28, 2024
@bclozel bclozel added this to the 6.x Backlog milestone May 28, 2024
@bclozel bclozel changed the title Adding ControllerAdviceBean programatically is not possible. Document ControllerAdviceBean as internal usage May 28, 2024
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: documentation A documentation task
Projects
None yet
Development

No branches or pull requests

7 participants