Fixes 4893 - error handling for malformed configurations #5026

fishd · 2018-02-14T14:26:08Z

This fixes #4893 . In the event that a pip conf has malformed headers, it will handle the error and raise a specific message. Previously, pip would exit with a traceback.

Two things I'm uncertain about:

This is a very specific error case, modeled after the original problem referenced in the ticket. Would it be better to handle more generic ConfigParser errors, or perhaps to add additional excepts for other parser exceptions?
Not sure if I put the test in the right location!

fishd · 2018-02-14T18:42:50Z

Hmm, I'm actually stumped about the build failure. It looks like one is a timeout and one is a mypy error.

The mypy error appears to relate to my first question as to which exceptions we want to be handling here:
src/pip/_internal/configuration.py:295: error: Exception type must be derived from BaseException

So maybe catching MissingSectionHeader is not appropriate? However, I'm still not sure why it would get flagged.

pradyunsg

So, this review has been sitting idle for a bit. Sorry for the wait. :(

pradyunsg · 2018-02-15T05:54:38Z

tests/unit/test_configuration.py

@@ -46,6 +46,17 @@ def test_environment_config_loading(self):
            assert self.configuration.get_value("test.hello") == "4", \
                self.configuration._config

+    def test_environment_config_errors_if_malformed(self):


I feel a better idea would be to create a new class called TestConfigurationLoadingErrors(ConfigurationMixin) in the same file and move this test there.

I imagine I'll add more tests to such a class in the future.

pradyunsg · 2018-02-15T05:56:06Z

news/4893.bugfix

@@ -0,0 +1 @@
+Add handling for configparser errors when conf is malformed.


Malformed configuration files now show helpful error messages, instead of tracebacks.

pradyunsg · 2018-02-15T06:01:36Z

src/pip/_internal/configuration.py

@@ -292,6 +292,13 @@ def _construct_parser(self, fname):
                    "Configuration file contains invalid %s characters.\n"
                    "Please fix your configuration, located at %s\n"
                ) % (locale.getpreferredencoding(False), fname))
+            except configparser.MissingSectionHeaderError:


The error here seems to be caused by an incorrect declaration in typeshed.

For now, you can add a # type: ignore to the end of this line and a comment above that links to python/typeshed#1883.

pradyunsg · 2018-02-15T06:04:16Z

src/pip/_internal/configuration.py

+                    "ERROR: "
+                    "Configuration file headers cannot be parsed. \n"
+                    "Please fix the section formatting of the "
+                    "configuration located at %s.\n" % fname


Please remove the "." at the end -- that way it is easier to just copy the entire path by selecting everything after "at".

This message should mention the lineno from the error raised. That would be helpful while trying to locate the error.

Another thought, we should also add a handler after this for ParsingError since they may be raised for other reasons.

pradyunsg · 2018-02-15T06:05:10Z

tests/unit/test_configuration.py

+
+            with pytest.raises(ConfigurationError):
+                self.configuration.load()
+


This should check for the word "malformed" (or something similar), the line number and the file path are mentioned in the error message.

Do you mean that it should check for all three of those things in the error message string? Presumably inside of the with block?

with pytest.raises(ConfigurationError) as err: self.configuration.load() assert "malformed" in str(err) assert lineno in str(err) assert filepath in str(err)

pradyunsg · 2018-03-03T19:39:52Z

About the mypy error, it was a bug in typeshed, fixed it in python/typeshed@b6bd582. I noted the workaround above. :)

pfmoore · 2018-03-26T13:32:25Z

@fishd Are you likely to have time to resolve this in the coming week? Pip 10 beta will be released at the weekend, and if this PR can't be merged by then, I'd prefer to defer it until post pip 10 final.

fishd · 2018-03-26T20:23:00Z

Yes, sorry that I sat on it for so long! I can get it resolved in the next few days.

pfmoore · 2018-03-30T21:22:40Z

Sorry, but this is going to have to wait until post-10.0 now.

pradyunsg · 2018-03-30T21:54:57Z

Fine by me.

pradyunsg · 2018-05-11T05:35:53Z

Ping @fishd

fishd · 2018-05-30T18:55:15Z

Hi, I am not dead. Thank you for your patience.

I have some clarifying questions. I'll attempt to add those as inline comments...

fishd · 2018-05-30T18:57:00Z

src/pip/_internal/configuration.py

+                raise ConfigurationError(
+                    "ERROR: "
+                    "Configuration file cannot be parsed. \n"
+                    "%s\n" % e.message


This line is killing the CI build (no "message" attribute), but I can't seem to reproduce it. I suspect something about what I'm doing is bad practice...?

You can just do str(e) here. It'll result in the same output: https://github.com/python/cpython/blob/3.6/Lib/configparser.py#L174

pradyunsg · 2018-06-08T19:27:46Z

@fishd You seem to have rebased incorrectly.

fishd · 2018-06-08T19:58:29Z

Whoops. I feel like that looks different than it did on my local branch...I'll investigate.

Remove period for easier path copying Move config parsing error to new class 4893 - Add line number, add handling for generic ParsingErrors. Remove whitespace. Convert parsing error to string PEP8 - reformat line break

pradyunsg

LGTM. Older inline comments still apply.

pradyunsg · 2018-07-05T08:31:39Z

@fishd Do you have the time to make the requested changes? If not, I'll be happy to take this from here -- I'd like this to be a part of the next release.

pradyunsg · 2018-07-15T09:38:58Z

Pushing this down the road to the next release since I don't think this'd happen in time for 18.0.

Adds more checks and skips creating a new class.

pradyunsg · 2018-09-18T13:51:08Z

I'll take this up and redo this PR, reusing messages from the stdlib configparser.

As commented.

lock · 2019-06-01T16:14:41Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

pradyunsg reviewed Mar 3, 2018

View reviewed changes

pradyunsg added this to the 10.0 milestone Mar 4, 2018

pradyunsg added the type: enhancement Improvements to functionality label Mar 4, 2018

This comment has been minimized.

Sign in to view

pradyunsg mentioned this pull request Mar 30, 2018

Plan for 10.0 #4981

Closed

dstufft modified the milestones: 10.0, 10.1 Mar 31, 2018

pradyunsg added the S: awaiting response Waiting for a response/more information label May 11, 2018

fishd commented May 30, 2018

View reviewed changes

Fixes 4893 - error handling for malformed configurations

ccd1886

Remove period for easier path copying Move config parsing error to new class 4893 - Add line number, add handling for generic ParsingErrors. Remove whitespace. Convert parsing error to string PEP8 - reformat line break

pradyunsg reviewed Jun 12, 2018

View reviewed changes

pradyunsg modified the milestones: 18.0, 18.1 Jul 15, 2018

pradyunsg self-assigned this Jul 15, 2018

pradyunsg added 2 commits August 13, 2018 23:22

Improve tests

974bc96

Adds more checks and skips creating a new class.

📰

d5bbd8b

pradyunsg previously approved these changes Aug 13, 2018

View reviewed changes

Fix test

5ba4c72

pradyunsg closed this Sep 18, 2018

pradyunsg removed this from the 18.1 milestone Sep 18, 2018

lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 1, 2019

lock bot locked as resolved and limited conversation to collaborators Jun 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes 4893 - error handling for malformed configurations #5026

Fixes 4893 - error handling for malformed configurations #5026

fishd commented Feb 14, 2018 •

edited by pradyunsg

Loading

fishd commented Feb 14, 2018

pradyunsg left a comment

pradyunsg Feb 15, 2018

pradyunsg Feb 15, 2018 •

edited

Loading

pradyunsg Feb 15, 2018 •

edited

Loading

pradyunsg Feb 15, 2018

pradyunsg Feb 15, 2018

pradyunsg Feb 15, 2018

pradyunsg Feb 15, 2018

fishd May 30, 2018

pradyunsg May 31, 2018 •

edited

Loading

pradyunsg commented Mar 3, 2018

pfmoore commented Mar 26, 2018

fishd commented Mar 26, 2018

This comment has been minimized.

pfmoore commented Mar 30, 2018

pradyunsg commented Mar 30, 2018

pradyunsg commented May 11, 2018

fishd commented May 30, 2018

fishd May 30, 2018

pradyunsg May 31, 2018

pradyunsg commented Jun 8, 2018

fishd commented Jun 8, 2018

pradyunsg left a comment

pradyunsg commented Jul 5, 2018

pradyunsg commented Jul 15, 2018

pradyunsg commented Sep 18, 2018

lock bot commented Jun 1, 2019

		@@ -0,0 +1 @@
		Add handling for configparser errors when conf is malformed.


		with pytest.raises(ConfigurationError):
		self.configuration.load()

Fixes 4893 - error handling for malformed configurations #5026

Fixes 4893 - error handling for malformed configurations #5026

Conversation

fishd commented Feb 14, 2018 • edited by pradyunsg Loading

fishd commented Feb 14, 2018

pradyunsg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradyunsg Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

pradyunsg Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradyunsg May 31, 2018 • edited Loading

Choose a reason for hiding this comment

pradyunsg commented Mar 3, 2018

pfmoore commented Mar 26, 2018

fishd commented Mar 26, 2018

This comment has been minimized.

pfmoore commented Mar 30, 2018

pradyunsg commented Mar 30, 2018

pradyunsg commented May 11, 2018

fishd commented May 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradyunsg commented Jun 8, 2018

fishd commented Jun 8, 2018

pradyunsg left a comment

Choose a reason for hiding this comment

pradyunsg commented Jul 5, 2018

pradyunsg commented Jul 15, 2018

pradyunsg commented Sep 18, 2018

lock bot commented Jun 1, 2019

fishd commented Feb 14, 2018 •

edited by pradyunsg

Loading

pradyunsg Feb 15, 2018 •

edited

Loading

pradyunsg Feb 15, 2018 •

edited

Loading

pradyunsg May 31, 2018 •

edited

Loading