Configurable ExceptionFactory

[ghost]

It would be quite convenient to allow some degree of configuration on how org.apache.ibatis.exceptions.ExceptionFactory builds the exception message (currently: org.apache.ibatis.executor.ErrorContext.toString()).

Our concern is that non-managed exceptions end up carrying out sensitive information (such as the SQL sentence) within the exception message.

Something in the line of PostgreSQL logServerErrorDetail or Spring Boot server.error.include-stacktrace would be ideal. I.e.: one or more MyBatis configuration properties to control the fields of ErrorContext (resource, object, activity, sql...) that are written in the exception message.

Thanks!

[harawata]

Hello, @nsanchob ,

This is a common requirement which, in my opinion, should be handled in higher layer of the application (i.e. service or presentation).
Spring, for example, provides @ExceptionHandler and HandlerExceptionResolver.

Modifying the original error message seems to be a bad idea as it leads to loss of information crucial to the developers (i.e. you and your team members).

[ghost]

Hello, Harawata.

Thanks for your answer.

We are currently handling it at a higher level, but the different elements of the ErrorContext are already concatenated within the BadSqlGrammarException message, so the handling code is quite ugly:

} catch (final BadSqlGrammarException e) { // (org.springframework.jdbc.BadSqlGrammarException)
	// Determines if MyBatis exception (@see org.apache.ibatis.executor.ErrorContext)
	if (!StringUtils.contains(e.getMessage(), "###")) {
		throw e;
	}
			
	final SQLException sqlException = e.getSQLException(); // (java.sql.SQLException)
	throw new BadSqlGrammarException(sqlException.getMessage(), "", sqlException);
}

That's the reason of our feature request: to be able to disable some elements of the ErrorContext.toString() (at least within ExceptionFactory) without having to rely on looking for magic strings (###).

I agree modifying the original error message would lead to loss of information, but our idea is to be able to configure it depending on the environment (e.g.: SQL hidden in production, shown elsewhere).

Actually, if ExceptionFactory was not an utility class but an injectable field of DefaultSqlSession (with an instance of the current implementation set by default), that would solve most of our problems, as we would be able to inject a custom ExceptionFactory... Does this sounds right?

[harawata]

Thank you for the explanation, @nsanchob ,

I am not sure what your requirement is, but a typical way to handle such unexpected database error is to log the original exception as-is and throw some project-specific exception with a generic message.

log.error("There is a bug", e);
throw new MyServiceException("unexpected db error occurred");

Altering the error message also makes it difficult for us (=maintainers) to investigate bug reports or to answer questions.
I'll keep this issue open as other devs might have a different opinoin.

[ghost]

Hello, @harawata !

Not a particular requirement... we are (ab)using the Problem Details for HTTP APIs and returning the exceptions to the client. This is quite useful in development environments, and gives us a fast workflow in QA environments.
Meanwhile, in production environments, all sensitive details (such as stack traces and parameter values) are disabled via configuration properties (such as the aforementioned PostgreSQL logServerErrorDetail or Spring Boot server.error.include-stacktrace).

We don't want to alter the error message everywhere... But in my opinion having ExceptionFactory as an interface, a DefaultExceptionFactory implementation providing the current behaviour, and allowing the exception factory to be an injectable field of DefaultSqlSession would give flexibility without compromising bug reports.

Edit: What's the best way to propose a code change without being a "proper" pull request? (because I prefer to know if the idea is going to be accepted/rejected before actually writing the corresponding unit tests and documentation)

[ghost]

Hi again, @harawata .

Please check this commit: https://github.com/nsanchob/mybatis-3/commit/e7599245dec42ba378e88d20ec9d12c5718e27c7. It is a (very rough) draft of my proposal, but I hope it helps to clarify my feature request.

The main chages are:

• src/main/java/org/apache/ibatis/exceptions/ExceptionFactory.java that is now an interface; the old behaviour kept within DefaultExceptionFactory (a singleton probably not respecting mybatis code guidelines but, again, this is a draft).
• src/main/java/org/apache/ibatis/session/defaults/DefaultSqlSession.java where the ExceptionFactory is now a field.
• src/main/java/org/apache/ibatis/session/defaults/DefaultSqlSessionFactory.java where the ExceptionFactory is now a field too.

[harawata]

Thank you for the draft implementation, @nsanchob !

What's the best way to propose a code change without being a "proper" pull request?

Creating a feature request issue like this is fine.
Other devs may add comments once they review the request.

[cchengubnt]

I agree with @nsanchob
Our concern is that non-managed exceptions end up carrying out sensitive information (such as the SQL sentence) within the exception message.
This is also what I concern about. I trace the ### SQL: xxx string in my logs and finally found same issue here.
It's good way to print specific sql to trace bugs, but a custom msg is better and safer for me.

[harawata]

Thank you for the comment, @cchengubnt .

There seems to be certain needs.
Let me discuss this with other devs.