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

configparser leaks implementation detail #63745

Closed
PCManticore mannequin opened this issue Nov 10, 2013 · 14 comments
Closed

configparser leaks implementation detail #63745

PCManticore mannequin opened this issue Nov 10, 2013 · 14 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@PCManticore
Copy link
Mannequin

PCManticore mannequin commented Nov 10, 2013

BPO 19546
Nosy @bitdancer, @PCManticore, @ambv
Files
  • configparser.patch
  • issue19546.patch: Remove trailing whitespace.
  • issue19546_1.patch: Small style fixes.
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/ambv'
    closed_at = <Date 2014-09-04.08:43:53.703>
    created_at = <Date 2013-11-10.17:16:52.435>
    labels = ['type-bug', 'library']
    title = 'configparser leaks implementation detail'
    updated_at = <Date 2014-09-04.08:45:13.784>
    user = 'https://github.com/PCManticore'

    bugs.python.org fields:

    activity = <Date 2014-09-04.08:45:13.784>
    actor = 'Claudiu.Popa'
    assignee = 'lukasz.langa'
    closed = True
    closed_date = <Date 2014-09-04.08:43:53.703>
    closer = 'lukasz.langa'
    components = ['Library (Lib)']
    creation = <Date 2013-11-10.17:16:52.435>
    creator = 'Claudiu.Popa'
    dependencies = []
    files = ['32564', '34837', '35791']
    hgrepos = []
    issue_num = 19546
    keywords = ['patch']
    message_count = 14.0
    messages = ['202538', '202552', '215334', '215336', '215337', '215590', '215591', '215604', '215621', '216159', '221674', '226350', '226351', '226352']
    nosy_count = 4.0
    nosy_names = ['r.david.murray', 'Claudiu.Popa', 'lukasz.langa', 'python-dev']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue19546'
    versions = ['Python 3.4', 'Python 3.5']

    @PCManticore
    Copy link
    Mannequin Author

    PCManticore mannequin commented Nov 10, 2013

    Various exceptions raised by configparser module leaks implementation detail, by chaining KeyErrors, as seen below:

    Python 3.4.0a4+ (default:0aa2aedc6a21+, Nov  5 2013, 17:10:42)
    [GCC 4.2.1 20070831 patched [FreeBSD]] on freebsd8
    Type "help", "copyright", "credits" or "license" for more information.
    >>> import configparser
    >>> parser = configparser.ConfigParser()
    >>> parser.remove_option('Section1', 'an_int')
    Traceback (most recent call last):
      File "/tank/libs/cpython/Lib/configparser.py", line 935, in remove_option
        sectdict = self._sections[section]
    KeyError: 'Section1'
    
    During handling of the above exception, another exception occurred:
    
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "/tank/libs/cpython/Lib/configparser.py", line 937, in remove_option
        raise NoSectionError(section)
    configparser.NoSectionError: No section: 'Section1'
    >>>

    There are multiple places where this happens: using basic and extended interpolation, using .options to retrieve non-existent options, using .get or .remove_options for non-existent options/sections. The attached patch tries to fixes all those issues by suppressing the initial exception.

    @PCManticore PCManticore mannequin added the stdlib Python modules in the Lib dir label Nov 10, 2013
    @bitdancer
    Copy link
    Member

    I'd vote -1 on this one. The extra context in this case is not confusing, and might be helpful to someone.

    @ambv ambv self-assigned this Nov 10, 2013
    @PCManticore
    Copy link
    Mannequin Author

    PCManticore mannequin commented Apr 1, 2014

    But the last traceback conveys enough information, the user can see immediately that the given section does not exist.
    My problem with the current behaviour is that the first error distracts the user, while the actual problem is the second traceback.
    But I have another example which might contradict my proposal. In the example in the second traceback, it's not clear what Bad value substitution stands for and the first traceback adds the relevant context (the fact that key is missing). But I guess this stands for another issue, for enhancing the message for InterpolationMissingOptionError to transmit the fact that a given option is missing.

    Traceback (most recent call last):
      File "C:\Python34\lib\configparser.py", line 410, in _interpolate_some
        v = map[var]
      File "C:\Python34\lib\collections\__init__.py", line 789, in __getitem__
        return self.__missing__(key)            # support subclasses that define __missing__
      File "C:\Python34\lib\collections\__init__.py", line 781, in __missing__
        raise KeyError(key)
    KeyError: 'home_dir1'
    
    During handling of the above exception, another exception occurred:
    
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "C:\Python34\lib\configparser.py", line 1204, in __getitem__
        return self._parser.get(self._name, key)
      File "C:\Python34\lib\configparser.py", line 773, in get
        d)
      File "C:\Python34\lib\configparser.py", line 374, in before_get
        self._interpolate_some(parser, option, L, value, section, defaults, 1)
      File "C:\Python34\lib\configparser.py", line 413, in _interpolate_some
        option, section, rest, var)
    configparser.InterpolationMissingOptionError: Bad value substitution:
            section: [Paths]
            option : my_dir
            key    : home_dir1
            rawval : /lumberjack

    @bitdancer
    Copy link
    Member

    Why? Just let the context convey the information. It's not like we are building a UI here, this is for the programmer.

    @bitdancer
    Copy link
    Member

    Although I will grant you that I have to guess at what the bad value substitution error message is trying to tell me, so that error message could use some improvement.

    @PCManticore
    Copy link
    Mannequin Author

    PCManticore mannequin commented Apr 5, 2014

    I've created a new issue for the InterpolationMissingOptionError message, bpo-21159. This issue can be closed.

    @PCManticore
    Copy link
    Mannequin Author

    PCManticore mannequin commented Apr 5, 2014

    Ups, sorry for the change of resolution.

    @bitdancer
    Copy link
    Member

    I've been thinking about this more, and I think I will revise my opinion. I haven't been able to think of a place where knowing that the key is missing in self._sections would in fact be helpful to a programmer using the module. So I'm OK with this being an example of a place where the new error fully encapsulates the information from the context and therefore the context can be suppressed.

    @ambv
    Copy link
    Contributor

    ambv commented Apr 5, 2014

    FWIW I agree with Claudiu that the internal exceptions are an implementation detail. If we ever made, say, a SQLite-based or memcache-based configparser, those would be different, but the external API would stay the same.

    Will fix.

    @PCManticore
    Copy link
    Mannequin Author

    PCManticore mannequin commented Apr 14, 2014

    If there is anything left to do for this patch, please tell me.

    @PCManticore
    Copy link
    Mannequin Author

    PCManticore mannequin commented Jun 27, 2014

    Łukasz, do you have some time to take a look at this patch?

    @PCManticore PCManticore mannequin added the type-bug An unexpected behavior, bug, or error label Jun 27, 2014
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 4, 2014

    New changeset 2b14665b7bce by Łukasz Langa in branch 'default':
    Fix bpo-19546: onfigparser exceptions expose implementation details. Patch by Claudiu Popa.
    http://hg.python.org/cpython/rev/2b14665b7bce

    New changeset 554ead559f24 by Łukasz Langa in branch 'default':
    Merge fix for bpo-19546: configparser exceptions leak implementation details
    http://hg.python.org/cpython/rev/554ead559f24

    @ambv ambv closed this as completed Sep 4, 2014
    @ambv
    Copy link
    Contributor

    ambv commented Sep 4, 2014

    Thank you Claudiu for your patch, sorry it took so long to respond.

    @PCManticore
    Copy link
    Mannequin Author

    PCManticore mannequin commented Sep 4, 2014

    No problem, thank you for committing it.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants