Support ScopeCollectionType in GlobusApp

[derek-globus]

Added

• Added support for ScopeCollectionType to GlobusApp's __init__ and
  add_scope_requirements methods.

Changed

• Updated ScopeCollectionType to be defined recursively.

  • Adds support for usage like scopes_to_str(["scope1", Scope.parse("scope2 scope 3")]) -> "scope1 scope2 scope3"

Development

• Added a scope normalization function globus_sdk.scopes.scopes_to_scope_list to
  translate from ScopeCollectionType to a list of Scope objects.

---

📚 Documentation preview 📚: https://globus-sdk-python--1020.org.readthedocs.build/en/1020/

[sirosen]

I think it's important to note that this means that the following is valid:

scopes: ScopeCollectionType = [[["foo"]]]

I'm not quite clear on why this is desirable. A quick glance over the rest of the PR doesn't clarify it for me.
What is the situation in which the recursive structure arises naturally?

[sirosen]

After a fuller read, unless I've missed a way in which this is likely to happen, I would like us not to change this type.

If it does change, there are two things which need to be included in this PR:

• tests specifically for the "recursive iterable containers" cases, at least for the scope helper functions
• review of any preexisting usage sites for this type to ensure they are all robust to the change (you might have already done this, but I want to call it out)

I'm still open to this if there's a case which produces these sorts of nested structures, but I'm not seeing one right now.

[derek-globus]

The case for this is about supporting the same interfaces for our different types. In it's current form, this is valid

scopes_to_str(["foo bar", "baz qux"])  # "foo bar baz qux"

but this is invalid

scopes_to_str([Scopes.parse("foo bar"), Scopes.parse("baz qux")])  # Raises error

---

I did review all usage sites (in this repo) as a part of this change. I'm happy to add those recursively iterable container changes if that's the only blocker.

[derek-globus]

Added some tests for both methods:

• scopes_to_str((Scope("scope1"), Scope("scope2")), "scope3 scope4") # => "scope1 scope2 scope3 scope4"
• scopes_to_str([[["bar"]]] # => "bar"

[sirosen]

Before you add tests, I want to take a little bit more time -- on the order of hours -- to think about this.

---

I'm not sure I find calls to parse() inside of a list to be compelling? Because this works just fine:

scopes_to_str(Scopes.parse("foo bar") + Scopes.parse("baz qux"))

as does

scopes_to_str([*Scopes.parse("foo bar"), *Scopes.parse("baz qux")])

I'm not outright rejecting this reasoning, but I'm trying, in this thread, to work out what I think.

I would not be very sympathetic to this usage, which is very similar:

scopes_to_str(["foo bar".split(), "baz qux".split()])

nor would I be sympathetic to

scopes_to_str(*Scope.parse("foo bar"))

I find the type being declared here to actually be minorly at odds with the documentation used throughout.
e.g.,

A dictionary of scope requirements indexed by resource server. The dict value may be a scope, scope string, or list of scopes or scope strings.

FWIW, I don't think I'd change this doc. Explaining the nesting behavior bloats it for minimal benefit. But it's notable that we're creating a discrepancy. I think a reasonable reader could see the distinction and wonder if we support nested structures or not.

A user "should not" expect that [Scope.parse("foo bar"), "baz"] should be allowed, as a bit of an extreme example. The only way that happens is if a user disobeys the declared contract here. I acknowledge the counterargument that this mistake is easy to make and easy for us to support -- but is it easy to make? The requirement for this is that you're converting scopes to a string and making an explicit intermediate parse call.
I'm still struggling a little to see how a user would trip over this and then struggle to true up their usage to the declared types. Isn't this pretty advanced usage? Should we not expect that the user for this will use the types we've told them to use?

---

To put a different spin on all of this, what about making scopes_to_str([["foo"]]) throw an explicit TypeError by doing a check

if not isinstance(scopes, str) and isinstance(scopes, collections.abc.Iterable):
    raise TypeError("oh, I get it. that's wrong but I get it. Fix like so...")

?

Why choose to expand the interface to include this usage rather than making sure that when we reject it, we do so "nicely"? To me, such a check + TypeError is a nicety which we can easily offer because we don't need to convince ourselves that the usage is valid or reasonable.

[derek-globus]

If I can quick throw a wrench into one of your unsympathetic cases (this one)

scopes_to_str(["foo bar".split(), "baz qux".split()])

What would you say to

scopes_to_scope_list(["fuzz foo[bar baz]", "qux"])

I think this argument relies on the idea that splitting scopes out into individually rooted scopes is trivially simple for a user which this case hopefully demonstrates is not the case.