Add conditional bean for jOOQ exception translator

38736

Comment From: MelleD

Hello,

thanks for the PR. I've added a comment regarding breaking the public API. Please take a look.

Yes I can change the name back. Do you have a proposal for the other class names?

Comment From: mhalbritter

For what class are you looking for a name? I'd leave JooqExceptionTranslator as it is, create an auto-configuration bean method for it and use it in the jooqExceptionTranslatorExecuteListenerProvider method. No new classes are needed, or am I missing something?

Comment From: MelleD

For what class are you looking for a name? I'd leave JooqExceptionTranslator as it is, create an auto-configuration bean method for it and use it in the jooqExceptionTranslatorExecuteListenerProvider method. No new classes are needed, or am I missing something?

For the new interface?

Comment From: mhalbritter

Which new interface? I'd leave JooqExceptionTranslator the class it is, and add a bean method for it. If users want to customize the exception translation, create a subclass of JooqExceptionTranslator, override some methods and provide it as a bean. The auto-configuration will then pick up the user class and exception translation is customized. That would work, wouldn't it?

Comment From: MelleD

Maybe yes it would work. But looks not really nice to create subclass which using nothing from the parent class just to have the same type to provide a bean. I like more the interface and „real“ implementation. If you would like to create a subclass to reuse things it‘s also possible or just implement the interface

Comment From: wilkinsona

But looks not really nice to create subclass which using nothing from the parent class just to have the same type to provide a bean

There was a proposal in #38736 to "change the existing JooqExceptionTranslator to make it easier to subclass". I think that makes sense and would be less disruptive than the introduction of an interface.

Comment From: MelleD

But looks not really nice to create subclass which using nothing from the parent class just to have the same type to provide a bean

There was a proposal in #38736 to "change the existing JooqExceptionTranslator to make it easier to subclass". I think that makes sense and would be less disruptive than the introduction of an interface.

Yes third option is with interface. And also you can subclass it. That's directly what the proposal was and what I did. For me this make a lot of sense.

Comment From: wilkinsona

I don't think we want a new interface. Instead, we'd prefer for JooqExceptionTranslator to remain a class and for it to be updated so that it's more useful as a super-class. This is what was proposed by @philwebb in #38736. For example, perhaps getTranslator(ExecuteContext) can be made protected so that it can be used by subclasses.

Another option would be to make things conditional on a bean with a specific name, somewhat similar (although hopefully a little simpler) to what we do for Spring MVC's DispatcherServlet.

Comment From: MelleD

I don't think we want a new interface.

But why?

JooqExceptionTranslator is still a class and you can sub-class it if you want. There is no difference to now.

Comment From: wilkinsona

It's an interface in your current proposal:

public interface JooqExceptionTranslator extends ExecuteListener

Comment From: MelleD

It's an interface in your current proposal:

java public interface JooqExceptionTranslator extends ExecuteListener

Yes I would like to rename the interface + rename the translator class back to JooqExceptionTranslator. That’s it and you have the advantage of both. You can subclass the JooqExceptionTranslator or implement a own class.

Comment From: philwebb

Yes I would like to rename the interface + rename the translator class back to JooqExceptionTranslator. That’s it and you have the advantage of both. You can subclass the JooqExceptionTranslator or implement a own class

Although the interface is more flexible, in this instance I think it's best if we make only the smallest change possible required to support the feature. We've learnt from experience that it's harder to remove code than add it, so from our point of view it would be better not to introduce new public API before we need to.

Comment From: MelleD

Yes I would like to rename the interface + rename the translator class back to JooqExceptionTranslator. That’s it and you have the advantage of both. You can subclass the JooqExceptionTranslator or implement a own class

Although the interface is more flexible, in this instance I think it's best if we make only the smallest change possible required to support the feature. We've learnt from experience that it's harder to remove code than add it, so from our point of view it would be better not to introduce new public API before we need to.

Yes I know, for our use case we don't need the JooqExceptionTranslator to subclass it. We just need the interface. So there is a use case.

I honestly don't understand the problem at all. I haven't heard any reason as to what exactly the problem is, other than personal opinion.

Although it is actually best practice to have an interface and an implementation.

What exactly is the problem?

Comment From: philwebb

Although it is actually best practice to have an interface and an implementation.

There's some subtly when it comes to best practices for a general project and best practices for an open source project with many users a long support window. In a general project, the cost of introducing new API isn't that great since the the project is self contained and it's easy to change things without impacting others.

For a project such as ours, we need to be more intentional about how and when we introduce new API. We need to consider back compatibility issue and long-term maintenance concerns.

For example, we can't accept the pull-request as it stands because it changes JooqExceptionTranslator from a class to an interface. This could break existing users and will break binary back-compatibility. That means we have to leave JooqExceptionTranslator as a class. We could introduce a new interface and retrofit it, but then we have an interesting problem of naming. What should we call the new interface given that JooqExceptionTranslator is already taken.

If we zoom out a bit, there's a lot of advantages to not adding an interface. We don't break back-compatibility and we don't need to come up with any new class names or introduce new public API. Furthermore, we've not prevented ourselves from adding an interface later if we decide that one is really needed.

I haven't heard any reason as to what exactly the problem is, other than personal opinion.

This is true, but it's personal opinion that's been formed from a lot of experience managing this project. As a general philosophy, making the smallest change possible required to support a given use-case has historically served us well.

Comment From: MelleD

I understand the reasons also for open source projects (have also some experience), but I don't see it here because the interface provides exactly the same thing as the class. It is just as public as it currently is. There is no new public API, because there is already an interface --> ExecuteListener ;). There is simply no difference except that it is more flexible and cleaner. In addition, as already mentioned, for our use case it perfectly fit .We don't need the parent class. That's actually better for backwards compatibility, because if you deprecated the class and removed it, we don't even notice it because we don't have to inherit from it. One dependency less.

How I said: I will revert the current class name (understand that this is breaking) and I will also find a name for the interface, but if there are already concrete ideas, you can simply change the PR as you think. So for me it's okay you can takeover the PR.

Comment From: philwebb

Thanks for the updates to the code @MelleD. Having looked at your changes I've changed my mind about the interface and I agree that it's the better approach. I've merged this into main and updated the interface name to ExceptionTranslatorExecuteListener. I've also taken the opportunity to deprecate JooqExceptionTranslator since we can now hide the implementation behind some statics on the interface.

Thanks for the PR!