Simplify error handling in XML API - BREAKING CHANGE #1043
Conversation
… this. Less code is best code.
More informative than AWS_ERROR_MALFORMED_INPUT_STRING if it boils up from some deep internal system.
| * return true to continue the parsing operation. | ||
| */ | ||
| typedef bool( | ||
| aws_xml_parser_on_node_encountered_fn)(struct aws_xml_parser *parser, struct aws_xml_node *node, void *user_data); | ||
| typedef int(aws_xml_parser_on_node_encountered_fn)(struct aws_xml_node *node, void *user_data); |
There was a problem hiding this comment.
So, if user wants to just stop parsing without error, they cannot do it now?
I guess we still have use case like I found the response I want and, now we can just stop parsing and it's not an error?
There was a problem hiding this comment.
Oh, I saw the description.
To me, I feel like the complexity is not too bad, just one more boolean, while it helps to improve performance? I just don't like the idea of having to do the extra parsing.
There was a problem hiding this comment.
Yes. Only errors can stop the parser now.
I found that in all cases but one, we were returning false because an error happened.
My 1st pass did have an extra bool *stop_parsing param that the callback could set, but I got sick of ignoring the param ((void)stop_parsing;) in every callback. Also, the stop_parsing code paths were not well tested. I felt better just removing the feature.
It was only used in the aws_xml_get_top_level_tag() functions. I changed it so its callback just immediately returns if its called again after finding the one thing it's looking for.
But in most cases we're gathering data from a few elements (e.g. credentials provider needs access key, secret, expiration, etc) and didn't use it
**Issue** The XML API was hard to use right, leading to bugs like this: #328 **Description of changes:** - Adapt to API changes from: awslabs/aws-c-common#1043 - Break up node traversal functions, to ensure we're processing the correct XML elements. - Previously, the same callback would be used for all XML elements. This could cause error if an element with the same name occurred at different parts of the document tree. - Improved error checking - Previously, many calls to `aws_xml_node_as_body()` weren't being checked for error. - Replace ~aws_xml_get_top_level_tag()~ and ~aws_xml_get_top_level_tag_with_root_name()~ with `aws_xml_get_body_at_path()` - ~aws_xml_get_top_level_tag()~ didn't check the name of the root node - ~aws_xml_get_top_level_tag_with_root_name()~ was clunky to use (IMHO) - so replace with an API that can retrieve an element at any depth (not just 2), checking names the whole way, and with a nicer API (IMHO). - new function gives `aws_byte_cursor` instead of `aws_string`, the user was usually just deleting it afterwards, which made their error-handling more complicated. - Trivial stuff: - Remove unused functions ~aws_s3_list_objects_operation_new()~ and ~aws_s3_initiate_list_parts()~ - `aws_replace_quote_entities()` returns `aws_byte_buf` by value, instead of as out-param - Some functions take `aws_byte_cursor` by value, instead taking `aws_string *` or `aws_byte_buf *` or `aws_byte_cursor *` by pointer
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_parser_new(), aws_xml_parser_destroy(), aws_xml_parser_parse(), replace withaws_xml_parse().aws_xml_node_get_name()just returnsaws_byte_cursor(), instead ofintAWS_ERROR_INVALID_XMLinstead ofAWS_ERROR_MALFORMED_INPUT_STRINGAPI 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 (awslabs/aws-c-auth#40), then moved to public in aws-c-common (#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.