521398 - Make legacy XML interface supply more data (flags, etc.)

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Gervase Markham [:gerv]]

I am using the legacy XML interface to get bug data out of Bugzilla for the REST API, because it's the only programmatic interface which provides nearly all the data. I say "nearly"; it doesn't provide the following:

- Details of flag types for attachments and bugs (so you know what flags _could_ be set)
- Details of unset groups for bugs (so you know what groups _could_ be set)
- delta_ts and is_url for attachments
- dupe_of and is_open for bugs

All this info is somewhere in the HTML or WebServices version of the bug, so I don't think it's a security risk to expose it. And you need it in order to write a UI for editing bugs.

Gerv

Comment 1

[Gervase Markham [:gerv]]

Attachment: Patch v.1

OK, here's a patch.

The way I've done groups is constrained by backwards-compatibility; you certainly wouldn't do it that way if starting again.

Attachment flags are now controlled by displayfields.flag, which seems sane to me, but I could easily put them back to being not so controlled if you preferred.

The id attribute on <flag> seemed to be missing from bugzilla.dtd.

Gerv

Comment 2

[Frédéric Buclin]

(In reply to comment #0)
> - Details of flag types for attachments and bugs (so you know what flags
> _could_ be set)

Flag types are not part of the bug itself, so this shouldn't be included in the XML file. This information is already available in the RDF format of config.cgi, see e.g. https://landfill.bugzilla.org/bugzilla-tip/config.cgi?ctype=rdf


> - Details of unset groups for bugs (so you know what groups _could_ be set)

Same here, this information must come from config.cgi, see bug 449139.


> - delta_ts and is_url for attachments

The <date> field already exists. No need for an additional field. The is_url part of the request is valid.


> - dupe_of and is_open for bugs

dupe_of, why not, but I don't see why is_open is useful. If the bug has no resolution, it's open.

Comment 3

[Max Kanat-Alexander]

Details of flag types belong in config.cgi, and are currently there, I believe.

Details of unset groups for bugs should also be in config.cgi.

I'm pretty surprised that dupe_of and is_open aren't there. Are you sure it's not just called dup_id?

Comment 4

[Frédéric Buclin]

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

I disagree per my previous comment.

Comment 5

[Max Kanat-Alexander]

Mmmm, yeah, is_open might not be necessary, per LpSolit's comment.

Comment 6

[Gervase Markham [:gerv]]

config.cgi?ctype=rdf? Yay - another data format to parse. ;-)

So applications using Bugzilla programmatically should read config.cgi at startup, and assume it never changes? Or is there some mechanism to be notified of changes?

is_open == 1 IFF resolution = "---"?

You're right about dup_id; I noticed that after creating the bug. 

On attachments, the <date> field is the creation date, not the last changed date. Not having the last changed date makes it harder (impossible?) to check for mid-airs.

If is_open isn't actually necessary, why did you put it on the WebServices interface? It's that which I'm copying...

Gerv

Comment 7

[Frédéric Buclin]

(In reply to comment #6)
> is_open == 1 IFF resolution = "---"?

No, the resolution is not set in the XML file if it's empty.

Comment 8

[Gervase Markham [:gerv]]

Attachment: Patch v.2

OK, here's the cut-down version. As you may have noticed, I've done a patch for the config.cgi bug too.

Gerv

Comment 9

[Gervase Markham [:gerv]]

Attachment: Patch v.3

We also need the IDs of comments, because otherwise we can't tell, when someone submits a bug update, which comments already existed and which ones are new and to be added.

Gerv

Comment 10

[Gervase Markham [:gerv]]

Attachment: Patch v.4

...and make sure attachment-related timestamps have seconds. Sorry :-)

Gerv

Comment 11

[Gervase Markham [:gerv]]

Attachment: Patch v.5

Incorporating group ID patch from bug 378681, but with a corresponding update to bugzilla.dtd.

Gerv

Comment 12

[Reed Loden [:reed]]

(In reply to comment #11)
> Incorporating group ID patch from bug 378681, but with a corresponding update
> to bugzilla.dtd.

Huh? Why is the patch from bug 378681 part of your patch? It's already landed.

Comment 13

[Reed Loden [:reed]]

Also, why is your patch against 3.4 branch instead of tip?

Comment 14

[Gervase Markham [:gerv]]

<sigh> Because I'm focussed on getting this on b.m.o. and b-s-t.m.o. at the moment. It's holding up my entire project. 

I'll do another tip one if you want, but is it really going to make much difference to the review? This is not a complex patch.

Gerv

Comment 15

[Gervase Markham [:gerv]]

Attachment: Patch v.6

Tip patch, now that tip and 3.4 have diverged due to bug 378681.

Gerv

Comment 16

[Reed Loden [:reed]]

(In reply to comment #15)
> Tip patch, now that tip and 3.4 have diverged due to bug 378681.

Huh? The patch in bug 378681 landed on both tip and 3.4... You shouldn't be seeing a difference.

Comment 17

[Gervase Markham [:gerv]]

My test installation is checked out from BUGZILLA-3_4-STABLE, not BUGZILLA-3_4-BRANCH.

Can we please focus this energy on getting this patch reviewed? :-) Or, alternatively, your schoolwork :-P

Gerv

Comment 18

[Reed Loden [:reed]]

Comment on attachment 407251 [details] [diff] [review]
Patch v.6

(In reply to comment #17)
> My test installation is checked out from BUGZILLA-3_4-STABLE, not
> BUGZILLA-3_4-BRANCH.

Then that's a problem. You should be using the branch for development work.

> Can we please focus this energy on getting this patch reviewed? :-)

Very well...

>-                  setter="[% flag.setter.login FILTER email FILTER xml %]"
>-              [% IF flag.requestee %]
>-                  requestee="[% flag.requestee.login FILTER email FILTER xml %]"
>-              [% END %]
...
>-                    setter="[% flag.setter.email FILTER email FILTER xml %]"
>-                    [% IF flag.status == "?" && flag.requestee %]
>-                      requestee="[% flag.requestee.email FILTER email FILTER xml %]"
>-                    [% END %]

Why is flag.setter.login used for bug flags and flag.setter.email used for attachment flags?
Why do attachment flags only display the requestee if the status is "?" while bug flags will always display it? What's the best choice of the two?

Your patch factors this code out to one block, so need to make sure the right values are used, as the two are currently different in a number of ways.

Comment 19

[Frédéric Buclin]

(In reply to comment #18)
> Then that's a problem. You should be using the branch for development work.

I agree. And even HEAD code, ideally. b.m.o will be upgraded to 3.4-BRANCH, AFAIK, not 3.4-STABLE. But if the patch doesn't break compatibility with the current schema, I may even agree to take it for 3.4.3 (as new attributes/fields will simply be ignored by external clients parsing the XML file).


> > Can we please focus this energy on getting this patch reviewed? :-)

Depends. If what I review cannot be applied as is on HEAD code, I won't waste my time reviewing it as another review will be necessary later.


> Why is flag.setter.login used for bug flags and flag.setter.email used for
> attachment flags?

Because when we implemented bug 307328 in Bugzilla 2.22, ghendricks and I didn't pay attention that all other user fields where using .email rather than .login. So for consistency, we must use .email everywhere, including for flag setters and requestees.


> Why do attachment flags only display the requestee if the status is "?" while
> bug flags will always display it? What's the best choice of the two?

In modern Bugzillas (i.e. 3.x), the requestee cannot exist if the flag status is not "?". So both should be fine. But to avoid any problem, we could/should use [% IF flag.status == "?" && flag.requestee %].


> Your patch factors this code out to one block, so need to make sure the right
> values are used, as the two are currently different in a number of ways.

Yeah, you are right. This refactoring is a good opportunity to clean this up. gerv, please update your patch accordingly.

Comment 20

[Gervase Markham [:gerv]]

Thank you both for your review comments.

My installation started life as a stable installation to test the API against. When I realised Bugzilla didn't supply enough information, nothing was more natural than to start making changes to that version.

If you get a patch that's not acceptable, I suggest that the polite thing to do is let the submitter know immediately, without being harsh or critical, and either pointing to the Bugzilla project's patch submission documentation (where is that?), or explaining what needs to be done. This patch has been here for 11 days, and that time included an "I'll review it very soon, probably tomorrow" promise, and yet no-one mentioned that it was unsuitable for review until today.

If I were a new contributor, I would already be feeling that the Bugzilla patch submission process was somewhat bruising. And I don't think existing contributors deserve a lesser level of politeness than new ones.

An updated patch is on its way.

Gerv

Comment 21

[Gervase Markham [:gerv]]

Attachment: Patch v.7

Review comments addressed.

Gerv

Comment 22

[Max Kanat-Alexander]

A few notes:

It is always better to use elements than attributes when possible.

It is generally better to always display an element and just have it be blank (as opposed to not displaying an item when it's blank), in terms of ease of parsing in various languages.

Comment 23

[Max Kanat-Alexander]

I won't accept this on the 3.4 branch--it's an enhancement. It's hard to know what people depend on on stable branches, in terms of stability--for example, somebody could be using a silly regex parser to parse the XML that will break when new elements are added. Granted, they shouldn't be doing that, but if they are (and this is just an example), we shouldn't break it on a stable branch.

Gerv: Our patch submission guidelines are here:

  http://wiki.mozilla.org/Bugzilla:Developers

Comment 24

[Max Kanat-Alexander]

  And by the way, reed and LpSolit--I agree with Gerv about the "bruising" nature that a new contributor would feel on the review process. If the patch didn't apply to HEAD (after actually trying to apply it) or if it was obvious that it wasn't going to apply, then it would be appropriate to politely direct the contributor to Bugzilla:Developers. Also, it helps retain new and current contributors when you also thank them for submitting the patch and indicate the parts that are good as well as pointing out (without personal criticism or any harshness) the pieces that need to be fixed.

  If there is a person who consistently violates the development guidelines and isn't learning, then it might be appropriate to be harsh if you think that's "the only way they'll get the message". But I don't think Gerv is in that category.

Comment 25

[Reed Loden [:reed]]

(In reply to comment #23)
> I won't accept this on the 3.4 branch--it's an enhancement. It's hard to know
> what people depend on on stable branches, in terms of stability--for example,
> somebody could be using a silly regex parser to parse the XML that will break
> when new elements are added. Granted, they shouldn't be doing that, but if they
> are (and this is just an example), we shouldn't break it on a stable branch.

So, why was bug 378681 taken on 3.4 then, as it's an "enhancement" in your terms?

I disagree with you here... Anything that can be done to fix all the issues mentioned by bug 449137 and friends should be acceptable on the current stable branch (within reason). What use is Bugzilla's API if there are major problems that cause API users to not be able to get basic information in order to get stuff done?

Comment 26

[Max Kanat-Alexander]

(In reply to comment #25)
> So, why was bug 378681 taken on 3.4 then, as it's an "enhancement" in your
> terms?

  I wouldn't have approved that either, had I been asked.

Comment 27

[Frédéric Buclin]

(In reply to comment #24)
>   And by the way, reed and LpSolit--I agree with Gerv about the "bruising"
> nature that a new contributor would feel on the review process.

I don't need this kind of comments, especially from you (see below why I say that). I'm involved in the project for long enough (5 years now) to know how things work, from the contributor/reviewer/tester perspectives. I'm the first one to say that reviews should be done promptly to keep contributors around, and in this area, I think I have always been pretty good at keeping my review queue short: I came back from vacation on Saturday, reviewed 9 patches since Sunday (I reviewed them in the order they were submitted), and I have only 2 left in my review queue: this bug and bug 520948. For comparison, you (mkanat) have 30 patches waiting for your review, some since early 2008. I doubt that's a better and friendlier way to keep contributors around (when was Noura's last contribution due to that?).

Now, about yelling that I should have said earlier to write the patch against HEAD, I only heard *today* that the patch was *not* written against HEAD, in comment 13 and then confirmed in comment 17. I don't know how I could have know this earlier. As I said, I reviewed other patches first because they were in my review queue for longer than gerv's one.

And finally, about being rude with gerv, you should point me to what hurt him/you exactly, because I didn't write anything with being rude in mind. I only said I agreed with reed once I knew the patch wasn't against HEAD. That's much more honest than to ignore this patch some more days and only complain in a week that the patch doesn't apply (bitrot).


(In reply to comment #20)
> This patch has been here for 11
> days, and that time included an "I'll review it very soon, probably tomorrow"
> promise

I reviewed patch v1 the day before leaving for vacation (see the email I sent to reviewers@ two weeks ago), and patches v3 and above were submitted after I was gone. Remind me how I could have done such a promise about v3 and newer?


I don't even know why we have this discussion here (*that* can be taken as being rude).

Comment 28

[Max Kanat-Alexander]

(In reply to comment #27)
> I think I have always been pretty good at keeping my review
> queue short: I came back from vacation on Saturday, reviewed 9 patches since
> Sunday (I reviewed them in the order they were submitted), and I have only 2
> left in my review queue: this bug and bug 520948. 

  Yeah, you're great at that, I agree.

> For comparison, you (mkanat)
> have 30 patches waiting for your review, some since early 2008. I doubt that's
> a better and friendlier way to keep contributors around (when was Noura's last
> contribution due to that?).

  Noura stopped contributing due to no longer working at Red Hat (or at least no longer working on Bugzilla), was my understanding.

> Now, about yelling that I should have said earlier to write the patch against
> HEAD, I only heard *today* that the patch was *not* written against HEAD, in
> comment 13 and then confirmed in comment 17. I don't know how I could have know
> this earlier. As I said, I reviewed other patches first because they were in my
> review queue for longer than gerv's one.

  Yeah, that comment was directed at reed, not at you.

> And finally, about being rude with gerv, you should point me to what hurt
> him/you exactly, because I didn't write anything with being rude in mind. I
> only said I agreed with reed once I knew the patch wasn't against HEAD. That's
> much more honest than to ignore this patch some more days and only complain in
> a week that the patch doesn't apply (bitrot).

  Actually, reading back over your most recent review of Gerv's patch on this bug, I don't see any problem at all, you're right. It looks totally fine and straightforward to me.

Comment 29

[Gervase Markham [:gerv]]

Yes, I should clarify that the bruised feeling was not due to anything you (LpSolit) said. And I did not know you were on vacation; that would certainly have changed how I was thinking about things. I apologise for my negative thoughts :-)

While we are on the subject though, it's worth pointing out one action that did make me feel as if I was being ignored:

- I submit a patch (v.2 or so here)
- I wait a few days
- You promise you'll review it, hopefully within the next day or so
  (yes, a promise is only that, fair enough)
- You then create, have reviewed and check in a separate patch on another bug 
  (bug 378681) which had lain dormant for ages. That patch conflicts with mine, 
  so I have to refresh it.
- My patch remained unreviewed.

I know you didn't mean to make me feel overlooked with that middle action, but I am just pointing out, hopefully in a constructive way, that was the effect of it. If I were a new contributor, I would find that quite frustrating.

Max: I hope I can convince you to take these patches on 3.4. Otherwise, they will just end up making the differences between b.m.o. and 3.4.x larger than they otherwise would be, and also make the BzAPI project fairly useless against a stock 3.4 Bugzilla. Both of those things would be a shame.

If there's any way I can change them or present them, or any testing I can do, to make them more acceptable, please let me know.

Gerv

Comment 30

[Max Kanat-Alexander]

Gerv:

There's basically nothing that people can do to convince me to take enhancements on stable branches. The idea of stable branches is to stabilize by making fewer and fewer changes in each point release--only bug fixes targeted toward having to make fewer bug fixes in the future.

Theoretically Bugzilla 3.6 isn't too far away. Probably what would be ideal would be to request and/or implement the XML-RPC methods that you need for BzAPI for 3.6, as the XML-RPC interface is the actual stable interface and the XML interfaces are deprecated (or at least never were declared to be stable).

Comment 31

[Max Kanat-Alexander]

Also, you're welcome to point 3.4 admins to the patches that they need for 3.4 to run BzAPI, or even provide those patches in the package.

Comment 32

[Gervase Markham [:gerv]]

The entire point of BzAPI is to decouple the development cycle of the new API from the Bugzilla development cycle (and the b.m.o. upgrade cycle). In general, making a proxy which uses existing interfaces has proved to be a very successful strategy. There's no way we could have got to the stage we have so fast, with people using the API for real projects, doing it any other way.

However, in a few limited cases, as represented by this patch and perhaps one other small forthcoming one, that's not quite possible. At least, without HTML screen scraping, which I would prefer not to do, as it would be fragile and the double request would kill performance. I and the Mozilla project would really appreciate it if the Bugzilla development team could see its way to bear with us to this small, limited extent, in terms of remedying the deficiencies of the interfaces we are using. We are doing so in a backwardly-compatible way; we aren't changing interactive behaviour or core code, only a text-based one-way interface which you say yourself is deprecated.

Asking admins to apply patches raises the barrier to using the BzAPI. I originally wanted to do a new drop-in template with the XML enhancements, but justdave told me he would much prefer to get the patches accepted upstream into 3.4 instead. Which is therefore what I've been attempting to do.

I realise that there are downsides to taking patches on a stable branch, but not all patches are equal. Unless this patch were to documentation, it's hard to see how it could be less risky. And yet it has the potential to make the current stable version of Bugzilla much easier to integrate with - for me, and for the other projects who do this.

Gerv

Comment 33

[Frédéric Buclin]

(In reply to comment #29)
> - You then create, have reviewed and check in a separate patch on another bug 
>   (bug 378681) which had lain dormant for ages. That patch conflicts with mine, 

The only reason it conflicts with yours is because you used 5 lines of context to create your patch. 4 lines would fix it. And I'm pretty sure your patch would still apply with simply fuzz1 as warning. Our patches are not about the same thing.

Comment 34

[Max Kanat-Alexander]

Gerv: Your argument is well-reasoned and well-stated. I think perhaps justdave may accept patching his 3.4 installation if he knows that the same patch is going upstream to HEAD--bmo in fact does this all the time, so that might be all that he wanted.

Comment 35

[Gervase Markham [:gerv]]

LpSolit: I agree our patches are not about the same thing. I'm not attributing malicious intent. I am just pointing out, for your consideration and that of others, that that course of events might have made a newer contributor or a more sensitive soul feel overlooked.

Max: you may well be right about Dave. Dave? :-)

However, I'm sure groups like Mylyn, Deskzilla and all the other clients on the long list would much prefer to be able to tell their customers "Update to the latest 3.4" rather than "Update and then apply this patch. And if there's another point release, you may have to back-out and reapply the patch due to merge conflicts. Sorry about that. What do you mean, you don't know what a patch is?" :-)

As I said, I understand your concern about 3.4 becoming a magnet for features people want in a stable Bugzilla now, now, now, and drawing developer attention away from 3.6, and so on. But I'd like to suggest that the slope is not all that slippery. And, in fact, the pressure to add stuff to 3.4 will reduce, not increase, if the ability of external clients to work properly is enhanced. And, even after 3.6 is released, Bugzillas take their time to be upgraded from one major release to another - as we very well know at the Mozilla project! People will be wanting to integrate with 3.4 for some time yet. If the changes needed were extensive or intrusive, I could understand your reservations. But I really think they aren't.

With regard to the XML-RPC API, I think we both know that, given development time constraints, we are some way from the time where it will be able to support all that clients want to do alone. In the mean time, we should be trying to help them use the methods they are using, and avoid screen-scraping.

Gerv

Comment 36

[Dave Miller [:justdave]]

(In reply to comment #35)
> Max: you may well be right about Dave. Dave? :-)

Yes, my intention was to get it accepted upstream.  If upstream commits it and it's easy to backport, I'm happy to backport it for b.m.o.  It means I only have to do it once and won't have to worry about porting it forward the next time we upgrade.

Comment 37

[Frédéric Buclin]

Comment on attachment 407299 [details] [diff] [review]
Patch v.7

>+            <commentid>[% c.id %]</commentid>

It must be XML-filtered.

#   Failed test '(en/default) template/en/default/bug/show.xml.tmpl has unfiltered directives:
#   72: c.id
# --ERROR'


>+[% BLOCK section_flags %]
>+  [% IF displayfields.flag %]

Better to write: [% RETURN UNLESS displayfields.flag %]. This removes one IF statement.


>+    [% FOREACH type = obj.flag_types %]
>+      [% FOREACH flag = type.flags %]

Nit: You could directly write: [% FOREACH flag = obj.flags %] as ->flags is now available for both bugs and attachments. This removes one FOREACH loop.


>+        <flag name="[% type.name FILTER xml %]"

With obj.flags, type.name would become flag.type.name.


>+              type_id="[% type.id FILTER xml %]"

and type.id would become flag.type_id.


Everything else looks good.

Comment 38

[Gervase Markham [:gerv]]

Attachment: Patch v.8

Here you are :-)

Gerv

Comment 39

[Frédéric Buclin]

Comment on attachment 408360 [details] [diff] [review]
Patch v.8

Works fine. r=LpSolit

Comment 40

[Gervase Markham [:gerv]]

Attachment: Patch v.8a (3.4-compatible)

Patch v.8 uses some trunk-only stuff which LpSolit wanted (see his review comments). This patch is the same but using the older interface, which makes it work on 3.4. This is the patch that would need uploading to b.m.o. and b-s-t.

Gerv

Comment 41

[Gervase Markham [:gerv]]

Fixed on trunk.

cvs -z3 -escite ci  bugzilla.dtd template/en/default/bug/show.xml.tmpl
Checking in bugzilla.dtd;
/cvsroot/mozilla/webtools/bugzilla/bugzilla.dtd,v  <--  bugzilla.dtd
new revision: 1.16; previous revision: 1.15
done
Checking in template/en/default/bug/show.xml.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/show.xml.tmpl,v  <--  show.xml.tmpl
new revision: 1.33; previous revision: 1.32
done

Gerv

Comment 42

[Gervase Markham [:gerv]]

Max: now this patch is in its final form, were the arguments I put forward convincing enough to persuade you to consider the 3.4 version of this patch (version 8a) for 3.4?

Gerv

Comment 43

[Frédéric Buclin]

Can someone tell me what changed in the URL field (see the bug history)??

Anyway, there are now valid reasons to not take this patch for 3.4:
- displayfields.flag now also affects attachment flags. That's not the case in 3.4.2.
- bug flags now use .email instead of .login for setters and requestees.
- <date> now gets seconds, which could mean a new parsing regexp for clients.

Comment 44

[Max Kanat-Alexander]

Added to the release notes in bug 547466.