Qpid
  1. Qpid
  2. QPID-3832

Qpid 0.14 broke transport connection setting

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.14
    • Fix Version/s: 0.15
    • Component/s: C++ Client
    • Labels:

      Description

      The transport connection setting was broken in r1156262.

      Consider the following test code:

      #include <qpid/messaging/Connection.h>
      
      int main(int argc, char *argv[]) {
          qpid::types::Variant::Map options;
          options["transport" ] = "ssl";
          qpid::messaging::Connection connection("localhost:5671", options);
          connection.open();
          return 0;
      }
      

      Using Qpid 0.12 client libraries, the above code would use SSL. But with Qpid 0.14, the code will not use SSL.

      The change in behaviour is a result of code changes to the ConnectionImpl::tryConnect function in cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp in r1156262

      Prior to that revision, the SimpleUrlParser was used, and settings passed to a Connection::open overload directly. This worked fine.

      Post that revision, the code looks like this:

      Url url(*i);
      if (url.getUser().size()) settings.username = url.getUser();
      if (url.getPass().size()) settings.password = url.getPass();
      QPID_LOG(debug, *i << " " << settings.protocol);
      QPID_LOG(debug, url);
      connection.open(url, settings);
      

      (I added the QPID_LOG calls).

      The QPID_LOG calls produce output like:

      localhost:5671 ssl
      amqp:tcp:localhost:5671
      

      The problem is that the Url constructor is defaulting the protocol to TCP (via UrlParser::protocolAddr). Then, when the Connection::open overload is called, the Url object's defaulted-but-now-explicit protocol value (TCP) is used in preference to the settings.protocol value (SSL).

      I think the correct solution here, thought it would be non-trivial, would be to pass settings (or at least settings.protocol) to the Url constructor as an optional default protocol. Then the Url class should use that default protocol instead of TCP when the address string does not include an explicit protocol.

      Another option would be to provide some sort of Url::protocolWasDefaulted flag so that the ConnectionImpl::tryConnect function could override from settings when appropriate - this would be less code change, but the above solution would be more elegant IMO.

      Note, the obvious workaround is to always use explicit protocols in URLs. That is, always use "ssl:hostname" instead of "hostname".

        Activity

        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4083/#review5495
        -----------------------------------------------------------

        Ship it!

        makes sense to me – that defaulting decision definitely belongs at higher level.

        • mick

        On 2012-02-28 15:51:40, Gordon Sim wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4083/

        -----------------------------------------------------------

        (Updated 2012-02-28 15:51:40)

        Review request for Alan Conway, Kenneth Giusti and michael goulish.

        Summary

        -------

        Previous unification of URL support broke the ability to supply the 'transport' (e.g. tcp, rdma, ssl etc) on a connection. This can still be supplied in the URL itself if the 0-10 URL format is used but the regression should be fixed.

        The change in this patch adds a method to the Url class to parse a string without populating any resulting addresses with TCP as the default protocol if none is specified. This allows the protocol in the Address to indicate what was explicitly provided and lets the decision on defaults be taken elsewhere (in this case in the underlying client API).

        This addresses bug QPID-3832.

        https://issues.apache.org/jira/browse/QPID-3832

        Diffs

        -----

        /trunk/qpid/cpp/include/qpid/Url.h 1294525

        /trunk/qpid/cpp/src/qpid/Url.cpp 1294525

        /trunk/qpid/cpp/src/qpid/client/Connection.cpp 1294525

        /trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp 1294525

        /trunk/qpid/cpp/src/tests/ssl_test 1294525

        Diff: https://reviews.apache.org/r/4083/diff

        Testing

        -------

        New case added to sssl tests that uses a mixture of url and connection-option. Make check passes.

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4083/#review5495 ----------------------------------------------------------- Ship it! makes sense to me – that defaulting decision definitely belongs at higher level. mick On 2012-02-28 15:51:40, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4083/ ----------------------------------------------------------- (Updated 2012-02-28 15:51:40) Review request for Alan Conway, Kenneth Giusti and michael goulish. Summary ------- Previous unification of URL support broke the ability to supply the 'transport' (e.g. tcp, rdma, ssl etc) on a connection. This can still be supplied in the URL itself if the 0-10 URL format is used but the regression should be fixed. The change in this patch adds a method to the Url class to parse a string without populating any resulting addresses with TCP as the default protocol if none is specified. This allows the protocol in the Address to indicate what was explicitly provided and lets the decision on defaults be taken elsewhere (in this case in the underlying client API). This addresses bug QPID-3832 . https://issues.apache.org/jira/browse/QPID-3832 Diffs ----- /trunk/qpid/cpp/include/qpid/Url.h 1294525 /trunk/qpid/cpp/src/qpid/Url.cpp 1294525 /trunk/qpid/cpp/src/qpid/client/Connection.cpp 1294525 /trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp 1294525 /trunk/qpid/cpp/src/tests/ssl_test 1294525 Diff: https://reviews.apache.org/r/4083/diff Testing ------- New case added to sssl tests that uses a mixture of url and connection-option. Make check passes. Thanks, Gordon
        Hide
        Gordon Sim added a comment -

        Change made as suggested by Paul.

        Show
        Gordon Sim added a comment - Change made as suggested by Paul.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4083/#review5461
        -----------------------------------------------------------

        Ship it!

        • Alan

        On 2012-02-28 15:51:40, Gordon Sim wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4083/

        -----------------------------------------------------------

        (Updated 2012-02-28 15:51:40)

        Review request for Alan Conway, Kenneth Giusti and michael goulish.

        Summary

        -------

        Previous unification of URL support broke the ability to supply the 'transport' (e.g. tcp, rdma, ssl etc) on a connection. This can still be supplied in the URL itself if the 0-10 URL format is used but the regression should be fixed.

        The change in this patch adds a method to the Url class to parse a string without populating any resulting addresses with TCP as the default protocol if none is specified. This allows the protocol in the Address to indicate what was explicitly provided and lets the decision on defaults be taken elsewhere (in this case in the underlying client API).

        This addresses bug QPID-3832.

        https://issues.apache.org/jira/browse/QPID-3832

        Diffs

        -----

        /trunk/qpid/cpp/include/qpid/Url.h 1294525

        /trunk/qpid/cpp/src/qpid/Url.cpp 1294525

        /trunk/qpid/cpp/src/qpid/client/Connection.cpp 1294525

        /trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp 1294525

        /trunk/qpid/cpp/src/tests/ssl_test 1294525

        Diff: https://reviews.apache.org/r/4083/diff

        Testing

        -------

        New case added to sssl tests that uses a mixture of url and connection-option. Make check passes.

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4083/#review5461 ----------------------------------------------------------- Ship it! Alan On 2012-02-28 15:51:40, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4083/ ----------------------------------------------------------- (Updated 2012-02-28 15:51:40) Review request for Alan Conway, Kenneth Giusti and michael goulish. Summary ------- Previous unification of URL support broke the ability to supply the 'transport' (e.g. tcp, rdma, ssl etc) on a connection. This can still be supplied in the URL itself if the 0-10 URL format is used but the regression should be fixed. The change in this patch adds a method to the Url class to parse a string without populating any resulting addresses with TCP as the default protocol if none is specified. This allows the protocol in the Address to indicate what was explicitly provided and lets the decision on defaults be taken elsewhere (in this case in the underlying client API). This addresses bug QPID-3832 . https://issues.apache.org/jira/browse/QPID-3832 Diffs ----- /trunk/qpid/cpp/include/qpid/Url.h 1294525 /trunk/qpid/cpp/src/qpid/Url.cpp 1294525 /trunk/qpid/cpp/src/qpid/client/Connection.cpp 1294525 /trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp 1294525 /trunk/qpid/cpp/src/tests/ssl_test 1294525 Diff: https://reviews.apache.org/r/4083/diff Testing ------- New case added to sssl tests that uses a mixture of url and connection-option. Make check passes. Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4083/
        -----------------------------------------------------------

        (Updated 2012-02-28 15:51:40.887275)

        Review request for Alan Conway, Kenneth Giusti and michael goulish.

        Changes
        -------

        Changed to use default protocol passed in, as suggested by Alan.

        Summary
        -------

        Previous unification of URL support broke the ability to supply the 'transport' (e.g. tcp, rdma, ssl etc) on a connection. This can still be supplied in the URL itself if the 0-10 URL format is used but the regression should be fixed.

        The change in this patch adds a method to the Url class to parse a string without populating any resulting addresses with TCP as the default protocol if none is specified. This allows the protocol in the Address to indicate what was explicitly provided and lets the decision on defaults be taken elsewhere (in this case in the underlying client API).

        This addresses bug QPID-3832.
        https://issues.apache.org/jira/browse/QPID-3832

        Diffs (updated)


        /trunk/qpid/cpp/include/qpid/Url.h 1294525
        /trunk/qpid/cpp/src/qpid/Url.cpp 1294525
        /trunk/qpid/cpp/src/qpid/client/Connection.cpp 1294525
        /trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp 1294525
        /trunk/qpid/cpp/src/tests/ssl_test 1294525

        Diff: https://reviews.apache.org/r/4083/diff

        Testing
        -------

        New case added to sssl tests that uses a mixture of url and connection-option. Make check passes.

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4083/ ----------------------------------------------------------- (Updated 2012-02-28 15:51:40.887275) Review request for Alan Conway, Kenneth Giusti and michael goulish. Changes ------- Changed to use default protocol passed in, as suggested by Alan. Summary ------- Previous unification of URL support broke the ability to supply the 'transport' (e.g. tcp, rdma, ssl etc) on a connection. This can still be supplied in the URL itself if the 0-10 URL format is used but the regression should be fixed. The change in this patch adds a method to the Url class to parse a string without populating any resulting addresses with TCP as the default protocol if none is specified. This allows the protocol in the Address to indicate what was explicitly provided and lets the decision on defaults be taken elsewhere (in this case in the underlying client API). This addresses bug QPID-3832 . https://issues.apache.org/jira/browse/QPID-3832 Diffs (updated) /trunk/qpid/cpp/include/qpid/Url.h 1294525 /trunk/qpid/cpp/src/qpid/Url.cpp 1294525 /trunk/qpid/cpp/src/qpid/client/Connection.cpp 1294525 /trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp 1294525 /trunk/qpid/cpp/src/tests/ssl_test 1294525 Diff: https://reviews.apache.org/r/4083/diff Testing ------- New case added to sssl tests that uses a mixture of url and connection-option. Make check passes. Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-02-28 14:21:27, Alan Conway wrote:

        > I would suggest something like this:

        > parse(str, defaultProtocol=Address::TCP)

        > URL(str, defaultProtocol=Address::TCP)

        >

        > That's backward compatible and lets you set the default protocol to whatever you like when you are parsing the URL.

        >

        > The problem I see with leaving the protcol unspecified is that the URL may be interpreted in a different context from where it was created and get a different default protocol from what was intended.

        Gordon Sim wrote:

        Yes, I did deliberate between this two approaches. I felt on balance that the population of the protocol field on the address where none was specified was 'losing' information. I would however be prepared to change that view and have the parsing simply allow the default to be specified.

        One question is whether API compatibility is sufficient or whether we want to avoid breaking ABI as well. (I think my suggestion does both).

        Good point, but you can preserve both with the other approach too. Instead of replacing an existing function with one that has a default argument, add a new overload with a non-default argument.

        • Alan

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4083/#review5389
        -----------------------------------------------------------

        On 2012-02-28 13:40:07, Gordon Sim wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4083/

        -----------------------------------------------------------

        (Updated 2012-02-28 13:40:07)

        Review request for Alan Conway, Kenneth Giusti and michael goulish.

        Summary

        -------

        Previous unification of URL support broke the ability to supply the 'transport' (e.g. tcp, rdma, ssl etc) on a connection. This can still be supplied in the URL itself if the 0-10 URL format is used but the regression should be fixed.

        The change in this patch adds a method to the Url class to parse a string without populating any resulting addresses with TCP as the default protocol if none is specified. This allows the protocol in the Address to indicate what was explicitly provided and lets the decision on defaults be taken elsewhere (in this case in the underlying client API).

        This addresses bug QPID-3832.

        https://issues.apache.org/jira/browse/QPID-3832

        Diffs

        -----

        /trunk/qpid/cpp/include/qpid/Url.h 1294525

        /trunk/qpid/cpp/src/qpid/Url.cpp 1294525

        /trunk/qpid/cpp/src/qpid/client/Connection.cpp 1294525

        /trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp 1294525

        /trunk/qpid/cpp/src/tests/ssl_test 1294525

        /trunk/qpid/cpp/src/tests/ssl_test 1294525

        Diff: https://reviews.apache.org/r/4083/diff

        Testing

        -------

        New case added to sssl tests that uses a mixture of url and connection-option. Make check passes.

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-02-28 14:21:27, Alan Conway wrote: > I would suggest something like this: > parse(str, defaultProtocol=Address::TCP) > URL(str, defaultProtocol=Address::TCP) > > That's backward compatible and lets you set the default protocol to whatever you like when you are parsing the URL. > > The problem I see with leaving the protcol unspecified is that the URL may be interpreted in a different context from where it was created and get a different default protocol from what was intended. Gordon Sim wrote: Yes, I did deliberate between this two approaches. I felt on balance that the population of the protocol field on the address where none was specified was 'losing' information. I would however be prepared to change that view and have the parsing simply allow the default to be specified. One question is whether API compatibility is sufficient or whether we want to avoid breaking ABI as well. (I think my suggestion does both ). Good point, but you can preserve both with the other approach too. Instead of replacing an existing function with one that has a default argument, add a new overload with a non-default argument. Alan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4083/#review5389 ----------------------------------------------------------- On 2012-02-28 13:40:07, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4083/ ----------------------------------------------------------- (Updated 2012-02-28 13:40:07) Review request for Alan Conway, Kenneth Giusti and michael goulish. Summary ------- Previous unification of URL support broke the ability to supply the 'transport' (e.g. tcp, rdma, ssl etc) on a connection. This can still be supplied in the URL itself if the 0-10 URL format is used but the regression should be fixed. The change in this patch adds a method to the Url class to parse a string without populating any resulting addresses with TCP as the default protocol if none is specified. This allows the protocol in the Address to indicate what was explicitly provided and lets the decision on defaults be taken elsewhere (in this case in the underlying client API). This addresses bug QPID-3832 . https://issues.apache.org/jira/browse/QPID-3832 Diffs ----- /trunk/qpid/cpp/include/qpid/Url.h 1294525 /trunk/qpid/cpp/src/qpid/Url.cpp 1294525 /trunk/qpid/cpp/src/qpid/client/Connection.cpp 1294525 /trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp 1294525 /trunk/qpid/cpp/src/tests/ssl_test 1294525 /trunk/qpid/cpp/src/tests/ssl_test 1294525 Diff: https://reviews.apache.org/r/4083/diff Testing ------- New case added to sssl tests that uses a mixture of url and connection-option. Make check passes. Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4083/#review5393
        -----------------------------------------------------------

        Ship it!

        From an aesthetics point of view, I prefer Alan's alternative syntax....

        • Kenneth

        On 2012-02-28 13:40:07, Gordon Sim wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4083/

        -----------------------------------------------------------

        (Updated 2012-02-28 13:40:07)

        Review request for Alan Conway, Kenneth Giusti and michael goulish.

        Summary

        -------

        Previous unification of URL support broke the ability to supply the 'transport' (e.g. tcp, rdma, ssl etc) on a connection. This can still be supplied in the URL itself if the 0-10 URL format is used but the regression should be fixed.

        The change in this patch adds a method to the Url class to parse a string without populating any resulting addresses with TCP as the default protocol if none is specified. This allows the protocol in the Address to indicate what was explicitly provided and lets the decision on defaults be taken elsewhere (in this case in the underlying client API).

        This addresses bug QPID-3832.

        https://issues.apache.org/jira/browse/QPID-3832

        Diffs

        -----

        /trunk/qpid/cpp/include/qpid/Url.h 1294525

        /trunk/qpid/cpp/src/qpid/Url.cpp 1294525

        /trunk/qpid/cpp/src/qpid/client/Connection.cpp 1294525

        /trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp 1294525

        /trunk/qpid/cpp/src/tests/ssl_test 1294525

        /trunk/qpid/cpp/src/tests/ssl_test 1294525

        Diff: https://reviews.apache.org/r/4083/diff

        Testing

        -------

        New case added to sssl tests that uses a mixture of url and connection-option. Make check passes.

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4083/#review5393 ----------------------------------------------------------- Ship it! From an aesthetics point of view, I prefer Alan's alternative syntax.... Kenneth On 2012-02-28 13:40:07, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4083/ ----------------------------------------------------------- (Updated 2012-02-28 13:40:07) Review request for Alan Conway, Kenneth Giusti and michael goulish. Summary ------- Previous unification of URL support broke the ability to supply the 'transport' (e.g. tcp, rdma, ssl etc) on a connection. This can still be supplied in the URL itself if the 0-10 URL format is used but the regression should be fixed. The change in this patch adds a method to the Url class to parse a string without populating any resulting addresses with TCP as the default protocol if none is specified. This allows the protocol in the Address to indicate what was explicitly provided and lets the decision on defaults be taken elsewhere (in this case in the underlying client API). This addresses bug QPID-3832 . https://issues.apache.org/jira/browse/QPID-3832 Diffs ----- /trunk/qpid/cpp/include/qpid/Url.h 1294525 /trunk/qpid/cpp/src/qpid/Url.cpp 1294525 /trunk/qpid/cpp/src/qpid/client/Connection.cpp 1294525 /trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp 1294525 /trunk/qpid/cpp/src/tests/ssl_test 1294525 /trunk/qpid/cpp/src/tests/ssl_test 1294525 Diff: https://reviews.apache.org/r/4083/diff Testing ------- New case added to sssl tests that uses a mixture of url and connection-option. Make check passes. Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-02-28 14:21:27, Alan Conway wrote:

        > I would suggest something like this:

        > parse(str, defaultProtocol=Address::TCP)

        > URL(str, defaultProtocol=Address::TCP)

        >

        > That's backward compatible and lets you set the default protocol to whatever you like when you are parsing the URL.

        >

        > The problem I see with leaving the protcol unspecified is that the URL may be interpreted in a different context from where it was created and get a different default protocol from what was intended.

        Yes, I did deliberate between this two approaches. I felt on balance that the population of the protocol field on the address where none was specified was 'losing' information. I would however be prepared to change that view and have the parsing simply allow the default to be specified.

        One question is whether API compatibility is sufficient or whether we want to avoid breaking ABI as well. (I think my suggestion does both).

        On 2012-02-28 14:21:27, Alan Conway wrote:

        > /trunk/qpid/cpp/include/qpid/Url.h, line 70

        > <https://reviews.apache.org/r/4083/diff/1/?file=86222#file86222line70>

        >

        > The name is a bit awkward.

        > What about parse(url, transport=TCP)?

        The name is indeed horrid!

        • Gordon

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4083/#review5389
        -----------------------------------------------------------

        On 2012-02-28 13:40:07, Gordon Sim wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4083/

        -----------------------------------------------------------

        (Updated 2012-02-28 13:40:07)

        Review request for Alan Conway, Kenneth Giusti and michael goulish.

        Summary

        -------

        Previous unification of URL support broke the ability to supply the 'transport' (e.g. tcp, rdma, ssl etc) on a connection. This can still be supplied in the URL itself if the 0-10 URL format is used but the regression should be fixed.

        The change in this patch adds a method to the Url class to parse a string without populating any resulting addresses with TCP as the default protocol if none is specified. This allows the protocol in the Address to indicate what was explicitly provided and lets the decision on defaults be taken elsewhere (in this case in the underlying client API).

        This addresses bug QPID-3832.

        https://issues.apache.org/jira/browse/QPID-3832

        Diffs

        -----

        /trunk/qpid/cpp/include/qpid/Url.h 1294525

        /trunk/qpid/cpp/src/qpid/Url.cpp 1294525

        /trunk/qpid/cpp/src/qpid/client/Connection.cpp 1294525

        /trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp 1294525

        /trunk/qpid/cpp/src/tests/ssl_test 1294525

        /trunk/qpid/cpp/src/tests/ssl_test 1294525

        Diff: https://reviews.apache.org/r/4083/diff

        Testing

        -------

        New case added to sssl tests that uses a mixture of url and connection-option. Make check passes.

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-02-28 14:21:27, Alan Conway wrote: > I would suggest something like this: > parse(str, defaultProtocol=Address::TCP) > URL(str, defaultProtocol=Address::TCP) > > That's backward compatible and lets you set the default protocol to whatever you like when you are parsing the URL. > > The problem I see with leaving the protcol unspecified is that the URL may be interpreted in a different context from where it was created and get a different default protocol from what was intended. Yes, I did deliberate between this two approaches. I felt on balance that the population of the protocol field on the address where none was specified was 'losing' information. I would however be prepared to change that view and have the parsing simply allow the default to be specified. One question is whether API compatibility is sufficient or whether we want to avoid breaking ABI as well. (I think my suggestion does both ). On 2012-02-28 14:21:27, Alan Conway wrote: > /trunk/qpid/cpp/include/qpid/Url.h, line 70 > < https://reviews.apache.org/r/4083/diff/1/?file=86222#file86222line70 > > > The name is a bit awkward. > What about parse(url, transport=TCP)? The name is indeed horrid! Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4083/#review5389 ----------------------------------------------------------- On 2012-02-28 13:40:07, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4083/ ----------------------------------------------------------- (Updated 2012-02-28 13:40:07) Review request for Alan Conway, Kenneth Giusti and michael goulish. Summary ------- Previous unification of URL support broke the ability to supply the 'transport' (e.g. tcp, rdma, ssl etc) on a connection. This can still be supplied in the URL itself if the 0-10 URL format is used but the regression should be fixed. The change in this patch adds a method to the Url class to parse a string without populating any resulting addresses with TCP as the default protocol if none is specified. This allows the protocol in the Address to indicate what was explicitly provided and lets the decision on defaults be taken elsewhere (in this case in the underlying client API). This addresses bug QPID-3832 . https://issues.apache.org/jira/browse/QPID-3832 Diffs ----- /trunk/qpid/cpp/include/qpid/Url.h 1294525 /trunk/qpid/cpp/src/qpid/Url.cpp 1294525 /trunk/qpid/cpp/src/qpid/client/Connection.cpp 1294525 /trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp 1294525 /trunk/qpid/cpp/src/tests/ssl_test 1294525 /trunk/qpid/cpp/src/tests/ssl_test 1294525 Diff: https://reviews.apache.org/r/4083/diff Testing ------- New case added to sssl tests that uses a mixture of url and connection-option. Make check passes. Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4083/#review5389
        -----------------------------------------------------------

        I would suggest something like this:
        parse(str, defaultProtocol=Address::TCP)
        URL(str, defaultProtocol=Address::TCP)

        That's backward compatible and lets you set the default protocol to whatever you like when you are parsing the URL.

        The problem I see with leaving the protcol unspecified is that the URL may be interpreted in a different context from where it was created and get a different default protocol from what was intended.

        /trunk/qpid/cpp/include/qpid/Url.h
        <https://reviews.apache.org/r/4083/#comment11715>

        The name is a bit awkward.
        What about parse(url, transport=TCP)?

        /trunk/qpid/cpp/src/qpid/Url.cpp
        <https://reviews.apache.org/r/4083/#comment11716>

        What about: URL(u, s, defaultProtocol=TCP)?

        • Alan

        On 2012-02-28 13:40:07, Gordon Sim wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4083/

        -----------------------------------------------------------

        (Updated 2012-02-28 13:40:07)

        Review request for Alan Conway, Kenneth Giusti and michael goulish.

        Summary

        -------

        Previous unification of URL support broke the ability to supply the 'transport' (e.g. tcp, rdma, ssl etc) on a connection. This can still be supplied in the URL itself if the 0-10 URL format is used but the regression should be fixed.

        The change in this patch adds a method to the Url class to parse a string without populating any resulting addresses with TCP as the default protocol if none is specified. This allows the protocol in the Address to indicate what was explicitly provided and lets the decision on defaults be taken elsewhere (in this case in the underlying client API).

        This addresses bug QPID-3832.

        https://issues.apache.org/jira/browse/QPID-3832

        Diffs

        -----

        /trunk/qpid/cpp/include/qpid/Url.h 1294525

        /trunk/qpid/cpp/src/qpid/Url.cpp 1294525

        /trunk/qpid/cpp/src/qpid/client/Connection.cpp 1294525

        /trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp 1294525

        /trunk/qpid/cpp/src/tests/ssl_test 1294525

        /trunk/qpid/cpp/src/tests/ssl_test 1294525

        Diff: https://reviews.apache.org/r/4083/diff

        Testing

        -------

        New case added to sssl tests that uses a mixture of url and connection-option. Make check passes.

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4083/#review5389 ----------------------------------------------------------- I would suggest something like this: parse(str, defaultProtocol=Address::TCP) URL(str, defaultProtocol=Address::TCP) That's backward compatible and lets you set the default protocol to whatever you like when you are parsing the URL. The problem I see with leaving the protcol unspecified is that the URL may be interpreted in a different context from where it was created and get a different default protocol from what was intended. /trunk/qpid/cpp/include/qpid/Url.h < https://reviews.apache.org/r/4083/#comment11715 > The name is a bit awkward. What about parse(url, transport=TCP)? /trunk/qpid/cpp/src/qpid/Url.cpp < https://reviews.apache.org/r/4083/#comment11716 > What about: URL(u, s, defaultProtocol=TCP)? Alan On 2012-02-28 13:40:07, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4083/ ----------------------------------------------------------- (Updated 2012-02-28 13:40:07) Review request for Alan Conway, Kenneth Giusti and michael goulish. Summary ------- Previous unification of URL support broke the ability to supply the 'transport' (e.g. tcp, rdma, ssl etc) on a connection. This can still be supplied in the URL itself if the 0-10 URL format is used but the regression should be fixed. The change in this patch adds a method to the Url class to parse a string without populating any resulting addresses with TCP as the default protocol if none is specified. This allows the protocol in the Address to indicate what was explicitly provided and lets the decision on defaults be taken elsewhere (in this case in the underlying client API). This addresses bug QPID-3832 . https://issues.apache.org/jira/browse/QPID-3832 Diffs ----- /trunk/qpid/cpp/include/qpid/Url.h 1294525 /trunk/qpid/cpp/src/qpid/Url.cpp 1294525 /trunk/qpid/cpp/src/qpid/client/Connection.cpp 1294525 /trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp 1294525 /trunk/qpid/cpp/src/tests/ssl_test 1294525 /trunk/qpid/cpp/src/tests/ssl_test 1294525 Diff: https://reviews.apache.org/r/4083/diff Testing ------- New case added to sssl tests that uses a mixture of url and connection-option. Make check passes. Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4083/
        -----------------------------------------------------------

        Review request for Alan Conway, Kenneth Giusti and michael goulish.

        Summary
        -------

        Previous unification of URL support broke the ability to supply the 'transport' (e.g. tcp, rdma, ssl etc) on a connection. This can still be supplied in the URL itself if the 0-10 URL format is used but the regression should be fixed.

        The change in this patch adds a method to the Url class to parse a string without populating any resulting addresses with TCP as the default protocol if none is specified. This allows the protocol in the Address to indicate what was explicitly provided and lets the decision on defaults be taken elsewhere (in this case in the underlying client API).

        This addresses bug QPID-3832.
        https://issues.apache.org/jira/browse/QPID-3832

        Diffs


        /trunk/qpid/cpp/include/qpid/Url.h 1294525
        /trunk/qpid/cpp/src/qpid/Url.cpp 1294525
        /trunk/qpid/cpp/src/qpid/client/Connection.cpp 1294525
        /trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp 1294525
        /trunk/qpid/cpp/src/tests/ssl_test 1294525
        /trunk/qpid/cpp/src/tests/ssl_test 1294525

        Diff: https://reviews.apache.org/r/4083/diff

        Testing
        -------

        New case added to sssl tests that uses a mixture of url and connection-option. Make check passes.

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4083/ ----------------------------------------------------------- Review request for Alan Conway, Kenneth Giusti and michael goulish. Summary ------- Previous unification of URL support broke the ability to supply the 'transport' (e.g. tcp, rdma, ssl etc) on a connection. This can still be supplied in the URL itself if the 0-10 URL format is used but the regression should be fixed. The change in this patch adds a method to the Url class to parse a string without populating any resulting addresses with TCP as the default protocol if none is specified. This allows the protocol in the Address to indicate what was explicitly provided and lets the decision on defaults be taken elsewhere (in this case in the underlying client API). This addresses bug QPID-3832 . https://issues.apache.org/jira/browse/QPID-3832 Diffs /trunk/qpid/cpp/include/qpid/Url.h 1294525 /trunk/qpid/cpp/src/qpid/Url.cpp 1294525 /trunk/qpid/cpp/src/qpid/client/Connection.cpp 1294525 /trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp 1294525 /trunk/qpid/cpp/src/tests/ssl_test 1294525 /trunk/qpid/cpp/src/tests/ssl_test 1294525 Diff: https://reviews.apache.org/r/4083/diff Testing ------- New case added to sssl tests that uses a mixture of url and connection-option. Make check passes. Thanks, Gordon
        Hide
        Gordon Sim added a comment -

        Sorry, that was my fault. Great analysis though! I'll try and get this fixed for the next release.

        Show
        Gordon Sim added a comment - Sorry, that was my fault. Great analysis though! I'll try and get this fixed for the next release.

          People

          • Assignee:
            Gordon Sim
            Reporter:
            Paul Colby
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development