146945 - clean up format ambiguities

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P2)

Description

[Myk Melez [:myk] [@mykmelez]]

The concept of formats is ambiguous because it represents two kinds of
variations in appearance: structural (f.e. classic and modern versions of the
query form) and representational (f.e. HTML and RDF versions of bug lists).  All
structural variations *have* a representation (i.e. content type), but all
representational variations *are* a representation, which leads to confusion
about how users should request variations.

Structural and representational variations should be disambiguated from each
other by distinguishing between the two in filenames and URL/form parameters via
the following scheme:

Variations in structure are called "formats."  Each format has a unique name,
and the name of a template file implementing a format consists of the name of
the script (or equivalent identifier) followed by a dash, the format name, a
period, the filename extension representing the format's content type, another
period, and the "tmpl" extension, f.e.: 

foo-classic.html.tmpl
foo-modern.html.tmpl

Formats may be specified by users via the "format" URL/form parameter.

Variations in representation are called "content types."  Each content type has
a unique extension, and the name of a template file implementing a content type
consists of the name of the script (or equivalent identifier), followed by a
period, the extension, another period, and the "tmpl" extension, f.e.:

bar.html.tmpl
bar.xml.tmpl

Content types may be specified by users via the "ctype" URL/form parameter.

Scripts may support both kinds of variations, in which case the "format"
parameter takes precedence over the "ctype" parameter.  Scripts may not
implement both kinds of variations in a single file, at least not until someone
implements a data structure for storing variations that indexes them by both
format name and extension (there is no need for this now, and it's unclear that
there ever will be, but the scheme above does not do anything to prohibit its
implementation if anyone ever discovers a reason to do it).

Bugzilla should fail gracefully when it cannot find the requested variation.
Bugzilla should have ways for installations and users to specify default variations.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: work-in-progress: complete but untested

Here's a patch implementing the scheme suggested by this bug report.

Comment 2

[Gervase Markham [:gerv]]

> Scripts may support both kinds of variations, in which case the "format"
> parameter takes precedence over the "ctype" parameter.

I don't understand what you mean by "take precedence". If I ask for
fred.cgi?format=foo&ctype=rdf
I should get the template
fred-foo.rdf.tmpl
or an error, if it doesn't exist.

Also, what impact does this have on 2.16? If we are going to rename lots of
templates (e.g. buglist-rdf.rdf.tmpl -> buglist.rdf.tmpl) then we should discuss
this idea and, if we decide it's a good one, do it now rather than later...

The default format should be "" (as now; i.e. fred.ctype.tmpl), and the default
ctype should be defined in the CGI script itself.

Gerv
Gerv

Comment 3

[Myk Melez [:myk] [@mykmelez]]

>I don't understand what you mean by "take precedence". If I ask for
>fred.cgi?format=foo&ctype=rdf
>I should get the template
>fred-foo.rdf.tmpl
>or an error, if it doesn't exist.

If someone asks for a template that doesn't exist, but half of their request
refers to a template that does exist, it's a good bet they wanted the existing
template, so it makes a lot more sense (because it makes our users happier more
of the time) to give them what they probably wanted then throw an error message.

>Also, what impact does this have on 2.16? If we are going to rename lots of
>templates (e.g. buglist-rdf.rdf.tmpl -> buglist.rdf.tmpl) then we should
>discuss this idea and, if we decide it's a good one, do it now rather than
>later...

It has no impact on 2.16, because 2.16 needs to ship and this is too big a
change to go in at the last minute.  It's a shame, but it's better if we ship
and then make big changes in 2.18 than hold 2.16 for another month or more while
we work these changes out.  Also, the idea that we can get all the big changes
into 2.16 if we just hold out a little longer is silly.  We're going to make big
changes in 2.18 no matter how long we hold 2.16.

>The default format should be "" (as now; i.e. fred.ctype.tmpl), and the default
>ctype should be defined in the CGI script itself.

This code doesn't specifically address default formats (in fact it specifically
bypasses that issue).  Let's go argue about that elsewhere. :-)

Comment 4

[Gervase Markham [:gerv]]

> If someone asks for a template that doesn't exist, but half of their request
> refers to a template that does exist, it's a good bet they wanted the existing
> template, so it makes a lot more sense (because it makes our users happier more
> of the time) to give them what they probably wanted then throw an error message.

This causes problems, because Template Toolkit looks for the given filename
along a variable path. We'd have to make a call to try one filename and, if that
failed, try another one. That's horrible and icky.

Why can't it be simple?

foo.cgi
<nothing>               -> foo.html.tmpl 
format=simple           -> foo-simple.html.tmpl
ctype=xml               -> foo.xml.tmpl
format=simple&ctype=xml -> foo-simple.xml.tmpl

As Bugzilla is primarily a web-based application, it makes sense for <nothing>
to map to ctype=html and format="".

This means we can get rid of all the GetOutputFormats() and
ValidateOutputFormats() stuff, and just construct the filename we look for along
the simple lines listed above. It separates structural and representational
variations, easily allows both at once (a simple RDF version, or whatever), and
is generally easy. We stop the user requesting invalid combinations using the UI.

What's wrong with it? :-) I'm sure there's something...

Gerv

Comment 5

[Tobias Burnus]

I like Gerv's idea and only
 ./list/list-rdf.rdf.tmpl
 ./bug/choose-xml.html.tmpl
need to be changed.
> the default ctype should be defined in the CGI script itself
Yes :-) This is e.g. needed for the emails which use the template system.

Comment 6

[Gervase Markham [:gerv]]

> > the default ctype should be defined in the CGI script itself
> Yes :-) This is e.g. needed for the emails which use the template system.

It's not, you know. :-) Those templates would just be called (as they, in fact,
are) someemail.txt.tmpl . And remember, using the formats system is optional for
any given call to the Template Toolkit - it's still perfectly possible to
hard-code template names (such as someemail.txt.tmpl).

Gerv

Comment 7

[Myk Melez [:myk] [@mykmelez]]

I'm uneasy about Gerv's proposal, but I can't put my finger on what exactly is
the matter with it.  As it stands, it sounds good, but something is nagging me
about it.

Comment 8

[Gervase Markham [:gerv]]

Attachment: Patch v.1

OK, here's an initial patch.

In addition to this patch, list/list-rdf.rdf.tmpl would need renaming to
list/list.rdf.tmpl, removing the redundancy. It would then be requested with
"ctype=rdf". This is the only file whose name doesn't fit the new mechanism.

Bug 150049 is a meta-bug for template format improvements. It has five
dependencies, including this bug. Here is the impact on the other four:

Bug 140513 - fixed (pass "txt" as the third parameter)
Bug 143604 - fixed (ctype=html does a no-op)
Bug 148133 - fixed (it now returns text/plain if the ctype is not found, which
		    seems reasonable)
Bug 149061 - not fixed. It's outside the scope.

Gerv

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 98718 [details] [diff] [review]
Patch v.1

>Index: globals.pl

>@@ -1515,119 +1515,27 @@

+    my ($templatename, $format, $ctype) = @_;

Nit: "name" is redundant in "templatename".

>+    return 
>+    { 
>+        'template'    => $templatename , 
>+        'extension'   => $ctype , 
>+        'contenttype' => $::contenttypes->{$ctype} || "text/plain" , 

Nit: It would make sense to change the name of the "contenttype" property 
to "ctype" since that's what we now use in the URL and in this function.


>@@ -202,12 +202,14 @@

>+my $format = GetFormat("reports/duplicates", 
>+                       $::FORM{'format'}, 
>+                       $::FORM{'ctype'});

Nit: better as

my $format = 
  GetFormat("reports/duplicates", $::FORM{'format'}, $::FORM{'ctype'});


>Index: enter_bug.cgi

>@@ -382,8 +382,10 @@

>+my $format = GetFormat("bug/create/create", 
>+                       $::FORM{'format'}, 
>+                       $::FORM{'ctype'});

See previous nit.


>Index: query.cgi

>@@ -363,7 +363,7 @@

+print "$format->{'contenttype'}\n\n";

-> print "Content-Type: $format->{'contenttype'}\n\n";

Are you going to create a separate patch to convert list-rdf.rdf.tmpl?

Comment 10

[Bradley Baetz (:bbaetz)]

So what if someone passes 'format=blah' into the script, where blahis not a
valid format?

Comment 11

[Gervase Markham [:gerv]]

-> me. I haven't beeng getting mail on this bug :-(

Gerv

Comment 12

[Gervase Markham [:gerv]]

Attachment: Patch v.2

Here's version 2, with Myk's comments addressed.

> So what if someone passes 'format=blah' into the script, where blahis not a
> valid format?

You get a standard "template not found" error. But I think this is reasonable -
the UI should only allow people to choose valid formats. Of course you can
define an invalid one by editing URLs, but you can create errors by editing
URLs all over Bugzilla. Nothing nasty or insecure happens.

Gerv

Comment 13

[Bradley Baetz (:bbaetz)]

Yes, but currently you get told that, rather than the ThrowTemplateError that
you'll now get.

If ctype/format are wrong, then you should throw an error/die rather than
silently accept it, I think

Comment 14

[Gervase Markham [:gerv]]

> If ctype/format are wrong, then you should throw an error/die rather than
> silently accept it, I think

The current patch lets Template Toolkit look for the file which ctype and/or
format defines, and throws an error if it's not found. The only other thing we
could do would be what happened previously - i.e. to replicate that process, and
throw an error if it's not found before we get to TT. I don't think we gain much
by throwing the error earlier, apart from the fact that we say "dodgy
format/ctype" instead of "template not found". And it means we have to duplicate
the code TT has for tracking down template files.

Gerv

Comment 15

[Bradley Baetz (:bbaetz)]

Hmm. I guess I'm convinced, as long as the template error message mentions the
template/format/etc name somewhere in there, so that an admin can diagnose it. I
think it will, since its actually a template name by the time TT gets to it.

What about the silent detainting?

Comment 16

[Gervase Markham [:gerv]]

> What about the silent detainting?

I don't quite understand... The ctype and format parameters are checked:

+    # Security - allow letters and a hyphen only
+    $ctype =~ s/[^a-zA-Z\-]//g;
+    $format =~ s/[^a-zA-Z\-]//g;

Is that what you mean?

Gerv

Comment 17

[Bradley Baetz (:bbaetz)]

Right, they're checked, but failures are silently corrected. Why should a format
of 'html*' become 'html' ?

ThrowUserError for non-valid stuff.

Comment 18

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 99441 [details] [diff] [review]
Patch v.2

Why should we throw a user error because of a typo?  If we probably know what
the user wanted, we should give it to them.  That behavior is better in more
situations, and actual errors (where the user mistypes one format name and gets
another) are exceedingly rare and easily spotted and corrected.

>+my $format = 
>+           GetFormat("reports/duplicates", $::FORM{'format'}, $::FORM{'ctype'});

Nit: Other parts of our code use half-level (two space) indentation rather than
right-justification in these situations, and I think it's more consistent,
readable, and maintainable:

my $format = 
  GetFormat("reports/duplicates", $::FORM{'format'}, $::FORM{'ctype'});

r=myk

This is a pretty obvious fix and doesn't need second review, but you do need to
fix existing template files (i.e. list-rdf.rdf.tmpl) and figure out how not to
break existing applications using format=rdf.

Comment 19

[Gervase Markham [:gerv]]

> Why should we throw a user error because of a typo?

Well, users shouldn't be typing format names - they should be manipulating
forms. But I agree - the current code is more friendly.

I'll just re-checkin list-rdf.rdf.tmpl as list.rdf.tmpl. It's only had a couple
of minor tweaks since it was moved last anyway.

> You need to... figure out how not to break existing applications using 
> format=rdf.

That's more of a problem. Are there already many of these applications?
Presumably mozbot is one. Can we just notify everyone and put this one down to
experience, or are we going to have to add a hack which converts format=rdf to
ctype=rdf on that CGI?

Gerv

Comment 20

[Gervase Markham [:gerv]]

Fixed.

Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.200; previous revision: 1.199
done
Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.193; previous revision: 1.192
done
Checking in duplicates.cgi;
/cvsroot/mozilla/webtools/bugzilla/duplicates.cgi,v  <--  duplicates.cgi
new revision: 1.26; previous revision: 1.25
done
Checking in enter_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/enter_bug.cgi,v  <--  enter_bug.cgi
new revision: 1.72; previous revision: 1.71
done
Checking in query.cgi;
/cvsroot/mozilla/webtools/bugzilla/query.cgi,v  <--  query.cgi
new revision: 1.104; previous revision: 1.103
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.65; previous revision: 1.64
done
Removing list-rdf.rdf.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/list-rdf.rdf.tmpl,v
 <--  list-rdf.rdf.tmpl
new revision: delete; previous revision: 1.3
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/list.rdf.tmpl,v
done
Checking in list.rdf.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/list.rdf.tmpl,v  <--
 list.rdf.tmpl
initial revision: 1.1
done


Removing list-rdf.rdf.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/list-rdf.rdf.tmpl,v
 <--  list-rdf.rdf.tmpl
new revision: delete; previous revision: 1.3
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/list.rdf.tmpl,v
done
Checking in list.rdf.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/list.rdf.tmpl,v  <--
 list.rdf.tmpl
initial revision: 1.1
done

Gerv

Comment 21

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: hack

I think we should hack it, and here it is.

Comment 22

[Gervase Markham [:gerv]]

Comment on attachment 99716 [details] [diff] [review]
patch v3: hack

r=gerv - seems OK code. I sort of disagree, though, because this seems like the
sort of nipped-it-in-the-bud situation where we can avoid having to carry a
hack for the next X years. 

But, I suppose we did do a release with the old way...

We should at least deprecate this in the 2.18 release notes. Do we have a
relnote keyword?

Gerv

Comment 23

[Bradley Baetz (:bbaetz)]

We could just upgrade botbot - hixie hasn't applied that patch to the sources yet.

Comment 24

[Myk Melez [:myk] [@mykmelez]]

I know of two tools using format=rdf (not including mozbot), and there are
undoubtedly others.  I don't think we can say we nipped it in the bud,
especially when some installations (b.m.o notwithstanding) don't upgrade
regularly (and thus may not upgrade from 2.16 to a later release for a few years).

Comment 25

[Gervase Markham [:gerv]]

OK, the hack it is, then. Unless we provide them a one-liner they can apply to
_their_ Bugzillas:
$::FORM{'format'} = "rdf" if $::FORM{'ctype'} = "rdf";
and then get them to upgrade their client software.

Well, I can hope. :-)

Gerv

Comment 26

[Myk Melez [:myk] [@mykmelez]]

Presumably you meant the reverse, but that won't work (whether in the tree or
distributed on demand) because it causes Bugzilla to look for
"list-rdf.rdf.tmpl" instead of "list.rdf.tmpl".  Hack checked in:

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.194; previous revision: 1.193
done

Comment 27

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #12)
> the UI should only allow people to choose valid formats. Of course you can
> define an invalid one by editing URLs, but you can create errors by editing
> URLs all over Bugzilla. Nothing nasty or insecure happens.

This mostly means "why should I bother about security here?". Your assumption is wrong, and bug 842038 is a perfect example of why invalid parameters must be rejected, not silently sanitized. bbaetz in comment 13 and comment 17 was right to worry about such partial validations. FYI, Bugzilla 4.4 and newer now loudly reject invalid format and ctype values.

Comment 28

[Gervase Markham [:gerv]]

Bug 842038, AIUI, is a case of using untrusted data before it's been sanitized, which is never a good idea. (It's somewhat hard to discuss it here as that bug is not yet open.)

Gerv