[
https://issues.apache.org/jira/browse/SLING-7510?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16499929#comment-16499929
]
Ian Boston commented on SLING-7510:
-----------------------------------
The API was created being pragmatic rather than perfectionist.
It took that approach to minimise the number of new API classes it introduced.
This was done to satisfy community feedback from Sling and Oak. As such it made
compromises and tried to use java.* classes as far as possible.
The logic was as follows:
There are many views on the IllegalArgumentException and how it should be used.
eg [https://airbrake.io/blog/java/illegalargumentexception]. Most centre on
using the exception when the arguments are illegal from an application logic
point of view, but are not illegal from a JVM point of view (as is the case
with a NPE). The example given in that post was Books. A call to
Book.getPage(1492) where there are Book implementation has 1000 pages only,
should result in an IllegalArgumentException.
By the same argument, a call URIProvider.toURI(Resource resource,
URIProvider.Scope scope, URIProvider.Operation operation) where resource, scope
or operation and not legal for the implementation of the URIProvider should
throw an IllegalArgumentException.
It is unfortunate that Java decided to make an IllegalArgumentException
unchecked, and did not provide a checked version.
That was pragmatic logic for the API design at the time it was created.
The null check in the JcrNodeResource code is defensive implementation to catch
those bad developers that fail to add findbugs to their code and read the
JavaDoc. No one is perfect. Everyone makes mistakes.
----------------------------------
In a perfectionists world all of the above is ugly.
A perfectionist might have create a URIHolder object with an isValid() method
and made the toURI call @Nonnull, but that would have added more API objects
and been against the community feedback in both Sling and Oak that surrounded
the introduction of this API.
An earlier version of the API did have more classes.
IIUC this API will be deprecated soon replaced by a completely new way of
handling Binaries that Alex is working on. Perhaps its better to close this
issue as wont fix and avoid investing more time in a API that will soon be
deprecated.
> UriProvider throws unchecked IllegalArgumentException that must be handled by
> consumers
> ---------------------------------------------------------------------------------------
>
> Key: SLING-7510
> URL: https://issues.apache.org/jira/browse/SLING-7510
> Project: Sling
> Issue Type: Improvement
> Components: API
> Affects Versions: API 2.16.4
> Reporter: Alexander Klimetschek
> Priority: Major
> Fix For: API 3.0.1
>
>
> h3. Status quo
> A consumer of the
> [UriProvider|https://github.com/apache/sling-org-apache-sling-api/blob/dfc41640031bc87ec271c648b22073e65f4f171a/src/main/java/org/apache/sling/api/resource/external/URIProvider.java#L45]
> currently is required to handle an unchecked {{IllegalArgumentException}},
> which is thrown when the provider is not able to handle the binary. Note that
> it is not supposed to ever return null per the javadoc. The
> [JcrNodeResource|https://github.com/apache/sling-org-apache-sling-jcr-resource/blob/0e2ebd0f1a5c7cb2044b2d754945eb0ee7641081/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrNodeResource.java#L233-L242]
> shows a typical consumer code (although it still does do a null check).
> For the use case of asking multiple providers and taking the first one that
> responds it's not an optimal pattern to rely on an unchecked exception for
> the expected failure case that one provider by design cannot handle a certain
> binary or request. Throwing an {{IllegalArgumentException}} if there is no
> problem with the argument passed from the client, but a limit or
> configuration setting of the provider, is misleading. Also, given there are
> multiple providers active, a client cannot know upfront which provider is the
> right one for a given binary and somehow prevent the "illegal argument" call
> in the first place.
> h3. Suggestion
> Often, {{null}} return values are used in such a case. The provider can log
> any possible useful information itself, on why it could not handle it, if
> needed. This would simplify the consumer code (no try/catch necessary) and
> remove unnecessary cost of exception handling for normal code paths.
> JcrNodeResource itself it uses a null return value to pass on the "could not
> retrieve anything" state to the upper layers.
> If the goal really is to use exceptions here, the API should add a
> {{@Nonnull}} annotation for the return value _and_ the expected failure
> exception should be a checked one such as a new {{UriProviderException}}.
> Then for any unexpected faults (e.g. network error), it's fine to allow
> providers to throw a unchecked runtime exception, and usually that's not
> something that is explicitly mentioned in javadoc, but would definitely not
> hurt.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
