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

Misleading error for unindented bare values in config file #2621

Open
dgw opened this issue Sep 15, 2024 · 0 comments
Open

Misleading error for unindented bare values in config file #2621

dgw opened this issue Sep 15, 2024 · 0 comments
Labels
Bug Things to squish; generally used for issues Needs Triage Issues that need to be reviewed and categorized

Comments

@dgw
Copy link
Member

dgw commented Sep 15, 2024

Description

If one manually edits Sopel's config file, it is possible to trigger an error case where the underlying RawConfigParser pulls in the value (intended) as if it is a key with None as the value.

Trying to process the malformed config file into ConfigSections fails, and the end of the resulting traceback misleads the user: ValueError: Missing required value for core.admin_accounts (core.admin_accounts is not a required setting)

This was discovered by a user who didn't realize that the channel list must be indented, but it can occur anywhere in the config file. Ultimately it's down to any string at the beginning of a line that is not followed by =.

Reproduction steps

Insert a bare key with no = and no value anywhere in Sopel's [core] section, for example:

[core]
nick = MyBot
foo
host = irc.libera.chat
# ...

Expected behavior

I expect one of two behavior options:

  1. Sopel catches the error thrown here in ConfigSection.__init__() when an empty value is None instead of '' and re-raises a friendlier ConfigurationError with the malformed string (so the user can easily Ctrl-F for it in their text editor).
  2. Sopel squelches the error, or coerces the None to '' so processing can continue.

Option 1 is the strictest, but I wouldn't feel too bad about forcing users who have already manually edited the config file to go back and recheck it for syntax errors. 😁

Option 2 would sort of hide the problem, even though #1973 means Sopel will log a warning (sopel.bot WARNING - Config option `core.foo` is not defined by its section and may not be recognized by Sopel.). It's unlikely anyone will look at the logs unless Sopel actually refuses to start.

Relevant logs

Traceback (most recent call last):
  File "/workspace/sopel/sopel/config/types.py", line 75, in __init__
    getattr(self, value)
  File "/workspace/sopel/sopel/config/types.py", line 227, in __get__
    section = getattr(settings, instance._section_name)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/workspace/sopel/sopel/config/__init__.py", line 316, in __getattr__
    section = self.ConfigSection(name, items, self)  # Return a section
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/workspace/sopel/sopel/config/__init__.py", line 295, in __init__
    value = item[1].strip()
            ^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'strip'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/workspace/.pyenv_mirror/user/3.12.3/bin/sopel", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/workspace/sopel/sopel/cli/run.py", line 467, in main
    return command(opts)
           ^^^^^^^^^^^^^
  File "/workspace/sopel/sopel/cli/run.py", line 290, in command_start
    settings = get_configuration(opts)
               ^^^^^^^^^^^^^^^^^^^^^^^
  File "/workspace/sopel/sopel/cli/run.py", line 226, in get_configuration
    settings = utils.load_settings(options)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/workspace/sopel/sopel/cli/utils.py", line 349, in load_settings
    return config.Config(filename)
           ^^^^^^^^^^^^^^^^^^^^^^^
  File "/workspace/sopel/sopel/config/__init__.py", line 136, in __init__
    self.define_section('core', core_section.CoreSection,
  File "/workspace/sopel/sopel/config/__init__.py", line 278, in define_section
    setattr(self, name, cls_(self, name, validate=validate))
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/workspace/sopel/sopel/config/types.py", line 82, in __init__
    raise ValueError(
ValueError: Missing required value for core.admin_accounts

Notes

I'd like to thank @Duckle29 for prompting me to investigate this. The error was intriguing because it makes absolutely no sense until you tease apart the traceback (and even then, I stopped before figuring out "why admin_accounts specifically?")

Thanks to writing up this issue, I'm now also thinking about a little reorganization of the Config class, because why is the ConfigSection class defined inside the Config class? (It's been that way since 3.1; the class definition was added 12 years ago.)

Sopel version

8.0.0

Installation method

pip install

Python version

3.12.3

Operating system

Ubuntu 22.04.3 LTS

IRCd

No response

Relevant plugins

No response

@dgw dgw added Bug Things to squish; generally used for issues Needs Triage Issues that need to be reviewed and categorized labels Sep 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Things to squish; generally used for issues Needs Triage Issues that need to be reviewed and categorized
Projects
None yet
Development

No branches or pull requests

1 participant