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

Fix issue with rest-xml serialization #495

Merged
merged 2 commits into from
Mar 24, 2015

Conversation

kyleknap
Copy link
Contributor

If a parameter was a payload and that was something empty like an empty dictionary, it would get serialized to an empty body but really needs to get serialized to an empty xml structure. This caused issues when trying to turn off logging or notification for s3. Here is an example of what happens:

>>> import botocore.session
>>> session = botocore.session.get_session()
>>> s3 = session.create_client('s3')
>>> s3.put_bucket_logging(Bucket='mybucket', BucketLoggingStatus={})
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "botocore/client.py", line 187, in _api_call
    return self._make_api_call(operation_name, kwargs)
  File "botocore/client.py", line 242, in _make_api_call
    raise ClientError(parsed_response, operation_name)
botocore.exceptions.ClientError: An error occurred (MalformedXML) when calling the PutBucketLogging operation: The XML you provided was not well-formed or did not validate against our published schema

From the CLI, here is a snippet of what we used to send over

DEBUG - Making request for <botocore.model.OperationModel object at 0x1044ac590> 
(verify_ssl=True) with params: {'query_string': {}, 'headers': {}, 'url_path': u'/mybucket?logging', 'body': '',
 'method': u'PUT'}

Note it is correctly this now:

DEBUG - Making request for <botocore.model.OperationModel object at 0x10cda6f10> (verify_ssl=True)
 with params: {'query_string': {}, 'headers': {}, 'url_path': u'/mybucket?logging', 'body': 
u'<BucketLoggingStatus xmlns="http://s3.amazonaws.com/doc/2006-03-01/" />', 'method': u'PUT'}

Here is reference to the s3 api docs as well: http://docs.aws.amazon.com/AmazonS3/latest/API/RESTBucketPUTlogging.html

cc @jamesls @danielgtaylor

If a parameter was a payload and that was something empty like an empty
dictionary, it would get serialized to an empty body but really needs to
get serialized to an empty xml structure. This caused issues when trying
to turn off logging or notification for s3.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.6% when pulling a74a471 on kyleknap:empty-structure-s3 into c7ad6a8 on boto:develop.

@danielgtaylor
Copy link
Member

LGTM 🚢-it.

Have you looked around at other calls to determine if it's easy to accidentally delete other values? E.g. setting {} as the body instead of None where before it did nothing but now deletes some configuration? I can't really think of any reason anybody would be doing this, but we should just double check.

@kyleknap
Copy link
Contributor Author

Yeah, I will take a look and see if there is anywhere it could be problematic.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.6% when pulling 4cd5a43 on kyleknap:empty-structure-s3 into c7ad6a8 on boto:develop.

@kyleknap
Copy link
Contributor Author

@danielgtaylor
I did an audit of services that would use this code path and I did not see any operations for which the scenario you are concerned about would happen.

Also, I added a test to the protocol test to make sure there is a difference between an empty dictionary and None.

@danielgtaylor
Copy link
Member

LGTM 🚢-it!

@jamesls
Copy link
Member

jamesls commented Mar 23, 2015

:shipit: Looks good.

kyleknap added a commit that referenced this pull request Mar 24, 2015
Fix issue with rest-xml serialization
@kyleknap kyleknap merged commit 54c4615 into boto:develop Mar 24, 2015
@kyleknap kyleknap deleted the empty-structure-s3 branch March 24, 2015 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants