(From Max's proposal in the newsgroup.) To include_fields, we add the following possibilities: _default, _all This is easy enough to implement in Bugzilla--in filter(), _default & _all would just cause it to return everything, and then we'd check for extra fields or _all within the methods. We'd add a should_include function to Bugzilla::WebService::Util that checked for a field name or _all being present. And for Bug.get: _custom which matches all custom fields. Gerv

Before I update POD, etc. I wanted feedback about these changes and if they look good. I will continue to work on the full patch in the meantime. One question, should we keep the default field listings in the individual webservice modules and pass the values to filter() or is it OK to leave them all in Constants.pm? Thoughts? dkl

IMO, hardcoding the list is prone to problems. You cannot guarantee that it will remain in sync with fields returned by the different methods.

(In reply to Frédéric Buclin from comment #2) > IMO, hardcoding the list is prone to problems. You cannot guarantee that it > will remain in sync with fields returned by the different methods. That is a fair point and I was going with the assumption that WS developers would just need to add any new fields to the Constants.pm file if they wanted it to be considered 'default'. Another idea would be to pass in a parameter to filter_wants() that designates the field that is being checked as a custom, default, or extra field. For example to filter if a user wants to see a 'default' field we could do: if (filter_wants($params, 'assigned_to', 'default') { $item{'assigned_to'} = $self->type('email', $bug->assigned_to->login); } For this we would include the field if 1) include_fields is empty, 2) include_fields contained _default, or 3) include_fields contained assigned_to explicitly. Then for a field that is not returned by default and is considered 'extra' we could do it like this: if (filter_wants($params, 'comments', 'extra') { $item{'comments'} = [ map { $self->_translate_comment($_) } $bug->comments ]; } Here the user would only get comments if 1) they explicitly included comments in include_fields such as include_fields=_default,comments or 2) include_fields list contained _all for everything. This way we would not need to maintain a hard coded list of the default fields for different object types. Does this sound better? dkl

Comment on attachment 8393118 [details] [diff] [review] 540818_1.patch clearing feedback? as this code will likely change. dkl's proposal in comment 3 looks reasonable to me.

dkl's proposal looks better, although it makes it less clear in the code which fields are returned when. Perhaps an explanatory comment will be enough. But the key thing is that the classification of each field (default/extra/whatever) should be specified exactly once. We also need to make it clear that changing the classification of an existing field such that it is returned less often (e.g. moving it from 'default' to 'extra') is a breaking API change. Gerv

(In reply to Gervase Markham [:gerv] from comment #5) > dkl's proposal looks better, although it makes it less clear in the code > which fields are returned when. Perhaps an explanatory comment will be > enough. But the key thing is that the classification of each field > (default/extra/whatever) should be specified exactly once. It would be specified once in that the type would be a param for the equivalent filter_wants which normally only happens once per field. Although there was an issue in Bug.get where the fields returned were being filtered twice which broken things. My latest patch fixed that as part of the new method. > We also need to make it clear that changing the classification of an > existing field such that it is returned less often (e.g. moving it from > 'default' to 'extra') is a breaking API change. Agreed. So basically all fields returned now would automatically be marked as 'default'. Anything we add in the future could be either specified as 'default' or be made 'extra' especially the latter if the field causes a performance decrease. dkl

Comments 1) All fields currently returned should be part of "_default". 2) I have added comments and attachments to Bug.get which are not returned by default and need to be specifically asked for or use "_extra". 3) Custom fields can be specifically asked for using "_custom" otherwise they are still returned by default. 4) The shortcut identifiers should also work the opposite in exclude_fields. dkl

Comment on attachment 8404044 [details] [diff] [review] 540818_2.patch Review of attachment 8404044 [details] [diff] [review]: ----------------------------------------------------------------- this mostly looks good; however there appears to be changes that belong to another bug here. ::: Bugzilla/WebService.pm @@ +319,5 @@ > > =back > > +There are several shortcut identifiers to ask for only certain groups of > +fields to be returned or excluded. nit: trailing whitespace ::: Bugzilla/WebService/Bug.pm @@ +1285,4 @@ > my @custom_fields = Bugzilla->active_custom_fields; > foreach my $field (@custom_fields) { > my $name = $field->name; > + next if !filter_wants($params, $name, ['default','custom']); nit: need a space after that comma @@ +1333,5 @@ > + next if $comment->is_private && !Bugzilla->user->is_insider; > + push(@result, $self->_translate_comment($comment, $params, 'extra', 'comments')); > + } > + $item{'comments'} = \@result; > + } returning comments isn't related to this change @@ +1341,5 @@ > + foreach my $attachment (@{ $bug->attachments }) { > + push(@result, $self->_attachment_to_hash($attachment, $params, 'extra', 'attachments')); > + } > + $item{'attachments'} = \@result; > + } returning attachments is also unrelated @@ +2727,5 @@ > +C<string> The login name of the user that created the attachment. > + > +Also returned as C<attacher>, for backwards-compatibility with older > +Bugzillas. (However, this backwards-compatibility will go away in Bugzilla > +5.0.) this feature is targeted for 5.0, so this note is incorrect. is there a bug for removing that field? @@ +2736,5 @@ > +for each attachment. Each flag hash contains the following items: > + > +=over > + > +=item C<id> nit: trailing whitespace @@ +2758,5 @@ > +C<dateTime> The timestamp when the flag was last modified. > + > +=item C<status> > + > +C<string> The current status of the flag. add an example here. eg. "+", "-", or "?" @@ +2782,5 @@ > +=over > + > +=item id > + > +C<int> The globally unique ID for the comment. even though this section of pod should be removed from this patch .. these IDs aren't globally unique. ::: Bugzilla/WebService/Util.pm @@ +107,4 @@ > return (\@old_flags, \@new_flags); > } > > +sub filter ($$;$$) { since we're touching this line, can you remove the space before the ( @@ +117,5 @@ > > return \%newhash; > } > > +sub filter_wants ($$;$$) { since we're touching this line, can you remove the space before the ( @@ +153,5 @@ > + $exclude_types{$1} = 1; > + delete $exclude{$key}; > + } > + > + # If user has asked to include all or exclude all nit: If the user .. @@ +155,5 @@ > + } > + > + # If user has asked to include all or exclude all > + return $cache->{$field} = 1 if $include_types{'all'}; > + return $cache->{$field} = 0 if $exclude_types{'all'}; exclude=_all should take priority over include=_all, swap the order of these lines.

Thanks. Will file a separate bug about adding comments and attachments and make it depend on this one.

Comment on attachment 8404915 [details] [diff] [review] 540818_3.patch r=glob

Thanks! To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git ad9b149..32931bb master -> master dkl