SLING-12394 improve ResourceResolver handling

[joerghoh]

• Open all ResourceResolvers in a try-with-resource-based approach, which make the code easier to read
• remove redundant nullchecks

[stefan-egli]

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?

[jsedding]

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.

[joerghoh]

These null-checks seem to be added rather by accident than really planned. Otherwise all places where no null-checks exist would need to be augmented with these checks as well, and that was not a problem for a very long time.

Silently swallowing these exceptions (and therefor not executing the expected action) is also not a good pattern. On the other hand side the LoginException happens mostly due to configuration issues (also rare, because createResourceResolver code is not exported, and thus will only be used by the Sling event code), and therefor they should be easily fixable. So in the end the actual type of issue does not seem to be a problem, which is likely to happen frequently.
And for that reason I think it's possible to adjust the way errors are handled, as this is contained in this code. We might want to add a Thread.UncaughtExceptionHandler to the threads of the job threadpool(s).

[jsedding]

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.

[jsedding]

Here we have a null check. Should we handle the exception here instead?

[jsedding]

Here we have a null check. Should we handle the exception here instead?

[jsedding]

Here we have a null check. Should we handle the exception here instead?

[jsedding]

Here we have a null check. Should we handle the exception here instead?

[jsedding]

Here we have a null check. Should we handle the exception here instead?

[jsedding]

Here we have a null check. Should we handle the exception here instead?

[jsedding]

Here we have a null check. Should we handle the exception here instead?

[jsedding]

Here we have a null check. Should we handle the exception here instead?

[jsedding]

Here we have a null check. Should we handle the exception here instead?

[sonarqubecloud]

Quality Gate failed

Failed conditions
60.7% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

[sonarqubecloud]

Quality Gate failed

Failed conditions
60.7% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

[sonarqubecloud]

Quality Gate failed

Failed conditions
60.7% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud