TokenStorage (v2) #980
TokenStorage (v2) #980
Conversation
aaschaer
requested review from
ada-globus,
derek-globus,
kurtmckee and
sirosen
as code owners
| """ | ||
|
|
||
| @abc.abstractmethod | ||
| def remove_tokens_for_resource_server(self, resource_server: str) -> bool: |
There was a problem hiding this comment.
I know this name is coming from the current sqliteadapater but I wonder, could we actually just change this? It's way more verbose than the other method names.
- "store" doesn't mention tokens at all
- "get" mentions tokens but not that it's for a resource server
- "remove" mentions tokens and that it's for a resource server
I guess store isn't bounded to a particular resource server and get/remove are so maybe remove_token_data(resource_server: str) to mirror get_token_data(resource_server: str)?
There was a problem hiding this comment.
I would like us to reevaluate all of the names in play, for the record. That goes for the class name as well, per one of your other comments.
There was a problem hiding this comment.
I went with
store_response
store_token_data_by_resource_server
get_token_data_by_resource_server
get_token_data
remove_token_data
Open to suggestions for changes, but it should at least be more uniform now
| "format_version": self.format_version, | ||
| "globus-sdk.version": __version__, |
There was a problem hiding this comment.
Why the difference in delimiter here? _ vs -
There was a problem hiding this comment.
I think this is the only comment that didn't get addressed in my last commit/rebase.
Essentially I just wanted to keep the value used by SimpleJSONStorageAdapter so there was one less thing for users to worry about when migrating.
As for why it was like this originally 🤷
aaschaer
force-pushed
the
storage_adapter_v2
branch
from
bf172dc
to
3e6546f
Compare
aaschaer
force-pushed
the
storage_adapter_v2
branch
from
c00cd1e
to
63dd12b
Compare
src/globus_sdk/experimental/globus_app/_validating_token_storage.py
Outdated
Show resolved
Hide resolved
Using rubrics as dividers and by coalescing related changes, this alteration aims to make the changelog more digestible. A small number of clarifying edits were made: - Some very long (perhaps technically grammatical, but hard to parse) sentences were split into several shorter sentences. Sub-bullets were used for some items. - `default_scope_requirements` is not described as an abstract property, but rather as a property whose default implementation raises an error. The distinction is very significant for developers who are implementing subclasses of BaseClient. This change seeks to reduce their potential level of alarm when reading the changelog. - Opportunistically, code-quoting was added to elements where it was missing. - PRs globus#978 and globus#980 were combined into a single item in the changelog. No other obvious opportunities for combinations were found, but perhaps this would be possible with a greater investment of effort.
Shortcut: https://app.shortcut.com/globus/story/33154/sdk-storageadapterv2
Change Summary (all in experimental):
TokenStoragewhich expands uponStorageAdapterby addingnamespacesupport and requires implementing classes to supportremove_tokens_for_resource_server. It also allows passing token data as adictin addition to aOAuthTokenResponseand removeson_refreshas that was only ever an alias forstoreTokenDatawhich is a data class for tokens and metadata, notably includingidentity_idwhich is needed byValidatingTokenStorageFileTokenStoragewhich is nearly much identical toFileAdapterbeyond implementingStorageAdapterV2instead ofStorageAdapterV2MemoryTokenStoragewhich remains fairly simple.JSONTokenStoragewhich now stores data under namespaced keys and has logic for migrating storage from aSimpleJSONFileAdapterSQLiteTokenStoragewhich is very similarSQLiteAdapterbeyond implementingStorageAdapterV2. Both classes should even be able to use the same storage without causing issuesValidatingStorageAdapatertoValidatingTokenStoragewhich now implementsTokenStorageinstead ofStorageAdapterIdentifiedOAuthTokenResponseas its functionality is now covered byTokenDataNotes:
Noneas originally spec'd this was to not require data migration forSQLiteTokenStorageto use storage previously used bySQLiteAdapternamespaceis defined at class initialization rather than passed to each function as originally spec'd. This was to minimize needed changes forSQLiteAdapterand better conform withGlobusAppwhich will also have onenamespaceper instance.MemoryTokenStoragechanged anything since initializing anotherMemoryAdapteralready partitioned the data in memory. Maybe that value should just be ignored?📚 Documentation preview 📚: https://globus-sdk-python--980.org.readthedocs.build/en/980/