Re: review: xml_is_well_formed

Hi Pavel,

Thanks for taking the time to review my patch. Attached is a new version
addressing your concerns.

On 29/07/10 14:21, Pavel Stehule wrote:
> I have a few issues:
> * broken regress test (fedora 13 - xmllint: using libxml version 20707)
>
> postgres=# SELECT xml_is_well_formed('<pg:foo
> xmlns:pg="http://postgresql.org/stuff";>bar</pg:foo>');
>   xml_is_well_formed
> --------------------
>   f
> (1 row)
>
> this xml is broken - but in regress tests is ok
>
> [pavel@pavel-stehule ~]$ xmllint xxx
> xxx:1: parser error : error parsing attribute name
> <pg:foo xmlns:pg="http://postgresql.org/stuff";>bar</pg:foo>
>

This is a problem that was observered recently by Thom Brown - the
commit fest webapp adds the semicolon after the quote. If you look at
the attachment I sent in a email client you'll find there is no
semicolon. The patch attached here will also not have the semicolon.

> * xml_is_well_formed returns true for simple text
>
> postgres=# SELECT xml_is_well_formed('ssss');
>   xml_is_well_formed
> --------------------
>   t
> (1 row)
>
> it is probably wrong result - is it ok??
>

Yes this is OK, pure text is valid XML content.

> * I don't understand to this fragment
>
>         PG_TRY();
> +       {
> +               size_t          count;
> +               xmlChar    *version = NULL;
> +               int                     standalone = -1;
> +.
> +               res_code = parse_xml_decl(string,&count,&version,
> NULL,&standalone);
> +               if (res_code != 0)
> +                       xml_ereport_by_code(ERROR, ERRCODE_INVALID_XML_CONTENT,
> +                                                 "invalid XML
> content: invalid XML declaration",
> +                                                       res_code);
> +.
> +               doc = xmlNewDoc(version);
> +               doc->encoding = xmlStrdup((const xmlChar *) "UTF-8");
> +               doc->standalone = 1;
>
> why? This function can raise exception when declaration is wrong. It
> is wrong - I think, this function should returns false instead
> exception.
>
>

You're quite right,  I should be returning false here and not causing an
exception. I have corrected this in the attached patch.

Regards,

--
Mike Fowler
Registered Linux user: 379787