[Discussion] post 4.0.0 refactor #597
Comments
|
First pass sounds reasonable to me. I assume the work you're doing on GAE SDK regarding the I also would like people to consider the topic of the Storage classes, and how |
We've mostly snuffed that out.
Absolutely. The first phase would be import google.oauth2.storage.redis
credentials = do_something_to_obtain_credentials()
storage = google.oauth2.storage.redis.Storage(...)
stored_credentials = storage.put(credentials)
# credentials = <google.oauth2.credentials.Credentials>
# stored_credentials = <google.oauth2.storage.StoredCredentials>
# `stored_credentials` will update the store on refresh, `credentials` will not. |
|
On naming: |
|
Interesting, yeah, I think it's time for us to align with the rest of the libraries. the repository name could be |
|
On code: no implementation inheritance in the public API. Not just no "twisted class hierarchy"; none whatsoever in the API. And probably none in the implementation; it tends to evaporate when the constraint of a legacy API is removed. |
|
Outside of those subtopics I think it sounds like "forward", which is the right direction for the library. 👍 |
I'm with you on this, and you'll be the primary reviewer so you can make sure this sits right with you. The only inheritance I envisioned was possibly an ABC for Credentials.
Woohoo. I think for the actual work of doing we should follow the example set by gcloud-python and write the usage guide first in rst then write the implementation. We do need to discuss the logistics of creating the new package (should it live in this repo on the master branch side-by-side with oauth2client? Should we shove it in a sub-folder? Should we start another branch?) |
|
General direction sounds reasonable to me. Do you still intend to depend on httplib2? |
|
@dhermes is addressing that before 4.0.0 On Tue, Aug 9, 2016, 3:00 PM thobrla [email protected] wrote:
|
|
Initially looks reasonable to me also. +1 to the plan of writing the usage guide first. I'll have more comments as things start shaping up. |
Does that imply that v. >4.0 will be more specifically a Google-focused library for authenticating against Google resources, and less of a general-purpose OAuth2 client? |
|
@jonparrott: Agreed - I said no implementation inheritance. Interfaces, defined with the |
@bjmc, I think it may mean the opposite. Like @jonparrott said, "The For the transition, maybe we could do something similar to google-api-python-client, in moving the code into the new package format, but having |
@bjmc honestly this library has never done a good job of being a general-purpose oauth2 client. It just hasn't. I don't think this transition will hurt that, but this transition alone isn't intended to fix that.
That's probably untenable. google-api-python-client was just a rename. This is far more significant. |
I don't think it's that bad. It has a lot of Google stuff hardcoded into it as defaults, but it's reasonably easy to override, and the We've been recommending it to our users for authenticating against our Authorization Server, and I'm trying to get a sense of how much commitment there is to maintaining it as a general-purpose client, or whether we should be exploring alternatives for the future. |
I don't either, but it would be good for us to stop advertising as such. :D
There's a lot of great general-purpose oauth2 clients out there, and some of them work with Google's servers as well. In general, oauth2 is a mess. |
|
@jonparrott I like splitting into |
|
@elibixby I don't think we need to go that far. ADC doesn't make a distinction, and I think just drawing the line at server-to-server vs user-to-server is enough. |
|
@dhermes any thoughts? |
|
None in particular |
|
Hmm.. that's someone concerning. Considering gcloud-python is a huge user of this library - how would this change impact you? Would it have a positive effect? Negative? |
|
We're mostly worried about server-to-server and using ADC without users having to worry about anything but an environment variable ( |
|
I figured as much, so it is logical for us to separate the server-to-server stuff in |
|
For interested parties: I put together a proof-of-concept for this refactor over at https://github.com/jonparrott/goth. Check it out if you're curious. Some highlights:
I believe this proves this is worthwhile and will land us in a much better state long-term. The interfaces are working quite well in practice, but they aren't entirely "pure" (they do provide some attributes and methods to subclasses, but I could be easily convinced to make them pure - I just don't want to repeat code). Would love your thoughts on that @nathanielmanistaatgoogle. If course, I still want to get #128 done and 3.0.0 released before I start trying to merge this in; I just had some free time to play around with this idea. I think the rough idea is that I will merge in one module at a time in a separate, orphan branch once 3.0.0 is out. |
|
Thank you for the hierarchy graphics; they definitely make clear how dramatic is the clean-up. I'm still negative on the mixture of concrete and abstract elements in the the It looks like the only abstract elements of Part of my enthusiasm for separating the concrete elements from the abstract elements is that it would more clearly structure the API boundary between that which our library provides and that which developer-users provide to our library - do you see that? Do you see that as valuable? |
Yes, that's fine IMO because the hierarchy is known before-hand (unlike oauth2client) and we're using mixins (ScopedCredentials, SigningCredentials) to indicate optional parts. oauth2client got in trouble because almost everything was shoved into the the base
I don't think it's useful to decompose this further. I could be convinced otherwise, but I'd have to see an example of what it would look like to be convinced.
I'm not sure I completely understand what you mean here. |
|
There are at least three ways in an API to do what providing a public partially-abstract partially-concrete class does. Of course the first is to specify a partially-abstract partially-concrete class: @six.add_metaclass(abc.ABCMeta)
class MixedSuperclass(object):
@abc.abstractmethod
def implemented_by_application_method(self):
raise NotImplementedError
def concrete_method(self):
<some statements that make use of implemented_by_application_method>(let’s call this @six.add_metaclass(abc.ABCMeta)
class ImplementedByApplication(object):
@abc.abstractmethod
def some_behavior(self):
raise NotImplementedError()
class ConcreteSuperclass(object):
def __init__(self, implemented_by_application):
"""Constructor.
Args:
implemented_by_application: An ImplementedByApplication.
"""
self._implemented_by_application = implemented_by_application
def concrete_method(self):
<some statements that make use of self._implemented_by_application>(let’s call this @six.add_metaclass(abc.ABCMeta)
class ImplementedByApplication(object):
@abc.abstractmethod
def some_behavior(self):
raise NotImplementedError()
@six.add_metaclass(abc.ABCMeta)
class ImplementedByLibrary(object):
@abc.abstractmethod
def some_other_behavior(self):
raise NotImplementedError()
class _ImplementedByLibrary(ImplementedByLibrary):
def __init__(self, implemented_by_application):
self._implemented_by_application = implemented_by_application
def some_other_behavior(self):
<some statements that make use of self._implemented_by_application>
def construct_implemented_by_library_from_implemented_by_application(
implemented_by_application):
return _ImplementedByLibrary(implemented_by_application)(let’s call this Concrete classes, be they partially or completely concrete, do at least three things: (1) they define a type, (2) they define a (partial or complete) implementation of that type, and (3) they define at least one means of constructing that implementation. I have nothing against these three things; it is expected that a library will have to do all three (perhaps several times over) on its way to affording value to its developer-users. It is the coupling of all three into a single code element that is often inadequately considered, that is without bearing on the utility afforded by the library, and that leads to maintenance problems, and the flawed assumption that leads to these problems is "our object will only ever make sense in terms of the one application-supplied object of which we are currently aware". Let’s look at how each implementation strategy handles, two years down the road, the revelation that there’s a slightly different input with which we want to allow applications to construct our object. @six.add_metaclass(abc.ABCMeta)
class MixedSuperclass(object):
@abc.abstractmethod
def implemented_by_application_method(self):
raise NotImplementedError
# Can't have @abc.abstractmethod on this without breaking existing code
def implemented_by_application_prime_method(self):
"""Does the a_prime behavior (or not).
Raises:
NotImplementedError: if this MixedSuperclass instance is implemented
in terms of a_method rather than a_prime_method.
"""
raise NotImplementedError()
def concrete_method(self):
try:
<some statements that make use of implemented_by_application_prime_method>
except NotImplementedError:
<some statements that make use of implemented_by_application_method>. @six.add_metaclass(abc.ABCMeta)
class ImplementedByApplication(object):
@abc.abstractmethod
def some_behavior(self):
raise NotImplementedError()
@six.add_metaclass(abc.ABCMeta)
class ImplementedByApplicationPrime(object):
@abc.abstractmethod
def some_behavior_prime(self):
raise NotImplementedError()
class ConcreteSuperclass(object):
def __init__(self, implemented_by_application, implemented_by_application_prime=None):
"""Constructor.
Args:
implemented_by_application: An ImplementedByApplication. Ignored
if a_prime is not None.
implemented_by_application_prime: An optional
ImplementedByApplicationPrime.
"""
self._implemented_by_application = implemented_by_application
self._implemented_by_application_prime = implemented_by_application_prime
def concrete_method(self):
if self._implemented_by_application_prime is None:
<some statements that make use of self._implemented_by_application>
else:
<some statements that make use of self._implemented_by_application_prime>. @six.add_metaclass(abc.ABCMeta)
class ImplementedByApplication(object):
@abc.abstractmethod
def some_behavior(self):
raise NotImplementedError()
@six.add_metaclass(abc.ABCMeta)
class ImplementedByApplicationPrime(object):
@abc.abstractmethod
def some_behavior_prime(self):
raise NotImplementedError()
@six.add_metaclass(abc.ABCMeta)
class ImplementedByLibrary(object):
@abc.abstractmethod
def some_other_behavior(self):
raise NotImplementedError()
class _ImplementedByLibrary(ImplementedByLibrary):
def __init__(self, implemented_by_application):
self._implemented_by_application = implemented_by_application
def some_other_behavior(self):
<some statements that make use of self._implemented_by_application>
class _ImplementedByLibraryPrime(ImplementedByLibrary):
def __init__(self, implemented_by_application_prime):
self._implemented_by_application_prime = implemented_by_application_prime
def some_other_behavior(self):
<some statements that make use of self._implemented_by_application_prime>
def construct_implemented_by_library_from_implemented_by_application(
implemented_by_application):
return _ImplementedByLibrary(implemented_by_application)
def construct_implemented_by_library_from_implemented_by_application_prime(
implemented_by_application_prime):
return _ImplementedByLibraryPrime(implemented_by_application_prime). From a developer-user perspective, it’s a problem that Moving from the theoretical, contrived, and short to the real and sizeable (since you asked for an example): take a look at So that’s consideration that I find most fearsome about exposing concrete classes in public APIs. There are also a few other considerations that to varying degrees push me in the same direction, and I’m not aware of anything other than "what most programmers are used to" and "no one likes writing forwarding methods" pushing back. I really like that I like and find useful the separation that comes from defining an interface for just the part that the developer-user of the library must implement ( Mixins are generally intended to help, but the help that they offer can’t be accepted without becoming a new kind of thing. For the classes you’ve drafted as mixins, how certain are you that the only legitimate way for application code to make use of the behavior that you’ve implemented is to implement the type you’ve defined? Why couple the type and the behavior rather than offer them a la carte? I hope this is persuasive - while I feel very strongly, I recognize that I’m not the one authoring the new code. And I don’t just feel strongly in abstract theoretical principle; I think that the general practice of "unintentionally (and often without awareness) making implementation promises in the API and restricting future choices in the library" is one of the morals of the |
I found this more overwhelming than persuasive. I know that wasn't your intent, but I presently have no idea what I would change over at googleapis/google-auth-library-python#8 to satisfy your concerns. Do you have any concrete recommendations for those 3 interfaces that you can show with code? Showing what the real interfaces would look like along with one or more concrete credentials could really help clarify what you want here. Feel free to send a rough PR to add the same module but in the way that you want- I can take it from there. In particular, I found these sections difficult to apply to the situation at hand:
There's no part of the credentials that I expect to be implemented by the consuming application - implementations are all first party but the interfaces are for the benefit of the users.
No one here is proposing we repeat what happened with ServiceAccountCredentials. I've shared the hierarchy and prototype code for the new implementation and all things considered, it's pretty clean.
Everything in
Your argument against mixins is that they force the implementing class to become an instance of the mixin. That is exactly what I want here. Application default credentials can return one of 5 classes, so it's important to be able to ascertain what can be done without without relying on checking for concrete classes: Similarly: These are use cases lifted directly from |
|
@jonparrott: Will try to craft a response over in the relevant code review thread. |
|
SG On Tue, Oct 11, 2016 at 1:00 PM Nathaniel Manista [email protected]
|
|
Thank you for creating this issue, however, this project is deprecatedand we will only be addressing critical security issues. You can read moreabout this deprecation here. If you need support or help using this library, we recommend that you ask yourquestion on StackOverflow. If you still think this issue is relevant and should be addressed, pleasecomment and let us know! |
With 3.0.0 out and 4.0.0 on the way, we've finally addressed most of the "cruft" in this library without making huge, sweeping, breaking changes. However, some huge obstacles remain - such as the twisted class hierarchy and the strange naming of some of the credentials classes.
I'd like to start a discussion on refactoring this library post-4.0.0. I'm not one to throw out well-tested and proven code, but the code that is here needs to be better organized to suit our downstream clients and our users.
I want to put forth the idea of this package slowly migrating to two new packages:
google.authandgoogle.oauth2. During the initial phases we'll retain code inside of and continue to publish theoauth2clientpackage, but once complete theoauth2clientpackage will be permanently deprecated.The
google.authpackage will focus solely on server-to-server authentication. A rough idea of what this package will look like is:google.auth.default()returns the application default credentials.google.auth.service_account.Credentialscredentials using a service account key to obtain an access token.google.auth.jwt.Credentialscredentials using a service account key to directly assert credentials.google.auth.gae.Credentialscredentials using App Engine identity credentials.google.auth.gce.Credentialscredentials using the Compute Engine metadata server.google.auth.access_token.Credentialscredential using a bare access token.This package won't contain anything related to storage or end-user based credentials.
The
google.oauth2package will contain the remainder of the current library and focus solely on oauth2 user-specific stuff, better organized:google.oauth2.flow- generic flow that can be easily used with web frameworks.google.oauth2.credentials- user credentials with access and refresh tokens.google.oauth2.storage- a wrapper that allows credentials to be stored.etc.
What are you initial thoughts @nathanielmanistaatgoogle @dhermes @waprin @thobrla @anthmgoogle @elibixby @pferate & any others.
The text was updated successfully, but these errors were encountered: