fix: set the dup flag for QoS 2

[tomekt-bolt]

This PR adds setting the dup flag for re-deliveries of QoS 2 packets.

It requires complementary changes in the aedes-persistence project and possibly some adjustments in concrete persistence layer implementations.

[tomekt-bolt]

The failing tests are the ones I've added, which require the mentioned change in aedes-persistence, I can open another PR for that change, but don't know how to link it to be used for tests in this one.

[robertsLando]

@tomekt-bolt Could you also attach links to mqtt specs on the peaces of code using dup? For example: http://docs.oasis-open.org/mqtt/mqtt/v3.1.1/os/mqtt-v3.1.1-os.html#_Toc385349262 ? Capther [MQTT-3.3.1-1]

[robertsLando]

I can open another PR for that change, but don't know how to link it to be used for tests in this one.

Firstly we need to fix persistence, make a new release and then update it here

Also this requires updates on mongodb and redis persistence (at least):

https://github.com/moscajs/aedes-persistence-mongodb/blob/67ca807e38ef647340ab3bc63b6a1050185cf97f/persistence.js#L507

https://github.com/moscajs/aedes-persistence-redis/blob/master/persistence.js#L412

[gnought]

I'm against the PR. We should add a dup flag in aedes packet.

[robertsLando]

aedes packet already has a dup flag: https://github.com/moscajs/aedes-packet/blob/master/packet.js#L11

[tomekt-bolt]

Also this requires updates on mongodb and redis persistence (at least)

aedes-persistence-redis works out of the box, as it updates the stored value with a freshly encoded packet: https://github.com/moscajs/aedes-persistence-redis/blob/9cc9864e512059ebb3add14ce7463544ad12b7fe/persistence.js#L335.

As for mongodb, I'd require some help (provided we're happy with the general direction of the change), as I have no competences in this area.

[tomekt-bolt]

aedes-persistence-redis works out of the box, as it updates the stored value with a freshly encoded packet: https://github.com/moscajs/aedes-persistence-redis/blob/9cc9864e512059ebb3add14ce7463544ad12b7fe/persistence.js#L335.

OK, now I see that it probably doesn't work out of the box, as the packet updated in the line I've pointed is shared among all the interested clients, so we're setting the dup flag for all of them here. This means, that the change required for aedes-prersistence-redis is not that trivial, as we need to store the dup flag on per-client basis.

Is this correct?

[mnowotka]

Hey @robertsLando! Will you move forward with this PR?

In the aedes readme it says:

Full compatible with MQTT 3.1 and 3.1.1

But without setting the dup flag I'm afraid this is not entirely true for the case of QoS == 2. If this change is not acceptable, can you please explain how can we contribute to fix this issue?

[mnowotka]

And I forgot to say: Many thanks for writing aedes, this is a great piece of software, thanks for making it open!

[robertsLando]

In order to move on wih this we firstly need to fix persistences. I would also know why @gnought is against this and how he think would be a better implementation of it, and also @mcollina thoughts about this as he is the main author of Aedes and his understanding in mqtt and Aedes is much better then mine

[tomekt-bolt]

Thanks @robertsLando for your comment. I will wait for some more feedback on the general direction of this change before investing in aligning the persistence layer implementations then.

[robertsLando]

@tomekt-bolt Could you link opened PR on persisistences please?

[tomekt-bolt]

I don't have any open PRs on persistences related to this issue. As I've mentioned above, I'd rather not invest in aligning the persistence without having a green light for the general approach of this PR.

I understand that the persistence changes would need to be applied prior to this PR, but what I want to avoid is spending the time on persistence PRs and later finding out that there's some inherent problem with what I've proposed here.

[robertsLando]

Agree, let's see what @gnought and @mcollina think about this so

[tomekt-bolt]

Hi @robertsLando , @gnought, @mcollina,

thanks for your feedback so far. Could you please let me know where do we stand with this subject at the moment?

Do you intend to merge this PR, provided the required changes would be added to the persistence implementations? If so, can I count on some assistance in updating the persistence parts?

Or is there some inherent problem with my approach?

[mcollina]

a test is failing

[tomekt-bolt]

a test is failing

The failing tests are the ones I've added, which require the mentioned change in aedes-persistence, I can open another PR for that change, but don't know how to link it to be used for tests in this one. https://github.com/moscajs/aedes/pull/603#issuecomment-815806689

[robertsLando]

I can open another PR for that change, but don't know how to link it to be used for tests in this one.

The procedure is to firstly fix and merge the changes needed, update the package here and then merge this one