sling-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Robert Munteanu (JIRA)" <j...@apache.org>
Subject [jira] [Updated] (SLING-7510) UriProvider throws unchecked IllegalArgumentException that must be handled by consumers
Date Fri, 17 Aug 2018 16:59:00 GMT

     [ https://issues.apache.org/jira/browse/SLING-7510?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]

Robert Munteanu updated SLING-7510:
-----------------------------------
    Fix Version/s:     (was: API 2.18.4)
                   API 2.18.6

> 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 2.18.6
>
>
> 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)

Mime
View raw message