Introduce SQL script exception hierarchy [SPR-11564]

[spring-projects-issues]

Sam Brannen opened SPR-11564 and commented

Status Quo

The org.springframework.jdbc.datasource.init package contains several exceptions related to reading, parsing, and executing SQL scripts; however, each of these exceptions extends RuntimeException. Thus there is no clean way to catch and handle all types of script-related exceptions.

As a side effect of refactoring, #14165 already unified the exception hierarchy within org.springframework.jdbc.datasource.init package by introducing a common ScriptException base class that extends RuntimeException, but this doesn't go far enough. ScriptException should actually extend DataAccessException to allow callers to handle all data access related exceptions in a single catch block.

Deliverables

1. Refactor ScriptException so that it extends DataAccessException.
2. Favor unchecked ScriptExceptions over checked SQLExceptions whenever feasible.

---

Affects: 4.0.2

Issue Links:

• Support multi-line SQL comments in ResourceDatabasePopulator and JdbcTestUtils [SPR-9531] #14165 Support multi-line SQL comments in ResourceDatabasePopulator and JdbcTestUtils

Referenced from: commits fbd2546, 92eb99a

[spring-projects-issues]

Sam Brannen commented

This has been addressed in GitHub commit fbd2546:

Introduce SQL script exception hierarchy

This commit continues the work began in #14165 as follows.

• ScriptException now extends DataAccessException.

• DatabasePopulator.populate() now explicitly throws ScriptException.

• Introduced UncategorizedScriptException.

• DatabasePopulatorUtils.execute() now throws an
  UncategorizedScriptException instead of a
  DataAccessResourceFailureException.

And in GitHub commit 92eb99a:

Favor ScriptException over SQLException

In ScriptUtils and related classes, SQLExceptions are now caught and
wrapped in ScriptExceptions wherever feasible.

Affected "throws" declarations have also been revised as appropriate.

[spring-projects-issues]

Michael Osipov commented

The method signatures in the commit are broken. You never declare a runtime exception in the throws block. Runtime exceptions are unrecoverable.

[spring-projects-issues]

Juergen Hoeller commented

Why not? We explicitly declare specific runtime exceptions in quite a few signatures, indicating what kind of exception is to be expected. This is just formal documentation of exception behavior, with no immediate indication of the recoverability of those exceptions...

Juergen

[spring-projects-issues]

Michael Osipov commented

Jürgen, if you do use then in a throws block, you break the contract of the unchecked exception. Unfortunately, this issue is spread through out the entire framework and is ill-designed. Unchecked exceptions, by nature, are indicating a programming error and a client is not able to recover from. That is the semantic of this exception. You are forcing me to catch an exception I cannot recover from. You should read Effective Java on that topic.

Documentation of an unchecked exceptions happens in a method's Javadoc. throws is not a documentation block!

[spring-projects-issues]

Juergen Hoeller commented

Michael,

Unchecked exceptions do not necessarily indicate an unrecoverable error; this is a matter of individual definition. The trend in Java API design is clearly towards fine-grained unchecked exception hierarchies, to some part unrecoverable but to some part recoverable as well.

Why do you feel that we're forcing you to catch exceptions? With unchecked exceptions, it's completely up to you whether you catch the exception or just let it flow through. The only difference with a "throws" declaration is that it's formally declared, included in the javadocs, and also introspectable via reflection - whereas in general javadoc comments, it's just loosely referenced, potentially outdated, etc.

Spring is also by no means the only framework following those relaxed rules about unchecked exceptions. We may have been setting the trend back then 10 years ago, with Hibernate 3 and others following soon... However, these days, it's simply industry standard. FWIW, have a close look at JPA, or core JDK APIs from the Java 5+ era, or almost any spec from the recent Java EE 7 umbrella, e.g. JMS 2.0:

http://docs.oracle.com/javaee/6/api/javax/persistence/EntityManager.html#merge%28T%29
http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ExecutorService.html#submit%28java.lang.Runnable%29
https://jms-spec.java.net/2.0/apidocs/javax/jms/JMSProducer.html#send%28javax.jms.Destination,%20java.lang.String%29

From a traditional Java perspective, this may all be unusual and bending the original design rules a bit. In that sense, it may not follow Effective Java's original rules either. At the same time, it's well-defined and consistent within the Spring and Java EE world these days.

Juergen

[spring-projects-issues]

Sam Brannen commented

Hi Michael,

You are forcing me to catch an exception I cannot recover from.

By declaring that a method potentially throws an unchecked exception, we are most certainly not enforcing you to catch an exception. Whether or not you choose to recover from an exception is up to you, depending on your use case. I think you are confusing checked (i.e., instances of Exception) and unchecked exceptions (i.e., instances of RuntimeException).

Consider the following example (which compiles and runs without any issues):

public class Exceptions {

	public static void throwsUncheckedException() throws RuntimeException {
		System.out.println("throwsUncheckedException()");
	}

	public static void throwsCheckedException() throws Exception {
		System.out.println("throwsCheckedException()");
	}

	public static void main(String[] args) {

		throwsUncheckedException();

		try {
			throwsCheckedException();
		}
		catch (Exception e) {
			e.printStackTrace();
		}

	}

}

When invoking throwsUncheckedException() which declares that it throws a RuntimeException, Java does not force you to catch the exception. You can simply ignore it, as can be seen by the lack of a try-catch block in the above main() method.

In contrast, an invocation of throwsCheckedException() must be wrapped in a try-catch block -- or the invoking method can redeclare the checked exception or a superclass thereof.

To reiterate, declaring that a method throws a ScriptException or DataAccessException (both of which are subclasses of RuntimeException) does not force the caller to catch such exceptions.

Hope this clears things up for you!

Regards,

Sam

[spring-projects-issues]

Michael Osipov commented

Juergen Hoeller, Sam Brannen, I am not talking about using unchecked exceptions at all, moreover, the exception translation pattern applied in the Spring Framework is great. Especially, the exception heirarchy. All I am saying is that the general contract of a unchecked exception is not to declare it in the throws block. This is item 62 in the book. Recheck the Javadoc references you have supplied. All they say refers to the Javadoc writter by its authors. If you check the source code on grepcode.com, you'll see that none of these methods has a throws declaration along with the message signature. If you take those sample into account, your signatures are definitively broken.

[spring-projects-issues]

Juergen Hoeller commented

Ok, point taken, those referenced examples do follow our exception philosophy but don't actually formally declare those runtime exceptions in the method signatures themselves - just in javadoc @throws. However, they do indicate signatures where runtime exceptions are being thrown that are potentially recoverable, so they do challenge a key point in your argument.

So your primary complaint is about the actual mentioning in a method declaration, and indeed, it is unusual to actually - and consistently - declare those like we do. However, from my perspective, there is nothing wrong with it either. I fail to see what negative effect we're causing here? You still don't have to catch it; it's no different from not declaring it.

And for an example where runtime exceptions are being explicitly declared just like in Spring, check out Hibernate:
http://docs.jboss.org/hibernate/annotations/3.5/api/org/hibernate/Session.html#flush%28%29

Juergen

[spring-projects-issues]

Juergen Hoeller commented

http://www.jroller.com/hackingarchitect/entry/declared_unchecked_exception_the_best
http://www.theserverside.com/news/thread.tss?thread_id=41661

"The approach I would like to present and promote here is known as Declared Runtime Exception. It is nothing new as it simply takes use of the current exception declaration and catching mechanism without any change. This is more of a design choice than anything else. It simply works this way:

• Define exceptions in your application/framework as unchecked exception and make them part of the API. Try to declare one high-level generic unchecked exception for the framework and many detailed exception derived from it.
• In any method that throws those exception, explicitly declare the unchecked exception at the throws clause.
• For exceptions indicating pure programming errors, such as NullPointerException, they can remain undeclared.
  ..."

http://docs.oracle.com/javase/tutorial/essential/exceptions/runtime.html

"Runtime exceptions can occur anywhere in a program, and in a typical one they can be very numerous. Having to add runtime exceptions in every method declaration would reduce a program's clarity. Thus, the compiler does not require that you catch or specify runtime exceptions (although you can)."

If you take that Java SE tutorial as a reference, we're simply adopting the "although you can" part for selected kinds of runtime exceptions. And in reference to the blog post above, we're doing exactly what the author is suggesting there, including the exception hierarchy part and the non-declaration of pure programming errors such as IllegalArgumentException. As he puts it very well: This is simply a design choice, and we choose to apply it as consistently as we can.

My main point is: This is perfectly valid to do, even if not originally idiomatic to Java back in 2000 when most of the exception literature was written. Effective Java is not a language specification, just a set of recommendations, so this is not "broken" but rather just "different". And it imposes no burden of any kind on client code since its effect is exactly equivalent to not declaring those runtime exceptions at all.

Juergen

[spring-projects-issues]

Michael Osipov commented

Jürgen, now you understand me. Negative effect is that you change simply the semantics of an unchecked exception. You don't need and shall not declare it. Period. Fortunately, it causes no harm in the current code but you should obstain creating new ones like it happened in the commits for this ticket.

The example of Hibernate is a bad one because their method declartion is simply wrong. You should not copy that. There is a good reason by Gosling, et al. created two type of exceptions.

[spring-projects-issues]

Juergen Hoeller commented

Looks like we commented at almost the same time here, so I suppose you haven't seen my latest one yet. The "shall not declare it - period" part isn't worded like that in the Java exception tutorial. It's rather clearly suggesting that you don't have to declare it but you can.

I fail to see how we're changing any semantics here; we're doing exactly what JPA, JMS 2.0 etc do, just additionally declaring exceptions in the signature. The semantics of the exceptions themselves are analogous; the only difference is a formal declaration with no effect.

We choose to favor consistency in the evolution of our framework which is why we keep applying our exception declaration pattern to new code as well. It worked for us for 10 years, and it worked for Hibernate for 9 years, so I see no need to change our approach here.

I appreciate that this can be argued differently, but in all fairness, this discussion is more academic than pragmatic. And as for Gosling's original choice to add checked and unchecked exceptions to the language, there's been so much debate about it for such a long time...

Juergen