Skip to content

Commit

Permalink
config: don't mutate main section fields
Browse files Browse the repository at this point in the history
- Add `general.interactive` value before validation.
- Compute `general.data` within the model.

This has a couple advantages:

- Disallowing model mutation kills a class of potential bugs.
- The `interactive` field is now validated, which would have avoided the
  bug fixed in #974.
  • Loading branch information
ryneeverett committed Oct 6, 2023
1 parent 7cd4612 commit 02df55b
Show file tree
Hide file tree
Showing 5 changed files with 25 additions and 21 deletions.
13 changes: 5 additions & 8 deletions bugwarrior/config/load.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
except ImportError:
import tomli as tomllib # backport

from . import data, schema
from . import schema

# The name of the environment variable that can be used to ovewrite the path
# to the bugwarriorrc file
Expand Down Expand Up @@ -101,16 +101,13 @@ def parse_file(configpath: str) -> dict:
return config


def load_config(main_section, interactive, quiet):
def load_config(main_section, interactive, quiet) -> dict:
configpath = get_config_path()
rawconfig = parse_file(configpath)
rawconfig[main_section]['interactive'] = interactive
config = schema.validate_config(rawconfig, main_section, configpath)
main_config = config[main_section]
main_config.interactive = interactive
main_config.data = data.BugwarriorData(
data.get_data_path(main_config.taskrc))
configure_logging(main_config.log_file,
'WARNING' if quiet else main_config.log_level)
configure_logging(config[main_section].log_file,
'WARNING' if quiet else config[main_section].log_level)
return config


Expand Down
15 changes: 10 additions & 5 deletions bugwarrior/config/schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

from bugwarrior.services import get_service

from .data import BugwarriorData
from .data import BugwarriorData, get_data_path

log = logging.getLogger(__name__)

Expand Down Expand Up @@ -103,16 +103,21 @@ class PydanticConfig(pydantic.BaseConfig):
class MainSectionConfig(pydantic.BaseModel):

class Config(PydanticConfig):
# To set BugwarriorData based on taskrc:
allow_mutation = True
arbitrary_types_allowed = True

# required
targets: ConfigList

# mutated after validation
# added during configuration loading
interactive: bool

# added during validation (computed field support will land in pydantic-2)
data: typing.Optional[BugwarriorData] = None
interactive: typing.Optional[bool] = None

@pydantic.root_validator
def compute_data(cls, values):
values['data'] = BugwarriorData(get_data_path(values['taskrc']))
return values

# optional
taskrc: TaskrcPath = pydantic.Field(
Expand Down
10 changes: 4 additions & 6 deletions tests/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import responses

from bugwarrior import config
from bugwarrior.config import data, schema
from bugwarrior.config import schema


class AbstractServiceTest(abc.ABC):
Expand Down Expand Up @@ -63,10 +63,9 @@ def inject_fixtures(self, caplog):
self.caplog = caplog

def validate(self):
conf = schema.validate_config(self.config, 'general', 'configpath')
conf['general'].data = data.BugwarriorData(self.lists_path)
conf['general'].interactive = False
return conf
self.config['general'] = self.config.get('general', {})
self.config['general']['interactive'] = False
return schema.validate_config(self.config, 'general', 'configpath')

def assertValidationError(self, expected):

Expand Down Expand Up @@ -110,7 +109,6 @@ def get_mock_service(

service_config = service_class.CONFIG_SCHEMA(**options[section])
main_config = schema.MainSectionConfig(**options['general'])
main_config.data = data.BugwarriorData(self.lists_path)

return service_class(service_config, main_config, section)

Expand Down
2 changes: 1 addition & 1 deletion tests/config/test_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ class TestGetDataPath(ConfigTest):
def setUp(self):
super().setUp()
rawconfig = {
'general': {'targets': ['my_service']},
'general': {'targets': ['my_service'], 'interactive': False},
'my_service': {
'service': 'github',
'login': 'ralphbean',
Expand Down
6 changes: 5 additions & 1 deletion tests/config/test_schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,11 @@ def test_valid(self):
def test_main_section_required(self):
del self.config['general']

self.assertValidationError("No section: 'general'")
with self.assertRaises(SystemExit):
schema.validate_config(self.config, 'general', 'configpath')

self.assertEqual(len(self.caplog.records), 1)
self.assertIn("No section: 'general'", self.caplog.records[0].message)

def test_main_section_missing_targets_option(self):
del self.config['general']['targets']
Expand Down

0 comments on commit 02df55b

Please sign in to comment.