MikeNeilson
commented
1 year ago I like this. The ApiServlet exception handlers cover everything that the Javalin layer can catch, so there's not really a limit. It just starts to get more generic.
But as for a controller manually setting not found, I think throwing notfound and centralizing the logic so it's always outputing correctly (the json format) is the right thing to do.
a DAO could do the same thing if it's easiest to do at that level. And considering how many nested queries we need that get otherwise odd errors when things aren't found that will happen a lot.
I was looking at issue #290 where a levels get request is not finding a level that doesn't exist and its returning 500 and not 404. Looking at the exception trace it seems like the thing to do is to look for the appropriate code in the inner SQLException and throw a NotFoundException. We had a similar issue a while back for TimeSeries.
That got me thinking. The code I'd be looking for in the exception isn't Oracle specific - its CWMS specific. I'm not sure exactly where in the pl/sql the cwms exceptions get defined but this file has a useful summary: https://bitbucket.hecdev.net/projects/CWMS/repos/cwms_database_origin_teamcity_work/browse/src/buildSqlScripts.py#4866
In looking for the error code I came across similar error handling I'd added to the old CWMSMobile. Which reminded me about how Spring Framework would wrap all the exceptions it encountered. Jooq does something somewhat similar - all the SQLExceptions are org.jooq.exception.DataAccessException.
I think we need to take a more pragmatic approach to exception handling and decide the roles/responsibilities of each "layer". In the current environment those layers are probably: Servlet, Controller, Dao. There's possibly also a Formatter pseudo-layer in there. If a Controller can't find the resource being requested should it be setting the ctx.status to SC_NOT_FOUND itself or should it be throwing a NotFoundException and letting ApiServlet handle it? If it is throwing exceptions - what classes of exceptions are Controller allowed to throw?
If a controller can't find the resource its going to be because the DAO couldn't find it. Similar question as before - what types of exceptions should we allow DAO to throw? If the DAO are throwing exceptions it may be because JOOQ is throwing exceptions. Jooq is going to throw DataAccessException with the cause wrapped inside. For cases where the cause is an exception from cwms pl/sql that cause is going to be SQLException with a code in the range 20001-20999.
If we've decided on a principled approach to exception handling we can wrap all jooq interactions with code that will map jooq's DataAccessException to our own business exception types.
In pr #298 I demonstrate how we could catch jooq exceptions and wrap them. Not saying that PR is exactly how we should do things - just wanted to verify that we could wrap the jooq exceptions in our Dao objects into something else and make it happen all in one place so that we don't have to do it call-by-call. It looks like the jooq DSLContext.connection(...) method doesn't go thru the ExecutionListeners the way the other jooq code does but we can have our Dao classes call our own connection method that calls the jooq version with extra try/catch wrapping code. I showed an example of that in the draft PR.
Thoughts?