Migration path for Uri to throw a checked exception: small step #1

This CL introduces a trivial wrapper UriException class, which wraps IllegalArgumentException (itself a RuntimeException), and changes the Uri class to throw it exclusively rather than IllegalArgumentException or RuntimeException directly.

This allows calling code to wrap relevant methods in semantically-bounded try/catch blocks without our having to have Uri's methods throw a checked exception, which in turn would necessitate large numbers of downstream changes.

In this way, we can slowly phase in use of UriException, to the point that eventually we may find it relatively easy to change this exception to a checked version of itself with minimal trouble.

Comments welcome.

[zhoresh, 2010-02-18 18:00:45]

LGTM

If I understand correctly, the ultimate goal is to have a non
RuntimeException here.
The reason for RuntimeException now, is so we can change in stages.

Maybe add some explanation in the class doc, of what the plan, and who need
to use it.


On Tue, Feb 16, 2010 at 4:10 PM, <johnfargo@gmail.com> wrote:

> Reviewers: shindig.remailer_gmail.com,
>
> Description:
> This CL introduces a trivial wrapper UriException class, which wraps
> IllegalArgumentException (itself a RuntimeException), and changes the
> Uri class to throw it exclusively rather than IllegalArgumentException
> or RuntimeException directly.
>
> This allows calling code to wrap relevant methods in
> semantically-bounded try/catch blocks without our having to have Uri's
> methods throw a checked exception, which in turn would necessitate large
> numbers of downstream changes.
>
> In this way, we can slowly phase in use of UriException, to the point
> that eventually we may find it relatively easy to change this exception
> to a checked version of itself with minimal trouble.
>
> Comments welcome.
>
> Please review this at http://codereview.appspot.com/210041/show
>
> Affected files:
>  java/common/src/main/java/org/apache/shindig/common/uri/Uri.java
>
>
> Index: java/common/src/main/java/org/apache/shindig/common/uri/Uri.java
> ===================================================================
> --- java/common/src/main/java/org/apache/shindig/common/uri/Uri.java
>  (revision 910762)
> +++ java/common/src/main/java/org/apache/shindig/common/uri/Uri.java
>  (working copy)
> @@ -97,7 +97,17 @@
>    * @return A new Uri, parsed into components.
>    */
>   public static Uri parse(String text) {
> -    return parser.parse(text);
> +    try {
> +      return parser.parse(text);
> +    } catch (IllegalArgumentException e) {
> +      // This occurs all the time. Wrap the exception in a Uri-specific
> +      // exception, yet one that remains a RuntimeException, so that
> +      // callers may catch a specific exception rather than a blanket
> +      // Exception, as a compromise between throwing a checked exception
> +      // here (forcing wide-scale refactoring across the code base) and
> +      // forcing users to simply catch abstract Exceptions here and there.
> +      throw new UriException(e);
> +    }
>   }
>
>   /**
> @@ -105,7 +115,7 @@
>    */
>   public static Uri fromJavaUri(URI uri) {
>     if (uri.isOpaque()) {
> -      throw new IllegalArgumentException("No support for opaque Uris " +
> uri.toString());
> +      throw new UriException("No support for opaque Uris " +
> uri.toString());
>     }
>     return new UriBuilder()
>         .setScheme(uri.getScheme())
> @@ -124,7 +134,7 @@
>       return new URI(toString());
>     } catch (URISyntaxException e) {
>       // Shouldn't ever happen.
> -      throw new IllegalArgumentException(e);
> +      throw new UriException(e);
>     }
>   }
>
> @@ -185,7 +195,7 @@
>     if (StringUtils.isEmpty(uri.authority) &&
>         StringUtils.isEmpty(uri.path) &&
>         StringUtils.isEmpty(uri.query)) {
> -      throw new IllegalArgumentException("Invalid scheme-specific part");
> +      throw new UriException("Invalid scheme-specific part");
>     }
>   }
>
> @@ -387,4 +397,14 @@
>     if (!(obj instanceof Uri)) {return false;}
>     return Objects.equal(text, ((Uri)obj).text);
>   }
> +
> +  public static class UriException extends IllegalArgumentException {
> +    private UriException(Exception e) {
> +      super(e);
> +    }
> +
> +    private UriException(String msg) {
> +      super(msg);
> +    }
> +  }
>  }
>
>
>

[johnfargo, 2010-02-19 18:05:56]

Sure, added another comment, this time to the exception itself.

Cheers,
John

On Thu, Feb 18, 2010 at 10:00 AM, Ziv Horesh <zhoresh@gmail.com> wrote:

> LGTM
>
> If I understand correctly, the ultimate goal is to have a non
> RuntimeException here.
> The reason for RuntimeException now, is so we can change in stages.
>
> Maybe add some explanation in the class doc, of what the plan, and who need
> to use it.
>
>
>
> On Tue, Feb 16, 2010 at 4:10 PM, <johnfargo@gmail.com> wrote:
>
>> Reviewers: shindig.remailer_gmail.com,
>>
>> Description:
>> This CL introduces a trivial wrapper UriException class, which wraps
>> IllegalArgumentException (itself a RuntimeException), and changes the
>> Uri class to throw it exclusively rather than IllegalArgumentException
>> or RuntimeException directly.
>>
>> This allows calling code to wrap relevant methods in
>> semantically-bounded try/catch blocks without our having to have Uri's
>> methods throw a checked exception, which in turn would necessitate large
>> numbers of downstream changes.
>>
>> In this way, we can slowly phase in use of UriException, to the point
>> that eventually we may find it relatively easy to change this exception
>> to a checked version of itself with minimal trouble.
>>
>> Comments welcome.
>>
>> Please review this at http://codereview.appspot.com/210041/show
>>
>> Affected files:
>>  java/common/src/main/java/org/apache/shindig/common/uri/Uri.java
>>
>>
>> Index: java/common/src/main/java/org/apache/shindig/common/uri/Uri.java
>> ===================================================================
>> --- java/common/src/main/java/org/apache/shindig/common/uri/Uri.java
>>  (revision 910762)
>> +++ java/common/src/main/java/org/apache/shindig/common/uri/Uri.java
>>  (working copy)
>> @@ -97,7 +97,17 @@
>>    * @return A new Uri, parsed into components.
>>    */
>>   public static Uri parse(String text) {
>> -    return parser.parse(text);
>> +    try {
>> +      return parser.parse(text);
>> +    } catch (IllegalArgumentException e) {
>> +      // This occurs all the time. Wrap the exception in a Uri-specific
>> +      // exception, yet one that remains a RuntimeException, so that
>> +      // callers may catch a specific exception rather than a blanket
>> +      // Exception, as a compromise between throwing a checked exception
>> +      // here (forcing wide-scale refactoring across the code base) and
>> +      // forcing users to simply catch abstract Exceptions here and
>> there.
>> +      throw new UriException(e);
>> +    }
>>   }
>>
>>   /**
>> @@ -105,7 +115,7 @@
>>    */
>>   public static Uri fromJavaUri(URI uri) {
>>     if (uri.isOpaque()) {
>> -      throw new IllegalArgumentException("No support for opaque Uris " +
>> uri.toString());
>> +      throw new UriException("No support for opaque Uris " +
>> uri.toString());
>>     }
>>     return new UriBuilder()
>>         .setScheme(uri.getScheme())
>> @@ -124,7 +134,7 @@
>>       return new URI(toString());
>>     } catch (URISyntaxException e) {
>>       // Shouldn't ever happen.
>> -      throw new IllegalArgumentException(e);
>> +      throw new UriException(e);
>>     }
>>   }
>>
>> @@ -185,7 +195,7 @@
>>     if (StringUtils.isEmpty(uri.authority) &&
>>         StringUtils.isEmpty(uri.path) &&
>>         StringUtils.isEmpty(uri.query)) {
>> -      throw new IllegalArgumentException("Invalid scheme-specific part");
>> +      throw new UriException("Invalid scheme-specific part");
>>     }
>>   }
>>
>> @@ -387,4 +397,14 @@
>>     if (!(obj instanceof Uri)) {return false;}
>>     return Objects.equal(text, ((Uri)obj).text);
>>   }
>> +
>> +  public static class UriException extends IllegalArgumentException {
>> +    private UriException(Exception e) {
>> +      super(e);
>> +    }
>> +
>> +    private UriException(String msg) {
>> +      super(msg);
>> +    }
>> +  }
>>  }
>>
>>
>>
>