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

Re-type config paths in ConfigErrors to be StrSequences #15615

Merged
merged 3 commits into from
May 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
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
1 change: 1 addition & 0 deletions changelog.d/15615.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Re-type config paths in `ConfigError`s to be `StrSequence`s instead of `Iterable[str]`s.
3 changes: 2 additions & 1 deletion synapse/config/_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import pkg_resources
import yaml

from synapse.types import StrSequence
from synapse.util.templates import _create_mxc_to_http_filter, _format_ts_filter

logger = logging.getLogger(__name__)
Expand All @@ -58,7 +59,7 @@ class ConfigError(Exception):
the problem lies.
"""

def __init__(self, msg: str, path: Optional[Iterable[str]] = None):
def __init__(self, msg: str, path: Optional[StrSequence] = None):
self.msg = msg
self.path = path

Expand Down
3 changes: 2 additions & 1 deletion synapse/config/_base.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,10 @@ from synapse.config import ( # noqa: F401
voip,
workers,
)
from synapse.types import StrSequence

class ConfigError(Exception):
def __init__(self, msg: str, path: Optional[Iterable[str]] = None):
def __init__(self, msg: str, path: Optional[StrSequence] = None):
self.msg = msg
self.path = path

Expand Down
8 changes: 4 additions & 4 deletions synapse/config/_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,17 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
from typing import Any, Dict, Iterable, Type, TypeVar
from typing import Any, Dict, Type, TypeVar

import jsonschema
from pydantic import BaseModel, ValidationError, parse_obj_as

from synapse.config._base import ConfigError
from synapse.types import JsonDict
from synapse.types import JsonDict, StrSequence


def validate_config(
json_schema: JsonDict, config: Any, config_path: Iterable[str]
json_schema: JsonDict, config: Any, config_path: StrSequence
) -> None:
"""Validates a config setting against a JsonSchema definition

Expand All @@ -45,7 +45,7 @@ def validate_config(


def json_error_to_config_error(
e: jsonschema.ValidationError, config_path: Iterable[str]
e: jsonschema.ValidationError, config_path: StrSequence
) -> ConfigError:
"""Converts a json validation error to a user-readable ConfigError

Expand Down
6 changes: 3 additions & 3 deletions synapse/config/oembed.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import attr
import pkg_resources

from synapse.types import JsonDict
from synapse.types import JsonDict, StrSequence

from ._base import Config, ConfigError
from ._util import validate_config
Expand Down Expand Up @@ -80,7 +80,7 @@ def _parse_and_validate_providers(
)

def _parse_and_validate_provider(
self, providers: List[JsonDict], config_path: Iterable[str]
self, providers: List[JsonDict], config_path: StrSequence
) -> Iterable[OEmbedEndpointConfig]:
# Ensure it is the proper form.
validate_config(
Expand Down Expand Up @@ -112,7 +112,7 @@ def _parse_and_validate_provider(
api_endpoint, patterns, endpoint.get("formats")
)

def _glob_to_pattern(self, glob: str, config_path: Iterable[str]) -> Pattern:
def _glob_to_pattern(self, glob: str, config_path: StrSequence) -> Pattern:
"""
Convert the glob into a sane regular expression to match against. The
rules followed will be slightly different for the domain portion vs.
Expand Down
4 changes: 2 additions & 2 deletions synapse/config/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
from twisted.conch.ssh.keys import Key

from synapse.api.room_versions import KNOWN_ROOM_VERSIONS
from synapse.types import JsonDict
from synapse.types import JsonDict, StrSequence
from synapse.util.module_loader import load_module
from synapse.util.stringutils import parse_and_validate_server_name

Expand Down Expand Up @@ -73,7 +73,7 @@ def _6to4(network: IPNetwork) -> IPNetwork:
def generate_ip_set(
ip_addresses: Optional[Iterable[str]],
extra_addresses: Optional[Iterable[str]] = None,
config_path: Optional[Iterable[str]] = None,
config_path: Optional[StrSequence] = None,
) -> IPSet:
"""
Generate an IPSet from a list of IP addresses or CIDRs.
Expand Down
8 changes: 8 additions & 0 deletions synapse/types/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,15 @@

# Collection[str] that does not include str itself; str being a Sequence[str]
# is very misleading and results in bugs.
#
# StrCollection is an unordered collection of strings. If ordering is important,
# StrSequence can be used instead.
StrCollection = Union[Tuple[str, ...], List[str], AbstractSet[str]]
# Sequence[str] that does not include str itself; str being a Sequence[str]
# is very misleading and results in bugs.
#
# Unlike StrCollection, StrSequence is an ordered collection of strings.
StrSequence = Union[Tuple[str, ...], List[str]]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

StrCollection allows unordered sets of path components, which is definitely not what a path is. So I defined a StrSequence type.

Copy link
Contributor

@MadLittleMods MadLittleMods May 17, 2023

Choose a reason for hiding this comment

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

Adding a comment about unordered vs ordered to each of the types would be helpful even it may be intrinsic to the name if you're familiar with Python.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 10e31db.



# Note that this seems to require inheriting *directly* from Interface in order
Expand Down
24 changes: 9 additions & 15 deletions synapse/util/module_loader.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,17 @@

import importlib
import importlib.util
import itertools
from types import ModuleType
from typing import Any, Iterable, Tuple, Type
from typing import Any, Tuple, Type

import jsonschema

from synapse.config._base import ConfigError
from synapse.config._util import json_error_to_config_error
from synapse.types import StrSequence


def load_module(provider: dict, config_path: Iterable[str]) -> Tuple[Type, Any]:
def load_module(provider: dict, config_path: StrSequence) -> Tuple[Type, Any]:
"""Loads a synapse module with its config

Args:
Expand All @@ -39,9 +39,7 @@ def load_module(provider: dict, config_path: Iterable[str]) -> Tuple[Type, Any]:

modulename = provider.get("module")
if not isinstance(modulename, str):
raise ConfigError(
"expected a string", path=itertools.chain(config_path, ("module",))
)
raise ConfigError("expected a string", path=tuple(config_path) + ("module",))

# We need to import the module, and then pick the class out of
# that, so we split based on the last dot.
Expand All @@ -55,19 +53,17 @@ def load_module(provider: dict, config_path: Iterable[str]) -> Tuple[Type, Any]:
try:
provider_config = provider_class.parse_config(module_config)
except jsonschema.ValidationError as e:
raise json_error_to_config_error(
e, itertools.chain(config_path, ("config",))
)
raise json_error_to_config_error(e, tuple(config_path) + ("config",))
except ConfigError as e:
raise _wrap_config_error(
"Failed to parse config for module %r" % (modulename,),
prefix=itertools.chain(config_path, ("config",)),
prefix=tuple(config_path) + ("config",),
e=e,
)
except Exception as e:
raise ConfigError(
"Failed to parse config for module %r" % (modulename,),
path=itertools.chain(config_path, ("config",)),
path=tuple(config_path) + ("config",),
) from e
else:
provider_config = module_config
Expand All @@ -92,17 +88,15 @@ def load_python_module(location: str) -> ModuleType:
return mod


def _wrap_config_error(
msg: str, prefix: Iterable[str], e: ConfigError
) -> "ConfigError":
def _wrap_config_error(msg: str, prefix: StrSequence, e: ConfigError) -> "ConfigError":
"""Wrap a relative ConfigError with a new path

This is useful when we have a ConfigError with a relative path due to a problem
parsing part of the config, and we now need to set it in context.
"""
path = prefix
if e.path:
path = itertools.chain(prefix, e.path)
path = tuple(prefix) + tuple(e.path)

e1 = ConfigError(msg, path)

Expand Down