Emit UserWarning when accessing Application items by str

[cdce8p]

What do these changes do?

In addition to emitting a UserWarning when setting a str key for Application items, also emit a warning when accessing it. Either via get or __getitem__. This will help users to update their code more easily.

I haven't added a CHANGES entry yet. Since the feature hasn't been shipped yet, it should still be covered with the original message. Please let me know if I should add one regardless. https://github.com/aio-libs/aiohttp/blob/cf97e5b0a20de31aeb51684add61b7dad4e92375/CHANGES/5864.feature#L1

/CC @Dreamsorcerer

Are there changes in behavior for the user?

The UserWarning will be emitted in more cases. Not just here https://github.com/aio-libs/aiohttp/blob/cf97e5b0a20de31aeb51684add61b7dad4e92375/aiohttp/web_app.py#L169-L178

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.35%. Comparing base (cf97e5b) to head (4f7215d). Report is 1155 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #7553   +/-   ##
=======================================
  Coverage   97.35%   97.35%           
=======================================
  Files         106      106           
  Lines       31481    31490    +9     
  Branches     3575     3579    +4     
=======================================
+ Hits        30648    30657    +9     
  Misses        630      630           
  Partials      203      203           

Flag	Coverage Δ
CI-GHA	97.30% <100.00%> (+<0.01%)	:arrow_up:
OS-Linux	96.97% <100.00%> (+<0.01%)	:arrow_up:
OS-Windows	95.45% <100.00%> (+<0.01%)	:arrow_up:
OS-macOS	96.65% <100.00%> (+<0.01%)	:arrow_up:
Py-3.10.11	95.37% <100.00%> (+<0.01%)	:arrow_up:
Py-3.10.12	96.86% <100.00%> (+<0.01%)	:arrow_up:
Py-3.11.4	96.56% <100.00%> (+<0.01%)	:arrow_up:
Py-3.8.10	95.34% <100.00%> (+<0.01%)	:arrow_up:
Py-3.8.17	96.79% <100.00%> (+<0.01%)	:arrow_up:
Py-3.9.13	95.34% <100.00%> (+<0.01%)	:arrow_up:
Py-3.9.17	96.82% <100.00%> (+<0.01%)	:arrow_up:
Py-pypy7.3.11	96.37% <100.00%> (+<0.01%)	:arrow_up:
VM-macos	96.65% <100.00%> (+<0.01%)	:arrow_up:
VM-ubuntu	96.97% <100.00%> (+<0.01%)	:arrow_up:
VM-windows	95.45% <100.00%> (+<0.01%)	:arrow_up:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[Dreamsorcerer]

I think this makes most sense in the context of #7555, so let's discuss that one first. In it's current form, you would only be able to access a string key after setting a string key, which means a warning would already have been emitted. I'd prefer not to spam the user constantly with warnings (note that we are not currently planning to deprecate string keys, I just figured that most users who enable warnings are probably the same users who want type safety).

[cdce8p]

I think this makes most sense in the context of #7555, so let's discuss that one first. In it's current form, you would only be able to access a string key after setting a string key, which means a warning would already have been emitted.

Not sure this really depends on #7555. When I first changed the keys to AppKey, I missed updating the accessors as well. Just because the warnings where only emitted for __setitem__. Even with find / replace it's possible to still miss something. Emitting a warning for each case feels more consistent and will make updating easier IMO.

I'd prefer not to spam the user constantly with warnings (note that we are not currently planning to deprecate string keys, I just figured that most users who enable warnings are probably the same users who want type safety).

That's valid, although if they don't want to change it, they might already ignore it and that would hide the new locations, too. It's easy enough to do. As mentioned earlier, I believe these new warning locations would help the users who want to update.

[Dreamsorcerer]

Not sure this really depends on #7555. When I first changed the keys to AppKey, I missed updating the accessors as well. Just because the warnings where only emitted for __setitem__.

In that case, you'd normally have got a KeyError, so the warning would be irrelevant.

However, now I've understood the Home Assistant case, what do you think about adding these warnings, but having aiohttp set the ignore filter by default, then home assistant can override that and reenable the warnings to ensure that extension developers get alerted that they are using the deprecated keys (assuming that's possible)?

[Dreamsorcerer]

My theory being that in aiohttp/web/init.py we can do: warnings.filterwarnings("ignore", ...)

Then in home assistant, anytime after importing aiohttp.web, you can just do the same thing, changing it to "default" or "always". Just before removing string keys, you could even change it to "error", with instructions for developers so they can change it back if they aren't ready to update everything immediately (but, ensuring that they are fully aware that it will be gone in the following release).

[cdce8p]

My theory being that in aiohttp/web/init.py we can do: warnings.filterwarnings("ignore", ...)

How is this going to interact with pytest? Personally I'm not a fan of unconditionally modifying the global warnings filter.

Maybe it would be better to add a custom warnings category for it to make it easier to ignore those? I believe sqlalchemy did something like this for their recent 2.0 migration.

[Dreamsorcerer]

How is this going to interact with pytest?

Well, the default test should test the default behaviour, i.e. no warnings (which should already be covered by us erroring on warnings).

You can then test the warning behaviour using with warnings.catch_warnings(...) (or, I would expect pytest.warns(...) will work the same way, in which case nothing extra is needed).

Personally I'm not a fan of unconditionally modifying the global warnings filter.

I'm not sure, I figure we're just filtering out our own warning by default which is easily overriden. I've just noticed we can also use append=True which means that even if someone enabled the warning before importing aiohttp, we won't override it.

Maybe it would be better to add a custom warnings category for it to make it easier to ignore those?

Well, yes, we probably want to do something like that, if it's not trivial to filter on the current one alone (I don't know if module is enough maybe? Probably still a little too general, I guess...).

[cdce8p]

I'll set the PR to draft for now. Even though setting warnings.filterwarnings("ignore", ...) globally would work, there are some issues with pytest that need to be fixed first. Mainly that if aiohttp is already imported during test collection, the warnings.filterwarnings call is just ignored for the actual test run.

I don't think this is really necessary for 3.9.0, so it should be fine for a little while longer.

[Dreamsorcerer]

With the 3.9 testing, I think my proposal appears more difficult now. If we add the ignore (which is what I've needed to do for the existing warning), then it will still appear when running with -Wdefault or -We, which is probably why you encountered the warning in pytest.

I'm not sure there's any way to have a warning that is explicitly opt-in. So, if we want to add this, we would probably need to add some kind of configuration option in aiohttp itself (like web.Application(warn_str_key_on_get=True) or something).

[Dreamsorcerer]

What's the status of this in homeassistant? Do we need to revisit this?

[cdce8p]

What's the status of this in homeassistant? Do we need to revisit this?

So far I had hold of on adding AppKey in Home Assistant. There is a bug in mypy 1.8.0 which could sometimes cause unexpected issues. I've fixed it upstream, but unfortunately the 1.9.0 release is delayed.

If you want, we can close this PR. In case there is still a need later, I can always reopen it or create a new one.

https://github.com/python/mypy/pull/16803

[Dreamsorcerer]

I'll close this, but let me know if homeassistant still needs it.