Synapse
  1. Synapse
  2. SYNAPSE-349

Transport base package is dependent on transport specific (VFS and Mail) classes

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.2
    • Fix Version/s: 2.0
    • Component/s: Transports
    • Labels:
      None

      Description

      The base transport package of Synapse is dependent on VFS and Mail specific classes, which seems wrong to me. Ideally the base transport classes should not depend on any transport specific aspects.

      Patch to follow shortly.

      Oleg

      1. basetransport.patch
        4 kB
        Oleg Kalnichevski

        Activity

        Hide
        Oleg Kalnichevski added a comment -

        Please review.

        Oleg

        Show
        Oleg Kalnichevski added a comment - Please review. Oleg
        Hide
        Andreas Veithen added a comment -

        Two comments:

        • getMinPollTime doesn't seem to be used anywhere. We might as well remove it completely, given that the logic of that method is not very sophisticated and it would be easy to reimplement it when the need for it arises.
        • Your patch partially undoes the modifications introduced to solve SYNAPSE-260 and will cause a regression. These modifications addressed a problem that appeared in the VFS transport. If they break other transports, then we need to figure out another solution that works correctly for all transports.
        Show
        Andreas Veithen added a comment - Two comments: getMinPollTime doesn't seem to be used anywhere. We might as well remove it completely, given that the logic of that method is not very sophisticated and it would be easy to reimplement it when the need for it arises. Your patch partially undoes the modifications introduced to solve SYNAPSE-260 and will cause a regression. These modifications addressed a problem that appeared in the VFS transport. If they break other transports, then we need to figure out another solution that works correctly for all transports.
        Hide
        Oleg Kalnichevski added a comment -

        Andreas,

        Is there a test case for that issue? All tests seem to pass for me. Anyways, recursive dependencies between base package and concrete protocol implementations strike me as odd.

        Oleg

        Show
        Oleg Kalnichevski added a comment - Andreas, Is there a test case for that issue? All tests seem to pass for me. Anyways, recursive dependencies between base package and concrete protocol implementations strike me as odd. Oleg
        Hide
        Andreas Veithen added a comment -

        There is currently no test case for SYNAPSE-260. It is true that the test coverage for the VFS transport is insufficient. I will try to work on this for the 1.3 release (see SYNAPSE-350).

        I totally agree on your comment about recursive dependencies. At first I didn't understand the problem with the piece of code related to content type and charset encoding. It is the fact that it introduces a dependency on javax.mail.internet, right?

        Show
        Andreas Veithen added a comment - There is currently no test case for SYNAPSE-260 . It is true that the test coverage for the VFS transport is insufficient. I will try to work on this for the 1.3 release (see SYNAPSE-350 ). I totally agree on your comment about recursive dependencies. At first I didn't understand the problem with the piece of code related to content type and charset encoding. It is the fact that it introduces a dependency on javax.mail.internet, right?
        Hide
        Oleg Kalnichevski added a comment -

        > It is the fact that it introduces a dependency on javax.mail.internet, right?

        Yes, it is. Ideally base transport package should not have dependencies on any specific transport interfaces or classes

        Oleg

        Show
        Oleg Kalnichevski added a comment - > It is the fact that it introduces a dependency on javax.mail.internet, right? Yes, it is. Ideally base transport package should not have dependencies on any specific transport interfaces or classes Oleg
        Hide
        Oleg Kalnichevski added a comment -

        All right. Can we tackle this issue with a series of small, incremental changes then? Can we decouple base transport package from VFS for a start?

        Oleg

        Show
        Oleg Kalnichevski added a comment - All right. Can we tackle this issue with a series of small, incremental changes then? Can we decouple base transport package from VFS for a start? Oleg
        Hide
        Andreas Veithen added a comment -

        What issues other than the presence of the getMinPollTime and the usage of javax.mail.internet.ContentType did you identify? What are the first incremental changes that you see?

        If there are many issues/changes, then we should first concentrate on the improvement of the unit tests (see SYNAPSE-246 and SYNAPSE-350).

        Show
        Andreas Veithen added a comment - What issues other than the presence of the getMinPollTime and the usage of javax.mail.internet.ContentType did you identify? What are the first incremental changes that you see? If there are many issues/changes, then we should first concentrate on the improvement of the unit tests (see SYNAPSE-246 and SYNAPSE-350 ).
        Hide
        Oleg Kalnichevski added a comment -

        Andreas,

        It is not about particular methods but rather about recursive dependencies between base transport package and individual transport packages. The proposed patch decouples the base package from VFS specific dependencies as the first step. We can deal with the dependency on javax.mail.internet later on .

        Oleg

        Show
        Oleg Kalnichevski added a comment - Andreas, It is not about particular methods but rather about recursive dependencies between base transport package and individual transport packages. The proposed patch decouples the base package from VFS specific dependencies as the first step. We can deal with the dependency on javax.mail.internet later on . Oleg
        Hide
        Andreas Veithen added a comment -

        The proposed patch looks fine for me.

        Show
        Andreas Veithen added a comment - The proposed patch looks fine for me.
        Hide
        Oleg Kalnichevski added a comment -

        Patch checked in.

        Oleg

        Show
        Oleg Kalnichevski added a comment - Patch checked in. Oleg
        Hide
        Andreas Veithen added a comment -

        The transport base classes now live in their own Maven module (in WS-Commons Transport) and this module only depends on AXIOM and the Axis2 kernel. It still (indirectly) depends on the javax.mail.internet package, but this is OK since both AXIOM and Axis2 depends on it. Therefore this issue can be considered as fixed.

        Show
        Andreas Veithen added a comment - The transport base classes now live in their own Maven module (in WS-Commons Transport) and this module only depends on AXIOM and the Axis2 kernel. It still (indirectly) depends on the javax.mail.internet package, but this is OK since both AXIOM and Axis2 depends on it. Therefore this issue can be considered as fixed.

          People

          • Assignee:
            Unassigned
            Reporter:
            Oleg Kalnichevski
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development