Attempting to create a new collective with an already existing name triggers a 500 #109

Closed mejo- closed 1 year ago

In GitLab by @azul on Sep 7, 2020, 09:47

We should either handle the error and respond somewhat meaningfully or adjust the name for example with a (2) postfix.

Actually returning 500 is done on purpose, because an Exception is thrown. I thought that this would actually be helpful for the JS client to detect the failure. The response also contains a meaningful error message:

Example Curl request & response (click to expand)

`> nc-curl.sh jane -X POST --data "name=Testkollektiv2" -i https://nextcloud.local/index.php/apps/collectives/_collectives` ``` HTTP/2 500 server: nginx date: Wed, 10 Mar 2021 14:13:58 GMT content-type: application/json; charset=utf-8 content-length: 47 x-powered-by: PHP/7.3.27 expires: Thu, 19 Nov 1981 08:52:00 GMT cache-control: no-cache, no-store, must-revalidate pragma: no-cache content-security-policy: default-src 'none';base-uri 'none';manifest-src 'self';frame-ancestors 'none' referrer-policy: no-referrer x-content-type-options: nosniff x-download-options: noopen x-frame-options: SAMEORIGIN x-permitted-cross-domain-policies: none x-robots-tag: none x-xss-protection: 1; mode=block feature-policy: autoplay 'none';camera 'none';fullscreen 'none';geolocation 'none';microphone 'none';payment 'none' "Collective name already taken: Testkollektiv2" ```

I'm not sure what's the best response for an API call if the to-be-created object already exists. Do you have suggestions @azul?

In GitLab by @azul on Mar 10, 2021, 18:03

@mejo- My suggestion would be 422 - Unprocessable Entity:

The 422 (Unprocessable Entity) status code means the server understands the content type of the request entity (hence a 415(Unsupported Media Type) status code is inappropriate), and the syntax of the request entity is correct (thus a 400 (Bad Request) status code is inappropriate) but was unable to process the contained instructions.

I would like to avoid 500 - Internal Server Error as much as possible exactly because it can be caused by any other error. Parsing the message inside the response can be error prone. Plus rejecting a new collective with an existing name is desired server behaviour - not an unexpected error.

mentioned in commit 254369825a45f324b6f0299e98e1bc1a249e582d

That makes a lot of sense, agreed :blush:

The backend now returns a 422 when trying to create an existing collective. But we still have to improve the frontend so that it shows a more meaningful message.

In GitLab by @azul on Mar 14, 2021, 22:41

@mejo- I'm tempted to just open the collective if the user happens to be a member of it. I'd still display a meaningful error message. Any reason not to do so from your point of view?

Yeah, I really like that idea :blush:

In GitLab by @azul on Apr 2, 2021, 10:47

So I think we want different html response codes for this:

422 Name taken (and you don't have access to the circle / collective)
409 Conflict - collective already exists and you have access... include collective data to redirect user there
20x Created - collective and circle created
20y Ok - collective created, circle already existed and you were admin so you could create the collective
403 Permission denied - no collective, circle exists - but you are not admin so you may not create a collective for it

In GitLab by @azul on Apr 5, 2021, 21:18

The merge request that closed this issue (!166) addressed the first 3 scenarios. Creating a collective for an existing circle and the corresponding 403 will be handled as part of #115 .

nextcloud / collectives

Attempting to create a new collective with an already existing name triggers a 500 #109