Qpid
  1. Qpid
  2. QPID-2930

JMS msg.getPropertyNames() method should not return x-amqp-0-10.routing-key

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.10
    • Component/s: Java Client
    • Labels:
      None

      Description

      Description of problem:
      JMS msg.getPropertyNames() method should not return x-amqp-0-10.routing-key,
      x-amqp-0-10.routing-key is internal property. It cause exception if loop via
      ProertyName enumeration.

      Code:
      ===
      Enumeration<String> enu = msg.getPropertyNames();
      while (enu.hasMoreElements())

      { String name = (String) enu.nextElement(); String value = msg.getStringProperty(name); }

      Exception
      =========
      Caused by: javax.jms.MessageFormatException:
      getString("x-amqp-0-10.routing-key") failed as value of type class [B is an array.
      at
      org.apache.qpid.client.message.AMQMessageDelegate_0_10.getStringProperty(AMQMessageDelegate_0_10.java:639)
      at
      org.apache.qpid.client.message.AbstractJMSMessage.getStringProperty(AbstractJMSMessage.java:254)

        Activity

        Hide
        Rajith Attapattu added a comment -

        should we not apply the same fix for the 0-8 code path ? or should we just leave things as it is and just exclude the test from 0-8 ?

        Show
        Rajith Attapattu added a comment - should we not apply the same fix for the 0-8 code path ? or should we just leave things as it is and just exclude the test from 0-8 ?
        Hide
        Rajith Attapattu added a comment - - edited

        Andrew,

        I had modified the test case for testing/debugging another issue and forgot to reapply the deleted contents before checking it in.
        Thanks for catching this. I have reapplied the necessary changes in rev 1084048 which should test the issue highlighted in the JIRA.

        As for documentation for set/getObjectProperty as an extension point, I haven't really seen any that says we support byte arrays in addition to the supported types listed in the spec.

        I don't know if filtering out x-amqp* and x-qpid* is the right option, especially if those properties are of a type supported by the JMS spec and some application wants to access them. On the other hand one can argue that these props are for internal purposes and not for applications.

        The reason why I omitted "x-amqp-0-10.routing-key" from the enumerated prop name list, is bcos it's value was not a supported JMS type. The user who complained about this issue has rightly pointed out that any property in a message should be accessible as 'getStringProperty'.

        So if we do decide to use set/getObjectProperty as an extension point then we need to ensure we handle it in a sensible manner and in a way that is also spec complaint. That is if getStringProperty is used to retrieve such a property then we need to provide,
        (a) some form of meaningful string representation
        (b) OR a warning message on how to retrieve it properly rather than throwing an exception. Ex "<list> please use getObjectProperty to retrieve this value correctly"
        (c) Or maybe by throwing a MessageFormatException, all though in this case the user wasn't happy about it. - However if we had documented the behaviour then I doubt they would have complained about it.

        So going forward we probably need to pay attention to these areas and make sure we document them properly to avoid users misunderstanding them.

        Show
        Rajith Attapattu added a comment - - edited Andrew, I had modified the test case for testing/debugging another issue and forgot to reapply the deleted contents before checking it in. Thanks for catching this. I have reapplied the necessary changes in rev 1084048 which should test the issue highlighted in the JIRA. As for documentation for set/getObjectProperty as an extension point, I haven't really seen any that says we support byte arrays in addition to the supported types listed in the spec. I don't know if filtering out x-amqp* and x-qpid* is the right option, especially if those properties are of a type supported by the JMS spec and some application wants to access them. On the other hand one can argue that these props are for internal purposes and not for applications. The reason why I omitted "x-amqp-0-10.routing-key" from the enumerated prop name list, is bcos it's value was not a supported JMS type. The user who complained about this issue has rightly pointed out that any property in a message should be accessible as 'getStringProperty'. So if we do decide to use set/getObjectProperty as an extension point then we need to ensure we handle it in a sensible manner and in a way that is also spec complaint. That is if getStringProperty is used to retrieve such a property then we need to provide, (a) some form of meaningful string representation (b) OR a warning message on how to retrieve it properly rather than throwing an exception. Ex "<list> please use getObjectProperty to retrieve this value correctly" (c) Or maybe by throwing a MessageFormatException, all though in this case the user wasn't happy about it. - However if we had documented the behaviour then I doubt they would have complained about it. So going forward we probably need to pay attention to these areas and make sure we document them properly to avoid users misunderstanding them.
        Hide
        Andrew Kennedy added a comment -

        Re: using set/getObjectProperty as an extension point, is this documented behaviour, somewhere?

        Show
        Andrew Kennedy added a comment - Re: using set/getObjectProperty as an extension point, is this documented behaviour, somewhere?
        Hide
        Andrew Kennedy added a comment -

        While I approve of adding tests, in this case the only assertion is that a sent message (with no properties set) is received, so this issue is never tested. I think the test case should set some properties, of varying allowed and non-allowed types, and then assert that the returned message still provides an enumeration with a list of those, and only those, properties.

        Since x-amqp-0-10.routing-key is internal, maybe it should be filtered out, along with all other x-amqp-* and x-qpid-* properties?

        Show
        Andrew Kennedy added a comment - While I approve of adding tests, in this case the only assertion is that a sent message (with no properties set) is received, so this issue is never tested. I think the test case should set some properties, of varying allowed and non-allowed types, and then assert that the returned message still provides an enumeration with a list of those, and only those, properties. Since x-amqp-0-10.routing-key is internal, maybe it should be filtered out, along with all other x-amqp-* and x-qpid-* properties?
        Hide
        Rajith Attapattu added a comment -

        Fixed along with a test case.

        Show
        Rajith Attapattu added a comment - Fixed along with a test case.
        Hide
        Rajith Attapattu added a comment -

        The Qpid implementation does allow a byte[] to be set using the setObjectProperty method, which is not correct as per the JMS specification.
        I am actually fine with utilizing the set/getObjectProperty method as an extension point to allow maps, lists, UUID, arrays etc to be used.

        But we need to ensure that we have a sane way of handling these extensions while ensure we are also spec complaint.
        Maybe if getStringProperty is called we could print something like, "<list> please use getObjectProperty to retrieve this value correctly".
        For things like UUID we could provide the toString representation of it.

        Thoughts ?

        Show
        Rajith Attapattu added a comment - The Qpid implementation does allow a byte[] to be set using the setObjectProperty method, which is not correct as per the JMS specification. I am actually fine with utilizing the set/getObjectProperty method as an extension point to allow maps, lists, UUID, arrays etc to be used. But we need to ensure that we have a sane way of handling these extensions while ensure we are also spec complaint. Maybe if getStringProperty is called we could print something like, "<list> please use getObjectProperty to retrieve this value correctly". For things like UUID we could provide the toString representation of it. Thoughts ?
        Hide
        Rajith Attapattu added a comment -

        The JMS specification allows any message property to be retrieved as a string.
        However the the valid types are boolean, byte, short, int, long, float, double, and String.
        Therefore in JMS, the property value cannot be a byte[] as it's not a valid type.

        However in AMQP you are allowed to have byte arrays,lists, maps, UUID's etc.. as property values.
        So a broker or a non JMS client could set such a property in a message that will be consumed by a JMS consumer.

        In such a case there are two possible ways of handling it.
        1) Ignore the properties (from the list provided in response to getPropertyNames() method) that does not have a value allowed by the JMS spec.
        2) Provide the standard string representation of the value.

        Option #2 is fairly useless if the value is of type array, list or map.

        I think option #1 is probably the best as a JMS client will not expect to receive any property with a value that is not valid as per the spec. Therefore it does not need to even know the name of a property that it cannot access.

        Show
        Rajith Attapattu added a comment - The JMS specification allows any message property to be retrieved as a string. However the the valid types are boolean, byte, short, int, long, float, double, and String. Therefore in JMS, the property value cannot be a byte[] as it's not a valid type. However in AMQP you are allowed to have byte arrays,lists, maps, UUID's etc.. as property values. So a broker or a non JMS client could set such a property in a message that will be consumed by a JMS consumer. In such a case there are two possible ways of handling it. 1) Ignore the properties (from the list provided in response to getPropertyNames() method) that does not have a value allowed by the JMS spec. 2) Provide the standard string representation of the value. Option #2 is fairly useless if the value is of type array, list or map. I think option #1 is probably the best as a JMS client will not expect to receive any property with a value that is not valid as per the spec. Therefore it does not need to even know the name of a property that it cannot access.
        Hide
        Andrew Kennedy added a comment -

        But, why are you trying to get this property value as a String in the first place?

        This is a byte array, so I think the exception is correct in this circumstance. If I set any byte array property then the code you describe will fail in the same way. Additionally, I don't think we want to go down the road of special-casing property names to be 'hidden' from the returned enumeration of property names. I would think thst if a property is set, on the message, and can be retreived its name should be returned.

        Finally, would it be possible, instead, to fix this by setting the value as a String in the first place? I assume this is the C++ broker, where OutgoingMessage.cpp and IncomingMessage.cpp seem to define this as const std::string X_ROUTING_KEY("x-amqp-0-10.routing-key");

        Show
        Andrew Kennedy added a comment - But, why are you trying to get this property value as a String in the first place? This is a byte array, so I think the exception is correct in this circumstance. If I set any byte array property then the code you describe will fail in the same way. Additionally, I don't think we want to go down the road of special-casing property names to be 'hidden' from the returned enumeration of property names. I would think thst if a property is set, on the message, and can be retreived its name should be returned. Finally, would it be possible, instead, to fix this by setting the value as a String in the first place? I assume this is the C++ broker, where OutgoingMessage.cpp and IncomingMessage.cpp seem to define this as const std::string X_ROUTING_KEY("x-amqp-0-10.routing-key");

          People

          • Assignee:
            Rajith Attapattu
            Reporter:
            Rajith Attapattu
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development