[
https://issues.apache.org/jira/browse/SLING-7510?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Alexander Klimetschek updated SLING-7510:
-----------------------------------------
Description:
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.
was:
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 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.
> UriProvider should throw a checked exception instead of
> IllegalArgumentException
> --------------------------------------------------------------------------------
>
> 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
>
> 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)
