Open Source, Physics & Politics

Recently, I spent a lot of time on debugging a nasty problem with Spring WebMVC and Spring Security.

I had a class annotated with @Controller and a method annotated with @RequestMapping. I wanted to protected this method using the @Secured annotation. So I turned on global method security by adding @EnabledGlobalMethodSecurity with the right parameters to my @Configuration class, but it did not work. The method could still be called without having the proper privileges (or being authenticated at all).

After hours of debugging, I found out that the AOP advice was not applied to my controller method because it would not find the method when processing the controller class. At that moment I realized that the method had been declared package private. AOP proxies are not applied to non-public methods (for CGLIB proxies this would be possible, but in general it is not desirable and Spring does not do it).

This left me with the question: Why does the request mapping work. The answer is simple: When looking for methods with the @RequestMapping annotation, Spring does not check the method's access modifiers. As the method is invoked using reflection, it will work even if the method has been declared private (unless there is a SecurityManager in charge, but for most Spring applications there will not be one).

This leaves us with a very awkward situation: Private methods might be called by external code and if there is an @Secured annotation on them, it will be ignored. In my opinion, this is a bug: The @RequestMapping annotation should only work on public methods. There are actually four places in Spring where this could be fixed (Spring 4.1.7):

1. AbstractHandlerMapping.java: line 172
2. AbstractHandlerMapping.java: line 207
3. HandlerMethodSelector.java: line 60
4. RequestMappingHandlerMapping.java: line 187

It would be completely sufficient to check whether the method is public in one of these places. Until this is fixed in Spring (and it might never get fixed because the fix would break backward compatibility), I use my own RequestMappingHandlerMapping which does the check:

public class PublicOnlyRequestMappingHandlerMapping extends
        RequestMappingHandlerMapping {

    @Override
    protected RequestMappingInfo getMappingForMethod(Method method,
            Class<?> handlerType) {
        RequestMappingInfo info = super
                .getMappingForMethod(method, handlerType);
        if (info != null && !Modifier.isPublic(method.getModifiers())) {
            logger.warn("Ignoring non-public method with @RequestMapping annotation: "
                    + method);
            return null;
        } else {
            return info;
        }
    }

}

As you can see, the implementation is very simple. I first call the super method and then check whether the method is public so that I can generate a warning message when @RequestMapping has been used on a non-public method. If one does not care about such a message, once can check the method's access modifier fist and only invoke the super method when the investigated method is public.

In order to use the custom RequestMappingHandlerMapping, we have to use a custom implementation of WebMvcConfigurationSupport (when using Java Config):

@Configuration
public class CustomWebMvcConfiguration extends
        DelegatingWebMvcConfiguration {

    @Bean
    @Override
    public RequestMappingHandlerAdapter requestMappingHandlerAdapter() {
        RequestMappingHandlerAdapter adapter = super
                .requestMappingHandlerAdapter();
        adapter.setIgnoreDefaultModelOnRedirect(true);
        return adapter;
    }

    @Bean
    @Override
    public RequestMappingHandlerMapping requestMappingHandlerMapping() {
        RequestMappingHandlerMapping handlerMapping = new PublicOnlyRequestMappingHandlerMapping();
        handlerMapping.setOrder(0);
        handlerMapping.setInterceptors(getInterceptors());
        handlerMapping
                .setContentNegotiationManager(mvcContentNegotiationManager());

        PathMatchConfigurer configurer = getPathMatchConfigurer();
        if (configurer.isUseSuffixPatternMatch() != null) {
            handlerMapping.setUseSuffixPatternMatch(configurer
                    .isUseSuffixPatternMatch());
        }
        if (configurer.isUseRegisteredSuffixPatternMatch() != null) {
            handlerMapping.setUseRegisteredSuffixPatternMatch(configurer
                    .isUseRegisteredSuffixPatternMatch());
        }
        if (configurer.isUseTrailingSlashMatch() != null) {
            handlerMapping.setUseTrailingSlashMatch(configurer
                    .isUseTrailingSlashMatch());
        }
        if (configurer.getPathMatcher() != null) {
            handlerMapping.setPathMatcher(configurer.getPathMatcher());
        }
        if (configurer.getUrlPathHelper() != null) {
            handlerMapping.setUrlPathHelper(configurer.getUrlPathHelper());
        }

        return handlerMapping;
    }

}

This implementation copies the implementation of requestMappingHandlerMapping() from the parent class, but replaces the actual implementation used with our own class. In addition to that, this configuration also overrides requestMappingHandlerAdapter() in order to the the ignoreDefaultModelOnRedirect attribute. This is the recommended setting for new Spring WebMVC applications, but it cannot be made the default in Spring because it would break backward compatibility. Of course, the two changes are completely independent, so you can choose to only implement either of them.

One of the rather obscure features of the Java programming language is the support for checked exceptions. Most other languages running on the Java virtual machine (JVM) do not have them and most non-JVM programming languages do not have them either.

You might be surprised that I call checked exceptions "obscure" even though it is easy to understand their concept and to use them. However, I suspect that most experienced Java developers share my sentiment (if you don't, please speak up in the comments), while it is anything but obvious to beginners why checked exceptions might be problematic.

Actually, I have to admit that when I first learned the Java programming language (which must have been around Java 1.2 or 1.3), I liked the concept of checked exceptions. I typically prefer statically typed languages over dynamically typed ones because I like to have every support that a compiler can give me in statically verifying my code. Checked exceptions seem to be a logical extension of this concept, where the compiler can check whether all error conditions that might occur are actually handled by the code.

Unfortunately, the concept of checked exceptions has rather severe limitations which become apparent in larger projects. In this article, I want to explore why checked exceptions are a good idea that unfortunately fails when being put to practical use. I hope that this might be useful to Java beginners who are writing their first library and have to decide where to use checked and where to use unchecked exceptions.

Before taking a closer look at the problems with checked exceptions, we want to quickly revisit the top level of the exception hierarchy in Java and how the three different types of exceptions are handled differently.

In Java, all exceptions inherit from Throwable. There are three distinct types of exceptions: Unchecked exceptions that signal an error condition in the JVM (for example when a class cannot be loaded or a memory allocation fails) are derived from Error, which in turn is derived from Throwable. These exceptions are unchecked, which means that they can be thrown by any code without having been declared explicitly. Exception, which is also derived from Throwable, is the base class for all checked exceptions. Checked exceptions have to be declared explicitly in a method declaration. If a method declares that it throws a checked exception, the calling code must either catch this exception or must also declare that it throws the exception. Finally, there is the RuntimeException which is derived from Exception, but like Error is a base class for unchecked exceptions.

Even though both Errors and RuntimeExceptions represent unchecked exceptions, they are used for different purposes. Errors are typically thrown by the JVM only and are typically non-recoverable. For example, it is hard to recover from an error when loading a class, because this is typical caused by a problem with the class file. Therefore, Errors are rarely caught but will typically lead to program termination. Even if they are caught, the program will often behave erratically after getting such an exception (everyone who has experienced an OutOfMemoryError in Eclipse knows what I am talking about). RuntimeExceptions, on the other hand, often signal errors in the program's logic. For example, a NoSuchElementException happens when trying to get a non-existing element from a List.

Exception is used for checked exceptions which are typically caused by an exceptional situation (not necessarily considered an error). For example, an IOException is triggered when an I/O operation cannot be finished. Such a situation might not necessarily indicate an error, because it can simply happen when trying to access a resouce that no longer exists (e.g. a network connection might have been closed by a peer).

In summary, exceptions of type Error are typically only thrown by the JVM, exceptions of type RuntimeException are thrown by Java code, but usually do not have to be expected, and exceptions of type Exception (checked exceptions) have to be expected and need to be handled somehow.

In my opinion, there are two flaws in this concept: a minor obvious one and two major less obvious ones. The minor flaw is the class hierarchy. RuntimeException, the base class for unchecked exceptions, is derived from Exception, the base class for checked exceptions. It would be more reasonable to derive RuntimeException from Throwable directly, but this design flaw does not cause any actual trouble.

The first major flaw is that the distinction between exceptions that have to be expected (and thus should be checked exceptions) and exceptions that do not have to be expected (and thus should be unchecked exceptions) is not always clear. What if an exception has to be expected but cannot be reasonably handled locally? For example, a FileNotFoundException might be non-recoverable if an important configuration or database file is missing. There are three potential solutions for such a case: We can let the exception bubble up the stack (which means that now a lot of methods have to declare that they throw a FileNotFoundException), we can wrap it in a different kind of exception (e.g. in a MyLibraryException), or we can wrap it in a RuntimeException.

The first solution is problematic because of the second flaw described in the next paragraph. The second solution (which is the one recommended in the API docs) is not perfect either, because it means that information about the actual cause is lost. The actual cause can still be attached to the new exception (since Java 1.4), however it gets harder to catch the individual cause because a catch clause cannot test the cause of an exception and there is typically no documentation about which kind of exceptions might be wrapped in another exception. The third solution (which is very common) converts the checked exception to an unchecked exception, but like the second one, information about the actual cause is now more difficult to access.

The second major flaw is related to the concept of having checked exceptions bubble up the stack. This approach does not work well when using an inversion of control (IoC) pattern. This pattern is very prominent (for reasons that are outside the scope of this article), but cannot be properly used with checked exceptions. The generic (framework) code has to know which checked exceptions are thrown by user code that is called from the generic code. Obviously, it cannot, so the interface to the user code has to specify no checked exceptions at all or a checked exception specific to the calling code. This means that the user code has to wrap its exceptions in the checked exception specified by the framework or in an unchecked exception. This leaves the problem described earlier, where the code calling the framework code would now need to unwrap exceptions and handle their causes, even though it cannot (always) know which exceptions might be causing the exception that is caught.

Now, let's see how this changes when we consistently use unchecked exceptions instead of checked exceptions. Unchecked exceptions can easily bubble up through framework code, so we can catch them where we want to, but we do not have to catch them where we cannot handle them anyway. We do not lose any information about the exception, meaning we can still catch a very specific exception at a rather high level.

Obviously, it is important to document which unchecked exceptions are thrown by a method, so that calling code can know which exceptions it might want to catch. If there is framework code in between, it might not always be obvious which exceptions can occur, however this is not worse than with an exceptions of an unknown type that is wrapped in a checked exception.

The Spring framework, for example, chooses to use unchecked exceptions for most error conditions. I tend to use the same approach when I write library code.

It is tempting to use checked exceptions in order to force the user of a library method to handle a certain situation, but this rarely works. The InterruptedException thrown by many standard library methods is a good example: This exception is thrown when a thread is interrupted while it is blocked waiting for some event. This makes sense, because the thread should not wait any longer, when it has been interrupted. However, it is very common to see code like the following:

try {
    someMethodThatThrowsAnInterruptedException();
} catch (InterruptedException e) {
    // Ignore the exception
}

This is very dangerous because Java will clear the interruption status of a thread when throwing an InterruptedException. This means that code on a higher level, that checks whether the thread has been interrupted (you will often find this as a loop condition), will never know that the thread has been interrupted.

Therefore, the correct way to handle an InterruptedException is the following:

try {
    someMethodThatThrowsAnInterruptedException();
} catch (InterruptedException e) {
    Thread.currentThread().interrupt();
}

This will mark the current thread as interrupted, so that code on a higher level will get correct results when checking whether the thread has been interrupted.

Now, imagine that InterruptedException was an unchecked exception: Code that did not want to deal with it simply would not. This would result in the exception bubbling up until it is handled explicitly or the current thread is terminated. In most cases, this is exactly what the programmer wants (ensure that the thread terminates when it is interrupted). In the few cases where one wants to react on the InterruptedException in a different way than terminating the thread, one could still catch the exception explicitly. Everyone doing this would probably be aware of the fact that the thread has to be interrupted again if other code should see that it has been interrupted. So use of an unchecked instead of a checked exception would actually result in better code in most cases.

Actually, the tendency to abandon checked exceptions can also be seen in other languages. C++ had a feature that is simlar to checked exceptions in C++ 98 and 03: A function can explicitly declare which exceptions it throws and the compiler will enforce that it does not throw any other exceptions (when it does, the program terminates). In C++ 11, this feature has been deprecated. Instead, C++ now offers the nothrow keyword in order to specify that a function does never throw any kind of exception. This is a consequence of the experience that it is rarely practical to explicitly specify which exceptions a function might throw.

In summary, checked exceptions are a nice concept that unfortunately is not useful in practice. Therefore, it is my opinion that they should be avoided in general and unchecked exceptions should be preferred where feasible.