A default implementation of DataFetcherExceptionHandler.onException #2614
Conversation
| DataFetcherExceptionHandlerResult onException(DataFetcherExceptionHandlerParameters handlerParameters); | ||
| default DataFetcherExceptionHandlerResult onException(DataFetcherExceptionHandlerParameters handlerParameters) { | ||
| return SimpleDataFetcherExceptionHandler.defaultImpl.onException(handlerParameters); | ||
| } |
There was a problem hiding this comment.
Should we undefault the other method in this same PR (and hence 18.0 version)?
There was a problem hiding this comment.
I think it's better to keep default implementation on the new method or otherwise existing implementations would not work until they implement it.
| DataFetcherExceptionHandlerResult onException(DataFetcherExceptionHandlerParameters handlerParameters); | ||
| default DataFetcherExceptionHandlerResult onException(DataFetcherExceptionHandlerParameters handlerParameters) { | ||
| return SimpleDataFetcherExceptionHandler.defaultImpl.onException(handlerParameters); | ||
| } |
There was a problem hiding this comment.
I think it's better to keep default implementation on the new method or otherwise existing implementations would not work until they implement it.
| @Deprecated | ||
| DataFetcherExceptionHandlerResult onException(DataFetcherExceptionHandlerParameters handlerParameters); | ||
| default DataFetcherExceptionHandlerResult onException(DataFetcherExceptionHandlerParameters handlerParameters) { | ||
| return SimpleDataFetcherExceptionHandler.defaultImpl.onException(handlerParameters); |
There was a problem hiding this comment.
Couldn't this be just IllegalStateException? Either a class implements the new method, in which case this is not used, or it overrides this one. Either way this implementation should never be called.
There was a problem hiding this comment.
The problem is more when do we decide to break older implementers. if the new CF based handleException does not call the older onException then every one out there that previous implemented onException will break. Because our code will call their method.
Do really it's a case of deciding WHEN we break backwards compat in behavior. That is all those old onException methods will stop working even if its deprecrated.
So ... should that be now? This new CF based method was introduced in 17.x - 18.x is next. Is one version enough? I kinda think it should be two versions
There was a problem hiding this comment.
Just to clarify, the new handleException should continue to call onException as it does now. I'm not suggesting otherwise. I'm not sure how long the deprecation period should continue, but I see no reason not to give it ample time. When that's over, drop onException and remove the the default implementation from handleException.
See #2545