Closed Bug 838846 Opened 7 years ago Closed 7 years ago

In Product.get, include_fields => ['components'] no longer returns data about components

Categories

(Bugzilla :: WebService, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
Bugzilla 4.4

People

(Reporter: mail, Assigned: mail)

References

Details

(Keywords: regression)

Attachments

(1 file)

With 4.4, the include_fields value in Product.get works differently than in 4.2.

In 4.2, you could mention
                      "include_fields": ["name", "components"]})

and it would return all information about components for the selected product. Now you would need to specify:
                      "include_fields": ["name", "components", "component.id", "component.name", "component.description", "component.default_assigned_to", "component.default_qa_contact", "component.sort_key", "component.is_active", "component.flag_types"],

Just to get the same information (the flag_types is new to 4.4). This seems very counter intuitive.

This change introduced it:
http://bzr.mozilla.org/bugzilla/4.4/revision/8444

So I assume it is intentional. Feel free to mark as RESOLVED/INVALID if it is.

  -- simon
As the original reviewer of the patch I can now see what this is indeed strange behaviour that we didn't take into account when originally implementing the feature.

There is a couple of ways to approach this:

1) If one specifies 'component.id', etc. in the include_fields, it should be implied that 'components' be added as well. 
2) If one specifies 'component.id', etc. in the include_fields, we should rely on the user to also add 'components' or they are ignored.
3) If one specifies 'components' in the include_fields, all component sub fields should be returned by default unless there are one or more 'component.*' values or one or more 'component.*' values exist in exclude_fields.

I don't feel right for #1. Maybe #2. But really #3 seems better IMO.

Thoughts?

dkl
This is a regression. Bug 741722 was about flag types and wasn't supposed to break the way Product.get works. This is a hard blocker for 4.4rc2.
Severity: normal → major
Depends on: 741722
Flags: blocking4.4+
Keywords: regression
Summary: Product.get and include_fields seems counter intuitive → include_fields => ['components'] no longer returns data about components
Target Milestone: --- → Bugzilla 4.4
Assignee: webservice → sgreen
Status: NEW → ASSIGNED
Summary: include_fields => ['components'] no longer returns data about components → In Product.get, include_fields => ['components'] no longer returns data about components
(In reply to David Lawrence [:dkl] from comment #1)
> 3) If one specifies 'components' in the include_fields, all component sub
> fields should be returned by default unless there are one or more
> 'component.*' values or one or more 'component.*' values exist in
> exclude_fields.
> 
> I don't feel right for #1. Maybe #2. But really #3 seems better IMO.
> 
> Thoughts?

4) Remove the sub-filtering in _component_to_hash. We don't do it in _version_to_hash or _milestone_to_hash, so why here?

If there is a valid reason for this, then I would opt for #3 too. But this also needs to be documented in the POD, since noone would know about it otherwise.
(In reply to David Lawrence [:dkl] from comment #1)
> I don't feel right for #1. Maybe #2. But really #3 seems better IMO.
> 
> Thoughts?

Ideally 4, else 2 + 3.
I think we need 2 + 3 but instead of 4 we should add the same logic to _version_to_hash and _milestone_to_hash.
We are going to add the same logic to version and milestones. After thing about it more, this is how I think it should work. It makes a much easier to code for.

If the parent is specified (e.g. 'components') in the include_field value, then only the exclude value will be honoured for the sub values
If the parent is specified in the exclude_field, then it is excluded. Neither the include_value or exclude_value of the sub values will be used.
If the parent is not specified, then both the include field and exclude field will be honoured for the sub values.

This means if you only want the names of components you would say include_fields => ['components.name']
If you want all component information except the flags you would say exclude_fields => ['components.flag_names']. You could optional say include_fields => ['components'] if you didn't want version or milestone information.
Attached patch v1 patchSplinter Review
So here is my first crack at fixing this. I needed to do the excludes before the includes in B/W/Utils::filter_wants, since we need to exclude a sub field if the parent is included.

I have tested this against the scenarios listed above, and it seems to work as described. Obviously if the above comment is not the desired implementation, then this patch won't be any good.

  -- simon
Attachment #711255 - Flags: review?(LpSolit)
Comment on attachment 711255 [details] [diff] [review]
v1 patch

>=== modified file 'Bugzilla/WebService.pm'

>+Some RPC calls support specifying sub fields.
>+
>+If an RPC call states that it support sub field restrictions, you can

Nit: I would merge them into a single paragraph. No need for the empty line, IMO.



>=== modified file 'Bugzilla/WebService/Product.pm'

> sub _flag_type_to_hash {
>     my ($self, $flag_type, $params) = @_;

$params is no longer used by this method, so you should remove it, from the callers also.


Otherwise looks good and works great. Thanks a lot for the patch! :) r=LpSolit
Attachment #711255 - Flags: review?(LpSolit) → review+
Flags: approval4.4+
Flags: approval+
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/WebService.pm
modified Bugzilla/WebService/Product.pm
modified Bugzilla/WebService/Util.pm
Committed revision 8576.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.4/
modified Bugzilla/WebService.pm
modified Bugzilla/WebService/Product.pm
modified Bugzilla/WebService/Util.pm
Committed revision 8517.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: testcase?
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.