777499 - WebService module to get, update, create and remove a product milestone

Status: ASSIGNED

(Bugzilla :: WebService, enhancement)

Description

[David Lawrence [:dkl]]

Create new WebService module allowing to manipulate milestones for a product via the WebService API.

Comment 1

[David Lawrence [:dkl]]

Attachment: Patch to add Milestone.pm to WebService API (v1)

Comment 2

[Koosha KM]

Comment on attachment 645881 [details] [diff] [review]
Patch to add Milestone.pm to WebService API (v1)

Since this patch is much like the patch for bug 777047, most of my comments on bug 777047 apply to this patch as well plus the following:

>=== added file 'Bugzilla/WebService/Milestone.pm'
>--- Bugzilla/WebService/Milestone.pm	1970-01-01 00:00:00 +0000
>+++ Bugzilla/WebService/Milestone.pm	2012-07-25 20:56:55 +0000
.
.
.
>+sub _milestone_to_hash {
>+    my ($self, $params, $milestone) = @_;
>+    return filter($params, {
>+        id           => $self->type('int', $milestone->id),
>+        name         => $self->type('string', $milestone->name),
>+        product_id   => $self->type('int', $milestone->product->id),
>+        product_name => $self->type('string', $milestone->product->name), 
>+        is_active    => $self->type('boolean', $milestone->is_active), 
>+        sort_key     => $self->type('int', $milestone->sortkey), 
>+    });
>+}

Probably leaks restricted products to unauthorized users. Also, I think you should call filter() after _milestone_to_hash() is called not in the body of _milestone_to_hash(). Calling filter() here may make _milestone_to_hash() prone to some problems in future if you include the returned hash of _milestone_to_hash() to a bigger one which may include a key name equivalent to an existing key name in _milestone_to_hash(). For example, if there are 'id' in both _milestone_to_hash() and the enclosing upper hash.

>+sub _get_milestone_objects {
>+    my ($function, $params) = @_;
>+
>+    my %milestone_objects;
>+    if (defined $params->{ids}) {
>+        foreach my $id (@{ $params->{ids} }) {
>+            # Return early if the supplied id is not an integer
>+            ThrowCodeError('param_invalid', 
>+                    { function => $function, 
>+                      param    => 'ids' }) unless $id =~ /^\d+$/;
>+             
>+            $milestone_objects{$id} ||= Bugzilla::Milestone->new($id);
>+
>+            ThrowUserError('milestone_unknown_id', { vers_id => $id })
>+                unless $milestone_objects{$id};
>+        }
>+    }

Undefined error: 'milestone_unknown_id'





>+=head2 Create Product Milestone
>+
>+=over
>+
>+=item C<create> B<EXPERIMENTAL>
>+
>+=over
>+
>+=item B<Description>
>+
>+Creates new milestone for an existing product.
>+
>+=item B<Params> 
>+
>+A hash containing the following milestone fields:
>+
>+=over
>+
>+=item product
>+
>+C<string> B<Required> The name of the product that the milestone 
>+will be added to.

Accepting product id will be an extra point.

Comment 3

[Koosha KM]

dkl: if you do not intend to work on this bug this soon, I would be happy to take on it as well as bug 777047. :-)
It would be nice if we could have it for 4.4.

Comment 4

[David Lawrence [:dkl]]

Attachment: Patch to add Milestone.pm to WebService API (v1)

Thanks for the earlier review. Here is a new patch addressing most of your comments.

dkl

Comment 5

[Frédéric Buclin]

Comment on attachment 684203 [details] [diff] [review]
Patch to add Milestone.pm to WebService API (v1)

All my comments from bug 419568 apply here as well. Please give them a look.


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

>         'Group'          => 'Bugzilla::WebService::Group',
>         'Product'        => 'Bugzilla::WebService::Product',
>         'User'           => 'Bugzilla::WebService::User',
>+        'Milestone'      => 'Bugzilla::WebService::Milestone',

The list is sorted alphabetically. Also, please list this new module in the POD of Bugzilla/WebService.pm.



>=== added file 'Bugzilla/WebService/Milestone.pm'

It's a pain to implement several methods at once. If one method is rejected by the reviewer, the whole review process must restart again. This delays reviews *a lot*.


>+use constant MAPPED_SETTERS => {
>+    sort_key => 'sortkey'
>+};

We don't use this constant is any other WS module. For consistency, please use what is already used in other update() methods.


>+sub create {

>+    # Check if the mandatory params are set. Ignore the optional ones.
>+    for my $param (qw(name product)) {
>+        if (not defined $params->{$param}) {
>+            ThrowCodeError('param_required', { function => 'Milestone.create',
>+                                               param => $param });
>+        }
>+    }

1) We use foreach instead of for everywhere else, 2) This is the job of Bugzilla::Milestone->create() to make sure mandatory params are set. This is also how we do it elsewhere. Here, you would throw a generic "param_required" error message, making it harder for the caller to determine which parameter is missing.


>+sub update {

>+    defined $params->{updates}

I don't understand what this param is. We should be consistent with other update() methods, where we never define a 'updates' hash but simply pass new field values.


>+    my $milestones = _get_milestone_objects('Milestone.update', $params, 1);
>+
>+    my $values = translate($params->{updates}, MAPPED_SETTERS);

Why defining _get_milestone_objects() instead of using params_to_objects() as we do everywhere else? This method seems overly complex to me. If the complexity comes from supporting milestones from several products at once, then this support should be dropped.


I didn't read the code very carefully, because all of my comments above are the same as in bug 419568 and they should be addressed first.

Comment 6

[Frédéric Buclin]

Too late for 4.4. We already released 4.4rc1, and 4.4rc2 is just around the corner. We don't have a plan for a 3rd release candidate, so this bug is postponed to 5.0.

Comment 7

[Matt Tyson]

I've been looking at implementing this in REST.

Could I get some agreement on what endpoints we should have?

Currently what I'm thinking of is this:

GET /rest/milestone/$id - Get a milestone by its ID.
GET /rest/milestone/$product_name - Get a list of milestones belonging to a product
GET /rest/milestone/$product_name/$milestone_name - Get a milestone given the product name and milestone name

PUT /rest/milestone/$id - Update a milestone.

DELETE /rest/milestone/$id - Delete a milestone with the given ID.
DELETE /rest/milestone/$product_name/$milestone_name - Delete a milestone given the product name and milestone name

POST /rest/milestone - Create a new milestone.

Comment 8

[Dylan Hardison [:dylan] (he/him)]

r=dylan, this sounds reasonable.

Comment 9

[Matt Tyson]

Attachment: Milestone.pm

Ok, Here's my first attempt at this.  Would anyone have the time to review it?

Save the file as Bugzilla/API/1_0/Resource/Milestone.pm and bugzilla should start responding to the REST calls.

Comment 10

[Matt Tyson]

Attachment: milestone.patch

I'm not sure why I threw a random .pm file at you.

Anyway, here's a patch, and I've also written .rst docs for the API.  That should make it a bit clearer what the API is intended to do.

Comment 11

[Matt Tyson]

Part of this patch includes a Milestone.get function.  This information is also available in the Product.get call.

I noticed that the Component rest endpoint has no get function, maybe we should remove the Milestone get to be consistent.

IMO it would make sense to update the docs to say that there's no get function and you should use the Product level get function to find this information.

Comment 12

[David Lawrence [:dkl]]

(In reply to Matt Tyson from comment #11)
> Part of this patch includes a Milestone.get function.  This information is
> also available in the Product.get call.
> 
> I noticed that the Component rest endpoint has no get function, maybe we
> should remove the Milestone get to be consistent.
> 
> IMO it would make sense to update the docs to say that there's no get
> function and you should use the Product level get function to find this
> information.

I think we would need to ask ourselves, how will people actually use the API? Will anyone ever just want to get information about a single milestone or component, or will they most likely always want to get a list of milestones or list of components in a single product? I suspect the latter will be true most of the time. If it is simple and not much duplication I suppose there is no harm to add simple get() methods to Component/Milestone/Version but maybe we should break out some of the duplicate code into separate Util type module and then reuse it in Milestone.get and Product.get. 

Originally I figured when someone wanted to get a list of milestones for a product they could just do /rest/product/Bugzilla?include_fields=milestones which would be equivalent to /rest/milestones/Bugzilla I suppose. But on the other hand having too many ways to get the same information will just end up being confusing to the end user and more work to document. So maybe not having get() for everything is not the best thing.

dkl

Comment 13

[Dylan Hardison [:dylan] (he/him)]

dkl makes a good point, perhaps this REST endpoint should always operate on a list of milestones?

can you give that a go? No need for any API to request a single milestone, that I can think of.

Comment 14

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8741208 [details] [diff] [review]
milestone.patch

Review of attachment 8741208 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/API/1_0/Resource/Milestone.pm
@@ +37,5 @@
> +    update
> +    delete
> +    create
> +);
> +

Basically we should have PUT (update/create/delete) and GET which take an array of milestone hashes (json objects).

I think that would be more resty and also map well with how you'd want to use it in code.

Comment 15

[Dylan Hardison [:dylan] (he/him)]

What I mean here is -- the set of milestones assigned to a product is a resource, and that resource is manipulated by uploading a new one.

API usage:

for product id 101:

GET http://bugzilla/rest/milestones/101
[
   {
      "id" : 1,
      "sortkey" : 10,
      "isactive" : true,
      "value" : "6.0"
   },
   {
      "id" : 2,
      "sortkey" : 11,
      "isactive" : true,
      "value" : "7.0"
   }
]

to rename 7.0 to 7.1

PUT http://bugzilla/rest/milestones/101
[
   {
      "id" : 1,
      "sortkey" : 10,
      "isactive" : true,
      "value" : "6.0"
   },
   {
      "id" : 2,
      "sortkey" : 11,
      "isactive" : true,
      "value" : "7.1"
   }
]

To add a new milestone:
PUT http://bugzilla/rest/milestones/101
[
   {
      "id" : 1,
      "sortkey" : 10,
      "isactive" : true,
      "value" : "6.0"
   },
   {
      "id" : 2,
      "sortkey" : 11,
      "isactive" : true,
      "value" : "7.1"
   },
   {
      "sortkey" : 11,
      "name" : "Batman",
      "isactive" : true,
      "value" : "8.0"
   },
]

it's up to the implementer if deleting a milestone is allowed.

Comment 16

[Frédéric Buclin]

(In reply to Dylan William Hardison [:dylan] from comment #14)
> Basically we should have PUT (update/create/delete)

This looks wrong to me and is very confusing. What we do currently follows "standards":

verb   -> action
-----------------
POST   -> create
PUT    -> update
DELETE -> delete
GET    -> read

http://www.restapitutorial.com/lessons/httpmethods.html

No reason to do it differently here.

Comment 17

[Matt Tyson]

Ok, so just to make sure I understand correctly how we want to do this.

Product is the resource being manipulated.  Identified by product ID.

GET http://bugzilla/rest/milestone/101 - Get a list of milestones for product 101.

PUT http://bugzilla/rest/milestone/101 - Update a list of milestones in product 101.
[
  {
    "id": 1,
    "name": "New Name"
  },
  {
    "id": 2,
    "name": "Another New Name"
  }
]

POST http://bugzilla/rest/milestone/101 - Create a list of milestones in product 101.
[
  {
    "name": "My New Milestone",
    "sortkey": 0
  },
  {
    "name": "Another New Milestone",
    "sortkey": 1
  }
]

DELETE http://bugzilla/rest/milestone/101 - Delete a list of milestones in product 101.

RFC7231 says that a payload within a DELETE has "no defined semantics".
Perhaps the delete call should be something like this:

DELETE http://bugzilla/rest/101?id=1&id=2&id=3

Another problem is that the milestone ids being submitted in the hashes may not belong to the product specified in the REST call.
What do we do in the case of attempted cross product modification?  I'd suggest we just die with a call to ThrowUserError with a message stating that you are modifiying a milestone that doesn't belong to the product.

Should we allow product names to be used as the identifier as well? EG:

GET http://bugzilla/rest/milestone/MyProduct
PUT http://bugzilla/rest/milestone/MyProduct
POST http://bugzilla/rest/milestone/MyProduct
DELETE http://bugzilla/rest/milestone/MyProduct

Dylan,  Can you let me know if I've understood this correctly?

Comment 18

[Frédéric Buclin]

IDs are purely internal values. Names make more sense than IDs, IMHO, as they are what users see and know.

Comment 19

[Matt Tyson]

(In reply to Frédéric Buclin from comment #18)
> IDs are purely internal values. Names make more sense than IDs, IMHO, as
> they are what users see and know.

I think we should support updating by using the Id or name. The problem with names is that they can change.  This will cause scripts to break. Allowing IDs makes the scripts a bit more robust.

Another oversight I think is the updating of milestone names.  The proposed API in comment 15 and comment 17 mean that the name could only be updated by the ID.

Comment 20

[David Lawrence [:dkl]]

(In reply to Matt Tyson from comment #19)
> (In reply to Frédéric Buclin from comment #18)
> > IDs are purely internal values. Names make more sense than IDs, IMHO, as
> > they are what users see and know.
> 
> I think we should support updating by using the Id or name. The problem with
> names is that they can change.  This will cause scripts to break. Allowing
> IDs makes the scripts a bit more robust.
> 
> Another oversight I think is the updating of milestone names.  The proposed
> API in comment 15 and comment 17 mean that the name could only be updated by
> the ID.

Just look for the existence of a key called 'new_name' and change the name if present.

dkl

Comment 21

[Dylan Hardison [:dylan] (he/him)]

> Another oversight I think is the updating of milestone names.  The proposed
> API in comment 15 and comment 17 mean that the name could only be updated by
> the ID.

My opinion is that we should not key off of product name for the URI, and updates to milestones require an id.

Let's run this by a user. Kohei is a big user of (BMO's) API, so I'd like him to weigh in on this, in particular what was stated in comment 15 and comment 17.