Mattermost-server: API: `DELETE /plugins/{plugin_id}` responds with a 400 when the plugin doesn't exist.

[irbrad]

Summary

DELETE /plugins/{plugin_id} responds with a 400 when the plugin doesn't exist.

Steps to reproduce

Issue a request against the API using either DELETE /plugins/{plugin_id} or by using AdminClient.RemovePlugin(pluginId) in which the plugin doesn't exist.

Expected behavior

A response code of 409 with appropriate body data to assist the application and/or developer with locating the issue. The API docs also need updated to reflect this change.

409 seems like the best match to use according to the RFC 7231 document:

The 409 (Conflict) status code indicates that the request could not
be completed due to a conflict with the current state of the target
resource.  This code is used in situations where the user might be
able to resolve the conflict and resubmit the request.  The server
SHOULD generate a payload that includes enough information for a user
to recognize the source of the conflict.

Observed behavior (that appears unintentional)

A response of 400 is seen.

[amyblais]

Ticket for reference: https://mattermost.atlassian.net/browse/MM-17006.

[rvillablanca]

I've created a PR but this would be the only case where a http 409 response code is returned. Maybe this http code is appropriate but it seems to be very specific and after having seen the api and the documentation, this kind of errors are returned as 400. Also the error has all required information in order to locate the issue. @jespino what do you think about this ?

[crspeller]

@irbrad I don't think that your interpretation of 409 is correct. A 409 implies a conflict. There is no conflict here, the plugin just doesn't exist. I think a generic 400 error is appropriate.

Is there a reason why you are looking for the code to be more specific?

[Ogek]

The 400 HTTP error is good to indicate syntax errors in request.

Source: https://tools.ietf.org/html/rfc7231#section-6.5.1.

I think, in this case, the most appropriate error code is 404 because the resource was not found.

EDIT:

I've noticed in official documentation, the users, bots and others endpoints, for DELETE method returns 404 if resource didn't exists. I think it would be right to adapt to the "standard".

So, because I want to make my first contribution to Mattermost, I think this can be good start.

[amyblais]

Hi @Ogek, you can open a PR for that and our devs will review it.

[lieut-data]

@Ogek, would you still be interested in helping us change the behaviour here?

[proishan11]

@lieut-data Can I work on this?

[jespino]

Yes @proishan11, go for it :)

[hanzei]

Hey @proishan11, how are you doing with this ticket?

[rvillablanca]

Hi guys. If @proishan11 is not available I would like to grab this issue. I have an old PR that change the http status code to 409 so I would have to change that to 404 and maybe update the docs.

[hanzei]

Go for it @rvillablanca

@proishan11 I hope you are fine with this

[hanzei]

Thanks for fixing this @rvillablanca :+1:

[rvillablanca]

You all are welcome! @hanzei