Refactor to scope the lifecycle of SQLite storage #1060
Conversation
The storage adapter is here rewritten entirely away from a "pure functional style" in which attributes and values live in global scope into a more object-oriented style. The reason for this refactor is that we want to better control the lifecycle of the storage and ensure that it is closed cleanly when the program exits; furthermore, we want to simulate that same open/close behavior in the testsuite. The overall tack taken here is - wrap SQLiteAdapter in a new container type, CLITokenstorage - give CLITokenstorage as many passthrough methods as are necessary to make this transition easy as a rewrite for various usage sites - take steps to rewrite usages which do not have a handle on a LoginManager, and use the LoginManager as a way of getting access to the token storage - start work on testsutie fixes This approach has proven more successful than alternatives which were attempted. At time of writing, `mypy` is passing on the result, some interactive testing seems to work, and tests are in need of numerous adjustments, but not so severe as some other failed rewrites: 50 failed, 977 passed, 13 errors Therefore, there should be at most 63 remediation steps, after which this should be a functioning option for us.
This resolves test failures due to the scope contract version data being absent.
The LoginManager now self-registers itself with the click context to call `call_on_close` when instantiated via the command decorator. In order to handle the effects of this, a few changes are needed: - CLITokenstorage.close no longer tries to clear `internal_native_client` -- with the attribute patched in many cases, this causes failures and does not provide a clear benefit - Uses of the LoginManager.requires_login decorator in tests now leverage the test_click_context in order to ensure that the context lookup will not fail - Reorder the closing semantics for `test_token_storage` used by the testsuite -- `close()` is mocked to do nothing, and then the true closing method is called when the fixture is finalized. As a result, we can inspect the storage after a command has run, but without changing the real runtime logic which cleans up this resource.
sirosen
requested review from
ada-globus,
derek-globus,
kurtmckee,
m1yag1,
MaxTueckeGlobus and
aaschaer
as code owners
|
I've finished getting the refactor in working order. Most notably, in the final change here I finally achieved one of the goals here, which is to have I don't have new tests which ensure that we use the new behavior yet. I'm still thinking a little about whether or not to add new tests and what they should contain. Given that all of the existing tests are passing, I think we can call this ready for review while I think about that. |
| return globus_sdk.ConfidentialAppAuthClient( | ||
| client_id, client_secret, app_name="Globus CLI" | ||
| ) |
There was a problem hiding this comment.
Is it worth thinking about caching this value?
I realize we want to do the is_client_login check, but could still throw it in an instance variable and return it if it's already been instantiated.
There was a problem hiding this comment.
Yeah, simply wrapping this in cached_property poses no issues, as far as I can tell (one mock needed a tweak, that was all). I'm including it.
There was a problem hiding this comment.
Do we need to evict the cache if someone calls delete_templated_client?
There was a problem hiding this comment.
I don't think it's necessary (although it would be more conceptually clean). It can only happen during logout when a user uses the (hidden) --delete-client flag, and the client is never used after that in that process.
Would you like me to add that? I believe it's just a matter of doing a del self.cli_confidential_client at the end of the delete.
There was a problem hiding this comment.
If we're going to cache it, then yeah we should evict on delete.
It's true that in the current usage mode, a cache eviction will also terminate the process (command ends after logging out). But that's not a part of this object's contract and could easily result in a future bug if we forget that subtlety.
There was a problem hiding this comment.
I added the del, nothing broke in existing tests (I didn't expect it would, but hey! good to confirm!), and I've "locked it in" with a couple of new tests.
Overall, this refactor is fairly minor. No major changes in functionality. Some naming and "level of abstraction" issues have been smoothed out, and it's overall less code for the same effect. - Rename `tokenstore.py` to `storage.py` - Rename `CLITokenstorage` to `CLIStorage` - Remove "passthrough" methods for the underlying storage adapter -- just expose and use it directly - Make CLITokenstorage.adapter non-nullable - Rename `internal_auth_client` and `internal_native_client` for clarity - Use a cached property for `cli_confidential_client` (was `internal_auth_client`) - Use eager imports where perf impact is negligible
The cached ConfidentialAppAuthClient gets cleared now as part of deletion. New unit tests confirm the before-and-after state of affairs.
The storage adapter is here rewritten entirely away from a "pure functional style" in which attributes and values live in global scopeinto a more object-oriented style. The reason for this refactor is that we want to better control the lifecycle of the storage and ensure that it is closed cleanly when the program exits; furthermore, we want to simulate that same open/close behavior in the testsuite.
The overall tack taken here is
This approach has proven more successful than alternatives which were attempted. At time of writing,
mypyis passing on the result, some interactive testing seems to work, and tests are in need of numerous adjustments, but not so severe as some other failed rewrites:50 failed, 977 passed, 13 errors
Therefore, there should be at most 63 remediation steps, after which this should be a functioning option for us.
Opened as a draft PR to share my work as I progress, and collect early feedback.