Repository: pypa/twine
Pull request #337 (2018-04-05 16:16:07)

Print the response content when upload fails
Recently we hit an issue using twine to upload to artifactory where uploads would fail with a suspicious `HTTPError: 400 Client Error: Bad Request for url`. This happened to coincide very close to SSL certificate changes, so we were debugging the certs, debugging the docker images, pretty much everything except twine itself. After many dead-ends I thought I would debug the HTTP response that twine is getting from the artifactory server where our internal PyPi repo is. This finally gave me an error message from the response that I could look into:
```python
ipdb> print(response.text)
{
  "errors" : [ {
    "status" : 400,
    "message" : "No enum constant org.jfrog.repomd.pypi.model.PypiMetadata.MetadataVersion.v2_1"
  } ]
}
```

This led me to https://www.jfrog.com/jira/browse/RTFACT-16189 which is a bug in artifactory not being able to handle packages built with newer versions of `wheel`/`setuptools`/`twine` that have metadata increased to v2.1.

Had the content from the server been in the output from twine, it would have saved so much time. It's very common for servers to reply with error messages in the content, so it seems this could help lots of debugging out there.

I'm not _positive_ this is the best place to put the change. My change catches the requests exception from `raise_for_status()`, prints the response content, and then re-raises the exception. I'm happy to change this if another approach is better.

Here is the output from twine without my change:
```
Uploading distributions to https://xxx/artifactory/api/pypi/pypi
Uploading xxx-2.3.0.post117+g0206843-py2.py3-none-any.whl
100%|███████████████████████████████████████████████████████████████████████████████████████| 810k/810k [00:00<00:00, 1.92MB/s]
HTTPError: 400 Client Error: Bad Request for url: https://xxx/artifactory/api/pypi/pypi
```

Here is the output with my change:
```
Uploading distributions to https://xxx/artifactory/api/pypi/pypi
Uploading xxx-2.3.0.post117+g0206843-py2.py3-none-any.whl
100%|███████████████████████████████████████████████████████████████████████████████████████| 810k/810k [00:00<00:00, 1.84MB/s]
Content received from server:
{
  "errors" : [ {
    "status" : 400,
    "message" : "No enum constant org.jfrog.repomd.pypi.model.PypiMetadata.MetadataVersion.v2_1"
  } ]
}
HTTPError: 400 Client Error: Bad Request for url: https://xxx/artifactory/api/pypi/pypi
```

# UPDATE
See [#issuecomment-381730323](#issuecomment-381730323) for updated changes to this PR.

brainwane: This looks reasonably good to me - Ian?
pradyunsg: I'll comment on the UX of it, not the code... 

While I do agree the message should come from the server, I'm not sure if the response text should just be printed as is. Basically, if we're printing a json object, it doesn't look clean.

Maybe twine should try to parse the contents of the response assuming it is formatted how it currently is and extract the error message and code from it. Failing that, just print whatever was the response.

jessejoe: @pradyunsg I think that would only be possible if we can guarantee that every PyPI server would return errors formatted the exact same way. Webservers and APIs in general can return multiple types of errors; some may return JSON, some may return HTML, some may return nothing. Unless all PyPI servers always return JSON and always return the same object format of `{"errors" : [ {"status" : XXX, "message" : "FOO"} ]}`, and always the same for any type of HTTP error (`4XX` and `5XX`), then I think it's safer to just print the response text as is, so the user has a clue of what the issue might be without filtering anything.
pradyunsg: To be clear, I'm fine with this PR going in as is. I'm just trying to improve the UX of it - happier if it's cleaner.

You are correct. Until we have a standard format, likely specified by a PEP, we can't reasonably expect that the response will take this format - this is why I'm suggesting falling back to printing the message as is if there's a different format.

I don't think we have lose anything with this - one (common?) case improves, the rest of them stay as is. We increase the code complexity a bit for it.
sigmavirus24: @pradyunsg I don't think Twine can parse a response body. However, I have exactly the same concerns as you. If someone sends this to a random server, that responds with an unruly and large HTML body, their terminal will be flooded with nonsense that's unhelpful. It may be wise to hide this behind a `--verbose` flag. What do you think @brainwane @jessejoe?
jessejoe: @sigmavirus24 I think `--verbose` is a good middle ground and relatively common CLI model to use. I think it's still possible that people will be unaware there are possible errors from the server hiding behind a flag, but you're right that it's not good to flood a terminal with HTML garbage. I'll update the PR.
jessejoe: @sigmavirus24 @pradyunsg @brainwane I've updated my commit to go the `--verbose` route. fd352b4 adds a `--verbose` option to the `upload` CLI argument and function, which will include the server response content when an upload fails with an HTTP error from the server:
```
$ twine upload --verbose dist/*
Uploading distributions to https://artifactory-pub.bit9.local/artifactory/api/pypi/pypi
Uploading cbd_support-2.4.0.post11+ge9d8c5c-py2.py3-none-any.whl
100%|█████████████████████████████████████| 831k/831k [00:00<00:00, 2.14MB/s]
Content received from server:
{
  "errors" : [ {
    "status" : 400,
    "message" : "No enum constant org.jfrog.repomd.pypi.model.PypiMetadata.MetadataVersion.v2_1"
  } ]
}
HTTPError: 400 Client Error: Bad Request for url: https://artifactory-pub.bit9.local/artifactory/api/pypi/pypi
```

I also added a little breadcrumb where if the upload fails with an HTTP error, but `--verbose` hasn't been provided, AND there is content from the server in the response it will suggest using `--verbose`:
```
$ twine upload dist/*
Uploading distributions to https://xxx/artifactory/api/pypi/pypi
Uploading cbd_support-2.4.0.post11+ge9d8c5c-py2.py3-none-any.whl
100%|█████████████████████████████████████| 831k/831k [00:00<00:00, 2.16MB/s]
NOTE: Try --verbose to see response content.
HTTPError: 400 Client Error: Bad Request for url: https://xxx/artifactory/api/pypi/pypi
```

Note: I also had to update 2 tests to comply with the new signature for the `upload()` function. I could make `verbose` and optional param by setting it to a keyword argument which defaults to `False`, but I did not since no other parameters were written this way.

Let me know if you have any other suggestions.

Patch:
diff --git a/AUTHORS b/AUTHORS
index deb63c1..e3a7c01 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -18,3 +18,4 @@ Andrew Watts <andrewwatts@gmail.com>
 Anna Martelli Ravenscroft <annaraven@gmail.com>
 Sumana Harihareswara <sh@changeset.nyc>
 Dustin Ingram <di@di.codes> (https://di.codes)
+Jesse Jarzynka <jesse@jessejoe.com> (http://jessejoe.com)
diff --git a/README.rst b/README.rst
index bfecb79..3829f6b 100644
--- a/README.rst
+++ b/README.rst
@@ -3,11 +3,9 @@ twine
 
 .. rtd-inclusion-marker-do-not-remove
 
-Twine is `a utility`_ for `publishing`_ Python packages on `PyPI`_.
+Twine is `a utility`_ for `publishing`_ packages on `PyPI`_.
 
-It provides build system independent uploads of source and binary
-`distribution artifacts <distributions>`_ for both new and existing
-`projects`_.
+Currently it only supports registering `projects`_ and uploading `distributions`_.
 
 
 Why Should I Use This?
@@ -17,16 +15,17 @@ The goal of ``twine`` is to improve PyPI interaction by improving
 security and testability.
 
 The biggest reason to use ``twine`` is that it securely authenticates
-you to `PyPI`_ over HTTPS using a verified connection regardless of
-the underlying Python version, while whether or
-``python setup.py upload`` will work correctly and securely depends
-on your build system, your Python version and the underlying operating
-system.
+you to `PyPI`_ over HTTPS using a verified connection, while ``python
+setup.py upload`` `only recently stopped using HTTP
+<https://bugs.python.org/issue12226>`_ in Python 2.7.9+ and Python
+3.2+. This means anytime you use ``python setup.py upload`` with an
+older Python version, you expose your username and password to being
+easily sniffed. Twine uses only verified TLS to upload to PyPI,
+protecting your credentials from theft.
 
 Secondly, it allows you to precreate your distribution files.
-``python setup.py upload`` only allows you to upload something that you're
-building with ``distutils`` or ``setuptools``, and created in the same
-command invocation. This means that you cannot test the
+``python setup.py upload`` only allows you to upload something that you've
+created in the same command invocation. This means that you cannot test the
 exact file you're going to upload to PyPI to ensure that it works before
 uploading it.
 
diff --git a/twine/commands/upload.py b/twine/commands/upload.py
index e2ebdba..0689cb3 100644
--- a/twine/commands/upload.py
+++ b/twine/commands/upload.py
@@ -73,7 +73,7 @@ def skip_upload(response, skip_existing, package):
 
 def upload(dists, repository, sign, identity, username, password, comment,
            sign_with, config_file, skip_existing, cert, client_cert,
-           repository_url):
+           repository_url, verbose):
     # Check that a nonsensical option wasn't given
     if not sign and identity:
         raise ValueError("sign must be given along with identity")
@@ -159,8 +159,7 @@ def upload(dists, repository, sign, identity, username, password, comment,
         if skip_upload(resp, skip_existing, package):
             print(skip_message)
             continue
-
-        utils.check_status_code(resp)
+        utils.check_status_code(resp, verbose)
 
     # Bug 28. Try to silence a ResourceWarning by clearing the connection
     # pool.
@@ -262,6 +261,11 @@ def main(args):
              "a .asc file to include an existing signature with the "
              "file upload.",
     )
+    parser.add_argument(
+        "--verbose",
+        action="store_true",
+        help="Show verbose output."
+    )
 
     args = parser.parse_args(args)
 
diff --git a/twine/utils.py b/twine/utils.py
index 6fd6655..4cd1a18 100644
--- a/twine/utils.py
+++ b/twine/utils.py
@@ -22,6 +22,8 @@ import sys
 import argparse
 import warnings
 
+from requests.exceptions import HTTPError
+
 try:
     import configparser
 except ImportError:  # pragma: no cover
@@ -151,7 +153,7 @@ def normalize_repository_url(url):
     return urlunparse(parsed)
 
 
-def check_status_code(response):
+def check_status_code(response, verbose):
     """
     Shouldn't happen, thanks to the UploadToDeprecatedPyPIDetected
     exception, but this is in case that breaks and it does.
@@ -167,7 +169,16 @@ def check_status_code(response):
               "(or https://test.pypi.org/legacy/) to upload your packages "
               "instead. These are the default URLs for Twine now. More at "
               "https://packaging.python.org/guides/migrating-to-pypi-org/ ")
-    response.raise_for_status()
+    try:
+        response.raise_for_status()
+    except HTTPError as err:
+        if response.text:
+            if verbose:
+                print('Content received from server:\n{}'.format(
+                    response.text))
+            else:
+                print('NOTE: Try --verbose to see response content.')
+        raise err
 
 
 def get_userpass_value(cli_value, config, key, prompt_strategy=None):


Test patch:
diff --git a/tests/test_upload.py b/tests/test_upload.py
index cbf8ca9..e58ffd4 100644
--- a/tests/test_upload.py
+++ b/tests/test_upload.py
@@ -83,7 +83,7 @@ def test_get_config_old_format(tmpdir):
                       username=None, password=None, comment=None,
                       cert=None, client_cert=None,
                       sign_with=None, config_file=pypirc, skip_existing=False,
-                      repository_url=None,
+                      repository_url=None, verbose=None,
                       )
     except KeyError as err:
         assert err.args[0] == (
@@ -112,7 +112,7 @@ def test_deprecated_repo(tmpdir):
                       username=None, password=None, comment=None,
                       cert=None, client_cert=None,
                       sign_with=None, config_file=pypirc, skip_existing=False,
-                      repository_url=None,
+                      repository_url=None, verbose=None,
                       )
 
         assert err.args[0] == (