Repository: boto/botocore (python)

Issue:
Unable to pass version to s3:CopyObject
I'm trying to copy a specific version of an object from one s3 bucket to another by adding ?versionId=someversion to the CopySource parameter of s3.copy_object. It looks like the code assumes that the value should be urlencoded, so it mangles the value.

```
        args = {
            'CopySource': '/{}/{}?versionId={}'.format(source_bucket, source_key, version),
            'Bucket': dest_bucket,
            'Key': dest_key,
        }
        session.create_client('s3').copy_object(**args)
```

```
DEBUG:botocore.endpoint:Making request for <botocore.model.OperationModel object at 0x108220a50> (verify_ssl=True) with params: {'query_string': {}, 'headers': {u'x-amz-copy-source': '/source_bucket/source_key%3FversionId%3DabCdeCghIjkLmnOp'}, 'url_path': u'/dest_bucket/dest_key', 'body': '', 'method': u'PUT'}
...
botocore.exceptions.ClientError: An error occurred (NoSuchKey) when calling the CopyObject operation: The specified key does not exist.
```

Is there another way to do this?

Pull request description:
Only quote path component of url for copy source
Fixes #509 

Fix is in https://github.com/boto/botocore/commit/5e568c5fa53e9bb8ec9b8809e1b53de271485511, there's an additional commit that cleans up some of the flake8 issues.

cc @kyleknap @mtdowling

Patch:
diff --git a/botocore/docs/bcdoc/style.py b/botocore/docs/bcdoc/style.py
index deefb136d..6e30f0007 100644
--- a/botocore/docs/bcdoc/style.py
+++ b/botocore/docs/bcdoc/style.py
@@ -151,7 +151,7 @@ class ReSTStyle(BaseStyle):
 
     def end_p(self):
         if self.do_p:
-            self.doc.write('\n\n%s' % self.spaces())
+            self.doc.write('\n\n')
 
     def start_code(self, attrs=None):
         self.doc.do_translation = True
diff --git a/botocore/docs/method.py b/botocore/docs/method.py
index d5fdce37b..43724abbc 100644
--- a/botocore/docs/method.py
+++ b/botocore/docs/method.py
@@ -159,7 +159,7 @@ def document_model_driven_method(section, method_name, operation_model,
     # Add the example section.
     example_section = section.add_new_section('example')
     example_section.style.new_paragraph()
-    example_section.style.bold('Request Syntax')
+    example_section.style.bold('Example')
     if operation_model.input_shape:
         RequestExampleDocumenter(
             service_name=operation_model.service_model.service_name,
@@ -196,7 +196,7 @@ def document_model_driven_method(section, method_name, operation_model,
         # Add an example return value
         return_example_section = return_section.add_new_section('example')
         return_example_section.style.new_line()
-        return_example_section.style.bold('Response Syntax')
+        return_example_section.style.bold('Response Example')
         return_example_section.style.new_paragraph()
         ResponseExampleDocumenter(
             service_name=operation_model.service_model.service_name,
diff --git a/botocore/handlers.py b/botocore/handlers.py
index c756f53e6..d1137fa26 100644
--- a/botocore/handlers.py
+++ b/botocore/handlers.py
@@ -42,7 +42,6 @@ REGISTER_FIRST = object()
 REGISTER_LAST = object()
 
 
-
 def check_for_200_error(response, **kwargs):
     # From: http://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectCOPY.html
     # There are two opportunities for a copy request to return an error. One
@@ -66,8 +65,8 @@ def check_for_200_error(response, **kwargs):
     http_response, parsed = response
     if _looks_like_special_case_error(http_response):
         logger.debug("Error found for response with 200 status code, "
-                        "errors: %s, changing status code to "
-                        "500.", parsed)
+                     "errors: %s, changing status code to "
+                     "500.", parsed)
         http_response.status_code = 500
 
 
@@ -110,7 +109,7 @@ def json_decode_template_body(parsed, **kwargs):
 
 def calculate_md5(params, **kwargs):
     request_dict = params
-    if request_dict['body'] and not 'Content-MD5' in params['headers']:
+    if request_dict['body'] and 'Content-MD5' not in params['headers']:
         md5 = hashlib.md5()
         md5.update(six.b(params['body']))
         value = base64.b64encode(md5.digest()).decode('utf-8')
@@ -207,8 +206,13 @@ def add_expect_header(model, params, **kwargs):
 def quote_source_header(params, **kwargs):
     if params['headers'] and 'x-amz-copy-source' in params['headers']:
         value = params['headers']['x-amz-copy-source']
-        params['headers']['x-amz-copy-source'] = quote(
-            value.encode('utf-8'), '/~')
+        p = urlsplit(value)
+        # We only want to quote the path.  If the user specified
+        # extra parts, say '?versionId=myversionid' then that part
+        # should not be quoted.
+        quoted = quote(p[2].encode('utf-8'), '/~')
+        final_source = urlunsplit((p[0], p[1], quoted, p[3], p[4]))
+        params['headers']['x-amz-copy-source'] = final_source
 
 
 def copy_snapshot_encrypted(params, request_signer, **kwargs):
@@ -270,7 +274,8 @@ def _decode_policy_types(parsed, shape):
             if member_shape.type_name == 'string' and \
                     member_shape.name == shape_name and \
                     member_name in parsed:
-                parsed[member_name] = decode_quoted_jsondoc(parsed[member_name])
+                parsed[member_name] = decode_quoted_jsondoc(
+                    parsed[member_name])
             elif member_name in parsed:
                 _decode_policy_types(parsed[member_name], member_shape)
     if shape.type_name == 'list':
@@ -342,8 +347,8 @@ def inject_account_id(params, **kwargs):
 
 def add_glacier_version(model, params, **kwargs):
     request_dict = params
-    request_dict['headers']['x-amz-glacier-version'] = \
-            model.metadata['apiVersion']
+    request_dict['headers']['x-amz-glacier-version'] = model.metadata[
+        'apiVersion']
 
 
 def add_glacier_checksums(params, **kwargs):


Test patch:
diff --git a/tests/unit/docs/bcdoc/test_style.py b/tests/unit/docs/bcdoc/test_style.py
index e66c2a500..5c75d8703 100644
--- a/tests/unit/docs/bcdoc/test_style.py
+++ b/tests/unit/docs/bcdoc/test_style.py
@@ -52,13 +52,6 @@ class TestStyle(unittest.TestCase):
         style.italics('foobar')
         self.assertEqual(style.doc.getvalue(), six.b('*foobar* '))
 
-    def test_p(self):
-        style = ReSTStyle(ReSTDocument())
-        style.start_p()
-        style.doc.write('foo')
-        style.end_p()
-        self.assertEqual(style.doc.getvalue(), six.b('\n\nfoo\n\n'))
-
     def test_code(self):
         style = ReSTStyle(ReSTDocument())
         style.code('foobar')
diff --git a/tests/unit/docs/test_client.py b/tests/unit/docs/test_client.py
index 01b07f4b8..5613e3c76 100644
--- a/tests/unit/docs/test_client.py
+++ b/tests/unit/docs/test_client.py
@@ -39,7 +39,7 @@ class TestClientDocumenter(BaseDocsTest):
             '  .. py:method:: get_paginator(operation_name)',
             '  .. py:method:: get_waiter(waiter_name)',
             '  .. py:method:: sample_operation(Biz=None)',
-            '    **Request Syntax**',
+            '    **Example**',
             '    ::',
             '      response = client.sample_operation(',
             '          Biz=\'string\'',
@@ -48,7 +48,7 @@ class TestClientDocumenter(BaseDocsTest):
             '    :param Biz:',
             '    :rtype: dict',
             '    :returns:',
-            '      **Response Syntax**',
+            '      **Response Example**',
             '      ::',
             '        {',
             '            \'Biz\': \'string\'',
diff --git a/tests/unit/docs/test_method.py b/tests/unit/docs/test_method.py
index f466f4abe..0d7e10e90 100644
--- a/tests/unit/docs/test_method.py
+++ b/tests/unit/docs/test_method.py
@@ -97,7 +97,7 @@ class TestDocumentModelDrivenMethod(BaseDocsTest):
         self.assert_contains_lines_in_order([
             '.. py:method:: foo(Bar=None)',
             '  This describes the foo method.',
-            '  **Request Syntax**',
+            '  **Example**',
             '  ::',
             '    response = client.foo(',
             '        Bar=\'string\'',
@@ -106,7 +106,7 @@ class TestDocumentModelDrivenMethod(BaseDocsTest):
             '  :param Bar:',
             '  :rtype: dict',
             '  :returns:',
-            '    **Response Syntax**',
+            '    **Response Example**',
             '    ::',
             '      {',
             '          \'Bar\': \'string\'',
@@ -128,7 +128,7 @@ class TestDocumentModelDrivenMethod(BaseDocsTest):
         self.assert_contains_lines_in_order([
             '.. py:method:: foo()',
             '  This describes the foo method.',
-            '  **Request Syntax**',
+            '  **Example**',
             '  ::',
             '    response = client.foo()',
             '  :returns: None',
@@ -149,7 +149,7 @@ class TestDocumentModelDrivenMethod(BaseDocsTest):
         self.assert_contains_lines_in_order([
             '.. py:method:: foo(Bar=None, Biz=None)',
             '  This describes the foo method.',
-            '  **Request Syntax**',
+            '  **Example**',
             '  ::',
             '    response = client.foo(',
             '        Bar=\'string\',',
@@ -161,7 +161,7 @@ class TestDocumentModelDrivenMethod(BaseDocsTest):
             '  :param Biz: biz docs',
             '  :rtype: dict',
             '  :returns:',
-            '    **Response Syntax**',
+            '    **Response Example**',
             '    ::',
             '      {',
             '          \'Bar\': \'string\'',
@@ -186,7 +186,7 @@ class TestDocumentModelDrivenMethod(BaseDocsTest):
         self.assert_contains_lines_in_order([
             '.. py:method:: foo(Bar=None)',
             '  This describes the foo method.',
-            '  **Request Syntax**',
+            '  **Example**',
             '  ::',
             '    response = client.foo(',
             '        Bar=\'string\'',
@@ -195,7 +195,7 @@ class TestDocumentModelDrivenMethod(BaseDocsTest):
             '  :param Bar:',
             '  :rtype: dict',
             '  :returns:',
-            '    **Response Syntax**',
+            '    **Response Example**',
             '    ::',
             '      {',
             '          \'Bar\': \'string\'',
@@ -219,7 +219,7 @@ class TestDocumentModelDrivenMethod(BaseDocsTest):
         self.assert_contains_lines_in_order([
             '.. py:method:: foo(Biz=None)',
             '  This describes the foo method.',
-            '  **Request Syntax**',
+            '  **Example**',
             '  ::',
             '    response = client.foo(',
             '        Biz=\'string\'',
@@ -228,7 +228,7 @@ class TestDocumentModelDrivenMethod(BaseDocsTest):
             '  :param Biz:',
             '  :rtype: dict',
             '  :returns:',
-            '    **Response Syntax**',
+            '    **Response Example**',
             '    ::',
             '      {',
             '          \'Bar\': \'string\'',
@@ -256,7 +256,7 @@ class TestDocumentModelDrivenMethod(BaseDocsTest):
         self.assert_contains_lines_in_order([
             '.. py:method:: foo(Bar=None, Biz=None)',
             '  This describes the foo method.',
-            '  **Request Syntax**',
+            '  **Example**',
             '  ::',
             '    response = client.foo(',
             '        Bar=\'string\'',
@@ -266,7 +266,7 @@ class TestDocumentModelDrivenMethod(BaseDocsTest):
             '  :param Biz:',
             '  :rtype: dict',
             '  :returns:',
-            '    **Response Syntax**',
+            '    **Response Example**',
             '    ::',
             '      {',
             '          \'Biz\': \'string\'',
diff --git a/tests/unit/docs/test_paginator.py b/tests/unit/docs/test_paginator.py
index 583418c88..73b1e0aef 100644
--- a/tests/unit/docs/test_paginator.py
+++ b/tests/unit/docs/test_paginator.py
@@ -42,7 +42,7 @@ class TestPaginatorDocumenter(BaseDocsTest):
             '  .. py:method:: paginate(Biz=None, PaginationConfig=None)',
             ('    Creates an iterator that will paginate through responses'
              ' from :py:meth:`myservice.Client.sample_operation`.'),
-            '    **Request Syntax**',
+            '    **Example**',
             '    ::',
             '      response_iterator = paginator.paginate(',
             '          Biz=\'string\',',
@@ -63,7 +63,7 @@ class TestPaginatorDocumenter(BaseDocsTest):
             '      - **StartingToken** *(string) --*',
             '    :rtype: dict',
             '    :returns:',
-            '      **Response Syntax**',
+            '      **Response Example**',
             '      ::',
             '        {',
             '            \'Biz\': \'string\',',
diff --git a/tests/unit/docs/test_waiter.py b/tests/unit/docs/test_waiter.py
index cf6919c85..4af246219 100644
--- a/tests/unit/docs/test_waiter.py
+++ b/tests/unit/docs/test_waiter.py
@@ -40,7 +40,7 @@ class TestWaiterDocumenter(BaseDocsTest):
             ('    This polls :py:meth:`myservice.Client.sample_operation` '
              'every 15 seconds until a successful state is reached. An error '
              'is returned after 40 failed checks.'),
-            '    **Request Syntax**',
+            '    **Example**',
             '    ::',
             '      waiter.wait(',
             '          Biz=\'string\'',
diff --git a/tests/unit/test_handlers.py b/tests/unit/test_handlers.py
index 6331e0817..c7e29fff8 100644
--- a/tests/unit/test_handlers.py
+++ b/tests/unit/test_handlers.py
@@ -61,6 +61,22 @@ class TestHandlers(BaseSessionTest):
             self.assertEqual(
                 params['headers']['x-amz-copy-source'], 'foo%2B%2Bbar.txt')
 
+    def test_only_quote_url_path_not_query_string(self):
+        request = {
+            'headers': {'x-amz-copy-source': '/foo/bar++baz?versionId=123'}
+        }
+        handlers.quote_source_header(request)
+        self.assertEqual(request['headers']['x-amz-copy-source'],
+                         '/foo/bar%2B%2Bbaz?versionId=123')
+
+    def test_quote_source_header_needs_no_changes(self):
+        request = {
+            'headers': {'x-amz-copy-source': '/foo/bar?versionId=123'}
+        }
+        handlers.quote_source_header(request)
+        self.assertEqual(request['headers']['x-amz-copy-source'],
+                         '/foo/bar?versionId=123')
+
     def test_presigned_url_already_present(self):
         params = {'body': {'PresignedUrl': 'https://foo'}}
         handlers.copy_snapshot_encrypted(params, None)