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.

Sign up for GitHub

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

Jump to bottom

Simplify error handling in XML API - BREAKING CHANGE #1043

Merged
merged 4 commits into from
Jul 18, 2023
Merged

Simplify error handling in XML API - BREAKING CHANGE #1043

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c4b49fb
Simplify XML API
graebm Jul 11, 2023
fa5be12
Remove ability to stop parsing without raising error. No one is using…
graebm Jul 11, 2023
fdcc2fd
Add AWS_ERROR_INVALID_XML error code.
graebm Jul 13, 2023
6f65d7d
Merge branch 'main' into xml-error-handling
graebm Jul 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions include/aws/common/error.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ enum aws_common_error {
AWS_ERROR_PLATFORM_NOT_SUPPORTED,
AWS_ERROR_INVALID_UTF8,
AWS_ERROR_GET_HOME_DIRECTORY_FAILED,
AWS_ERROR_INVALID_XML,
AWS_ERROR_END_COMMON_RANGE = AWS_ERROR_ENUM_END_RANGE(AWS_C_COMMON_PACKAGE_ID)
};

Expand Down
2 changes: 1 addition & 1 deletion include/aws/common/private/xml_parser_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include <aws/common/xml_parser.h>

struct aws_xml_node {
struct aws_xml_parser *parser;
struct aws_byte_cursor name;
struct aws_array_list attributes;
struct aws_byte_cursor doc_at_body;
Expand All @@ -25,7 +26,6 @@ struct aws_xml_parser {
struct aws_byte_cursor split_scratch[11];
size_t max_depth;
int error;
bool stop_parsing;
};

#endif /* AWS_COMMON_PRIVATE_XML_PARSER_IMPL_H */
47 changes: 16 additions & 31 deletions include/aws/common/xml_parser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@

AWS_PUSH_SANE_WARNING_LEVEL

struct aws_xml_parser;
struct aws_xml_node;

struct aws_xml_attribute {
Expand All @@ -24,62 +23,51 @@ struct aws_xml_attribute {
/**
* Callback for when an xml node is encountered in the document. As a user you have a few options:
*
* 1. reject the document parsing at this point by returning false. This will immediately stop doc parsing.
* 1. fail the parse by returning AWS_OP_ERR (after an error has been raised). This will stop any further parsing.
* 2. call aws_xml_node_traverse() on the node to descend into the node with a new callback and user_data.
* 3. call aws_xml_node_as_body() to retrieve the contents of the node as text.
*
* You MUST NOT call both aws_xml_node_traverse() and aws_xml_node_as_body() on the same node.
*
* 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


struct aws_xml_parser_options {
/* xml document to parse. */
struct aws_byte_cursor doc;

/* Max node depth used for parsing document. */
size_t max_depth;
};

AWS_EXTERN_C_BEGIN
/* Callback invoked on the root node */
aws_xml_parser_on_node_encountered_fn *on_root_encountered;

/**
* Allocates an xml parser.
*/
AWS_COMMON_API
struct aws_xml_parser *aws_xml_parser_new(
struct aws_allocator *allocator,
const struct aws_xml_parser_options *options);
/* User data for callback */
void *user_data;
};

/*
* De-allocates an xml parser.
*/
AWS_COMMON_API
void aws_xml_parser_destroy(struct aws_xml_parser *parser);
AWS_EXTERN_C_BEGIN

/**
* Parse the doc until the end or until a callback rejects the document.
* on_node_encountered will be invoked when the root node is encountered.
* Parse an XML document.
* WARNING: This is not a public API. It is only intended for use within the aws-c libraries.
*/
AWS_COMMON_API
int aws_xml_parser_parse(
struct aws_xml_parser *parser,
aws_xml_parser_on_node_encountered_fn *on_node_encountered,
void *user_data);
int aws_xml_parse(struct aws_allocator *allocator, const struct aws_xml_parser_options *options);

/**
* Writes the contents of the body of node into out_body. out_body is an output parameter in this case. Upon success,
* out_body will contain the body of the node.
*/
AWS_COMMON_API
int aws_xml_node_as_body(struct aws_xml_parser *parser, struct aws_xml_node *node, struct aws_byte_cursor *out_body);
int aws_xml_node_as_body(struct aws_xml_node *node, struct aws_byte_cursor *out_body);

/**
* Traverse node and invoke on_node_encountered when a nested node is encountered.
*/
AWS_COMMON_API
int aws_xml_node_traverse(
struct aws_xml_parser *parser,
struct aws_xml_node *node,
aws_xml_parser_on_node_encountered_fn *on_node_encountered,
void *user_data);
Expand All @@ -88,7 +76,7 @@ int aws_xml_node_traverse(
* Get the name of an xml node.
*/
AWS_COMMON_API
int aws_xml_node_get_name(const struct aws_xml_node *node, struct aws_byte_cursor *out_name);
struct aws_byte_cursor aws_xml_node_get_name(const struct aws_xml_node *node);

/*
* Get the number of attributes for an xml node.
Expand All @@ -100,10 +88,7 @@ size_t aws_xml_node_get_num_attributes(const struct aws_xml_node *node);
* Get an attribute for an xml node by its index.
*/
AWS_COMMON_API
int aws_xml_node_get_attribute(
const struct aws_xml_node *node,
size_t attribute_index,
struct aws_xml_attribute *out_attribute);
struct aws_xml_attribute aws_xml_node_get_attribute(const struct aws_xml_node *node, size_t attribute_index);

AWS_EXTERN_C_END
AWS_POP_SANE_WARNING_LEVEL
Expand Down
3 changes: 3 additions & 0 deletions source/common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,9 @@ static struct aws_error_info errors[] = {
AWS_DEFINE_ERROR_INFO_COMMON(
AWS_ERROR_GET_HOME_DIRECTORY_FAILED,
"Failed to get home directory"),
AWS_DEFINE_ERROR_INFO_COMMON(
AWS_ERROR_INVALID_XML,
"Invalid XML document"),
};
/* clang-format on */

Expand Down
Loading