Qpid
  1. Qpid
  2. QPID-3273

JMSDeliveryMode has to be used as a string in a selector

    Details

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

      Description

      Section 3.8.13 of the JMS spec says that JMSDeliveryMode has to be used as a string in a selector (i.e. JMSDeliveryMode = 'PERSISTENT').
      But Qpid only supports it as an integer (i.e JMSDeliveryMode = 2).

      1. 0001-QPID-3273.patch
        5 kB
        Rajith Attapattu

        Activity

        Hide
        Robbie Gemmell added a comment -

        Hi Rajith, I have a few comments on the patch:

        The broker does not currently depend on the JMS specification jar, so it cant use the javax.jms.DeliveryMode constants; there are a matching set of constants in the BasicContentHeaderProperties class, if not others.

        The [pre-existing] log statements will now continue to output the int value for the DeliveryMode whilst the return value is updated to be a String, I think it should probably log the String now, or both.

        There should be a constant (one may or may not exist in the code base) for the PERSISTENT and NON_PERSISTENT strings instead of sprinkling more literals around the code base for them.

        The patch introduces a number of tabs instead of using spaces.

        Show
        Robbie Gemmell added a comment - Hi Rajith, I have a few comments on the patch: The broker does not currently depend on the JMS specification jar, so it cant use the javax.jms.DeliveryMode constants; there are a matching set of constants in the BasicContentHeaderProperties class, if not others. The [pre-existing] log statements will now continue to output the int value for the DeliveryMode whilst the return value is updated to be a String, I think it should probably log the String now, or both. There should be a constant (one may or may not exist in the code base) for the PERSISTENT and NON_PERSISTENT strings instead of sprinkling more literals around the code base for them. The patch introduces a number of tabs instead of using spaces.
        Hide
        Rajith Attapattu added a comment -

        I already noticed that the broker does not depend on the JMS spec jar and fixed that - but thanks for pointing it out.
        It seems the anyedit plugin is somewhat doing the exact opposite with the tabs

        I will fix the log statements to probably include both.
        As for using constants for PERSISTENT and NON_PERSISTENT, I will check to see if we have any existing constants.
        If it doesn't exist, I am wondering where to include the constants. Do you know of an existing class that contains constants?
        I am not too keen to add a new class just for this.

        Show
        Rajith Attapattu added a comment - I already noticed that the broker does not depend on the JMS spec jar and fixed that - but thanks for pointing it out. It seems the anyedit plugin is somewhat doing the exact opposite with the tabs I will fix the log statements to probably include both. As for using constants for PERSISTENT and NON_PERSISTENT, I will check to see if we have any existing constants. If it doesn't exist, I am wondering where to include the constants. Do you know of an existing class that contains constants? I am not too keen to add a new class just for this.
        Hide
        Rajith Attapattu added a comment -

        I could include the constants in the PropertyExpression.java itself. I guess for the time being thats the best option.

        Show
        Rajith Attapattu added a comment - I could include the constants in the PropertyExpression.java itself. I guess for the time being thats the best option.
        Hide
        Rajith Attapattu added a comment -

        Used enums instead of string literals.
        The log statements now print the textual representation instead of an int.

        Show
        Rajith Attapattu added a comment - Used enums instead of string literals. The log statements now print the textual representation instead of an int.
        Hide
        Robbie Gemmell added a comment -

        I think the enum should be static since it makes no use of the containing class, and probably private too (if it warrants being public then in this case it should be in its own file rather than being duplicated).

        Also, it occurs to me that there cant be an existing test for this otherwise the patch would be breaking it, so I think adding a unit test to cover the change is in order.

        Show
        Robbie Gemmell added a comment - I think the enum should be static since it makes no use of the containing class, and probably private too (if it warrants being public then in this case it should be in its own file rather than being duplicated). Also, it occurs to me that there cant be an existing test for this otherwise the patch would be breaking it, so I think adding a unit test to cover the change is in order.
        Hide
        Rajith Attapattu added a comment -

        I will make it private. It would have been nice if there was a way to not duplicate it in both classes, but I think such a change was out of scope. Perhaps this is something we should keep in mind when we redesign things.

        As for the tests, I will try to add it to an existing test case.
        I don't think adding a new test case for each commit is a very useful thing (this is not a criticism of your statement, but rather an observation of how we deal with this as a project). We currently have a very bloated test suite that is becoming unmanageable. However that's topic we need to discuss on a separate forum

        Show
        Rajith Attapattu added a comment - I will make it private. It would have been nice if there was a way to not duplicate it in both classes, but I think such a change was out of scope. Perhaps this is something we should keep in mind when we redesign things. As for the tests, I will try to add it to an existing test case. I don't think adding a new test case for each commit is a very useful thing (this is not a criticism of your statement, but rather an observation of how we deal with this as a project). We currently have a very bloated test suite that is becoming unmanageable. However that's topic we need to discuss on a separate forum
        Hide
        Robbie Gemmell added a comment -

        The way not to duplicate the enum would just be to declare it in its own file in the common tree, not exactly a large change and certainly in scope considering you are adding it just now, but I dont feel strongly about that given what its used for.

        I do fairly strongly disagree with much of your comment about testing though. Many of our existing systests certainly do suck, but we absolutely should be ensuring we improve unit test coverage as we go and the clear way to do that is by adding specifically targeted unit tests of functionality whenever we know we are changing untested code. We certainly wont improve our testing situation by perpetuating it.

        Show
        Robbie Gemmell added a comment - The way not to duplicate the enum would just be to declare it in its own file in the common tree, not exactly a large change and certainly in scope considering you are adding it just now, but I dont feel strongly about that given what its used for. I do fairly strongly disagree with much of your comment about testing though. Many of our existing systests certainly do suck, but we absolutely should be ensuring we improve unit test coverage as we go and the clear way to do that is by adding specifically targeted unit tests of functionality whenever we know we are changing untested code. We certainly wont improve our testing situation by perpetuating it.
        Hide
        Rajith Attapattu added a comment -

        Fixed along with a test case.

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

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development