486006 - importxml.pl must not use format_time() for deadlines

Status: RESOLVED FIXED

(Bugzilla :: Bug Import/Export & Moving, defect)

Description

[Greg Hendricks]

Now that Bug 182238 is fixed, importxml is broken since it used format_time to generate valid time strings for inserting into mysql. It now returns an error:

The 'hour' parameter (undef) to DateTime::new was an 'undef', which is not one of the allowed types: scalar
 at /usr/lib/perl5/site_perl/5.10.0/i586-linux-thread-multi/DateTime.pm line 171
	DateTime::new(undef, 'HASH(0x9a9c894)') called at Bugzilla/Util.pm line 435
	Bugzilla::Util::format_time('2009-04-02', '%Y-%m-%d') called at ./importxml.pl line 806
	main::process_bug('XML::Twig=HASH(0x95a8124)', 'XML::Twig::Elt=HASH(0x99cff6c)') called at /usr/lib/perl5/vendor_perl/5.10.0/XML/Twig.pm line 1926
	XML::Twig::_twig_end('XML::Parser::Expat=HASH(0x9592f84)', 'bug') called at /usr/lib/perl5/vendor_perl/5.10.0/i586-linux-thread-multi/XML/Parser/Expat.pm line 474
	XML::Parser::Expat::parse('XML::Parser::Expat=HASH(0x9592f84)', '<?xml version="1.0" encoding="UTF-8" standalone="yes" ?>\x{a}<!DO...') called at /usr/lib/perl5/vendor_perl/5.10.0/i586-linux-thread-multi/XML/Parser.pm line 187
	eval {...} called at /usr/lib/perl5/vendor_perl/5.10.0/i586-linux-thread-multi/XML/Parser.pm line 186
	XML::Parser::parse('XML::Twig=HASH(0x95a8124)', '<?xml version="1.0" encoding="UTF-8" standalone="yes" ?>\x{a}<!DO...') called at /usr/lib/perl5/vendor_perl/5.10.0/XML/Twig.pm line 658
	eval {...} called at /usr/lib/perl5/vendor_perl/5.10.0/XML/Twig.pm line 658
	XML::Twig::parse('XML::Twig=HASH(0x95a8124)', '<?xml version="1.0" encoding="UTF-8" standalone="yes" ?>\x{a}<!DO...') called at ./importxml.pl line 1328
 at ./importxml.pl line 1328

Comment 1

[Frédéric Buclin]

I can reproduce. I will investigate later.

Comment 2

[Max Kanat-Alexander]

Why would you need to format Bugzilla's date output into anything else? Shouldn't it be insertable into MySQL or any DB as-is?

Comment 3

[Greg Hendricks]

Attachment: V1

As near as I can tell, format_time does not ever have timezone info passed in except from importxml.

Comment 4

[Greg Hendricks]

(In reply to comment #2)
> Why would you need to format Bugzilla's date output into anything else?
> Shouldn't it be insertable into MySQL or any DB as-is?

We are not converting output, we are converting input. 
The point is validation. bug xml can contain 

<creation_timestamp>Mar 29, 2009 13:01:03</creation_timestamp> or
<creation_timestamp>2009-03-29 12:00:00</creation_timestamp> or
<creation_timestamp>Tuesday March 31, 2009</creation_timestamp>
etc.

Mysql does not handle all of these correctly unfortunately (and I am assuming postgres doesn't either)

Format time converts them into a standard format that mysql will handle. The patch above also converts the timezone so that a bug exported in timezone A will be converted to timezone B on import.

Comment 5

[Max Kanat-Alexander]

(In reply to comment #4)
> <creation_timestamp>Mar 29, 2009 13:01:03</creation_timestamp> or
> <creation_timestamp>2009-03-29 12:00:00</creation_timestamp> or
> <creation_timestamp>Tuesday March 31, 2009</creation_timestamp>

  Why would bug XML ever have the first or third format in them, when Bugzilla doesn't produce those formats?

Comment 6

[Greg Hendricks]

> (In reply to comment #5)
>   Why would bug XML ever have the first or third format in them, when Bugzilla
> doesn't produce those formats?

Part of the point of am import tool is to be able to allow importing from any source. Just because Bugzilla doesn't produce a certain format doesn't mean Jira, or Gnats, or some other tool that I am exporting from doesn't. We need to support multiple data formats as there is not a way to exclude one or the other in a DTD. If we were using an XSD to define the xml input, we could do so. This would require another major refactoring of the import system though, and I am not ready to do that yet.

So far, my experience has been that the majority of users of the importxml script are using it to convert their bugs from some system other than Bugzilla. The rule goes something like "Enforce strict output, be lax on input". We are just trying to comply. :)

Comment 7

[Max Kanat-Alexander]

(In reply to comment #6)
> Part of the point of am import tool is to be able to allow importing from any
> source. 

  Mmm, I'd rather they had to convert to a canonical format. Nobody exports anything like our XML format anyway, so anybody exporting has to write a transformation anyhow. It would greatly simplify the fix for this bug to just not format or parse input dates at all, but just validate their form.

Comment 8

[Greg Hendricks]

Attachment: V2

I was wrong, Mysql does not ignore the timezone on insert. It just inserts 0000-00-00 00:00:00 instead. :(

Comment 9

[Greg Hendricks]

(In reply to comment #7)
>   Mmm, I'd rather they had to convert to a canonical format. Nobody exports
> anything like our XML format anyway, so anybody exporting has to write a
> transformation anyhow. It would greatly simplify the fix for this bug to just
> not format or parse input dates at all, but just validate their form.

Agreed. This is the ideal solution. Not sure how we can enforce a canonical format until we convert to using XSD as mentioned above though. Until then, this is what we have to do to have an import.

Comment 10

[Frédéric Buclin]

Comment on attachment 370494 [details] [diff] [review]
V2

>Index: Bugzilla/Util.pm

>+                                time_zone => Bugzilla->local_timezone->offset_as_string($time[6]) 
>+                                    || Bugzilla->local_timezone});

Nit: it would be nicer to vertically align || with Bugzilla->local_timezone->...

With the combination of bug 487865, this is now working fine. r=LpSolit

Comment 11

[Frédéric Buclin]

Rewording the bug summary as you still have to use format_time() for all date fields except deadlines.

Comment 12

[Greg Hendricks]

tip: 
    Checking in importxml.pl;
    /cvsroot/mozilla/webtools/bugzilla/importxml.pl,v  <--  importxml.pl
    new revision: 1.91; previous revision: 1.90
    done
    Checking in Bugzilla/Util.pm;
    /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
    new revision: 1.88; previous revision: 1.87
    done

3.4 branch:
    Checking in Bugzilla/Util.pm;
    /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
    new revision: 1.86.2.2; previous revision: 1.86.2.1
    done
    Checking in importxml.pl;
    /cvsroot/mozilla/webtools/bugzilla/importxml.pl,v  <--  importxml.pl
    new revision: 1.90.2.1; previous revision: 1.90