Issue:
It's hard to report errors with the current API. Errors are being accidentally ignored, and some errors are never checked (perhaps because it was too much effort?).
Diagnosis:
The current callback returns bool of whether to continue parsing, rather than our typical int/AWS_OP_SUCCESS/aws_raise_error()error handling.
This seems like a simple design. But the inconsistency in return type leads to errors being mistakenly swallowed. And it makes it hard when you do want to "bubble up" an error from the callback. Callbacks needs to store a custom error_code in their user_data to report an error. Most callbacks never bothered to do this, maybe because it was extra work?
Description of changes:
XML traversal callback returns int instead of bool.
If a callback fails, the whole parse() fails.
You can no longer stop parsing without causing failure. But in nearly all use-cases we were stopping due to error. I found 1 case where we stopped because we found what we were looking for. But in this case, it didn't really hurt to continue parsing. The reduced complexity seemed worth the change.
Remove ~aws_xml_parser_new(), aws_xml_parser_destroy(), aws_xml_parser_parse()~, replace with aws_xml_parse().
The new() and destroy() calls were unnecessary. Removing them simplifies use.
aws_xml_node_get_name() just returns aws_byte_cursor(), instead of ~int~
This can't fail. Changing the signature simplifies use.
Raise AWS_ERROR_INVALID_XML instead of ~AWS_ERROR_MALFORMED_INPUT_STRING~
This change is more wishy-washy. It seemed useful to get this new, more specific, error code if it bubbling up from deep within some larger operation, like an S3 meta-request.
API BREAK:
We don't know any external uses of this API, so it seems safe to change. The API is only intended for internal use by the aws-c libraries, which are being fixed up now. This API was quickly written as private code in aws-c-auth (https://github.com/awslabs/aws-c-auth/pull/40), then moved to public in aws-c-common (https://github.com/awslabs/aws-c-common/pull/674) when aws-c-s3 also needed to parse XML. The fact that it was originally private is why this API didn't get more scrutiny originally.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Issue: It's hard to report errors with the current API. Errors are being accidentally ignored, and some errors are never checked (perhaps because it was too much effort?).
Diagnosis: The current callback returns
boolof whether to continue parsing, rather than our typicalint/AWS_OP_SUCCESS/aws_raise_error()error handling.This seems like a simple design. But the inconsistency in return type leads to errors being mistakenly swallowed. And it makes it hard when you do want to "bubble up" an error from the callback. Callbacks needs to store a custom
error_codein theiruser_datato report an error. Most callbacks never bothered to do this, maybe because it was extra work?Description of changes:
intinstead ofbool.aws_xml_parse().aws_xml_node_get_name()just returnsaws_byte_cursor(), instead of ~int~AWS_ERROR_INVALID_XMLinstead of ~AWS_ERROR_MALFORMED_INPUT_STRING~API BREAK: We don't know any external uses of this API, so it seems safe to change. The API is only intended for internal use by the aws-c libraries, which are being fixed up now. This API was quickly written as private code in aws-c-auth (https://github.com/awslabs/aws-c-auth/pull/40), then moved to public in aws-c-common (https://github.com/awslabs/aws-c-common/pull/674) when aws-c-s3 also needed to parse XML. The fact that it was originally private is why this API didn't get more scrutiny originally.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.