Handling an invalid SQL query exception

Question

This is my DAO object that takes the Cypher query as a string, and returns a JSON representation of the graph it returns.

Because it's using a Spring JdbcTemplate to access the database, the jdbc query will return any errors wrapped in a DataAccessException.

What I'm wanting to do here, is detect whether the error being thrown is due to an invalid query (in which case I want to return a message to the user saying so), or for now, any other error, in which case I'll file it as 'An unexpected error occurred'.

public String runQuery(String q) throws JsonGenerationException, JsonMappingException, UncategorizedSQLException, DataAccessException 
 {   
    Graph g = null;
            try {
                g = jdbcTemplate.query(q, new Neo4jGraphResultSetExtractor());
            } catch (DataAccessException e) {

                if (e instanceof UncategorizedSQLException)
                    {
                    //Invalid cypher query
                    throw (UncategorizedSQLException)e;
                    }

                else throw e; 
            }

    json = g.toJson(); //throws JsonGenerationException, JsonMappingException
    return json;
} 

When I call the DAO from my Business Layer:

public String submitCypherQuery(String query) {

    try {
        return neoConn.runQuery(query);
    } catch (ClassNotFoundException e) {

        return handleUnexpectedError(e);

    } catch (JsonGenerationException e) {
        return handleUnexpectedError(e);
    } catch (JsonMappingException e) {
        return handleUnexpectedError(e);
    } catch (IOException e) {
        return handleUnexpectedError(e);
    }
    catch (UncategorizedSQLException e)
    {           
        return new ErrorMessage(errorInvalidQuery).toJson();
    }
    catch (DataAccessException e)
    {
        return handleUnexpectedError(e);
    }   

}

Is this an OK way of doing this?

One question I have is that both UncategorizedSQLException, DataAccessException are unchecked exceptions, meaning that even though the DAO has declared them as being thrown - the compiler is fine with calling these methods without being surrounded in a try/catch.

This isn't especially useful - this is the exact case I want to force the developers to handle!

Answer 1

If you want to make the compiler spit out errors when the exceptions aren't caught, make your classes that derive from Exception, but not RuntimeException. Note that unless you have control over the source from whence they came, since you can't change the definition of classes, you have to create your own custom ones.

You've got a bunch of random newlines that just make your code look ugly. I'd suggest removing all of them in the code you've posted.

Rather than having separate catch blocks for each, you can put them all into one. The general form is this:

catch (ExceptionTypeA | ExceptionTypeB name) { /* ... */ }

Also, the standard in Java is to put the opening brace on the same line as the statement it belongs to, so

catch (UncategorizedSQLException e)
{           
    return new ErrorMessage(errorInvalidQuery).toJson();
}

should really be

catch (UncategorizedSQLException e) {           
    return new ErrorMessage(errorInvalidQuery).toJson();
}

---

        } catch (DataAccessException e) {

            if (e instanceof UncategorizedSQLException)
                {
                //Invalid cypher query
                throw (UncategorizedSQLException)e;
                }

            else throw e; 
        }

Ignoring the wacky indentation and unconventional brackets, there are two tips I wanna give:

1. catch statements are done in order, using (what's basically) an instanceof check that stops after the first match. This means that if you put a superclass before a subclass, that subclass will never get hit -- so just put the subclass first:

   catch (UncatergorizedSQLException e) {
       throw e;
   }
   catch (DataAccessException e) {
        throw e;
   }
   

2. Of course, you can sidestep that whole issue by getting rid of the try and declaring that your method throws those two exceptions -- since you're doing nothing but rethrowing, it's simpler and easier. Basically, since you've already declared that you're throwing, just remove the try and its associated catches.

Follow that, and your first method (with formatting) becomes just this:

public String runQuery(String q) throws JsonGenerationException, JsonMappingException,
                                        UncategorizedSQLException, DataAccessException {
    Graph g = jdbcTemplate.query(q, new Neo4jGraphResultSetExtractor());
    json = g.toJson(); //throws JsonGenerationException, JsonMappingException
    return json;
}

And your second becomes (changing the whitespace to follow the majority of the catches):

public String submitCypherQuery(String query) {
    try {
        return neoConn.runQuery(query);
    } catch (UncategorizedSQLException e) {
        return new ErrorMessage(errorInvalidQuery).toJson();
    } catch (DataAccessException | ClassNotFoundException | JsonGenerationException |
             IOException | JsonMappingException e) {
        return handleUnexpectedError(e);
    }
}