[discident-glib] ean: Better errors when parsing XML responses from Amazon

[Bastien Nocera <hadess src gnome org>]

commit 45de8ecb4a571f43c63266d3032b2d052268d7c1
Author: Bastien Nocera <hadess hadess net>
Date:   Mon Dec 21 23:10:52 2015 +0100

    ean: Better errors when parsing XML responses from Amazon
    
    Propagate the error messages from Amazon requests, to make debugging
    easier. This includes tests for each of the error cases.

 discident-glib/Makefile.am                  |    2 +-
 discident-glib/amz-response-error.xml       |    1 +
 discident-glib/discident-ean-amz-glib.c     |   56 ++++++++++++++++++++-------
 discident-glib/discident-ean-glib.h         |    1 +
 discident-glib/discident-ean-private-glib.h |    3 +-
 discident-glib/test-diglib.c                |   37 +++++++++++++++++-
 6 files changed, 83 insertions(+), 17 deletions(-)
---
diff --git a/discident-glib/Makefile.am b/discident-glib/Makefile.am
index e78f695..aaea605 100644
--- a/discident-glib/Makefile.am
+++ b/discident-glib/Makefile.am
@@ -1,6 +1,6 @@
 include $(top_srcdir)/Makefile.decl
 
-EXTRA_DIST = discident-glib.symbols rl-response.xml amz-response.xml
+EXTRA_DIST = discident-glib.symbols rl-response.xml amz-response.xml amz-response-error.xml
 BUILT_GIRSOURCES =
 
 lib_LTLIBRARIES = libdiscident-glib.la
diff --git a/discident-glib/amz-response-error.xml b/discident-glib/amz-response-error.xml
new file mode 100644
index 0000000..73a899a
--- /dev/null
+++ b/discident-glib/amz-response-error.xml
@@ -0,0 +1 @@
+<?xml version="1.0" ?><ItemLookupResponse 
xmlns="http://webservices.amazon.com/AWSECommerceService/2011-08-01";><OperationRequest><RequestId>b54b29b4-5477-4ae9-8b17-ef1df2e21768</RequestId><Arguments><Argument
 Name="AWSAccessKeyId" Value="AKIAINXKEW45HPDO6CBA"></Argument><Argument Name="AssociateTag" 
Value="hadessnet0b-21"></Argument><Argument Name="IdType" Value="EAN"></Argument><Argument Name="ItemId" 
Value="2253101125"></Argument><Argument Name="Operation" Value="ItemLookup"></Argument><Argument 
Name="ResponseGroup" Value="ItemAttributes,Images"></Argument><Argument Name="SearchIndex" 
Value="All"></Argument><Argument Name="Service" Value="AWSECommerceService"></Argument><Argument 
Name="Signature" Value="0m/MWiMSS4/ck9Fj4fCADOknKekzmiILuEX21E30Oq0="></Argument><Argument Name="Timestamp" 
Value="2015-12-21T21:54:17Z"></Argument></Arguments><RequestProcessingTime>0.0338240000000000</RequestProcessingTime></OperationRequest><Items><Request><IsValid>True</IsValid><ItemLookupRequ
 
est><IdType>EAN</IdType><ItemId>2253101125</ItemId><ResponseGroup>ItemAttributes</ResponseGroup><ResponseGroup>Images</ResponseGroup><SearchIndex>All</SearchIndex><VariationPage>All</VariationPage></ItemLookupRequest><Errors><Error><Code>AWS.InvalidParameterValue</Code><Message>2253101125
 is not a valid value for ItemId. Please change this value and retry your 
request.</Message></Error></Errors></Request></Items></ItemLookupResponse>
diff --git a/discident-glib/discident-ean-amz-glib.c b/discident-glib/discident-ean-amz-glib.c
index 0fb2349..1bdfd18 100644
--- a/discident-glib/discident-ean-amz-glib.c
+++ b/discident-glib/discident-ean-amz-glib.c
@@ -234,7 +234,8 @@ get_value_for_xpath (xmlDocPtr           doc,
        xpath_obj = xmlXPathEvalExpression (BAD_CAST (path), xpath_ctx);
        if (xpath_obj == NULL)
                return NULL;
-       if (xpath_obj->nodesetval == NULL) {
+       if (xpath_obj->nodesetval == NULL ||
+           xpath_obj->nodesetval->nodeTab == NULL) {
                xmlXPathFreeObject (xpath_obj);
                return NULL;
        }
@@ -248,13 +249,19 @@ get_value_for_xpath (xmlDocPtr           doc,
 gboolean
 _amz_parse_lookup_response (const char  *response,
                            char       **ret_title,
-                           char       **ret_img_url)
+                           char       **ret_img_url,
+                           GError     **error)
 {
        xmlDocPtr doc;
        xmlXPathContextPtr xpath_ctx;
+       char *error_msg;
+       gboolean ret = FALSE;
 
-       if (response == NULL)
-               return FALSE;
+       if (response == NULL) {
+               g_set_error_literal (error, DISCIDENT_ERROR, DISCIDENT_ERROR_PARSE,
+                                    "Empty response");
+               return ret;
+       }
 
        doc = xmlParseMemory (response, strlen (response));
        if (doc == NULL)
@@ -266,7 +273,9 @@ _amz_parse_lookup_response (const char  *response,
           g_ascii_strcasecmp ((char *)doc->children->name, "ItemLookupResponse") != 0) {
                if (doc != NULL)
                        xmlFreeDoc (doc);
-               return FALSE;
+               g_set_error_literal (error, DISCIDENT_ERROR, DISCIDENT_ERROR_PARSE,
+                                    "Could not parse XML in response");
+               return ret;
        }
 
        xpath_ctx = xmlXPathNewContext(doc);
@@ -274,17 +283,29 @@ _amz_parse_lookup_response (const char  *response,
        /* Register namespace */
        xmlXPathRegisterNs (xpath_ctx, BAD_CAST ("ns"), BAD_CAST (RESPONSE_NAMESPACE));
 
+       error_msg = get_value_for_xpath (doc, xpath_ctx, "//ns:Message");
+       if (error_msg != NULL) {
+               g_set_error_literal (error, DISCIDENT_ERROR, DISCIDENT_ERROR_PARSE, error_msg);
+               g_free (error_msg);
+               goto bail;
+       }
+
        *ret_title = get_value_for_xpath (doc, xpath_ctx, "//ns:Title");
-       if (*ret_title == NULL)
+       if (*ret_title == NULL) {
+               g_set_error_literal (error, DISCIDENT_ERROR, DISCIDENT_ERROR_PARSE,
+                                    "No title found in response");
                goto bail;
+       }
 
        *ret_img_url = get_value_for_xpath (doc, xpath_ctx, "//ns:Item/ns:LargeImage/ns:URL");
 
+       ret = TRUE;
+
 bail:
        xmlXPathFreeContext(xpath_ctx);
        xmlFreeDoc (doc);
 
-       return TRUE;
+       return ret;
 }
 
 static SoupMessage *
@@ -336,9 +357,10 @@ discident_ean_amz_lookup_sync (DiscidentEan *ean,
                return FALSE;
        }
 
-       if (_amz_parse_lookup_response (response, title, img_url) == FALSE) {
+       if (_amz_parse_lookup_response (response, title, img_url, error) == FALSE) {
                g_free (response);
-               g_set_error (error, DISCIDENT_ERROR, DISCIDENT_ERROR_PARSE, "Failed to parse response from 
EAN service");
+               if (*error == NULL)
+                       g_set_error (error, DISCIDENT_ERROR, DISCIDENT_ERROR_PARSE, "Failed to parse response 
from EAN service");
                return FALSE;
        }
 
@@ -360,6 +382,7 @@ got_body_query (SoupMessage *msg,
        const char *barcode;
        char *title = NULL;
        char *img_url = NULL;
+       GError *error = NULL;
 
        if (!SOUP_STATUS_IS_SUCCESSFUL (msg->status_code) ||
            msg->response_body == NULL) {
@@ -371,11 +394,16 @@ got_body_query (SoupMessage *msg,
                goto out;
        }
 
-       if (_amz_parse_lookup_response (msg->response_body->data, &title, &img_url) == FALSE) {
-               g_simple_async_result_set_error (data->simple,
-                                                DISCIDENT_ERROR,
-                                                DISCIDENT_ERROR_PARSE,
-                                                "Failed to parse response from EAN service");
+       if (_amz_parse_lookup_response (msg->response_body->data, &title, &img_url, &error) == FALSE) {
+               if (error == NULL) {
+                       g_simple_async_result_set_error (data->simple,
+                                                        DISCIDENT_ERROR,
+                                                        DISCIDENT_ERROR_PARSE,
+                                                        "Failed to parse response from EAN service");
+               } else {
+                       g_simple_async_result_set_from_error (data->simple, error);
+                       g_error_free (error);
+               }
                goto out;
        }
 
diff --git a/discident-glib/discident-ean-glib.h b/discident-glib/discident-ean-glib.h
index c562c3a..84284eb 100644
--- a/discident-glib/discident-ean-glib.h
+++ b/discident-glib/discident-ean-glib.h
@@ -24,6 +24,7 @@
 #define DISCIDENT_EAN_GLIB_H
 
 #include <glib.h>
+#include <discident-error.h>
 
 G_BEGIN_DECLS
 
diff --git a/discident-glib/discident-ean-private-glib.h b/discident-glib/discident-ean-private-glib.h
index f2a386f..8c1f347 100644
--- a/discident-glib/discident-ean-private-glib.h
+++ b/discident-glib/discident-ean-private-glib.h
@@ -59,6 +59,7 @@ gboolean _rl_parse_lookup_response (const char  *response,
                                    char       **ret_img_url);
 gboolean _amz_parse_lookup_response (const char  *response,
                                     char       **ret_title,
-                                    char       **ret_img_url);
+                                    char       **ret_img_url,
+                                    GError     **error);
 
 #endif /* DISCIDENT_EAN_PRIVATE_GLIB_H */
diff --git a/discident-glib/test-diglib.c b/discident-glib/test-diglib.c
index 7b4b1b9..432555e 100644
--- a/discident-glib/test-diglib.c
+++ b/discident-glib/test-diglib.c
@@ -114,6 +114,7 @@ test_amz_parse (void)
        char *response;
        gboolean ret;
        char *img_url, *title;
+       GError *error = NULL;
 
        g_file_get_contents (SRCDIR "/amz-response.xml",
                             &response,
@@ -121,10 +122,44 @@ test_amz_parse (void)
                             NULL);
        ret = _amz_parse_lookup_response (response,
                                         &title,
-                                        &img_url);
+                                        &img_url,
+                                        NULL);
        g_assert (ret);
        g_assert_cmpstr (title, ==, "JavaScript Pocket Reference (Pocket Reference (O'Reilly))");
        g_assert_cmpstr (img_url, ==, "http://ecx.images-amazon.com/images/I/51UL7NX3-6L.jpg";);
+       g_free (title);
+       g_free (img_url);
+
+       g_file_get_contents (SRCDIR "/amz-response-error.xml",
+                            &response,
+                            NULL,
+                            NULL);
+       ret = _amz_parse_lookup_response (response,
+                                         &title,
+                                         &img_url,
+                                         &error);
+       g_assert_false (ret);
+       g_assert_error (error, DISCIDENT_ERROR, DISCIDENT_ERROR_PARSE);
+       g_assert_cmpstr (error->message, ==, "2253101125 is not a valid value for ItemId. Please change this 
value and retry your request.");
+       g_clear_error (&error);
+
+       ret = _amz_parse_lookup_response (NULL,
+                                         &title,
+                                         &img_url,
+                                         &error);
+       g_assert_false (ret);
+       g_assert_error (error, DISCIDENT_ERROR, DISCIDENT_ERROR_PARSE);
+       g_assert_cmpstr (error->message, ==, "Empty response");
+       g_clear_error (&error);
+
+       ret = _amz_parse_lookup_response ("<Item><Item>",
+                                         &title,
+                                         &img_url,
+                                         &error);
+       g_assert_false (ret);
+       g_assert_error (error, DISCIDENT_ERROR, DISCIDENT_ERROR_PARSE);
+       g_assert_cmpstr (error->message, ==, "Could not parse XML in response");
+       g_clear_error (&error);
 }
 
 static void