523325 - JSON template for config.cgi

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: General, enhancement)

Description

[Gervase Markham [:gerv]]

config.cgi could do with a JSON template. JSON is now commonly used as an API output format, and it's easier to work with than RDF and more flexible than raw JavaScript.

Gerv

Comment 1

[Frédéric Buclin]

So you should update the existing JS template as it's probably of no use as is (I even had the plan to kill it and keep the RDF template only).

Comment 2

[Gervase Markham [:gerv]]

Are you not concerned that people might be using it?

JS and JSON are different things. For example, JSON is not executable, and not all JS constructs are valid JSON (e.g. comments). JSON can also be consumed in any number of different programming languages. So this definitely needs to be a different ctype and therefore filename. (config.json.tmpl). We could get rid of config.js.tmpl at the same time if you like.

Gerv

Comment 3

[Frédéric Buclin]

(In reply to comment #2)
> Are you not concerned that people might be using it?

Honestly? No. We stopped improving the JS template for a long time, and new stuff only goes into the RDF template.

Comment 4

[Gervase Markham [:gerv]]

Attachment: Patch v.0.1

OK, here's an initial patch. It produces the JSON output I specced for the Bugzilla HTTP API here:
https://wiki.mozilla.org/Bugzilla:REST_API:Objects:Configuration

It's not suitable for checkin, because it does some temporary hacks to do with field name conversion and field type assignment. But it's exactly those things, and how to do them in the long term, that I need guidance on. So I'm hoping it will allow you to guide me as to whether my approach is sane. :-)

Gerv

Comment 5

[Max Kanat-Alexander]

If you're going to output JSON, it would be better to use JSON.pm.

Comment 6

[Gervase Markham [:gerv]]

How would that work, in terms of fitting into Bugzilla's template-based architecture? Where would the code go which massages all the data into the appropriate structure?

Gerv

Comment 7

[Max Kanat-Alexander]

(In reply to comment #6)
> How would that work, in terms of fitting into Bugzilla's template-based
> architecture? 

  Well, programmatic output doesn't *really* need to be templatized, I'd say.

> Where would the code go which massages all the data into the
> appropriate structure?

  Mm, somewhere architecturally intelligent, I'd imagine.

Comment 8

[Gervase Markham [:gerv]]

Thank you for that spectacularly uninformative answer :-))

I'll look into possibilities. But fundamentally, at the moment, we have config.cgi calling a template, and it would be great if the JSON output could work within that mechanism rather than require config.cgi, and any other CGI outputting JSON, to be rewritten.

Gerv

Comment 9

[Max Kanat-Alexander]

(In reply to comment #8)
> Thank you for that spectacularly uninformative answer :-))

  Hahahaha, sorry. I have a tremendous number of things that need to get done, both for the Bugzilla Project and for many other things that are going on, right at this time. I actually hadn't planned to modify config.cgi any further, and in particular, definitely had no plans to add additional formats, because the WebServices interface is the "way of the future" for Bugzilla. :-) So I haven't thought at all about how we should architect such a solution, because it was totally outside the scope of my plans for Bugzilla.

> I'll look into possibilities. But fundamentally, at the moment, we have
> config.cgi calling a template, and it would be great if the JSON output could
> work within that mechanism rather than require config.cgi, and any other CGI
> outputting JSON, to be rewritten.

  Mmmm, I have a suspicion, though, that using JSON.pm would actually make things a lot easier, because it will handle any tricky bits for you, eliminating a lot of defect possibility.

  In any case, something like this would be being added to HEAD, so a bit of re-architecture is OK.

Comment 10

[Frédéric Buclin]

I don't really understand where these OLD2NEW constants come from. Why not simply "cloning" the RDF template?

Comment 11

[Gervase Markham [:gerv]]

The HTTP API (as defined in those wiki pages giving the objects) changes a lot of field names to be more sensible than the column names in the database. This is something Max started on the XML-RPC API, and I've just taken a bit further.

The RDF template also has a lot of these renames (e.g. bug_status -> status, rep_platform -> platform). I don't think any of my renames disagree with RDF. There are just more of them.

The shape of the data is not the same as the shape of the RDF data because RDF has a different data model. For example, I think it makes much more sense to have the permitted values of a field be a property of the field object for that field, rather than somewhere else in the structure entirely. Then you can do:
$config->{'fields'}->{$field}->{'values'}
$config->{'fields'}->{$field}->{'default'}
which makes good sense to me.

I suppose we could have the Bugzilla JSON template spit out the new field names for the ones the RDF template renamed and the old field names for the others, and have the proxy convert the rest. But I don't see that as gaining much.

It may well be true that the current mapping has some entries which would not be used in this context, because it's a general mapping used through the BzAPI code, for queries, single bugs etc. If you want me to trim it down, I can. But it might make keeping the two in sync harder.

Gerv

Comment 12

[Gervase Markham [:gerv]]

So does that answer your concerns?

Gerv

Comment 13

[Max Kanat-Alexander]

OK. The idea of the mapping is OK, but if you're going to add such a mapping to Bugzilla, it should be in a central code-related place, not in the templates.

Comment 14

[Frédéric Buclin]

Comment on attachment 407302 [details] [diff] [review]
Patch v.0.1

I will let mkanat decide what to do with it. I have no real opinion on how this template should work.

Comment 15

[Gervase Markham [:gerv]]

Moving the field name mappings out, and using JSON.pm, while probably the right thing to do, would be a much more intrusive fix. justdave prefers the less intrusive version for b.m.o., and so I'm moving this bug over to the b.m.o. component.

Once I've got this patch up there (as it's needed for the HTTP API, which is my main focus) then I may be able to come back and get config.cgi to speak JSON on the trunk in an architecturally better way.

Of course, it would be highly preferable if the interface didn't change when the implementation did. So if any Bugzilla hacker has views on whether I've designed the structure right:
https://wiki.mozilla.org/Bugzilla:REST_API:Objects:Configuration
then I'd love to hear them. To make it easier to evaluate, I'll also attach the output of config.cgi?ctype=json from a stock 3.4 install.

Gerv

Comment 16

[Gervase Markham [:gerv]]

Attachment: config.json - example output of upcoming patch

Comment 17

[Gervase Markham [:gerv]]

Attachment: Patch v.1

A patch for b.m.o. In order to get the status transition stuff in, there's also two lines of change to config.cgi as well as the FILTER I mentioned.

Gerv

Comment 18

[Dave Miller [:justdave]]

modified config.cgi
modified Bugzilla/Template.pm
Committed revision 6642.
added template/en/default/config.json.tmpl
Committed revision 6643.

Should be live on bugzilla-stage-tip sometime in the next couple hours.

Comment 19

[Gervase Markham [:gerv]]

Hi Dave,

I'm afraid there's a bug in the template which causes it to output malformed JSON when the last flag in the list is inactive. I'll attach an updated template you can just drop in.

Gerv

Comment 20

[Gervase Markham [:gerv]]

Attachment: Updated config.json.tmpl

Comment 21

[Dave Miller [:justdave]]

Comment on attachment 411443 [details]
Updated config.json.tmpl

--- template/en/default/config.json.tmpl        2009-11-09 12:37:12 +0000
+++ template/en/default/config.json.tmpl        2009-11-10 17:33:21 +0000
@@ -148,10 +148,13 @@
           "flag_type": [
           [% flag_types = 
               component.flag_types.bug.merge(component.flag_types.attachment) %]
+          [%-# "first" flag used to get commas right; can't use loop.last() in case
+             # last flag is inactive %]
+          [% first = 1 %]
           [% FOREACH flag_type = flag_types %]
             [% NEXT UNLESS flag_type.is_active %]
             [% all_visible_flag_types.${flag_type.id} = flag_type %]
-            [% flag_type.id FILTER json %][% ',' UNLESS loop.last() %]
+            [% ',' UNLESS first %][% flag_type.id FILTER json %][% first = 0 %]
           [% END %]]
         } [% ',' UNLESS loop.last() %]
       [% END %]

Comment 22

[Dave Miller [:justdave]]

modified template/en/default/config.json.tmpl
Committed revision 6674.

Should be live on stage-tip momentarily.