Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: issue with list-errors and mismatched config types #2151

Merged
merged 7 commits into from
Jun 19, 2024
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
156 changes: 89 additions & 67 deletions src/ape/api/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
from enum import Enum
from functools import cached_property
from pathlib import Path
from typing import TYPE_CHECKING, Any, Optional, TypeVar, cast
from typing import Any, Optional, TypeVar, cast

import yaml
from ethpm_types import PackageManifest, PackageMeta, Source
Expand All @@ -24,9 +24,6 @@
)
from ape.utils.misc import load_config

if TYPE_CHECKING:
from ape.managers.config import ConfigManager

ConfigItemType = TypeVar("ConfigItemType")


Expand Down Expand Up @@ -56,9 +53,6 @@ class PluginConfig(BaseSettings):
a config API must register a subclass of this class.
"""

# NOTE: This is probably partially initialized at the time of assignment
_config_manager: Optional["ConfigManager"]

model_config = SettingsConfigDict(extra="allow")

@classmethod
Expand All @@ -74,7 +68,11 @@ def update(root: dict, value_map: dict):

return root

return cls(**update(default_values, overrides))
data = update(default_values, overrides)
try:
return cls.model_validate(data)
except ValidationError as err:
raise ConfigError(str(err)) from err

@only_raise_attribute_error
def __getattr__(self, attr_name: str) -> Any:
Expand Down Expand Up @@ -139,6 +137,85 @@ class DeploymentConfig(PluginConfig):
"""


def _get_problem_with_config(errors: list, path: Path) -> Optional[str]:
# Attempt to find line numbers in the config matching.
cfg_content = Source(content=path.read_text(encoding="utf8")).content
if not cfg_content:
# Likely won't happen?
return None

err_map: dict = {}
for error in errors:
if not (location := error.get("loc")):
continue

lineno = None
loc_idx = 0
depth = len(location)
list_counter = 0
for no, line in cfg_content.items():
loc = location[loc_idx]
clean_line = line.lstrip()

if isinstance(loc, int):
if not clean_line.startswith("- "):
# Not a list item.
continue

elif list_counter != loc:
# Still search for index.
list_counter += 1
continue

# If we get here, we have found the index.
list_counter = 0

elif not clean_line.startswith(f"{loc}:"):
continue

# Look for the next location up the tree.
loc_idx += 1

# Even if we don't find the full path for some reason
# (perhaps it's missing), we still have the last lineno noticed.
lineno = no

if loc_idx < depth:
continue

# Found.
break

if lineno is not None:
err_map[lineno] = error
# else: we looped through the whole file and didn't find anything.

if not err_map:
# raise ValidationError as-is (no line numbers found for some reason).
return None

error_strs: list[str] = []
for line_no, cfg_err in err_map.items():
sub_message = cfg_err.get("msg", cfg_err)
line_before = line_no - 1 if line_no > 1 else None
line_after = line_no + 1 if line_no < len(cfg_content) else None
lines = []
if line_before is not None:
lines.append(f" {line_before}: {cfg_content[line_before]}")
lines.append(f"-->{line_no}: {cfg_content[line_no]}")
if line_after is not None:
lines.append(f" {line_after}: {cfg_content[line_after]}")

file_preview = "\n".join(lines)
sub_message = f"{sub_message}\n{file_preview}\n"
error_strs.append(sub_message)

# NOTE: Using reversed because later pydantic errors
# appear earlier in the list.
final_msg = "\n".join(reversed(error_strs)).strip()
return f"'{clean_path(path)}' is invalid!\n{final_msg}"


class ApeConfig(ExtraAttributesMixin, BaseSettings, ManagerAccessMixin):
"""
The top-level config.
Expand Down Expand Up @@ -282,65 +359,10 @@ def validate_file(cls, path: Path, **overrides) -> "ApeConfig":
# TODO: Support JSON configs here.
raise # The validation error as-is

# Several errors may have been collected from the config.
all_errors = err.errors()
# Attempt to find line numbers in the config matching.
cfg_content = Source(content=path.read_text(encoding="utf8")).content
if not cfg_content:
# Likely won't happen?
raise # The validation error as-is

err_map: dict = {}
for error in all_errors:
if not (location := error.get("loc")):
continue

lineno = None
loc_idx = 0
depth = len(location)
for no, line in cfg_content.items():
loc = location[loc_idx]
if not line.lstrip().startswith(f"{loc}:"):
continue

# Look for the next location up the tree.
loc_idx += 1
if loc_idx < depth:
continue

# Found.
lineno = no
break

if lineno is not None and loc_idx >= depth:
err_map[lineno] = error
# else: we looped through the whole file and didn't find anything.

if not err_map:
# raise ValidationError as-is (no line numbers found for some reason).
raise

error_strs: list[str] = []
for line_no, cfg_err in err_map.items():
sub_message = cfg_err.get("msg", cfg_err)
line_before = line_no - 1 if line_no > 1 else None
line_after = line_no + 1 if line_no < len(cfg_content) else None
lines = []
if line_before is not None:
lines.append(f" {line_before}: {cfg_content[line_before]}")
lines.append(f"-->{line_no}: {cfg_content[line_no]}")
if line_after is not None:
lines.append(f" {line_after}: {cfg_content[line_after]}")

file_preview = "\n".join(lines)
sub_message = f"{sub_message}\n{file_preview}\n"
error_strs.append(sub_message)

# NOTE: Using reversed because later pydantic errors
# appear earlier in the list.
final_msg = "\n".join(reversed(error_strs)).strip()
final_msg = f"'{clean_path(path)}' is invalid!\n{final_msg}"
raise ConfigError(final_msg)
if final_msg := _get_problem_with_config(err.errors(), path):
raise ConfigError(final_msg)
else:
raise ConfigError(str(err)) from err

@classmethod
def from_manifest(cls, manifest: PackageManifest, **overrides) -> "ApeConfig":
Expand Down
4 changes: 2 additions & 2 deletions src/ape/managers/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -151,14 +151,14 @@ def _merge_configs(base: dict, secondary: dict) -> dict:
if key not in secondary:
result[key] = value

elif not isinstance(value, dict):
elif not isinstance(value, dict) or not isinstance(secondary[key], dict):
# Is a primitive value found in both configs.
# Must use the second one.
result[key] = secondary[key]

else:
# Merge the dictionaries.
sub = _merge_configs(base[key], secondary[key])
sub = _merge_configs(value, secondary[key])
result[key] = sub

# Add missed keys from secondary.
Expand Down
41 changes: 38 additions & 3 deletions tests/functional/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
from typing import Optional, Union

import pytest
from pydantic import ValidationError
from pydantic_settings import SettingsConfigDict

from ape.api.config import ApeConfig, ConfigEnum, PluginConfig
Expand Down Expand Up @@ -68,6 +67,33 @@ def test_validate_file_expands_env_vars():
del os.environ[env_var_name]


def test_validate_file_shows_linenos():
with create_tempdir() as temp_dir:
file = temp_dir / "ape-config.yaml"
file.write_text("name: {'test': 123}")

expected = (
f"'{temp_dir / 'ape-config.yaml'}' is invalid!"
"\nInput should be a valid string\n-->1: name: {'test': 123}"
)
with pytest.raises(ConfigError) as err:
_ = ApeConfig.validate_file(file)

assert expected in str(err.value)


def test_validate_file_shows_linenos_handles_lists():
with create_tempdir() as temp_dir:
file = temp_dir / "ape-config.yaml"
file.write_text("deployments:\n ethereum:\n sepolia:\n - foo: bar")
with pytest.raises(ConfigError) as err:
_ = ApeConfig.validate_file(file)

assert str(file) in str(err.value)
assert "sepolia:" in str(err.value)
assert "-->4" in str(err.value)


def test_deployments(networks_connected_to_tester, owner, vyper_contract_container, project):
_ = networks_connected_to_tester # Connection needs to lookup config.

Expand Down Expand Up @@ -300,6 +326,15 @@ def test_merge_configs_short_circuits():
assert merge_configs(ex, {}) == merge_configs({}, ex) == ex


def test_merge_configs_wrong_type():
cfg_0 = {"foo": 123}
cfg_1 = {"foo": {"bar": 123}}
actual = merge_configs(cfg_0, cfg_1)
assert actual["foo"] == {"bar": 123}
actual = merge_configs(cfg_1, cfg_0)
assert actual["foo"] == 123


def test_plugin_config_getattr_and_getitem(config):
config = config.get_config("ethereum")
assert config.mainnet is not None
Expand Down Expand Up @@ -389,14 +424,14 @@ def test_get_config_unknown_plugin(config):
def test_get_config_invalid_plugin_config(project):
with project.temp_config(node={"ethereum": [1, 2]}):
# Show project's ApeConfig model works.
with pytest.raises(ValidationError):
with pytest.raises(ConfigError):
project.config.get_config("node")

# Show the manager-wrapper also works
# (simple wrapper for local project's config,
# but at one time pointlessly overrode the `get_config()`
# which caused issues).
with pytest.raises(ValidationError):
with pytest.raises(ConfigError):
project.config_manager.get_config("node")


Expand Down
Loading