Skip to content
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 3 commits into from
Jan 27, 2020
Merged

Conversation

sirosen
Copy link
Member

@sirosen sirosen commented Jan 3, 2020

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 (maybe the CLI implementation should have been called EagerIdentityMap)
  • it has an external interface more clearly defined for consumers to interact with it (because that's the point of doing this work, basically)
  • 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.

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 and DeleteData objects).

Resolves #366

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.
@sirosen sirosen added the enhancement New feature or improvement label Jan 3, 2020
@sirosen sirosen requested a review from jaswilli January 3, 2020 21:35
globus_sdk/auth/identity_map.py Outdated Show resolved Hide resolved
globus_sdk/auth/identity_map.py Outdated Show resolved Hide resolved
globus_sdk/auth/identity_map.py Outdated Show resolved Hide resolved
globus_sdk/auth/identity_map.py Show resolved Hide resolved
- 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).
@sirosen
Copy link
Member Author

sirosen commented Jan 24, 2020

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.)

@jaswilli jaswilli merged commit 7f10ab9 into master Jan 27, 2020
@jaswilli jaswilli deleted the add-identity-map branch January 27, 2020 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Add LazyIdentityMap, based on globus-cli's LazyIdentityMap
2 participants