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

Additional type hints for config module, part 2. #11480

Merged
merged 4 commits into from
Dec 9, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 21 additions & 15 deletions synapse/config/key.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,14 @@
import hashlib
import logging
import os
from typing import Any, Dict
from typing import Any, Dict, Iterator, List, Optional

import attr
import jsonschema
from signedjson.key import (
NACL_ED25519,
SigningKey,
VerifyKey,
decode_signing_key_base64,
decode_verify_key_bytes,
generate_signing_key,
Expand All @@ -31,6 +33,7 @@
)
from unpaddedbase64 import decode_base64

from synapse.types import JsonDict
from synapse.util.stringutils import random_string, random_string_with_symbols

from ._base import Config, ConfigError
Expand Down Expand Up @@ -81,14 +84,13 @@
logger = logging.getLogger(__name__)


@attr.s
@attr.s(slots=True, auto_attribs=True)
class TrustedKeyServer:
# string: name of the server.
server_name = attr.ib()
# name of the server.
server_name: str

# dict[str,VerifyKey]|None: map from key id to key object, or None to disable
# signature verification.
verify_keys = attr.ib(default=None)
# map from key id to key object, or None to disable signature verification.
verify_keys: Optional[Dict[str, VerifyKey]] = None


class KeyConfig(Config):
Expand Down Expand Up @@ -279,15 +281,15 @@ def generate_config_section(
% locals()
)

def read_signing_keys(self, signing_key_path, name):
def read_signing_keys(self, signing_key_path: str, name: str) -> List[SigningKey]:
"""Read the signing keys in the given path.

Args:
signing_key_path (str)
name (str): Associated config key name
signing_key_path
name: Associated config key name

Returns:
list[SigningKey]
The signing keys read from the given path.
"""

signing_keys = self.read_file(signing_key_path, name)
Expand All @@ -296,7 +298,9 @@ def read_signing_keys(self, signing_key_path, name):
except Exception as e:
raise ConfigError("Error reading %s: %s" % (name, str(e)))

def read_old_signing_keys(self, old_signing_keys):
def read_old_signing_keys(
self, old_signing_keys: Optional[JsonDict]
) -> Dict[str, VerifyKey]:
if old_signing_keys is None:
return {}
keys = {}
Expand Down Expand Up @@ -340,7 +344,7 @@ def generate_files(self, config: Dict[str, Any], config_dir_path: str) -> None:
write_signing_keys(signing_key_file, (key,))


def _perspectives_to_key_servers(config):
def _perspectives_to_key_servers(config: JsonDict) -> Iterator[JsonDict]:
"""Convert old-style 'perspectives' configs into new-style 'trusted_key_servers'

Returns an iterable of entries to add to trusted_key_servers.
Expand Down Expand Up @@ -402,7 +406,9 @@ def _perspectives_to_key_servers(config):
}


def _parse_key_servers(key_servers, federation_verify_certificates):
def _parse_key_servers(
key_servers: List[Any], federation_verify_certificates: bool
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 wasn't really thrilled with this Any here, we do assert that the types are correct below (via JSON schema), but I don't think that all quite works with mypy...

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this type could beList[JsonDict]?
Especially since that's what _perspectives_to_key_servers is typed to return, and it returns data in the same format as key_servers.

Copy link
Member Author

Choose a reason for hiding this comment

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

All we know is that it is a list, we then verify that the list is the proper form below, using JSON schema. The input is from YAML so could really be anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, hmmm I see what you're saying it is either a hard-coded value of the result of _perspectives_to_key_servers, so maybe we can do better here, yes!

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, but we can't since we extend it with _perspectives_to_key_servers, so the original gunk could still just be weird values. We really have no idea what it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yeah good point.

) -> Iterator[TrustedKeyServer]:
try:
jsonschema.validate(key_servers, TRUSTED_KEY_SERVERS_SCHEMA)
except jsonschema.ValidationError as e:
Expand Down Expand Up @@ -444,7 +450,7 @@ def _parse_key_servers(key_servers, federation_verify_certificates):
yield result


def _assert_keyserver_has_verify_keys(trusted_key_server):
def _assert_keyserver_has_verify_keys(trusted_key_server: TrustedKeyServer) -> None:
if not trusted_key_server.verify_keys:
raise ConfigError(INSECURE_NOTARY_ERROR)

Expand Down
6 changes: 4 additions & 2 deletions synapse/config/metrics.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,12 @@

@attr.s
class MetricsFlags:
known_servers = attr.ib(default=False, validator=attr.validators.instance_of(bool))
known_servers: bool = attr.ib(
default=False, validator=attr.validators.instance_of(bool)
)

@classmethod
def all_off(cls):
def all_off(cls) -> "MetricsFlags":
"""
Instantiate the flags with all options set to off.
"""
Expand Down
4 changes: 3 additions & 1 deletion synapse/config/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -1257,7 +1257,9 @@ def add_arguments(parser: argparse.ArgumentParser) -> None:
help="Turn on the twisted telnet manhole service on the given port.",
)

def read_gc_intervals(self, durations) -> Optional[Tuple[float, float, float]]:
def read_gc_intervals(
self, durations: Optional[List[Any]]
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we using Any here because it's more readable than Union[str, int]? Or is there another reason?

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 don't think we know that this is a str | int at this point? It is just passed whatever is in the YAML object so it can be a dict, list, None, int, string, boolean.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes indeed. Though following this logic, how do we know that it's a list? afaict we feed it the config directly without checking the type.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! I'll change it to Any.

) -> Optional[Tuple[float, float, float]]:
"""Reads the three durations for the GC min interval option, returning seconds."""
if durations is None:
return None
Expand Down
2 changes: 1 addition & 1 deletion synapse/config/tls.py
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ def read_config(self, config: dict, config_dir_path: str, **kwargs):
self.tls_certificate: Optional[crypto.X509] = None
self.tls_private_key: Optional[crypto.PKey] = None

def read_certificate_from_disk(self):
def read_certificate_from_disk(self) -> None:
"""
Read the certificates and private key from disk.
"""
Expand Down