Carsten Ziegeler updated SLING-7510:
    Fix Version/s:     (was: API 2.16.6)
                   API 2.17.2

> 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.17.2
> 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

Reply via email to