Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add missing type hints to synapse.app. #11287

Merged
merged 13 commits into from
Nov 10, 2021
Merged

Add missing type hints to synapse.app. #11287

merged 13 commits into from
Nov 10, 2021

Conversation

clokep
Copy link
Member

@clokep clokep commented Nov 9, 2021

Finishes off type hints for synapse.app.

This should be reviewable commit-by-commit. Please see my comments for explanations of why changes were made.

@DMRobertson DMRobertson self-assigned this Nov 10, 2021
synapse/app/_base.py Outdated Show resolved Hide resolved
return r
# IReactorTCP returns an object implementing IListeningPort from listenTCP,
# but we know it will be a Port instance.
return r # type: ignore[return-value]
Copy link
Member Author

@clokep clokep Nov 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I should have used a cast here? This is vaguely unsafe in that the implementation, not the interface, defines this. Realistically this is deep in Twisted and I can't imagine this changing.

(This pretty much applies to listen_ssl too.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that there are not other lines that follow, I think a cast and a type-ignore are broadly equivalent.

@@ -65,16 +68,11 @@ class AdminCmdSlavedStore(


class AdminCmdServer(HomeServer):
DATASTORE_CLASS = AdminCmdSlavedStore
DATASTORE_CLASS = AdminCmdSlavedStore # type: ignore
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm essentially punting handling this attribute (for the 3 sub-classes of HomeServer) to #11165.


# We always include a health resource.
resources: Dict[str, IResource] = {"/health": HealthResource()}
resources: Dict[str, Resource] = {"/health": HealthResource()}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's many places where I changed IResource to Resouce this is necessary because create_resource_tree calls resource.listNames() which is not part of the interface, it is an implementation detail of Resource.

Comment on lines +108 to +109
# ConchFactory is a Factory, not a ServerFactory, but they are identical.
return factory # type: ignore[return-value]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should be a cast?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, mypy won't do any analysis based on the type of factory because it's the last line of this function. I think this is fine.

Comment on lines +333 to +335
# IReactorSSL incorrectly declares that an int is returned from listenSSL,
# it actually returns an object implementing IListeningPort, but we know it
# will be a Port instance.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clokep clokep requested a review from DMRobertson November 10, 2021 13:35
@clokep clokep marked this pull request as ready for review November 10, 2021 13:35
@clokep clokep requested a review from a team as a code owner November 10, 2021 13:35
Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for this, and your comments too. They preempted most of my questions!

I think the only change I'm really not sure about is the change of NoReturn to None.

@@ -413,30 +413,6 @@ def format_config_error(e: ConfigError) -> Iterator[str]:


def run(hs: HomeServer):
PROFILE_SYNAPSE = False
if PROFILE_SYNAPSE:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this broken, sorry?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code doesn't work:

type object 'ThreadPool' has no attribute '_worker'

It also doesn't have anyway to enable it without modifying code, so I figure this seems less than useful. I believe that cProfile has too much overhead for actually running synapse, but 🤷 I can attempt to get it to work if you'd like.

@@ -207,7 +212,7 @@ def _configure_named_resource(self, name, compress=False):
if name == "consent":
from synapse.rest.consent.consent_resource import ConsentResource

consent_resource = ConsentResource(self)
consent_resource: Resource = ConsentResource(self)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is to make the assignment on 217 work?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is to generalize that consent_resouce is a Resource and not necessarily ConsentResource, yes.

synapse/app/generic_worker.py Show resolved Hide resolved
synapse/app/_base.py Outdated Show resolved Hide resolved
synapse/app/_base.py Outdated Show resolved Hide resolved
synapse/app/_base.py Outdated Show resolved Hide resolved
return r
# IReactorTCP returns an object implementing IListeningPort from listenTCP,
# but we know it will be a Port instance.
return r # type: ignore[return-value]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that there are not other lines that follow, I think a cast and a type-ignore are broadly equivalent.

@@ -38,7 +38,7 @@
logger = logging.getLogger(__name__)


class ReplicationStreamProtocolFactory(Factory):
class ReplicationStreamProtocolFactory(ServerFactory):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the motivation here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, this explains it. Nice spot!

Comment on lines +108 to +109
# ConchFactory is a Factory, not a ServerFactory, but they are identical.
return factory # type: ignore[return-value]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, mypy won't do any analysis based on the type of factory because it's the last line of this function. I think this is fine.

@callahad callahad requested a review from a team November 10, 2021 14:46
@callahad
Copy link
Contributor

(Oh, weird, had a race condition with DMR's review... thought GitHub had somehow glitched and mistaken my sporadic comments as a review)

@clokep clokep requested review from DMRobertson and removed request for a team November 10, 2021 17:22
Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Dunno if @callahad has anything else on his mind?

@@ -70,6 +72,11 @@
if TYPE_CHECKING:
from synapse.server import HomeServer

# Twisted injects the global reactor to make it easier to import, this confuses
# mypy which thinks it is a module. Tell it that it a more proper type.
reactor = cast(ISynapseReactor, _reactor)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunate, but hopefully worth it to get the extra checking!

(If we end up making use of this elsewhere, perhaps we can do this import-cast dance in synapse.types or somewhere similar?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had that thought briefly, but decided it wasn't worth tying things more tightly together at the moment!

@clokep
Copy link
Member Author

clokep commented Nov 10, 2021

LGTM! Dunno if @callahad has anything else on his mind?

I think I hit all of the thoughts Dan had, I'm going to merge this but can make more changes if necessary!

@clokep clokep merged commit 5cace20 into develop Nov 10, 2021
@clokep clokep deleted the clokep/app-types branch November 10, 2021 20:06
@DMRobertson
Copy link
Contributor

The diff on mypy.ini is great to see! 🎆

babolivier added a commit to matrix-org/synapse-dinsic that referenced this pull request Dec 6, 2021
Synapse 1.48.0 (2021-11-30)
===========================

This release removes support for the long-deprecated `trust_identity_server_for_password_resets` configuration flag.

This release also fixes some performance issues with some background database updates introduced in Synapse 1.47.0.

No significant changes since 1.48.0rc1.

Synapse 1.48.0rc1 (2021-11-25)
==============================

Features
--------

- Experimental support for the thread relation defined in [MSC3440](matrix-org/matrix-spec-proposals#3440). ([\#11161](matrix-org/synapse#11161))
- Support filtering by relation senders & types per [MSC3440](matrix-org/matrix-spec-proposals#3440). ([\#11236](matrix-org/synapse#11236))
- Add support for the `/_matrix/client/v3` and `/_matrix/media/v3` APIs from Matrix v1.1. ([\#11318](matrix-org/synapse#11318), [\#11371](matrix-org/synapse#11371))
- Support the stable version of [MSC2778](matrix-org/matrix-spec-proposals#2778): the `m.login.application_service` login type. Contributed by @tulir. ([\#11335](matrix-org/synapse#11335))
- Add a new version of delete room admin API `DELETE /_synapse/admin/v2/rooms/<room_id>` to run it in the background. Contributed by @dklimpel. ([\#11223](matrix-org/synapse#11223))
- Allow the admin [Delete Room API](https://matrix-org.github.io/synapse/latest/admin_api/rooms.html#delete-room-api) to block a room without the need to join it. ([\#11228](matrix-org/synapse#11228))
- Add an admin API to un-shadow-ban a user. ([\#11347](matrix-org/synapse#11347))
- Add an admin API to run background database schema updates. ([\#11352](matrix-org/synapse#11352))
- Add an admin API for blocking a room. ([\#11324](matrix-org/synapse#11324))
- Update the JWT login type to support custom a `sub` claim. ([\#11361](matrix-org/synapse#11361))
- Store and allow querying of arbitrary event relations. ([\#11391](matrix-org/synapse#11391))

Bugfixes
--------

- Fix a long-standing bug wherein display names or avatar URLs containing null bytes cause an internal server error when stored in the DB. ([\#11230](matrix-org/synapse#11230))
- Prevent [MSC2716](matrix-org/matrix-spec-proposals#2716) historical state events from being pushed to an application service via `/transactions`. ([\#11265](matrix-org/synapse#11265))
- Fix a long-standing bug where uploading extremely thin images (e.g. 1000x1) would fail. Contributed by @Neeeflix. ([\#11288](matrix-org/synapse#11288))
- Fix a bug, introduced in Synapse 1.46.0, which caused the `check_3pid_auth` and `on_logged_out` callbacks in legacy password authentication provider modules to not be registered. Modules using the generic module interface were not affected. ([\#11340](matrix-org/synapse#11340))
- Fix a bug introduced in 1.41.0 where space hierarchy responses would be incorrectly reused if multiple users were to make the same request at the same time. ([\#11355](matrix-org/synapse#11355))
- Fix a bug introduced in 1.45.0 where the `read_templates` method of the module API would error. ([\#11377](matrix-org/synapse#11377))
- Fix an issue introduced in 1.47.0 which prevented servers re-joining rooms they had previously left, if their signing keys were replaced. ([\#11379](matrix-org/synapse#11379))
- Fix a bug introduced in 1.13.0 where creating and publishing a room could cause errors if `room_list_publication_rules` is configured. ([\#11392](matrix-org/synapse#11392))
- Improve performance of various background database updates. ([\#11421](matrix-org/synapse#11421), [\#11422](matrix-org/synapse#11422))

Improved Documentation
----------------------

- Suggest users of the Debian packages add configuration to `/etc/matrix-synapse/conf.d/` to prevent, upon upgrade, being asked to choose between their configuration and the maintainer's. ([\#11281](matrix-org/synapse#11281))
- Fix typos in the documentation for the `username_available` admin API. Contributed by Stanislav Motylkov. ([\#11286](matrix-org/synapse#11286))
- Add Single Sign-On, SAML and CAS pages to the documentation. ([\#11298](matrix-org/synapse#11298))
- Change the word 'Home server' as one word 'homeserver' in documentation. ([\#11320](matrix-org/synapse#11320))
- Fix missing quotes for wildcard domains in `federation_certificate_verification_whitelist`. ([\#11381](matrix-org/synapse#11381))

Deprecations and Removals
-------------------------

- Remove deprecated `trust_identity_server_for_password_resets` configuration flag. ([\#11333](matrix-org/synapse#11333), [\#11395](matrix-org/synapse#11395))

Internal Changes
----------------

- Add type annotations to `synapse.metrics`. ([\#10847](matrix-org/synapse#10847))
- Split out federated PDU retrieval function into a non-cached version. ([\#11242](matrix-org/synapse#11242))
- Clean up code relating to to-device messages and sending ephemeral events to application services. ([\#11247](matrix-org/synapse#11247))
- Fix a small typo in the error response when a relation type other than 'm.annotation' is passed to `GET /rooms/{room_id}/aggregations/{event_id}`. ([\#11278](matrix-org/synapse#11278))
- Drop unused database tables `room_stats_historical` and `user_stats_historical`. ([\#11280](matrix-org/synapse#11280))
- Require all files in synapse/ and tests/ to pass mypy unless specifically excluded. ([\#11282](matrix-org/synapse#11282), [\#11285](matrix-org/synapse#11285), [\#11359](matrix-org/synapse#11359))
- Add missing type hints to `synapse.app`. ([\#11287](matrix-org/synapse#11287))
- Remove unused parameters on `FederationEventHandler._check_event_auth`. ([\#11292](matrix-org/synapse#11292))
- Add type hints to `synapse._scripts`. ([\#11297](matrix-org/synapse#11297))
- Fix an issue which prevented the `remove_deleted_devices_from_device_inbox` background database schema update from running when updating from a recent Synapse version. ([\#11303](matrix-org/synapse#11303))
- Add type hints to storage classes. ([\#11307](matrix-org/synapse#11307), [\#11310](matrix-org/synapse#11310), [\#11311](matrix-org/synapse#11311), [\#11312](matrix-org/synapse#11312), [\#11313](matrix-org/synapse#11313), [\#11314](matrix-org/synapse#11314), [\#11316](matrix-org/synapse#11316), [\#11322](matrix-org/synapse#11322), [\#11332](matrix-org/synapse#11332), [\#11339](matrix-org/synapse#11339), [\#11342](matrix-org/synapse#11342))
- Add type hints to `synapse.util`. ([\#11321](matrix-org/synapse#11321), [\#11328](matrix-org/synapse#11328))
- Improve type annotations in Synapse's test suite. ([\#11323](matrix-org/synapse#11323), [\#11330](matrix-org/synapse#11330))
- Test that room alias deletion works as intended. ([\#11327](matrix-org/synapse#11327))
- Add type annotations for some methods and properties in the module API. ([\#11341](matrix-org/synapse#11341))
- Fix running `scripts-dev/complement.sh`, which was broken in v1.47.0rc1. ([\#11368](matrix-org/synapse#11368))
- Rename internal functions for token generation to better reflect what they do. ([\#11369](matrix-org/synapse#11369), [\#11370](matrix-org/synapse#11370))
- Add type hints to configuration classes. ([\#11377](matrix-org/synapse#11377))
- Publish a `develop` image to Docker Hub. ([\#11380](matrix-org/synapse#11380))
- Keep fallback key marked as used if it's re-uploaded. ([\#11382](matrix-org/synapse#11382))
- Use `auto_attribs` on the `attrs` class `RefreshTokenLookupResult`. ([\#11386](matrix-org/synapse#11386))
- Rename unstable `access_token_lifetime` configuration option to `refreshable_access_token_lifetime` to make it clear it only concerns refreshable access tokens. ([\#11388](matrix-org/synapse#11388))
- Do not run the broken MSC2716 tests when running `scripts-dev/complement.sh`. ([\#11389](matrix-org/synapse#11389))
- Remove dead code from supporting ACME. ([\#11393](matrix-org/synapse#11393))
- Refactor including the bundled relations when serializing an event. ([\#11408](matrix-org/synapse#11408))
Fizzadar added a commit to Fizzadar/synapse that referenced this pull request Jan 5, 2022
Synapse 1.48.0 (2021-11-30)
===========================

This release removes support for the long-deprecated `trust_identity_server_for_password_resets` configuration flag.

This release also fixes some performance issues with some background database updates introduced in Synapse 1.47.0.

No significant changes since 1.48.0rc1.

Synapse 1.48.0rc1 (2021-11-25)
==============================

Features
--------

- Experimental support for the thread relation defined in [MSC3440](matrix-org/matrix-spec-proposals#3440). ([\matrix-org#11161](matrix-org#11161))
- Support filtering by relation senders & types per [MSC3440](matrix-org/matrix-spec-proposals#3440). ([\matrix-org#11236](matrix-org#11236))
- Add support for the `/_matrix/client/v3` and `/_matrix/media/v3` APIs from Matrix v1.1. ([\matrix-org#11318](matrix-org#11318), [\matrix-org#11371](matrix-org#11371))
- Support the stable version of [MSC2778](matrix-org/matrix-spec-proposals#2778): the `m.login.application_service` login type. Contributed by @tulir. ([\matrix-org#11335](matrix-org#11335))
- Add a new version of delete room admin API `DELETE /_synapse/admin/v2/rooms/<room_id>` to run it in the background. Contributed by @dklimpel. ([\matrix-org#11223](matrix-org#11223))
- Allow the admin [Delete Room API](https://matrix-org.github.io/synapse/latest/admin_api/rooms.html#delete-room-api) to block a room without the need to join it. ([\matrix-org#11228](matrix-org#11228))
- Add an admin API to un-shadow-ban a user. ([\matrix-org#11347](matrix-org#11347))
- Add an admin API to run background database schema updates. ([\matrix-org#11352](matrix-org#11352))
- Add an admin API for blocking a room. ([\matrix-org#11324](matrix-org#11324))
- Update the JWT login type to support custom a `sub` claim. ([\matrix-org#11361](matrix-org#11361))
- Store and allow querying of arbitrary event relations. ([\matrix-org#11391](matrix-org#11391))

Bugfixes
--------

- Fix a long-standing bug wherein display names or avatar URLs containing null bytes cause an internal server error when stored in the DB. ([\matrix-org#11230](matrix-org#11230))
- Prevent [MSC2716](matrix-org/matrix-spec-proposals#2716) historical state events from being pushed to an application service via `/transactions`. ([\matrix-org#11265](matrix-org#11265))
- Fix a long-standing bug where uploading extremely thin images (e.g. 1000x1) would fail. Contributed by @Neeeflix. ([\matrix-org#11288](matrix-org#11288))
- Fix a bug, introduced in Synapse 1.46.0, which caused the `check_3pid_auth` and `on_logged_out` callbacks in legacy password authentication provider modules to not be registered. Modules using the generic module interface were not affected. ([\matrix-org#11340](matrix-org#11340))
- Fix a bug introduced in 1.41.0 where space hierarchy responses would be incorrectly reused if multiple users were to make the same request at the same time. ([\matrix-org#11355](matrix-org#11355))
- Fix a bug introduced in 1.45.0 where the `read_templates` method of the module API would error. ([\matrix-org#11377](matrix-org#11377))
- Fix an issue introduced in 1.47.0 which prevented servers re-joining rooms they had previously left, if their signing keys were replaced. ([\matrix-org#11379](matrix-org#11379))
- Fix a bug introduced in 1.13.0 where creating and publishing a room could cause errors if `room_list_publication_rules` is configured. ([\matrix-org#11392](matrix-org#11392))
- Improve performance of various background database updates. ([\matrix-org#11421](matrix-org#11421), [\matrix-org#11422](matrix-org#11422))

Improved Documentation
----------------------

- Suggest users of the Debian packages add configuration to `/etc/matrix-synapse/conf.d/` to prevent, upon upgrade, being asked to choose between their configuration and the maintainer's. ([\matrix-org#11281](matrix-org#11281))
- Fix typos in the documentation for the `username_available` admin API. Contributed by Stanislav Motylkov. ([\matrix-org#11286](matrix-org#11286))
- Add Single Sign-On, SAML and CAS pages to the documentation. ([\matrix-org#11298](matrix-org#11298))
- Change the word 'Home server' as one word 'homeserver' in documentation. ([\matrix-org#11320](matrix-org#11320))
- Fix missing quotes for wildcard domains in `federation_certificate_verification_whitelist`. ([\matrix-org#11381](matrix-org#11381))

Deprecations and Removals
-------------------------

- Remove deprecated `trust_identity_server_for_password_resets` configuration flag. ([\matrix-org#11333](matrix-org#11333), [\matrix-org#11395](matrix-org#11395))

Internal Changes
----------------

- Add type annotations to `synapse.metrics`. ([\matrix-org#10847](matrix-org#10847))
- Split out federated PDU retrieval function into a non-cached version. ([\matrix-org#11242](matrix-org#11242))
- Clean up code relating to to-device messages and sending ephemeral events to application services. ([\matrix-org#11247](matrix-org#11247))
- Fix a small typo in the error response when a relation type other than 'm.annotation' is passed to `GET /rooms/{room_id}/aggregations/{event_id}`. ([\matrix-org#11278](matrix-org#11278))
- Drop unused database tables `room_stats_historical` and `user_stats_historical`. ([\matrix-org#11280](matrix-org#11280))
- Require all files in synapse/ and tests/ to pass mypy unless specifically excluded. ([\matrix-org#11282](matrix-org#11282), [\matrix-org#11285](matrix-org#11285), [\matrix-org#11359](matrix-org#11359))
- Add missing type hints to `synapse.app`. ([\matrix-org#11287](matrix-org#11287))
- Remove unused parameters on `FederationEventHandler._check_event_auth`. ([\matrix-org#11292](matrix-org#11292))
- Add type hints to `synapse._scripts`. ([\matrix-org#11297](matrix-org#11297))
- Fix an issue which prevented the `remove_deleted_devices_from_device_inbox` background database schema update from running when updating from a recent Synapse version. ([\matrix-org#11303](matrix-org#11303))
- Add type hints to storage classes. ([\matrix-org#11307](matrix-org#11307), [\matrix-org#11310](matrix-org#11310), [\matrix-org#11311](matrix-org#11311), [\matrix-org#11312](matrix-org#11312), [\matrix-org#11313](matrix-org#11313), [\matrix-org#11314](matrix-org#11314), [\matrix-org#11316](matrix-org#11316), [\matrix-org#11322](matrix-org#11322), [\matrix-org#11332](matrix-org#11332), [\matrix-org#11339](matrix-org#11339), [\matrix-org#11342](matrix-org#11342))
- Add type hints to `synapse.util`. ([\matrix-org#11321](matrix-org#11321), [\matrix-org#11328](matrix-org#11328))
- Improve type annotations in Synapse's test suite. ([\matrix-org#11323](matrix-org#11323), [\matrix-org#11330](matrix-org#11330))
- Test that room alias deletion works as intended. ([\matrix-org#11327](matrix-org#11327))
- Add type annotations for some methods and properties in the module API. ([\matrix-org#11341](matrix-org#11341))
- Fix running `scripts-dev/complement.sh`, which was broken in v1.47.0rc1. ([\matrix-org#11368](matrix-org#11368))
- Rename internal functions for token generation to better reflect what they do. ([\matrix-org#11369](matrix-org#11369), [\matrix-org#11370](matrix-org#11370))
- Add type hints to configuration classes. ([\matrix-org#11377](matrix-org#11377))
- Publish a `develop` image to Docker Hub. ([\matrix-org#11380](matrix-org#11380))
- Keep fallback key marked as used if it's re-uploaded. ([\matrix-org#11382](matrix-org#11382))
- Use `auto_attribs` on the `attrs` class `RefreshTokenLookupResult`. ([\matrix-org#11386](matrix-org#11386))
- Rename unstable `access_token_lifetime` configuration option to `refreshable_access_token_lifetime` to make it clear it only concerns refreshable access tokens. ([\matrix-org#11388](matrix-org#11388))
- Do not run the broken MSC2716 tests when running `scripts-dev/complement.sh`. ([\matrix-org#11389](matrix-org#11389))
- Remove dead code from supporting ACME. ([\matrix-org#11393](matrix-org#11393))
- Refactor including the bundled relations when serializing an event. ([\matrix-org#11408](matrix-org#11408))
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants