Skip to content

The stub for AbstractBaseSession() get_decoded() is Dict[str, int] but should include str #1289

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

Closed
danielmoessner opened this issue Dec 8, 2022 · 4 comments
Labels
bug Something isn't working good first issue Good for newcomers stubs Issues in stubs files (.pyi)

Comments

@danielmoessner
Copy link

Bug report

I found a wrong stub.

What's wrong

The file base_session.pyi contains the stubs for AbstractBaseSession. The stub for the method get_decoded is wrong.
It is Dict[str, int] but obviously the decoded session can also contain strings.

How is that should be

Steps to reproduce:

  1. Have any django project running with session login.
  2. Login so that there is at least one session in the database.
  3. Open the python shell with python manage.py shell

Run the following code from within the shell:

from django.contrib.sessions.models import Session
s = Session.objects.first()
s.get_decoded()

# Example response:
# {'_auth_user_id': '1', '_auth_user_backend': 'django.contrib.auth.backends.ModelBackend', '_auth_user_hash': '1c92e5298f2609bd14615aef4f2bd07a13a7e741b8acc475623ae3f1386d4879'}

As you can see event the auth_user_id is a string. Therefore I think the stub should be Dict[str, str] but at the moment I'm not sure if it can include numbers, so Dict[str, Union[str, int]] might also be correct.

System information

django-stubs==1.13.0

@danielmoessner danielmoessner added the bug Something isn't working label Dec 8, 2022
@intgr
Copy link
Collaborator

intgr commented Dec 9, 2022

It looks like it's currently dict[str, Any]

def get_decoded(self) -> dict[str, Any]: ...

Ideally this would be a TypedDict, if the dict keys are always the same. PRs welcome.

@intgr intgr added good first issue Good for newcomers stubs Issues in stubs files (.pyi) labels Dec 9, 2022
@danielmoessner
Copy link
Author

That seems fine, probably the most correct. I didn't notice that it changed recently. I'm not sure if TypedDict is the correct way to go, as the session can be extended with any key you want. In my opinion this issue can be closed.

@ngnpope
Copy link
Contributor

ngnpope commented Jan 9, 2023

I'm not sure this makes sense to change from dict[str, Any], especially as TypedDict doesn't always play nicely. People may not use django.contrib.auth or the session may be empty, so keys added to support session variables added there would need to be NotRequired, or we can use total=False for the whole TypedDict:

from typing import TypedDict

class _DecodedSession(TypedDict, total=False):
    _auth_user_id: str
    _auth_user_backend: str
    _auth_user_hash: str
    _csrftoken: str
    _password_reset_token: str

Unfortunately, the session can also contain virtually anything. It's not limited to what django.contrib.auth adds. Until TypedDict supports extra values that are not defined we cannot change from dict[str, Any].

See python/mypy#4617

@danielmoessner
Copy link
Author

I would close this issue and come back to it once TypedDict supports extra values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers stubs Issues in stubs files (.pyi)
Development

No branches or pull requests

3 participants