James Mime4j
  1. James Mime4j
  2. MIME4J-77

Decide Whether MimeException should extend IOException

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.4
    • Fix Version/s: 0.6
    • Component/s: None
    • Labels:
      None

      Description

      Decide whether MimeException should extend IOException

      1. mimeex.patch
        6 kB
        Oleg Kalnichevski

        Issue Links

          Activity

          Hide
          Robert Burrell Donkin added a comment -

          Forking discussion

          Show
          Robert Burrell Donkin added a comment - Forking discussion
          Hide
          Robert Burrell Donkin added a comment -

          From https://issues.apache.org/jira/browse/MIME4J-73:

          Markus Wiederkehr:
          > Speaking of MimeException, is it a good idea that it extends IOException? The MessagingException from javax.mail does not do that.
          > I think a MimeException should indicate a malformed message and that has nothing to do with an I/O error.

          Oleg Kalnichevski:
          >> * MimeException extends Exception
          >
          >+1. I generally like a clear distinction between protocol exceptions (usually fatal) and I/O exceptions (usually recoverable).
          > However, this can be potentially contentious and certainly should be discussed as a separate issue.

          Show
          Robert Burrell Donkin added a comment - From https://issues.apache.org/jira/browse/MIME4J-73: Markus Wiederkehr: > Speaking of MimeException, is it a good idea that it extends IOException? The MessagingException from javax.mail does not do that. > I think a MimeException should indicate a malformed message and that has nothing to do with an I/O error. Oleg Kalnichevski: >> * MimeException extends Exception > >+1. I generally like a clear distinction between protocol exceptions (usually fatal) and I/O exceptions (usually recoverable). > However, this can be potentially contentious and certainly should be discussed as a separate issue.
          Hide
          Oleg Kalnichevski added a comment -

          Robert,
          This would be a pretty major change. Is it okay if we address the issue in 0.6?

          Oleg

          Show
          Oleg Kalnichevski added a comment - Robert, This would be a pretty major change. Is it okay if we address the issue in 0.6? Oleg
          Hide
          Robert Burrell Donkin added a comment -

          Two separate exception hierarchies separating possibly recoverable I/O exceptions from probably fatal protocol exceptions is the conventional approach. Mime4j previously used this approach. MimeException was switched to subclass IOException between 0.4 to trial an less conventional approach with the advantage of a more concise API.

          At the very least, since this approach is unconventional it should be well documented.

          The argument in favour of a single hierarchy is that it allows a more concise and maintainable API.

          In use cases where the user does not care about recovery, at most a single catch is needed. This catch is a general java exception (as opposed to a mime4j one) so in many typical use cases, a single general (ie not library specific) catch or throws will suffice.

          This approach has the disadvantage that the correct exception to catch in use cases when the user cares about recovery is not self-documenting. In other words, the API is less obvious for these cases. The user will need to consult the Mime4J documentation and catch then rethrow or handle the MimeException.

          My estimation is that advanced users working at a low level are more likely to care about recovery whereas users working at a high level are unlikely to care. So I think that this approach is worthwhile.

          Show
          Robert Burrell Donkin added a comment - Two separate exception hierarchies separating possibly recoverable I/O exceptions from probably fatal protocol exceptions is the conventional approach. Mime4j previously used this approach. MimeException was switched to subclass IOException between 0.4 to trial an less conventional approach with the advantage of a more concise API. At the very least, since this approach is unconventional it should be well documented. The argument in favour of a single hierarchy is that it allows a more concise and maintainable API. In use cases where the user does not care about recovery, at most a single catch is needed. This catch is a general java exception (as opposed to a mime4j one) so in many typical use cases, a single general (ie not library specific) catch or throws will suffice. This approach has the disadvantage that the correct exception to catch in use cases when the user cares about recovery is not self-documenting. In other words, the API is less obvious for these cases. The user will need to consult the Mime4J documentation and catch then rethrow or handle the MimeException. My estimation is that advanced users working at a low level are more likely to care about recovery whereas users working at a high level are unlikely to care. So I think that this approach is worthwhile.
          Hide
          Robert Burrell Donkin added a comment -

          Addressing in 0.6 is fine by me

          Show
          Robert Burrell Donkin added a comment - Addressing in 0.6 is fine by me
          Hide
          Robert Burrell Donkin added a comment -

          If anyone disagrees and thinks it needs to be addressed now, switch it back.

          Show
          Robert Burrell Donkin added a comment - If anyone disagrees and thinks it needs to be addressed now, switch it back.
          Hide
          Oleg Kalnichevski added a comment -

          > My estimation is that advanced users working at a low level are more likely to care about recovery whereas users working at a high level are unlikely to care. So I think that this approach is worthwhile.

          Robert et al,

          That's the approach we chose for HttpClient 4.0. All core and client components make a clear distinction between IOExceptions and HttpExceptions. The former are considered recoverable, the latter are not. At the topmost level (that is, in the HttpClient class, which is what the overwhelming majority of users are likely to interact with) HttpExceptions are caught and re-thrown as special subclass of IOException. All seem happy.

          We could make MimeException a separate class of exceptions distinct from IOException in all low level components and then catch MimeExcetions in the Message class and rethrow them as IOException.

          Oleg

          Show
          Oleg Kalnichevski added a comment - > My estimation is that advanced users working at a low level are more likely to care about recovery whereas users working at a high level are unlikely to care. So I think that this approach is worthwhile. Robert et al, That's the approach we chose for HttpClient 4.0. All core and client components make a clear distinction between IOExceptions and HttpExceptions. The former are considered recoverable, the latter are not. At the topmost level (that is, in the HttpClient class, which is what the overwhelming majority of users are likely to interact with) HttpExceptions are caught and re-thrown as special subclass of IOException. All seem happy. We could make MimeException a separate class of exceptions distinct from IOException in all low level components and then catch MimeExcetions in the Message class and rethrow them as IOException. Oleg
          Hide
          Markus Wiederkehr added a comment -

          My concern is mainly that extending a class means establishing an is-a relationship. And in my opinion a MimeException simply is not an IOException.

          That being said it don't think it's a major issue. If I know I have to catch MimeException before catching IOException then all works well. And users who don't need to distinguish can simply catch IOException and be done with it.

          If you decide to leave it as it is I think the API should properly declare the MimeException wherever it is thrown. What I mean is a method or constructor should declare "throws MimeException, IOException" instead of simply declaring "throws IOException". This can be easily checked by temporarily changing the super class of MimeException to Exception and running the compiler.

          Show
          Markus Wiederkehr added a comment - My concern is mainly that extending a class means establishing an is-a relationship. And in my opinion a MimeException simply is not an IOException. That being said it don't think it's a major issue. If I know I have to catch MimeException before catching IOException then all works well. And users who don't need to distinguish can simply catch IOException and be done with it. If you decide to leave it as it is I think the API should properly declare the MimeException wherever it is thrown. What I mean is a method or constructor should declare "throws MimeException, IOException" instead of simply declaring "throws IOException". This can be easily checked by temporarily changing the super class of MimeException to Exception and running the compiler.
          Hide
          Oleg Kalnichevski added a comment -

          Here's the patch for your consideration.

          • MimeException no longer extends IOException
          • Message and Field constructors rethrow MimeExceptions as MimeIOExceptions enabling the user to implement a simple exception recovery with a single catch clause

          Please review

          Oleg

          Show
          Oleg Kalnichevski added a comment - Here's the patch for your consideration. MimeException no longer extends IOException Message and Field constructors rethrow MimeExceptions as MimeIOExceptions enabling the user to implement a simple exception recovery with a single catch clause Please review Oleg
          Hide
          Oleg Kalnichevski added a comment -

          Folks,
          Are there any objections to committing this patch?

          Oleg

          Show
          Oleg Kalnichevski added a comment - Folks, Are there any objections to committing this patch? Oleg
          Hide
          Robert Burrell Donkin added a comment -

          Markus - Mime4j is a IO library. One fault shared by many early attempts at java libraries was a proliferation of meaningless top level exceptions. The top level API of an IO library should throw IOExceptions.

          Oleg - I'm happy to follow the strategy you outline. I think that there are some corner cases where it might be difficult to distinguish between the cases (recoverable and not) but I don't think this should be a major worry.

          Commiting this patch is fine by me.

          Show
          Robert Burrell Donkin added a comment - Markus - Mime4j is a IO library. One fault shared by many early attempts at java libraries was a proliferation of meaningless top level exceptions. The top level API of an IO library should throw IOExceptions. Oleg - I'm happy to follow the strategy you outline. I think that there are some corner cases where it might be difficult to distinguish between the cases (recoverable and not) but I don't think this should be a major worry. Commiting this patch is fine by me.
          Hide
          Markus Wiederkehr added a comment -

          @Robert:

          You are using one Java class (IOException in this case) for two completely unrelated classes of errors that can occur when loading a message: true I/O errors and parsing errors in case of malformed messages. This invites the user to be sloppy with the error handling. Or worse, the user may not even become aware of the fact that there are two classes of errors if he/she does not study the API carefully enough.

          Using only one exception and a subclass instead of two independent exceptions is a bit unconventional, you said so yourself. It is certainly not what I would expect, but I can live with it.

          If you decide to stick to this decision Oleg's patch is a good compromise because it eliminates the is-a relationship between MimeException and IOException by introducing a third class MimeIOException that serves as an adapter.

          @Oleg:

          Please include a getter in MimeIOException to retrieve the underlying MimeException without having to cast.

          I think you should also declare "throws MimeIOException, IOException" where appropriate (Header and Message). This has the advantage that an IDE like Eclipse can create a catch block for MimeIOException automatically.

          Also your patch does not seem to cover MaxLineLimitException.

          Show
          Markus Wiederkehr added a comment - @Robert: You are using one Java class (IOException in this case) for two completely unrelated classes of errors that can occur when loading a message: true I/O errors and parsing errors in case of malformed messages. This invites the user to be sloppy with the error handling. Or worse, the user may not even become aware of the fact that there are two classes of errors if he/she does not study the API carefully enough. Using only one exception and a subclass instead of two independent exceptions is a bit unconventional, you said so yourself. It is certainly not what I would expect, but I can live with it. If you decide to stick to this decision Oleg's patch is a good compromise because it eliminates the is-a relationship between MimeException and IOException by introducing a third class MimeIOException that serves as an adapter. @Oleg: Please include a getter in MimeIOException to retrieve the underlying MimeException without having to cast. I think you should also declare "throws MimeIOException, IOException" where appropriate (Header and Message). This has the advantage that an IDE like Eclipse can create a catch block for MimeIOException automatically. Also your patch does not seem to cover MaxLineLimitException.
          Hide
          Oleg Kalnichevski added a comment -

          Patch checked in

          Oleg

          Show
          Oleg Kalnichevski added a comment - Patch checked in Oleg
          Hide
          Robert Burrell Donkin added a comment -

          Closing all issues fixed previously, after a brief review of each.

          Show
          Robert Burrell Donkin added a comment - Closing all issues fixed previously, after a brief review of each.

            People

            • Assignee:
              Unassigned
              Reporter:
              Robert Burrell Donkin
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development