-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add globus_sdk.IdentityMap for Auth ID lookups #367
Merged
Merged
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
The IdentityMap is mostly a port of the LazyIdentityMap found within the Globus CLI. It has several notable differences, however: - it accepts usernames or identity IDs and internally differentiates them - it is lazier than the LazyIdentityMap because it only looks up one batch at a time - it has an external interface more clearly defined for consumers to interact with it - it will "prioritize" a given key by forcing it into a record batch for lookup - it returns full record dicts, not just usernames In addition to the IdentityMap itself, this includes tests which exercise almost all of the IdentityMap behaviors. The IdentityMap is primarily a wrapper over an internal `_cache` dictionary which maps both usernames and IDs, undifferentiated, to records. You can `del` items from the cache to force fresh lookups if you believe data has gone stale, and `IdentityMap.get` has the same signature as `dict.get`. It's important to note that IdentityMap does not implement the full `Mapping` protocol (defined in modern pythons by the collections.abc module). The issue for us in this case is that certain operations like `__contains__` are very unclear. For example, if the map has a key added to it but it hasn't looked it up yet, does it "contain" that key? If you were checking to see if a key had already been added the answer is "yes". But if you wanted to know if `idmap[key]` will KeyError, the answer is "maybe/unknown". The only way to answer "contains" if you want to know that is to actually do a lookup, and having `in` checks side-effecting in python seems bad. Therefore, `in` is intentionally left undefined. We can add new methods, similar to `has_key(key, resolved/unresolved=True/False)` in the future if users come to us with a desire for this information and can clarify their use-cases. For the most part, `idmap.get(key) is not None` should suffice if anyone ever cares to check. IdentityMap is included in docs on the same page as the AuthClient object types, much like the TransferData and DeleteData helpers for the Transfer client.
jaswilli
reviewed
Jan 24, 2020
- Switch from regex parsing of username strings to checking if values can be handled by uuid.UUID() - Minor doc fixes All tests still pass with these changes.
httpretty route registration does not record query parameters. Rather than plugging these in at all, omit them. This makes it more obvious when you read the tests that things don't need to match up (and, in fact, if you want to check the params used, you need to do so explicitly).
I've done some cleanup per review, and also because I realized that I had used httpretty in a slightly confusing way. I also went ahead and implemented the "pass a custom cache" variant, but kept it in a separate branch ( sirosen@bbf7983 ). We can pull it in, or just discard. (Or, if you strongly disagree and want a default shared cache, we can discuss further, of course.) |
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.
The IdentityMap is mostly a port of the LazyIdentityMap found within the Globus CLI. It has several notable differences, however:
In addition to the IdentityMap itself, this includes tests which exercise almost all of the IdentityMap behaviors.
The IdentityMap is primarily a wrapper over an internal
_cache
dictionary which maps both usernames and IDs, undifferentiated, to records. You candel
items from the cache to force fresh lookups if you believe data has gone stale, andIdentityMap.get
has the same signature asdict.get
.It's important to note that IdentityMap does not implement the full
Mapping
protocol (defined in modern pythons by the collections.abc module). The issue for us in this case is that certain operations like__contains__
are very unclear. For example, if the map has a key added to it but it hasn't looked it up yet, does it "contain" that key? If you were checking to see if a key had already been added theanswer is "yes". But if you wanted to know if
idmap[key]
will KeyError, the answer is "maybe/unknown". The only way to answer "contains" if you want to know that is to actually do a lookup, and havingin
checks side-effecting in python seems bad.Therefore,
in
is intentionally left undefined. We can add new methods, similar tohas_key(key, resolved/unresolved=True/False)
in the future if users come to us with a desire for this information and can clarify their use-cases.For the most part,
idmap.get(key) is not None
should suffice if anyone ever cares to check.I worked through tests mostly with the goal of complete line coverage, but there are some untested behaviors I can target if it seems important.
Absent a more obvious place to put this in docs, I've included it on the Auth clients page (similar to the
TransferData
andDeleteData
objects).Resolves #366