Allow @ControllerAdvice to be cofigured with a join point to target a subset of controller [SPR-10222]

[spring-projects-issues]

Adib Saikali opened SPR-10222 and commented

I have two types of controllers in my spring application.

• View controllers that forward to views to generate HTML
• API controllers that return JSON directly from the controllers

Both the API and View controllers are part of the same spring dispatcher servlet.

The documentation implies that @ControllerAdvice will be applied to every controller associated with a Dispatcher Servlet. With advice as part of the name I expected to be able to specify the pointcut and or join points that the advice applies to, but can't find out how that can be.

For example in my scenario I want a @ControllerAdvice for my View Controllers and separate @ControllerAdvice for my API controllers.

It would be great to provide a way to configure which controllers @ControllerAdvice will apply to.

---

Affects: 3.2 GA, 3.2.1

Referenced from: commits c4a8bf9, cfb6625, 4f28c77

1 votes, 6 watchers

[spring-projects-issues]

Michael Osipov commented

A very valid point, I have the same problem with the @ControllerAdvice - it is too general. The only option is to create two distinct DispatcherServlets. Although most exception handlers are not REST compatible, imho.

[spring-projects-issues]

Rossen Stoyanchev commented

We can use one or more type filters. How do you plan to distinguish view controllers vs JSON serving controllers, package, class name?

[spring-projects-issues]

Michael Osipov commented

In my case I share DAOs but seperate controllers by package like this: com.company.ou.app for views and com.company.ou.rest for REST.

[spring-projects-issues]

Rossen Stoyanchev commented

The current proposal is to allow configuring package names through the annotation (see example test). If you've additional use cases that you'd like to have considered, please speak up.

[spring-projects-issues]

Adib Saikali commented

It is not clear from the test how the rules apply to sub packages. For example in my application I have.

com.example.api.GlobalErrorHandler // contain exception handlers or api controllers located in module common.jar 
com.example.feature1.Feature1Controller // Feature1Controller extends BaseApiController and is located in feature1.jar
com.example.feature2.Featuer2Controller // Feature2Controller extends BaseApiController and is located in feature2.jar 

As I understand it from the example test I will have to annotate the my GolbalErrorHandler like so.

@ControllerAdvice({"com.example.feature1","com.example.feature2"})
public class GlobalErrorHandler { 
   // common @ExceptionHandler methods are defined here
}

When I add feature3 in com.example.feature3 which will be packaged in feature3.jar I will have go to my common.jar and add the feature3 package to the common error handler. So this means that general purpose error handling code in the global error handler is now not really general purpose because it has to be explicitly configured for features / jar that it does not need to know that they are on the classpath.

Could we have it target by saying something like

@ControllerAdvice(subClass=BaseApiController.class)
public class GlobalErrorHandler { 
   // common @ExceptionHandler methods are defined here
}

I think this approach works better because both BaseApiController.class and GlobalErrorHandler.class are in the same common.jar module and therefore when component scanning picks up the controllers in feature1.jar, feature2.jar, feature3.jar .... etc where Feature1Controller extends BaseApiController, Feature2Controller extends BaseApiController ... etc all the right things will just happen without having to go back and modify any extra configuration.

[spring-projects-issues]

Brian Clozel commented

Hi Adib Saikali!

We're trying to cover most needs with the simplest implementation here.
Many developers are organizing Controllers in packages, depending on their actual purpose (REST API, views, etc) - applying @ControllerAdvice on specific packages makes sense in that case.

It looks like you're trying to build your app in modules (maybe you're even using the great spring-plugin project?). This implementation does support base packages, so you could try:

@ControllerAdvice({"com.example.web.api"})
public class GlobalErrorHandler { 
   // common @ExceptionHandler methods are defined here
}

com.example.web.api.feature1.Feature1Controller 
com.example.web.api.feature2.Featuer2Controller

Your use case is interesting and there are many possible ways to apply @ControllerAdvice on Controllers; we also could use user-specific "marker annotations" (kind of like @RestController) - we could actually consider that for @ControllerAdvice in the future. Take a look at how @ComponentScan handles those cases.

For the time being, I think base packages cover a lot of use cases - and we should see how things unfold in the next RCs.

[spring-projects-issues]

Adib Saikali commented

Hi Brian!

I am not using the spring-plugin project. At the start my application was partitioned by stereotype of object. I had packages such as com.example.web,com.example.persistence,com.exmaple.service ... etc but as the application grew and some features were stable and other new features were being rapidly added the organization of the code would not scale.

So I have since partitioned my application into 5 utility modules and 20 separate feature modules that produce jars, and 1 web module that produces a war. The web module only contains static resources, js files, and few traditional MVC controller rather than rest controllers.

By doing the split into feature modules working on the application became a lot easier. It is much easier to find all the code for a feature, git history is easier to deal with to see the evolution of a specific feature, there are feature modules that I have not looked at in months because they just work and when refactoring they don't get touched.

Building on top of component scanning, Spring Java Config, and flywaydb organizing into feature modules is a better solution and more practical solution, for integration testing you can easily start up subsets of the whole application just run the tests for those parts of the system. So if I am working on feature module #7 I can use the Spring MVC Test to just test my controller, services, repositories all the way down to the database simply from the the IDE. I have integration testng tests that launch an embedded tomcat and through the magic of component scanning end up launching a subset of the application which is great for isolating where problems and keep the application rest backend partitioned into services that can be split into into separate servers when the time is right.

I don't know if my use case is the most common use case but I believe that it is the better programming model for working with spring based applications. I think that projects such as Spring Boot with its emphasis on an just having a main method to launch things from will make decomposition into feature modules more desirable and practical. So I hope that you create a design and implementation that can accommodate the feature module use case for Spring 4.0.

[spring-projects-issues]

Rossen Stoyanchev commented

Adib Saikali, just to be sure this is clear, you don't need to specify any packages at all if you want @ControllerAdvice to apply everywhere. You probably know that but I mention it because it wasn't clear to me from the example you gave what controllers need to be excluded. That's a fairly important question in order to understand how this applies to your use case.

[spring-projects-issues]

Adib Saikali commented

Rossen Stoyanchev, Thanks for asking me to clarify the usecase. Here is my ultimate use case in more detail.

I have a utility module called rest-utils.jar in which has a bunch of code that allows to do global error handling for REST APIs by having a RestApiController base class and it has helper classes for working with SpringRestTemplates for example there is a nice login method to log in to the API, there are nice tracing options to print out the request json and response json so I can use these helpers to create example of how to use the API that the JavaScript developers can use. Much of the code in res-utils.jar is not specific to one application can be used in many different apps that use the same architecture as the application I am currently working on.

What I want is way to put global error handling code in rest-utils.jar such that if I drop the rest-utils.jar into my classpath of an application then the error handling logic in the rest-utils.jar would apply to those controllers.

Here is a subset of what is currently in my BaseApiController which is in rest-utils.jar

@Controller
public class BaseApiController
{
	private static Logger logger = LoggerFactory.getLogger(BaseApiController.class);
	protected final static String JSON = "application/json";

	protected ResponseEntity<RestApiError> createResponseEntity(RestApiError restApiError)
	{
		HttpHeaders headers = new HttpHeaders();
		headers.setContentType(MediaType.APPLICATION_JSON);

		HttpStatus responseStatus = HttpStatus.valueOf(restApiError.getHttpStatus());
		ResponseEntity<RestApiError> result = new ResponseEntity<>(restApiError, headers, responseStatus);
		return result;
	}

	@ExceptionHandler(RestApiException.class)
	public ResponseEntity<RestApiError> handleRestApiException(RestApiException e)
	{
		logger.error("Api Error caused by exception", e);
		return this.createResponseEntity(e.getRestApiError());
	}


	@ExceptionHandler(InvalidInputException.class)
	public ResponseEntity<RestApiError> handleInvalidInputException(InvalidInputException e)
	{
		logger.error("Api Error caused by exception", e);
		RestApiError restApiError = new RestApiError(RestApiHttpStatus.BAD_REQUEST);
		restApiError.setApiCode(ApiErrorCodes.ERROR_CODE_TYPE_MISMATCH_EXCEPTION);
		restApiError.setUserMessage("An Error has occured and the request could not be completed");
		restApiError.setDeveloperMessage(e.getMessage());

		return createResponseEntity(restApiError);
	}
}

Here is what a feature controller would look like in my news.jar which also contains the database entities, services ... etc.

@Controller
public class NewsController extends BaseApiController
{
	@Autowired
	private NewsService newsService;

	@RequestMapping(value = Company.NEWS_POSTS, method = RequestMethod.POST)
	@ResponseStatus(value = HttpStatus.OK)
	@ResponseBody
	public NewsPostJson createNewPost(@Valid @PathVariable("id") Integer id, @Valid @RequestBody NewsPostJson newsPostJson)
	{
		
	}
}

What I want to be able to do is to convert the BaseApiController into some sort of advice such that the NewsController would not need to extend BaseApiController but when I drop rest-utils.jar into the application classpath I get the standard error handling mechanism applied to my rest controllers. I know this make previous suggestion to target the advice based on the class subtype wrong.

If it was possible for me to define my own controller sterotype like so.

@Target({ElementType.TYPE})
@Retention(RetentionPolicy.RUNTIME)
@Controller
public @interface StandardController 
{
}

Then the BaseApiController becomes

@ControllerAdvice(steroype=StandardController.class)
public class StandardApiErrorHandler
{
	private static Logger logger = LoggerFactory.getLogger(BaseApiController.class);
	protected final static String JSON = "application/json";

	protected ResponseEntity<RestApiError> createResponseEntity(RestApiError restApiError)
	{
		HttpHeaders headers = new HttpHeaders();
		headers.setContentType(MediaType.APPLICATION_JSON);

		HttpStatus responseStatus = HttpStatus.valueOf(restApiError.getHttpStatus());
		ResponseEntity<RestApiError> result = new ResponseEntity<>(restApiError, headers, responseStatus);
		return result;
	}

	@ExceptionHandler(RestApiException.class)
	public ResponseEntity<RestApiError> handleRestApiException(RestApiException e)
	{
		logger.error("Api Error caused by exception", e);
		return this.createResponseEntity(e.getRestApiError());
	}


	@ExceptionHandler(InvalidInputException.class)
	public ResponseEntity<RestApiError> handleInvalidInputException(InvalidInputException e)
	{
		logger.error("Api Error caused by exception", e);
		RestApiError restApiError = new RestApiError(RestApiHttpStatus.BAD_REQUEST);
		restApiError.setApiCode(ApiErrorCodes.ERROR_CODE_TYPE_MISMATCH_EXCEPTION);
		restApiError.setUserMessage("An Error has occured and the request could not be completed");
		restApiError.setDeveloperMessage(e.getMessage());

		return createResponseEntity(restApiError);
	}
}

I hope this clarifies my use case.

[spring-projects-issues]

Brian Clozel commented

Adib Saikali, thank for this valuable feedback.
I just updated the PR (still work in progress though) to give you a chance to take another look at it.
You can check one of the test classes for examples and the new documentation for @ControllerAdvice.

Thanks!

[spring-projects-issues]

Adib Saikali commented

Brian this is looking really great, being able to select controller to apply the advice to based on subtypes, packages, or annotations is really cool, I think this will meet my needs. Thanks very much for being open to the feedback.