oauthlib: typing for gettattr overrides

[jvanasco]

First: I am not sure this is the correct way to handle this situation. If a better technique is recommended, please educate me.

Problem: oauthlib manages many (most) attributes for the oauthlib.common.Request object within a dict named self._params

For reference: https://github.com/oauthlib/oauthlib/blob/master/oauthlib/common.py#L360-L401

This creates issues for typing projects that rely on this, as these attributes don't actually exist on that class.

In this PR: I made a section for these managed attributes, and then set them to Any

[github-actions[bot]]

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

[Daverball]

This is already handled by the __getattr__ method, so you technically don't need to explicitly add those attributes. As long as this magic method has been defined, any attributes that don't exist will be treated as if they existed and use the return type from that method, which would be Any currently. Any | None would probably better though, since it seems like it's initializing all these attributes to None. It would still be beneficial to explicitly declare the attributes for LSPs/auto-complete.

But you should probably change them to read only properties, since there's no __setattr__ so they can't be modified without directly modifying _params.

[Daverball]

Also it seems to me like all of these should actually be str | None, since it's basically just using the result from urllib.parse.parse_qsl to initialize the params (but it first converts any bytes results to str using utf-8)

[jvanasco]

Let's put this on hold. I'm going to open a ticket with the project, as the overall project documentation and code teaches away from these being purely decode http(s) request params in several spots.

[Daverball]

@jvanasco Sure, you can could technically pass in an iterable of (key, value) tuples or a mapping as the body and set encoding to None, then it would bypass the parsing and you could pass in values that would not end up as strings, but that doesn't seem intentional to me, since even the comment says to assume everything is already unicode if encoding has been set to None.

So I'm fairly confident the value will always end up as a string, with the default being None if it wasn't part of the request.

[jvanasco]

@Daverball I've opened an issue against the package here: https://github.com/oauthlib/oauthlib/issues/856

TLDR: in practice, the oauthlib request object will eventually have objects on the .client and .user attributes . I believe at least one of the token types is expected to be an object, and several are expected to be Lists.

I think the project grew over time, and some of the infrastructure evolved or was repurposed. Within the context of that file, your comments are correct - however I think that file is incorrect and several items need to be turned into explicit attributes.

For example:

https://github.com/oauthlib/oauthlib/blob/4a7db54f005686128102d7f7ac5c3d783c244669/oauthlib/oauth2/rfc6749/request_validator.py#L58-L61

    After the client identification succeeds, this method needs to set the
   client on the request, i.e. request.client = client. A client object's
   class must contain the 'client_id' attribute and the 'client_id' must have
   a value.

And again we have https://github.com/oauthlib/oauthlib/blob/4a7db54f005686128102d7f7ac5c3d783c244669/oauthlib/oauth2/rfc6749/request_validator.py#L83-L85 :

    Note, while not strictly necessary it can often be very convenient
   to set request.client to the client object associated with the
   given client_id.

So basically the rest of the library teaches away from the doctstring and obvious interpretation in that file.

This is the sort of problem that typing surfaces.

[Daverball]

Yeah you're right, I've glossed over the part where to_unicode will just pass through the value if it's none of the special cased value types, so it's possible to pass in arbitrary data. So annotating it as Any | None seems fine.

[jvanasco]

These are all read-only (there is no setattr), so they would be better typed with @property, e.g.

That's a great idea. I'll have to audit the code to ensure these all match that pattern and adjust the PR accordingly. This package doesn't have a standardized system or style guide, and typing+tests have been surfacing some oddities.

[github-actions[bot]]

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

[jvanasco]

These are all read-only (there is no __setattr__), so they would be better typed with @property, e.g.

@property
def access_token(self) -> Any: ...

I've gone through the code to find all the potential request attributes and added them.

Instead of typing to Any, I have typed them to the documented or actual usage within an Optional typing. I also updated the existing class attributes to this as well. I would be happy to change these all to Any if that is preferred.

I also used this format: Optional[Union[Dict, List]]

instead of Union[Dict, List, None]

as I could not find a preferred style, and would be happy to change if requested

Notes:

These use Multiple types: claims prompt oauth_params

Not used anyhere: request_token max_age

New: callback expires_in params password realm realms resource_owner_key response_mode response_type signature signature_method timestamp - str, cast to int as needed username using_default_redirect_uri validator_log verifier

[github-actions[bot]]

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

[github-actions[bot]]

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

[jvanasco]

Thank you all for the review and fixing the Union + style changes. I'm copy/pasting between far too many windows and environments trying to handle this.

After testing out the @property method, I wonder if this is the correct way to handle this. My usage of this package is two-fold:

1- I maintain an integration package from the Pyramid framework. 2- I am an end-consumer.

The oauthlib package itself is quite barebones and expects to be subclassed for integration - it basically defines an API that must be met.

After type-checking my code against this version, @property is problematic as it raises dozens of mypy "[misc]" errors when setting all of the required attributes.

I'm not sure what the best way to handle this is, but this approach doesn't seem to improve typing support.

Edit: I have also incorrectly typed the attributes set in init: they should not have Optionals:

    uri: str
    http_method: str
    headers: CaseInsensitiveDict
    body: str
    decoded_body: List
    oauth_params: Union[Dict, List]
    validator_log: Dict
    _params: Dict

[JelleZijlstra]

Sorry for the lack of response here. At the minimum, a few changes are necessary to make CI pass. It might also be helpful to add test cases covering your usage to make sure things work correctly for typical usage.

[jvanasco]

Sorry for the lack of response here. At the minimum, a few changes are necessary to make CI pass. It might also be helpful to add test cases covering your usage to make sure things work correctly for typical usage.

No worries. This can be closed. I previously opened a ticket on oauthlib here https://github.com/oauthlib/oauthlib/issues/856

There are several issues with oauthlib related to typing that are best solved in that project.