Ensure sessions are correctly deleted from ETS by passing the pid along

[bobwaycott]

ETS records weren't being correctly removed from SessionStore, leaving stale sessions around in SessionStore.list_sessions_for. By returning the pid from the :ets.select, we can ensure a correct match & delete of the session.

Could be useful to add a defdelegate active_sessions in the main Wallaby interface that calls SessionStore.list_sessions_for, but didn't want to make that assumption in this PR.

Fixes the issue pointed out in #550

[codecov-commenter]

Codecov Report

Merging #558 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #558   +/-   ##
=======================================
  Coverage   92.73%   92.73%           
=======================================
  Files          26       26           
  Lines         853      853           
=======================================
  Hits          791      791           
  Misses         62       62           

Flag	Coverage Δ
#IntegrationTest	87.69% <100.00%> (ø)
#UnitTest	43.96% <100.00%> (+4.57%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
lib/wallaby/session_store.ex	95.45% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 834bdb7...23e7e77. Read the comment docs.

[mhanberg]

Thanks for the PR! Do you mind adding some tests?

[bobwaycott]

There are currently no tests for SessionStore in the project. What are your thoughts on how SessionStore should be tested?

[mhanberg]

It think it's fine to create a new unit test module for the session store, and test that it can store and delete multiple sessions for the same test pid, and remove them both when that test pid goes down.

[mhanberg]

should this be [{ref, _session, pid}]?

[bobwaycott]

No, but I can switch the return value order to match the way the data is stored in ETS. This match is based on the way the result is returned from, not stored in ETS (you can see the :ets.delete call matches the storage order.

EDIT: The ordering is a result of this call:

result =
      :ets.select(:session_store, [
        {{{:"$1", :"$2", :"$3"}, :"$4"}, [{:==, :"$2", session.id}], [{{:"$1", :"$3", :"$4"}}]}
        ])

I can swap the [{{:"$1", :"$3", :"$4"}}]} and make it [{{:"$1", :"$4", :"$3"}}]} to keep the order matched.

However, do we really even need to return the session itself? We could probably get away with just returning the session.id value (or not at all). We already have the session passed in, and only need the session.id to successfully delete.

EDIT 2: I think it'd be worth making that return value just the ref, pid pair, because we can use that + the passed in session.id to cleanly and correctly delete.

[mhanberg]

Oh, I see now that I misread it.

[bobwaycott]

@mhanberg added a couple straightforward tests to verify the ETS table adds & correctly removes sessions. I also streamlined the queries just a tad for unused params

[mhanberg]

thanks!

[bobwaycott]

Happy to help!

[bobwaycott]

@mhanberg sorry to comment again on a closed PR, but wanted to check in on what the release cycle is like for wallaby being updated in Hex? Would love to have this fix available, and curious if I should install from github, or if a new release will be made with this fix?

[mhanberg]

I'm currently adding some more tests. I'll release a patch release once I'm done with that. You can install from github master until then if you'd like.