Re: [lmi] [PATCH] validate documents against schema when loading

[Vaclav Slavik]

Hi,

On 2013-04-30 15:37, Greg Chicares wrote:
> I observed the following warnings. Only the first concerns xmlwrapp itself:
> 
> /MinGW_/bin/g++  -MMD -MP -MT xml_xslt_wrapp.o -MF xml_xslt_wrapp.d   -c -I 
> /opt/lmi/src/lmi -I /opt/lmi/src/lmi/tools/pete-2.1.1 -I 
> /opt/lmi/local/lib/wx/include/i686-pc-mingw32-msw-unicode-2.9 -I 
> /opt/lmi/local/include/wx-2.9 -I /opt/lmi/third_party/include -I 
> /opt/lmi/third_party/src -I /opt/lmi/local/include -I 
> /opt/lmi/local/include/libxml2  -DLMI_WX_NEW_USE_SO    -DLIBXML_USE_DLL  
> -DSTRICT       -D_LARGEFILE_SOURCE=unknown -DWXUSINGDLL -D__WXMSW__  
> -DBOOST_STRICT_CONFIG    -std=gnu++98
> -posix  -pedantic-errors -Werror  -Wall  -Wcast-align  -Wconversion  
> -Wdeprecated-declarations  -Wdisabled-optimization  -Wimport  -Wmultichar  
> -Wpacked  -Wpointer-arith  -Wsign-compare  -Wundef  -Wwrite-strings   
> -Wno-long-long  -Wctor-dtor-privacy  -Wdeprecated  -Wnon-template-friend  
> -Woverloaded-virtual  -Wpmf-conversions  -Wsynth    -W  -Wcast-qual  
> -Wredundant-decls    -Wno-uninitialized       --param ggc-min-expand=25 
> --param ggc-min-heapsize=32768 -ggdb -O2
> /opt/lmi/src/lmi/xml_xslt_wrapp.cpp -oxml_xslt_wrapp.
> /opt/lmi/third_party/src/libxml/errors.cxx: In member function `virtual 
> std::string xml::impl::errors_collector::format_for_print(const 
> xml::error_message&) const':
> /opt/lmi/third_party/src/libxml/errors.cxx:189: warning: control reaches end 
> of non-void function
> /opt/lmi/src/lmi/workhorse.make:762: recipe for target `xml_xslt_wrapp.o' 
> failed
> make[1]: *** [xml_xslt_wrapp.o] Error 1

Thanks, fixed.

> /opt/lmi/src/lmi/xml_lmi.cpp: In constructor 
> `xml_lmi::dom_parser::dom_parser(const std::string&)':
> /opt/lmi/src/lmi/xml_lmi.cpp:79: warning: `get_error_message' is deprecated 
> (declared at /opt/lmi/third_party/include/xmlwrapp/tree_parser.h:159)
> /opt/lmi/src/lmi/xml_lmi.cpp: In constructor 
> `xml_lmi::dom_parser::dom_parser(const char*, size_t)':
> /opt/lmi/src/lmi/xml_lmi.cpp:113: warning: `get_error_message' is deprecated 
> (declared at /opt/lmi/third_party/include/xmlwrapp/tree_parser.h:159)
> /opt/lmi/src/lmi/xml_lmi.cpp: In constructor 
> `xml_lmi::dom_parser::dom_parser(const std::istream&)':
> /opt/lmi/src/lmi/xml_lmi.cpp:159: warning: `get_error_message' is deprecated 
> (declared at /opt/lmi/third_party/include/xmlwrapp/tree_parser.h:159)
> /opt/lmi/src/lmi/xml_lmi.cpp: In constructor 
> `xml_lmi::xml_document::xml_document(const std::string&)':

If you want to collect all errors and warnings, then replacing with
messages().print() does the trick:


diff --git a/xml_lmi.cpp b/xml_lmi.cpp
index 041b3da..e4b1a38 100644
--- a/xml_lmi.cpp
+++ b/xml_lmi.cpp
@@ -76,7 +76,7 @@ xml_lmi::dom_parser::dom_parser(std::string const& filename)
         if(true == parser_->operator!())
             {
             throw std::runtime_error
-                ("Parser failed: " + parser_->get_error_message()
+                ("Parser failed: " + parser_->messages().print()
                 );
             }
         }
@@ -110,7 +110,7 @@ xml_lmi::dom_parser::dom_parser(char const* data, 
std::size_t length)
         if(true == parser_->operator!())
             {
             throw std::runtime_error
-                ("Parser failed: " + parser_->get_error_message()
+                ("Parser failed: " + parser_->messages().print()
                 );
             }
         }
@@ -156,7 +156,7 @@ xml_lmi::dom_parser::dom_parser(std::istream const& is)
         if(true == parser_->operator!())
             {
             throw std::runtime_error
-                ("Parser failed: " + parser_->get_error_message()
+                ("Parser failed: " + parser_->messages().print()
                 );
             }
         }


If you only care about the result, then you can pass xml::throw_on_error
to the constructor so that it will throw on the first error it encounters.

In fact, that confuses me about this code a bit: throw_on_error is the
default (and was before 0.7, in the form of allow_exceptions=true
argument). So as far as I can tell, the affected code in
xml_lmi::dom_parser::dom_parser() is never called:

        if(true == parser_->operator!())
            {
            throw std::runtime_error
                ("Parser failed: " + parser_->messages().print()
                );
            }

Am I missing something?

> /opt/lmi/src/lmi/xml_lmi.cpp:238: warning: `__comp_ctor' is deprecated 
> (declared at /opt/lmi/third_party/include/xmlwrapp/document.h:103)
> /opt/lmi/src/lmi/workhorse.make:762: recipe for target `xml_lmi.o' failed

This just needs explicit creation of xml::node (to avoid confusion with using 
filename argument elsewhere and to make it possible to eventually have 
xml:document(filename) in the far future):


diff --git a/xml_lmi.cpp b/xml_lmi.cpp
index 041b3da..e4b1a38 100644
--- a/xml_lmi.cpp
+++ b/xml_lmi.cpp
@@ -235,7 +235,7 @@ xml::element const& xml_lmi::dom_parser::root_node
 }
 
 xml_lmi::xml_document::xml_document(std::string const& root_node_name)
-    :document_(new xml_lmi::Document(root_node_name.c_str()))
+    :document_(new xml_lmi::Document(xml::element(root_node_name.c_str())))
 {
 }


> Right now, we see only the first schema-failure message. Would the new error-
> reporting API let us display all of them?

Yes, that was the main motivation behind these changes: getting complete
and more structured errors and warnings information. By default, only
the first error is reported, but you can use a custom xml::error_handler
argument to various constructors and parsing functions to handle errors
differently. Or you can use the provided xml::error_messages handler
that collects all errors and warnings. It even has a convenience print()
method and xml::exception ctor that uses it, so a simple way to display
all errors is this:


diff --git a/multiple_cell_document.cpp b/multiple_cell_document.cpp
index 84cb084..50ba03f 100644
--- a/multiple_cell_document.cpp
+++ b/multiple_cell_document.cpp
@@ -418,7 +418,9 @@ void 
multiple_cell_document::validate_with_xsd_schema(xml::document const& d) co
 {
     try
         {
-        xsd_schema().validate(cell_sorter().apply(d));
+        xml::error_messages err;
+        if (!xsd_schema().validate(cell_sorter().apply(d), err))
+            throw xml::exception(err);
         }
     catch(...)
         {


(And identically in single_cell_document.cpp.
Arguably, it would be better to rewrite the code to avoid that throw
right before catch, but this is a smaller change and so better for testing.)

Regards,
Vaclav