Fix queue declaration arguments

[TomVS]

Hi,

Thanks for your work. I encountered a bug when trying to declare a queue with x-max-priority = 4 set in optional arguments. The argument set didn't have any effect. I fixed the issue in the code (variable name clash).

Best regards, Thomas

[mbroadst]

@TomVS this arguments => args seems really fishy to me, can you explain what happened again? You're saying there was an arguments clash, but a compiler definitely would have picked that up.

Also, can you provide an example where this failed? It would be best to include a test to make sure we don't experience this again.

[TomVS]

Hi,

I realized I pushed another commit to my master for the MessageId property. I can remove that as it is not relevant to the bug fix I want pull into your master. (commit 2270f0b) While I'm on that topic, what is your reasoning for setting the MessageId to 0? Does it have to be set at all before publishing a message?

The issue with that arguments variable was that you were writing to the QByteArray arguments through the QDataStream out, then writing the QByteArray arguments local variable into the out stream instead of the QAmqpTable arguments from the QAmqpChannelPrivate class. The local variable was being used instead of the class variable. There are two easy ways of fixing this that I thought of:

• rename the local variable (what I did), or
• use this->arguments to make sure we are writing the actual arguments into the frame.

A simple example:

...
    m_client->createQueue("queue-key");
    QAmqpTable args;
    args.insert("x-max-priority", 4);
    tmpQueue->declare(QAmqpQueue::Durable, args);
...

The queue declared and created with this code does not have any x-max-priority property. With my bug fix, the priority queue is declared correctly.

Let me know what you think. I shall look into writing a test for this.

[mbroadst]

wow thats totally bizarre that the compiler allowed the variable to be named the same thing, never heard of such a thing. okay sounds good, thanks for the fix! I think it should be incredibly easy to copy one of the existing tests to prove that this declared properly if you don't mind taking the time.

Also, the messageId was a holdover from the original implementation by fuCtor, so no particular reason that its hard coded to 0. We can keep that in this PR, that's fine