GH-82 TransportContext: merge and key iterator

[szysas]

[sbernard31]

It's take me few minutes to get how "merge with override" works. :thinking:
Actually, when you merge if there is 2 value with same key both are kept in the chained list but only the first one will be returned by the get(), correct ?

That's surprising me but if it works why not. I guess one drawback is that merged context takes probably a bit more memory than necessary.

I didn't test it but reading the code I guess that :

TransportContext ctx1 = TransportContext.of(DUMMY_KEY, "111").with(DUMMY_KEY2, "222");
TransportContext ctx2 = TransportContext.of(DUMMY_KEY, "aaa");
TransportContext ctx1Ctx2 = ctx1.with(ctx2);

TransportContext ctx3 = TransportContext.of(DUMMY_KEY, "aaa").with(DUMMY_KEY2, "222");   
// OR TransportContext ctx3 = TransportContext.of(DUMMY_KEY2, "222").with(DUMMY_KEY, "aaa");  
// not sure about the merge order : this makes me think 
// that maybe a better name would be addFirst() or addLast() (instead of with())
// Just because order seems relevant for equals()

assertEquals(ctx1Ctx2, ctx3); 
// From user point of view, I guess this should be equal, but is it ? 
// reading the code I'm not sure. 

and also if I iterate over ctx1Ctx2 and print all key, I will get DUMMY_KEY twice correct ?

(I maybe totally missed something, sorry I didn't take time to execute code)

[szysas]

and also if I iterate over ctx1Ctx2 and print all key, I will get DUMMY_KEY twice correct ?

Yes, that is correct.

[szysas]

and also if I iterate over ctx1Ctx2 and print all key, I will get DUMMY_KEY twice correct ?

@sbernard31 Now that it is fixed by returning Set with keys.

[sbernard31]

I looked at this. The keys set resolve the "twice" error.

But not the "equals" one ? See code comment in code block of https://github.com/open-coap/java-coap/pull/85#issuecomment-2072391807.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 93.15%. Comparing base (ef8b0e2) to head (0d78208).

Additional details and impacted files

@@             Coverage Diff              @@
##             master      #85      +/-   ##
============================================
+ Coverage     93.11%   93.15%   +0.03%     
- Complexity     1992     1998       +6     
============================================
  Files           132      132              
  Lines          4505     4513       +8     
  Branches        616      616              
============================================
+ Hits           4195     4204       +9     
  Misses          168      168              
+ Partials        142      141       -1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[szysas]

I looked at this. The keys set resolve the "twice" error. I'm not sure what do you mean.

But not the "equals" one ? See code comment in code block of #85 (comment).

Well, that is true, equals will not be true.

[sbernard31]

(let me know if I should review this again ?)

[szysas]

(let me know if I should review this again ?)

Yes, please ;)

[sbernard31]

I'm still a bit confused by the idea that a not used key/value pair should still be stored in memory :sweat_smile: (https://github.com/open-coap/java-coap/pull/85#issuecomment-2072391807)

Actually, when you merge if there is 2 value with same key both are kept in the chained list but only the first one will be returned by the get(), correct ? That's surprising me but if it works why not. I guess one drawback is that merged context takes probably a bit more memory than necessary.

But alternative will probably lead to more costly(and complex) way to merge 2 TransportContext, so maybe this is the good way. :shrug:

So this looks good to me.

[szysas]

Purpose for TransportContext is to have short living, simple and small object. When we will see that this is not enough then alternative would be to use some HashMap underneath.