[sql-11] sessions: interface massage #969
Conversation
ellemouton
added
the
no-changelog
| session.Expiry = time.Unix(int64(expiry), 0).UTC() | ||
| session.CreatedAt = time.Unix(int64(createdAt), 0).UTC() |
There was a problem hiding this comment.
Hmmm can this somehow lead to some discrepancy between current sessions that were created without UTC, which are now deserialised with UTC?
There was a problem hiding this comment.
yeah def - but i think it's ok. worst case the expiry/created_at times now differ by a couple of hours.
I think it's an ok tradeoff and i dont think it is worth the added complexity to make sure we read everything exactly as it was/add a migration etc.
thoughts?
There was a problem hiding this comment.
did some testing just to make sure no issues.
There was a problem hiding this comment.
So I guess the worst case scenario would be that a user after upgrading has a very recently created session that's now deserialized with a timestamp that's a few hours in the future. Alternatively the same could occur for recently revoked sessions, that's seen as having been revoked in the future.
Other than that such scenarios would be pretty confusing (which I think would be fine), there's one edge case where that could lead to a recently created session being automatically revoked:
lightning-terminal/session_rpcserver.go
Lines 442 to 448 in c0e85e0
But given that it's such an edge case where the worst case scenario would jsut be that a session gets revoked, we deemed this to be an ok tradeoff offline.
Make sure to compare the full expected and actual Session struct during tests.
For now, it makes no DB calls. But this is in prepartion for letting this call persist a new session. This will also let us use a shared `clock` for the time fields in a Session.
And be consistent with UTC conversions.
ellemouton
force-pushed
the
sql11Sessions3
branch
from
f75b2a3 to
6239428
Compare
| @@ -77,13 +80,15 @@ const ( | |||
| // BoltStore is a bolt-backed persistent store. | |||
| type BoltStore struct { | |||
| *bbolt.DB | |||
|
|
|||
| clock clock.Clock | |||
There was a problem hiding this comment.
Can you shed a little bit of light on the benefits of injecting a shared clock with the DB? My assumption is it's primarily used to add stability for tests, but not sure if there's more to it than that.
There was a problem hiding this comment.
yeah it is mainly for deterministic tests so that we can guarantee stable (and incrementing) timestamps
This is a tiny refactor PR that does various things to the session DB/Store
in preparation for making the interface ready for a SQL implementation.
Part of #966