ObserveComposite for timestamped data support on server side

[JaroslawLegierski]

ObserveComposite for timestamped data suport on server side - described in issue #1089.

[sbernard31]

Just to be sure, is it still a draft or should I review it ?

(Rereading myself, I realize that this could be taken the wrong way 😬 , this wasn't aggressive it was just to now if I should take time to review it now OR if this is too soon 😅 )

[JaroslawLegierski]

Just to be sure, is it still a draft or should I review it ?

(Rereading myself, I realize that this could be taken the wrong way 😬 , this wasn't aggressive it was just to now if I should take time to review it now OR if this is too soon 😅 )

No Problem 😉 I created it initially as a PoC, however I think it's a good idea to turn it into PR and explain some not clear topics in dedicated issues such as #1554 or during review.

[sbernard31]

@JaroslawLegierski I added some commit about tests.

Second test fails and following #1089 (comment), I think it should pass. Do you agree ?

[JaroslawLegierski]

@JaroslawLegierski I added some commit about tests.

Second test fails and following #1089 (comment), I think it should pass. Do you agree ?

As I understand the timestampednodes.getNodes() method should be changed because currently:

"In case of the same path conflict the most recent one is taken. Null timestamp is considered as most recent one."
.... so we should filter out null values ?

Passing assertThat(response.getTimestampedLwM2mNodes()).isEqualTo(timestampednodes); requires encoder modification:

because I don't see value=null in COAP packets , that's right ?

[sbernard31]

Not really.

As explain at #1089 (comment)
if you observe /1/0/1 and /3/0/15 then you should get both resources for each timestamp and no more than that.
I tried to put the null case as example but maybe that was not a so good idea.
Here is more examples, maybe this will be clearer :

// valid  
t1 => { 
    "/1/0/1"     => LwM2mResource
    "/3/0/15"     => LwM2mResource
},
// valid
t2 => { 
    "/1/0/1"     => LwM2mResource
    "/3/0/15"     => null  // case where resource has no value, a single notif answer maybe with NOT_FOUND
},
// invalid  : 3/0/15 is missing
t3 => { 
    "/1/0/1"     => LwM2mResource  
},
// invalid : 3/0/1 should not be here
t4 => { 
    "/1/0/1"     => LwM2mResource
    "/3/0/1"     => LwM2mResource
    "/3/0/15"     => LwM2mResource
}

Passing assertThat(response.getTimestampedLwM2mNodes()).isEqualTo(timestampednodes); requires encoder modification:

Nope, the issue is not at encoder level because you can not encode null value in SenML.

The issue is at decoder level, you change the Decoder API to add list of path which is good. But you don't use it.
In my opinion, it should be used to create the right timestampNodes structure.
The list of path must be used ensure that for each timestamp with have a value for each path and only for this path.
(Maybe TimestampedLwM2mNodes.Builder should be adapted to take optionally a list of Path which will be used to be sure the built structure will be correct)

[JaroslawLegierski]

The list of path must be used ensure that for each timestamp with have a value for each path and only for this path.
(Maybe TimestampedLwM2mNodes.Builder should be adapted to take optionally a list of Path which will be used to be sure the built structure will be correct)

Ok - I understand thanks for the clarification. Do you have in mind something like this ?

[sbernard31]

Yep something like this. (but maybe better to add a new constructor with List<LwM2mPath> paths argument instead of a setter like setPaths?

[JaroslawLegierski]

Yep something like this. (but maybe better to add a new constructor with List<LwM2mPath> paths argument instead of a setter like setPaths?

OK - I added new constructor in this commit.

[sbernard31]

(I rebased this on master)

[sbernard31]

I pushed some more modification. I think this is in reviewable stable.
@JaroslawLegierski Let me know if this sounds good to you, then I integrate this in master in squashed in 1 commit.

(Last thing missing, I plan to change ObserveCompositeResponse argument order to be more consistent with other Response but I think this is maybe better to do it after this PR)

[JaroslawLegierski]

I pushed some more modification. I think this is in reviewable stable.
@JaroslawLegierski Let me know if this sounds good to you, then I integrate this in master in squashed in 1 commit.

I tested this today and it looks OK. I think that it can be integrated into the master.

[sbernard31]

Ok I will re-read the code one more time. Re-organize commit then integrate it in master.

[sbernard31]

I found some cosmetic little issue but also some more real issue.

I think it's good to integrate master now.

[sbernard31]

This is now integrated in master (squashed commit b303aa4)