Refactor experimental SQLiteTokenStorage ("v2") to better accord with its parent class #1004
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sirosen
requested review from
aaschaer,
ada-globus,
derek-globus and
kurtmckee
as code owners
This comment was marked as resolved.
This comment was marked as resolved.
|
The above guess is not borne out by the diff. I'm wondering if this has been a standing issue which is somehow now exposed by the slight changes to the testsuite. |
sirosen
force-pushed
the
experimental-tokenstorage-refactor
branch
from
51a49cd
to
a08dda4
Compare
Support for the 'config_storage' table, and associated methods, has been removed after deliberation about the modeling of tokenstorage adapters and their interchangeability. The issue with config storage is that none of the other adapters provide and support this interface. Therefore, what appears to be generic and pluggable is, in fact, a locked-in decision for certain applications (e.g., `globus-cli`) and will not allow us to provide swappable token storage for those applications. Such extension points can easily be patched into place for migration purposes in the few applications which need them, conditional on the presence/absence of the `config_storage` table. Developing migration workflows is left as a future problem.
Use of a ':memory:' database indicates an opportunity to use the memory tokenstorage. We now reject ':memory:' with an appropriate and detailed usage error. At the same time, various uses of 'dbname' rather than 'filename' have been conformed to 'filename'. This further reinforces that the tokenstorage object is meant to refer to some known file, not a pure memory DB. Tests which relied on the memory database have been updated to use a tempfile. One test which needed significant changes was completely refactored at this time, to provide better and clearer testing.
Rather than this being a specialized characteristic of the JSONTokenStorage, this is now part of the base class functionality. As a result, both the JSON and SQLiteTokenStorage inherit this behavior (with a small change to the SQLiteTokenStorage initializer to call super() at the right time). This allows both of these storage adapters to inherit this beneficial behavior.
sirosen
force-pushed
the
experimental-tokenstorage-refactor
branch
from
8e3c19b
to
81bdd27
Compare
|
I've rebased onto #1007, and dropped the added commit here which explored that solution for the |
kurtmckee
requested changes
Jul 17, 2024
Co-authored-by: Kurt McKee <[email protected]>
kurtmckee
approved these changes
Jul 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These changes are all related, although not necessarily in a perfectly obvious way.
The story runs thusly:
:memory:databases. (Probably solvable, but a red flag about being a "FileTokenStorage" but not having a file.)One notable addition to the above:
2for the database. It's not yet certain, IMO, that this field will be useful, but we could use it to trivially check ifconfig_storageis expected to be present and to manage data migrations.I have already started to think about what a data migration for globus-cli would look like, and it's definitely nontrivial.
We will need to develop some kind of compatibility layer which migrates data and presents a uniform interface to its callers, but doing so in a way which doesn't break the upgrade-downgrade path will require careful thought.
I'm not certain that this will turn out to be worth the pain, but we could, at the very least, provide a distinct (non-TokenStorage) interface for accessing the
config_storagetable found in the sqlite DB. That is: the remodeling of classes done here is robust to the possibility of our deciding against a migration of theconfig_storagetable to some other source of truth.📚 Documentation preview 📚: https://globus-sdk-python--1004.org.readthedocs.build/en/1004/