Replace dict-like interface with new state dict

[Dreamsorcerer]

What do these changes do?

Much better support for static typing by replacing the dict-like interface on Application with a new state attribute which can be typed as a generic.

Are there changes in behavior for the user?

• Users will need to change app[..] to app.state[...].
• Those using strict typing will also need to add the generic information to their app and handlers:
  app: Application[MyState] = Application()
def handler(request: Request[MyState]):
  etc.

Minor points:

• When trying to assign to the state of a frozen app, it will now be a TypeError rather than RuntimeError.

Additional points

There are some points that should probably be thought about and changed in a future PR around subapps.
For example, Request.config_dict won't have any static typing support currently. Not sure what the best approach is to improve on that.
It would also be possible to provide type-safety to Application.add_routes() etc. to ensure handlers are used on the correct apps. But, this would likely require making UrlDispatcher, AbstractRouteDef and Handler into generics. I'm not sure it's worth the extra work to get this working perfectly.

This can probably be backported to 3.x with deprecation warnings when assigning to the old dict-like interface.

Related issue number

Fixes #5864

Checklist

• I think the code is well written
• Unit tests for the changes exist
• Documentation reflects the changes

[codecov]

Codecov Report

Merging #5893 (a352f80) into master (ae23fd6) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head a352f80 differs from pull request most recent head 009a7fe. Consider uploading reports for the commit 009a7fe to get more accurate results

@@            Coverage Diff             @@
##           master    #5893      +/-   ##
==========================================
- Coverage   96.75%   96.75%   -0.01%     
==========================================
  Files          44       44              
  Lines        9851     9847       -4     
  Branches     1591     1591              
==========================================
- Hits         9531     9527       -4     
  Misses        182      182              
  Partials      138      138              

Flag	Coverage Δ
unit	96.64% <100.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiohttp/abc.py	100.00% <100.00%> (ø)
aiohttp/http_websocket.py	98.66% <100.00%> (+<0.01%)	⬆️
aiohttp/pytest_plugin.py	97.50% <100.00%> (+0.01%)	⬆️
aiohttp/test_utils.py	99.68% <100.00%> (-0.01%)	⬇️
aiohttp/typedefs.py	100.00% <100.00%> (ø)
aiohttp/web.py	99.16% <100.00%> (+<0.01%)	⬆️
aiohttp/web_app.py	97.23% <100.00%> (-0.10%)	⬇️
aiohttp/web_middlewares.py	100.00% <100.00%> (ø)
aiohttp/web_request.py	95.99% <100.00%> (ø)
aiohttp/web_runner.py	97.76% <100.00%> (+0.02%)	⬆️
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae23fd6...009a7fe. Read the comment docs.

[AustinScola]

I agree this is probably the right thing to do. I tried to take a look at ways this could be fixed but couldn't come up with any. I was thinking something along the lines of overloading __init__ but that doesn't seem to be the right approach?

[Dreamsorcerer]

We're literally ignoring the type and assigning an empty dict. I don't think there's any way to achieve this without cheating the typing or doing something worse. I want to avoid asking the user to pass in a pre-made dict (which would be required for technically correct typing), as most of the dict values should typically be created in an app context.

[AustinScola]

If I may ask, what is the thought process behind naming this state?

[Dreamsorcerer]

Mainly that I saw the old (private) variable was named state. It makes as much sense to me as anything else.

If others come up with an agreed name, we can change it.

[AustinScola]

My initial thought is that it might have an adverse affect of causing users to using it for reading and writing when it seems to me that the intended purpose is to act more as global context that is set up in the beginning and then only read from? I also don't know if that is the case though. This is also just a minor nit, I think the name state would probably be fine.

[Dreamsorcerer]

While it's expected to have the top-level variables set on startup (before it freezes), you'll see plenty of examples of mutable objects being initialised for state, so I'm not sure that's an issue. For example, the web_ws.py example stores open web sockets in a list using the app object.

It's semi-read-only once frozen, so users trying to assign to the dict afterwards would get an error anyway, thus making it clear the intended behaviour.

[Dreamsorcerer]

My bigger concern would be the verbosity of request.app.state[...], so I'd also consider a shorter variable name, such as flask's g. Making it atleast a little bit shorter to write: request.app.g[...].

[AustinScola]

I hadn't thought about verbosity. What do you think about continuing to support __getitem__?

[Dreamsorcerer]

I don't see any way to support static typing with it, and:

There should be one-- and preferably only one --obvious way to do it.

[AustinScola]

Fair enough, can't argue with zen.

[Dreamsorcerer]

That should be everything ready now.

[asvetlov]

No sure if I like the proposed design.

I can imagine something like contextvars but getting the value from app/request:

appvar: AppVar[int] = AppVar('var', app)

or 'define type statically':

APP_INT_ARG:AppVarType[int] = AppVarType('var')

appvar: AppVar[int] = APP_INT_ARG(arg)  # type can be inferred

What do you think?

[Dreamsorcerer]

What do you think?

I'm not sure I follow. Wouldn't that basically make all the variables globals? Which seems like something heavily avoided in aiohttp (in contrast to flask where globals are used all the time, which also resulted in my previous company serving Chinese translations to most users due to a library messing it up...).

Taking just the relevant parts of one of the examples, this is the what the usage looks like with the current proposal:

class StateDict(TypedDict):
    db: Database

async def handler(request: web.Request[StateDict]):
    reveal_type(request.app.state["db"])  # Revealed type is Database

app: web.Application[StateDict] = web.Application()
app.state["db"] = Database()  # Assigning another type would give a mypy error.

Maybe if you can convert this example to demonstrate your proposal?

[asvetlov]

No global variables, you are correct.

My translation of your example could look like the following:

class AppVarType[Generic[T]]:
    # Static type definition, not bound to app instance
    # Consider it as a syntax sugar for AppVar class
    def __init__(self, name: str) -> None:
        self._name = name

    def __call__(self, app: Application) -> AppVar:
        # return app var instance that is bound to app
        return AppVar(self._name, app)


class AppVar[Generic[T]]:
    # Bound to application app-var instance
    def __init__(self, name: str, app: Application) -> None:
        self._name = name
        self._app = app

    def get(self) -> T:
        return cast(T, self._app[self._name])

    def set(self, val: T) -> None:
        self._app[self._name] = val


# Usage

DB: AppVarType[Database] = AppVarType("db")

async def handler(request: web.Request):
    db = DB(request.app).get()
    reveal_type(db)  # Revealed type is Database


async def init(app: Application):
    DB(app).set(Database())  # Assigning another type would give a mypy error.

In this approach, I like that app and request are not generic objects, only AppVarType / AppVar has a static type.
This approach supports composition better. For example, aiohttp_jinja2 stores jinja environment in app instance.
Another plugin can use other app key(s).

I have no idea how to mix all these plugins into the same app without typing.cast usage. With AppVar the task is trivial.

Please feel free to ask if you need more explanations / examples, I ma be bad in describing relative complex things.

[Dreamsorcerer]

DB: AppVarType[Database] = AppVarType("db")

async def handler(request: web.Request):
db = DB(request.app).get()
reveal_type(db) # Revealed type is Database

async def init(app: Application):
DB(app).set(Database()) # Assigning another type would give a mypy error.

This feels even more verbose than simply annotating a variable every time:

async def handler(request: web.Request):
    db: Database = request.app["db"]

It's slightly more type safe, but I think it's even less likely to actually be used. I think most users already lose typing from not wanting (or not remembering) to add these annotations every single time.

My goal is really to minimise the amount of work needed by users and make it less likely that they lose type safety. As an example, taking some snippets from my own project, a user might write a handler like this:

async def handler(request: web.Request):
    async with request.app["db_session"]() as sess:
        ...

    f = Fernet(request.app["config"]["KEY_PW"])
    ...

    await request.app["vault_analytics"].update_settings(...)

Most users do not want to add the extra imports and annotations to every single handler. Every handler currently needs boilerplate code like:

from types import MappingProxyType
from sqlalchemy.ext.asyncio import AsyncSession
from .vault import VaultAnalytics

async def handler(request: web.Request):
    db_session: AsyncSession = request.app["db_session"]
    config: MappingProxyType = request.app["config"]
    vault_analytics: VaultAnalytics = request.app["vault_analytics"]
    ...

With this proposal, none of that boilerplate would be needed, just update the request type and then everything is there:

async def handler(request: MyRequest):

This approach supports composition better. For example, aiohttp_jinja2 stores jinja environment in app instance. Another plugin can use other app key(s).

I understand your argument, but I think it's easy enough to deal with. As it's only type checking, we can bypass it in the libraries. It might also be possible to do something with inheritance where the user would define their config something like class MyConfig(aiohttp_jinja2.Config) in order to have full type safety.

I can put together a draft PR for aiohttp_jinja2 later to extend this proposal and see what it looks like.

[Dreamsorcerer]

My goal is really to minimise the amount of work needed by users and make it less likely that they lose type safety.

As an example from a random project using aiohttp and clearly interested in type safety:
https://github.com/home-assistant/core/blob/10bfc783651a49ffbe2e7ad8d3e5d5e3223b3180/homeassistant/components/frontend/__init__.py#L585

They've diligently added type annotations to all the functions and the object they are referencing is fully typed:
https://github.com/home-assistant/core/blob/dc5e4392ae7da417293700e8e39f425a903aeb92/homeassistant/core.py#L210

But, they've lost all type safety on that object as they didn't add an annotation to the assignment. This is the typical use case where I want to ensure type safety is not lost, without overcomplicating things for the user.

[asvetlov]

Wait, Request -> Request[T] and Application -> Application[T] replacement is not backward compatible.
It breaks all existing code that uses mypy --strict mode.
All code that I wrote at my job for the last 3-4 years for example :)

[Dreamsorcerer]

Wait, Request -> Request[T] and Application -> Application[T] replacement is not backward compatible. It breaks all existing code that uses mypy --strict mode. All code that I wrote at my job for the last 3-4 years for example :)

It was mentioned in the PR description. If a user has opted in to strict mode of mypy, it means they want this typing information to be available (it's basically a bug that they have lost the typing information). They'll just need to add the config dict and find/replace the handler signatures. Or possibly just change the import in views.py from from aiohttp import Request to from .config import MyRequest as Request or similar.

I don't think a change to mypy errors counts as backwards incompatible. Otherwise, any new type annotation or change could potentially be backwards-incompatible (updating any typed library, or mypy itself, often results in new errors appearing in mypy).

[webknjaz]

This PR has conflicts now. Needs rebasing.

[Dreamsorcerer]

This PR has conflicts now. Needs rebasing.

Let's agree that the change is desirable and should be merged, before I spend more time on it. This one can safely miss the 3.8 release.

[Dreamsorcerer]

@asvetlov I've tried this out with aiohttp-jinja2, so you can take a look at how the integration works.

The strict typing version is: aio-libs/aiohttp-jinja2#513
I think this works pretty elegantly, a strict typing user just needs to inherit from the library's config in order to ensure type safety. Other users can ignore it and won't need to make any changes.

This also neatly summarises all the keys used by the library: https://github.com/aio-libs/aiohttp-jinja2/pull/513/files#diff-a36dd5434a69167cd4f6bc392daaa6205b9cb70aee122c6b573c08eee5c92b67R21-R26
Which does highlight that we use a static_root_url, which I think I've also seen in aiohttp-devtools, but does not appear to be documented anywhere.

The lazy version using Any, that might be fine for other libraries is: aio-libs/aiohttp-jinja2#515

[asvetlov]

@Dreamsorcerer I have no strong opinion here.

Could you take a look at an alternative approach again :)

pytest has a Stash storage that was designed to replace a type-safe dictionary.

The value type information is stored in a key, different keys can interoperate without clashing.

We can adopt this approach easily without making Application and Request classes generic.

The backward compatibility is preserved by keeping both overloaded versions, e.g.

  @overload
  def __getitem__(self, key: StashKey[T]) -> T:
        ...
  @overload
  def __getitem__(self, key: str) -> Any:
        ...
  def __getitem__(self, key: Any) -> Any:
        return self._state[key]

We can even raise a DeprecationWarning if the key is a string.

The only thing that I would like to improve in pytest's implementation is improving ShashKey.__repr__ to provide a key's name for introspection.

So far pytest's solution is the preferable approach in my eyes.

I very appreciate your opinion about the proposal. Thank you for your patience!

[Dreamsorcerer]

pytest has a Stash storage that was designed to replace a type-safe dictionary.

OK, that looks like a decent approach. I have one question regarding the implementation though, should we:

1. Disallow str keys in future (i.e. aiohttp 4), which risks annoying users who don't care about type safety.
2. Continue to allow str keys, which risks users who do care missing the feature entirely or accidentally using str keys somewhere by mistake.
3. Allow str keys with a warning. I'm guessing that most users who don't care about type safety also won't have warnings enabled, and most users who do care will likely have warnings enabled.

[asvetlov]

I like the third option: raise a warning and keep this behavior for a very long time

[Dreamsorcerer]

Also, to include a key's name in __repr__, does that mean we would need to take the name as a parameter, the same way as TypeVar (e.g. my_var = AppKey("my_var", str))? I'm not aware of a way to inspect a variable name.

[Dreamsorcerer]

Superseeded by #6498.