Add proper error handling to findBy method in JdbcOAuth2AuthorizationService

[Suvink]

Describe the bug
I recently came across the java.lang.IllegalArgumentException in Authorization Code flow with JdbcOAuth2AuthorizationService. More info on the issue has been discussed in #397.

While investigating this issue, I could not identify the exact point that this error pops up and had to add debug points and struggle to identify the line that was failing.
Upon inspecting the code, I noticed in the findBy method, a try block is used without a catch block.

private OAuth2Authorization findBy(String filter, List<SqlParameterValue> parameters) {
	try (LobCreator lobCreator = getLobHandler().getLobCreator()) {
		PreparedStatementSetter pss = new LobCreatorArgumentPreparedStatementSetter(lobCreator,
					parameters.toArray());
		List<OAuth2Authorization> result = getJdbcOperations().query(LOAD_AUTHORIZATION_SQL + filter, pss, getAuthorizationRowMapper());
		return !result.isEmpty() ? result.get(0) : null;
	}
}

It would be easy to identify the error if we can have a catch block and handle this error properly.

To Reproduce
Please refer to #397.

Expected behavior
Handle the errors in findBy method with a catch block.

Sample

private OAuth2Authorization findBy(String filter, List<SqlParameterValue> parameters) {
	try (LobCreator lobCreator = getLobHandler().getLobCreator()) {
		PreparedStatementSetter pss = new LobCreatorArgumentPreparedStatementSetter(lobCreator,
					parameters.toArray());
		List<OAuth2Authorization> result = getJdbcOperations().query(LOAD_AUTHORIZATION_SQL + filter, pss, getAuthorizationRowMapper());
		return !result.isEmpty() ? result.get(0) : null;
	} catch (Exception e){
              ....handle the exception
    }
}

Reports that include a sample will take priority over reports that do not.
At times, we may require a sample, so it is good to try and include a sample up front.

[sjohnr]

Hi @Suvink!

Upon inspecting the code, I noticed in the findBy method, a try block is used without a catch block.

The try in this method is a try-with-resources. I feel that adding a catch for a RuntimeException here would be counter-productive as we don't have any behavior we can use to gracefully handle the exception, and we certainly don't want to "swallow" it and as if nothing happened. It's not clear to me what adding a catch in this particular bit of code will solve and I don't see a suggestion in your comment.

I'm going to close this issue for now as I don't think I agree with adding catch here.

If you're still having trouble, please ensure you have enabled trace logging during development as demonstrated in the Getting Started example and I believe you will have an easier time tracking down these issues. If I'm missing details please let me know. If you have another specific suggestion for improving the debugging situation for this case, please open a new issue.

[Suvink]

Hey @sjohnr,

The try in this method is a try-with-resources.

I see. I didn't know about this. Thanks.

as we don't have any behavior we can use to gracefully handle the exception

I agree. The user has to provide the mixin and solve the issue.

It's not clear to me what adding a catch in this particular bit of code will solve and I don't see a suggestion in your comment.

After getting to know about try-with-resources, I agree that adding a catch block here doesn't make sense. My concern is that even with having trace logging enabled, I didn't get the exact error message (the RuntimeException) logged anywhere. I had to wrap the code where I call this method with a try-catch block to catch it and see what the exception is all about. So I think it would be helpful if we can at least log the exact exception thrown by the findBy method.

At the moment, I don't have any proposals on top of my mind but I'll do some research and create a new issue when I have something solid.

[sjohnr]

Hi @Suvink, thanks for following up!

My concern is that even with having trace logging enabled, I didn't get the exact error message (the RuntimeException) logged anywhere.

Typically, this error should be caught at the highest level of the servlet stack and logged as an uncaught exception to prevent the servlet thread from terminating. Are you sure it didn't get logged at all? That doesn't seem to make sense to me. I have tested this myself by generating an IllegalArgumentException from the OAuth2AuthorizationService and it indeed is logged by the servlet handling the request.

From my perspective, handling runtime exceptions at the framework level will cover bugs that need to be addressed at development time. Since this is a bug in your configuration (or in this case missing configuration) and is a development-time issue which does get logged eventually, I don't think we will add anything here.