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

Proof of concept: reject invalid inherited methods #783

Closed

Conversation

sirosen
Copy link
Member

@sirosen sirosen commented Jul 14, 2023

AuthClient is gaining a number of methods related to authenticated usages which require token auth. These methods will not be functional for NativeAppAuthClient and ConfidentialAppAuthClient.

Because it is difficult to make a backwards-compatible change which alters the inheritance relationship for AuthClient and the other client types, it may be worth exploring other methods for ensuring that these "never functional" methods provide clear user-feedback that they do not work.
Possibly -- arguably -- the inheritance relationship that produces isinstance(NativeAppAuthClient("foo"), AuthClient) could be considered outside of our public interfaces. That would leave us free to make all three client types inherit from a new base (BaseAuthClient?) and attach the new methods only to AuthClient. However, if we consider this part of the public interface, then an alternative solution is needed.

This changeset introduces a class decorator
(globus_sdk.utils.replace_notimplemented_methods) which replaces named inherited methods with stubs which raise NotImplementedError with a generated message. The result is a slightly awkward trace, but a clear error on bad usage:

>>> nc.get_identities(ids="foo")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File ".../foo/bar/globus_sdk/utils.py", line 34, in injected_method
    raise NotImplementedError(
NotImplementedError: NativeAppAuthClient does not support get_identities

If the methodology is considered sound, we can decorate NativeAppAuthClient and ConfidentialAppAuthClient with known-unsupported method names for SDK v3.x and plan to change the inheritance structure in v4.0 . A full enumeration of supported and unsupported usages would be needed to make this feasible, along with a detailed changelog enumerating the now NotImplementedError-raising methods.


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

AuthClient is gaining a number of methods related to authenticated
usages which require token auth. These methods will not be functional
for NativeAppAuthClient and ConfidentialAppAuthClient.

Because it is difficult to make a backwards-compatible change which
alters the inheritance relationship for AuthClient and the other
client types, it may be worth exploring other methods for ensuring
that these "never functional" methods provide clear user-feedback that
they do not work.
Possibly -- arguably -- the inheritance relationship that produces
`isinstance(NativeAppAuthClient("foo"), AuthClient)` could be
considered outside of our public interfaces. That would leave us free
to make all three client types inherit from a new base
(`BaseAuthClient`?) and attach the new methods only to `AuthClient`.
However, if we consider this part of the public interface, then an
alternative solution is needed.

This changeset introduces a class decorator
(`globus_sdk.utils.replace_notimplemented_methods`) which replaces
named inherited methods with stubs which raise `NotImplementedError`
with a generated message. The result is a slightly awkward trace, but
a clear error on bad usage:

    >>> nc.get_identities(ids="foo")
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File ".../foo/bar/globus_sdk/utils.py", line 34, in injected_method
        raise NotImplementedError(
    NotImplementedError: NativeAppAuthClient does not support get_identities

If the methodology is considered sound, we can decorate
NativeAppAuthClient and ConfidentialAppAuthClient with
known-unsupported method names for SDK v3.x and plan to change the
inheritance structure in v4.0 . A full enumeration of supported and
unsupported usages would be needed to make this feasible, along with a
detailed changelog enumerating the now NotImplementedError-raising
methods.
@sirosen
Copy link
Member Author

sirosen commented Aug 31, 2023

Although this utility might have independent life in the future (not sure I believe that?), this is part of the series of discussions and PRs which led to #849. I don't see benefit in keeping it open.

@sirosen sirosen closed this Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant