Parse empty XML string nodes as an empty Python string

[danielgtaylor]

This changes the output of the XML-based parsers and adds protocol tests for
each of query, ec2, and rest-xml. This is required to support new
waiters as in aws/aws-cli#1252.

cc @jamesls @kyleknap do either of you know why the code had a custom string
handler? Everything seems to work fine without it and removing it fixes the CLI
bug linked above.

[jamesls]

I'm guessing it was just a mistake to add a custom string handler. We even have a comment that says that we want to have an empty string (https://github.com/boto/botocore/blob/develop/botocore/parsers.py#L144-L146).

However, this is causing multiple test failures in the old response parsing tests.

I would actually be in favor removing those tests only if:

• There is no drop in coverage (including branch coverage). This would mean that this case is already covered in a protocol test.
• If there is a drop in coverage, if we port the service specific test over to a protocol test, then it should be safe to remove the older test.

Once we get those failing tests in order, the code change itself looks good.

Even though this is a "bug fix", this does have the potential to affect a lot of responses that were previously returning None that I hadn't considered. What are your thoughts on that? (cc @kyleknap )

[kyleknap]

I am sort of hesitant on making this change. I feel that this has potential to break a fair amount of users (one of the more obvious scenarios being if statements where users may have been checking for None instead of using not).

If we make this change, I would feel more comfortable about it if we ran all of the unit and integration tests (especially in the CLI, I do not know if there are any customizations relying on it).

As to switching the response parse test to protocol test, I am for that. However, can we make this change on the clients-only branch. I have a feeling if this gets merged in the develop branch there may be some merge conflicts when I rebase clients-only.

[jamesls]

@kyleknap were the changes to the response parsing tests in the clients only branch? I'd only expect conflicts if we messed with those files in those branches.

Otherwise to get the tests passing we'd have to fix all those tests to expect an empty string instead of None.

[kyleknap]

I was actually referring to possible conflicts in the protocol tests. I think I only changed the input files but I may have touched the output files. I am not sure.

The only thing I changed in the response parsing test was the main python test script to be compatible with clients. I did not touch any of the individual response parsing test cases.

[danielgtaylor]

@kyleknap @jamesls it seems like we're still not sure whether to make this change because it's going to potentially break people? Do we still want to do this or would you rather I make a customization to just update the broken waiter?

If we decide to move forward on this PR, then I'll update the broken tests and clean it up.

[kyleknap]

@danielgtaylor

I would say we add it if the integration test pass (across the various projects) with this PR. This will make sure that none of the higher level features are broken. If the test pass, I would be fine with going forward with the PR. Empty string is the correct value. I feel that it is better to fix this now instead of keeping it because we cannot change it once we go GA. If the test do not pass, I say we make a decision based on what breaks (as it can help us get a sense of how much it may be used).

[danielgtaylor]

@jamesls @kyleknap alright, I've rebased on develop, fixed the response parsing tests, and ran integration tests for Botocore/CLI/Boto3 and all are passing with these changes. Our customizations don't appear to be affected, however it's important to note that any scripts parsing the output of the CLI (or using the --query argument) have the potential to break if they are looking for null in the output. We should make sure to call this out in the associated package changelogs.

[coveralls]

Coverage decreased (-0.0%) to 94.63% when pulling 34c03b5 on danielgtaylor:xml-empty-str into 19b051f on boto:develop.

[jamesls]

@danielgtaylor Would you mind updating the CLI changelog with this change. I agree that we should call out this bug fix has potential for breakage with --query.