Deprecate Starlette and Router decorators

[Kludex]

Description

The motivation of this PR is to deprecate part of the code that is discouraged for around 4 years.

As we discussed, this PR only deprecates decorators! The methods that are used by them are not deprecated.

Deprecation Table

Term	Description
✔️	Stay
❌	Deprecated
➖	N/A

Methods

-	add_middleware	add_event_handler	add_route	add_websocket	mount	host	add_exception_handler
Starlette	✔️	✔️	✔️	✔️	✔️	✔️	✔️
Router	➖	✔️	✔️	✔️	✔️	✔️	➖

Decorators

-	middleware	on_event	route	websocket	exception_handler
Starlette	❌	❌	❌	❌	❌
Router	➖	❌	❌	❌	➖

[alex-oleshkevich]

I'd keep this test as long as the feature code exists.

[Kludex]

I'll readd it with the warning filter.

[alex-oleshkevich]

Won't it emit two same warnings when using app.mount? If so then keeping warnings in router is enough.

[Kludex]

Won't it emit two same warnings when using app.mount? If so then keeping warnings in router is enough.

yep, it's written on the description. I'll remove the duplications.

[Kludex]

I've readded the methods, because I think they are actually handy, and only deprecate the decorators. Based mainly on...

[adriangb]

I'd lean towards removing the methods as well. I remember users complaining about add_middleware rebuilding the entire middleware stack -> instantiating their middleware twice. I think no one should rely on their class being instantiated only once but I do agree it's confusing behavior.

You literally already mentioned that, my bad. I'd still vote for removing all of the methods in favor of more "immutable" classes, but mostly because of personal preference.

[Kludex]

You literally already mentioned that, my bad. I'd still vote for removing all of the methods in favor of more "immutable" classes, but mostly because of personal preference.

Those methods may be handy for third party packages. For example, I have a workaround to duplicate endpoint paths that end with "/" with "" and vice-versa, so my application doesn't redirect. Let's not focus on how valid the example is (👀), but the fact that people may be using them for other ideas.

Given that the discussions previously were focusing on the decorators, I would prefer to deal with them separately, or even not deal with them at all, and let them stay.

[tomchristie]

Okay, I needed a bit of time to get back to speed on this.

So, everything listed here is clearly marked in the codebase as "don't use this", and none of these methods or decorators exist in the documentation. Personally I think everything here is on a deprecation line, we just haven't escalated that into warnings or removals yet. But anyways.

Where are we?...

• Given that there's some lack of clarity over if we want to treat the methods and the decorators listed here in the same way we probably should consider them as separate issues.
• Wrt. the decorators, I think we're all okay with pushing forward with their deprecation. Is there any disagreement there?
• I don't think this pull request should be removing any of the code comments warning against usage. I think it should either... (1) add warnings to each of the decorator cases. or (2) outright remove each of the decorator cases, possibly just leaving stubs with error exceptions to help guide anyone who was still using them.

[Kludex]

Given that there's some lack of clarity over if we want to treat the methods and the decorators listed here in the same way we probably should consider them as separate issues.

Sure. We should take a decision on the methods before 1.0.0.

Possible paths:

1. Deprecate and remove before 1.0.0.
2. Document and add tests (as there are pragma: no cover there).

Wrt. the decorators, I think we're all okay with pushing forward with their deprecation. Is there any disagreement there?

No disagreement, we can proceed.

I don't think this pull request should be removing any of the code comments warning against usage. I think it should either... (1) add warnings to each of the decorator cases. or (2) outright remove each of the decorator cases, possibly just leaving stubs with error exceptions to help guide anyone who was still using them.

I'll readd the docstrings.

[Kludex]

I'll remove the deprecation on add_middleware from this PR, just to shrink the scope. But add_middleware is a different case than the other methods, as it has a side effect (mentioned in the description).

[Kludex]

The docstrings I've removed are from methods, not from decorators. JFYK

[Kludex]

This is ready for review. 👀

[florimondmanca]

I'm quite happy with the separation b/w treating decorators vs methods. This keeps an escape hatch for users (including frameworks) to use the methods to alter the app after-the-fact, but officialises that as far as Starlette is concerned, the decorator API shouldn't exist anymore.

Should there be tests on deprecated code usage?

Using this: https://docs.pytest.org/en/7.1.x/how-to/capture-warnings.html#ensuring-code-triggers-a-deprecation-warning

def test_decorator_deprecations():
    app = Starlette()

    with pytest.deprecated_call():
        app.middleware(...) 

    ...

[tiangolo]

Yep, looks good! There's probably a bunch of things that will have to be done in FastAPI but this makes sense! 🤓

[Kludex]

I'm not going to include this on the 0.22.0 release because it implies FastAPI changes. I'll include this (with tests @florimondmanca 🙏) on the 0.23.0 release.