SLING-12394 improve ResourceResolver handling #39
Conversation
| * @throws RuntimeException if the resolver can't be created. | ||
| */ | ||
| public ResourceResolver createResourceResolver() { | ||
| ResourceResolver resolver = null; | ||
| public @NotNull ResourceResolver createResourceResolver() { |
There was a problem hiding this comment.
IIUC this change will result in exceptions rather than silent swallows if the factory is null. That might its own consequences. What is the reasoning behind this change?
There was a problem hiding this comment.
I agree with @stefan-egli's concern. There are quite a few places where we had a null check before.
@joerghoh I suggest to throw a NullPointerException (or something more specific) instead of a RuntimeException and to catch and log this exception in the places where we used to have a null check.
| * @throws RuntimeException if the resolver can't be created. | ||
| */ | ||
| public ResourceResolver createResourceResolver() { | ||
| ResourceResolver resolver = null; | ||
| public @NotNull ResourceResolver createResourceResolver() { |
There was a problem hiding this comment.
I agree with @stefan-egli's concern. There are quite a few places where we had a null check before.
@joerghoh I suggest to throw a NullPointerException (or something more specific) instead of a RuntimeException and to catch and log this exception in the places where we used to have a null check.
| final ResourceResolver resolver = configuration.createResourceResolver(); | ||
| if ( resolver != null ) { |
There was a problem hiding this comment.
Here we have a null check. Should we handle the exception here instead?
| ResourceResolver resolver = configuration.createResourceResolver(); | ||
| if ( resolver != null ) { |
There was a problem hiding this comment.
Here we have a null check. Should we handle the exception here instead?
| final ResourceResolver resolver = configuration.createResourceResolver(); | ||
| if ( resolver != null ) { |
There was a problem hiding this comment.
Here we have a null check. Should we handle the exception here instead?
| final ResourceResolver resolver = this.configuration.createResourceResolver(); | ||
| if ( resolver != null ) { |
There was a problem hiding this comment.
Here we have a null check. Should we handle the exception here instead?
| final ResourceResolver resolver = this.configuration.createResourceResolver(); | ||
| if ( resolver != null ) { |
There was a problem hiding this comment.
Here we have a null check. Should we handle the exception here instead?
| final ResourceResolver resolver = this.configuration.createResourceResolver(); | ||
| if ( resolver == null ) { |
There was a problem hiding this comment.
Here we have a null check. Should we handle the exception here instead?
| final ResourceResolver resolver = configuration.createResourceResolver(); | ||
| if ( resolver == null ) { |
There was a problem hiding this comment.
Here we have a null check. Should we handle the exception here instead?
| final ResourceResolver resolver = configuration.createResourceResolver(); | ||
| if ( resolver != null ) { |
There was a problem hiding this comment.
Here we have a null check. Should we handle the exception here instead?
| final ResourceResolver resolver = configuration.createResourceResolver(); | ||
| if ( resolver != null ) { |
There was a problem hiding this comment.
Here we have a null check. Should we handle the exception here instead?
try-with-resource-based approach, which make the code easier to readnullchecks