Fix indexing issue in ObservableMap

[gferon]

The client we're building is panicking on startups in some random cases with the following error message Value should be present or has just been inserted, but it's missing. There seems to be a pretty serious bug in observable_map.rs which I initially demonstrated with an extension of the existing unit tests in 6d6f8e4.

Any .remove operation called on a ObservableMap will not re-index values after the removed position, which means any later operation on elements inserted after the previously removed key will either be fetched wrongly, or panic when using .get_or_create.

This PR fixes these two related bugs and adds extra test cases.

[bnjbvr]

Duh, great catch, not sure how we've missed that.

[gferon]

Do you want me to come up with a fix? I'd gladly spend some time doing that.

[bnjbvr]

Absolutely, thanks!

[bnjbvr]

Can we do a swap-remove and only update the position of the swapped value?

[gferon]

@bnjbvr just gave it a shot, the extra test cases now pass.

This is a trade-off. This implementation is simple enough for the moment, and basically does the job

It's obviously not the best possible solution in terms of complexity, but I think it fulfills the criteria defined in the code comment quoted above. 😄

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.00%. Comparing base (0b16d48) to head (200f20d).
Report is 111 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4346      +/-   ##
==========================================
- Coverage   85.02%   85.00%   -0.02%     
==========================================
  Files         274      274              
  Lines       30107    30089      -18     
==========================================
- Hits        25599    25578      -21     
- Misses       4508     4511       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[gferon]

Can we do a swap-remove and only update the position of the swapped value?

Absolutely better indeed, why didn't I think of that?! You will still have to scan the entire mapping map to find the right value, though.

Unfortunately, there's no ObservableVec::swap_remove - I could do it in two separate function calls though.

[gferon]

It looks like I cannot even call self.values.swap on the &mut self ref because of some missing trait bound in eyeball-im (EDIT: found it, it's on purpose "No DerefMut because all mutating must go through inherent methods that notify subscribers"). @bnjbvr I sent you a DM on Matrix, so we can communicate directly.

EDIT: swap_remove doesn't seem to be what we're looking for as it doesn't guarantee the remaining elements order, see https://doc.rust-lang.org/std/vec/struct.Vec.html#method.swap_remove

[bnjbvr]

Thanks, this is a good bandaid solution, and we can implement something using a manual swap-remove later, if needs be.

[Hywan]

Thanks for the fix!

[gferon]

Thanks for reviewing and merging this quickly. I feel like this fix warrants a 0.8.1 release on crates.io since the indexing issue leads to having get_room return to the wrong room after any call of forget_room (or any operation that deleted a room from the store as a side effect).

[bnjbvr]

Our plan is to do a release somewhere around the end of the month (Christmas maybe), which will include this fix.