Shouldn't trying to delete a non-existent Confluence space raise an ApiNotFoundError?

[stevecj]

I don't yet know if this problem exists with other methods, but when I use the delete_space method of a Confluence instance with a key that does not match an existing space, I get a plain ApiError and have to drill into its details to find out that it is caused by a 404. Shouldn't an ApiNotFoundError be raised instead of a generic ApiError?

[stevecj]

I see a comment in the code about that.

                # Raise ApiError as the documented reason is ambiguous

It is true that there is some ambiguity about the meaning, but I think that is only as a security measure so a user can't use the response code to determine whether something exists.

It is still useful for a client app to know whether the object is reported to be not-found or not, and purposefully raising ApiError instead of ApiNotFoundError only makes that harder to do and makes the checking more likely to have bugs.

[Spacetown]

I think raising an error is sufficient. To check if a page exists you should use https://github.com/atlassian-api/atlassian-python-api/blob/1b5cf5eee871ab44ce31c1dd71f7452a67d186b2/atlassian/confluence.py#L88.

[stevecj]

Checking whether an object exists first is an extra API call and also can be subject to race conditions.

[Spacetown]

As described in the comment the reason is ambiguous. From the docs:

 404
Returned if there is no content with the given id, or if 
the calling user does not have permission to trash or
 purge the content. 

[stevecj]

Yes. That is not unusual for an API as a way of obscuring whether an object really does not exist vs being inaccessible to you. IMO, that does not mean the client of the API library does not want to know that the service says the object was not found.

I presume there are many users of this library like myself who are having to each write their own custom workarounds to this purposeful omission.

[Spacetown]

ApiNotFound is wrong, it could also be ApiPermissionError. What's wrong to catch an ApiError?

[stevecj]

If, for example, I want to delete an object object if it exists, I want to ignore the error if (and only if) it is because the object does not exist. Yes, that response might be a lie, and I run into have another exception later in that case, but I still want to handle the case in which the object is reported to not exist differently — especially in the case where I know I am an admin so I will never get the error for the alternative reason.

[stevecj]

From https://authress.io/knowledge-base/choosing-the-right-http-error-code-401-403-404#:~:text=You%20may%20still%20want%20to,this%20case%20return%20the%20404.

You may still want to share the missing permissions or two to request access from. However, if the user shouldn’t know about the resource, then neither return who to contact, the missing permissions, nor a 403 suggesting that the resource exists. In this case return the 404.

So this is not something unique and strange to Atlassian. It is a legitimate case for 404. I think that makes it also a legitimate case for a ApiNotFound exception to the response code.

[Spacetown]

And the API can't distinguish between the two cases because both result in a 404. ApiError is the result of this 404 and nothing changes if we name it ApiNotFoundOrPermissionDeniedError.

[stevecj]

Yes, but it can distinguish between that case and other cases BESIDES those 2. It is also appropriate for the client to proceed as if the resource doesn't exist when it gets a 404. The 404 means that as far as the current user is concerned, the resource does not exist.

There are also cases where permission denied is reported instead of a 404, so I don't think there is a reason for an ambiguous ApiNotFoundOrPermissionDeniedError exception.

[Spacetown]

Please read the API docs (https://docs.atlassian.com/ConfluenceServer/rest/7.17.2/#content-delete) this can't be distinguished here. Other APIs like the GET (https://docs.atlassian.com/ConfluenceServer/rest/7.17.2/#content-getContent) specify only one error reason.

[stevecj]

I already know what that says, and I just read it again. I am not arguing that it says something different.

What I am arguing is that in the same way that the 404 is useful in spite of having some ambiguity, it is useful to raise ApiNotFoundError for a 404 response in spite of having some ambiguity. Why force the client code to dig into the exception to find that out because we purposefully chose to discard the information?

[Spacetown]

But as I wrote ApiNotFound is only the half truth. Do you have an example of your workaround?

[stevecj]

Well, I have to get the reason (exception) from the original exception and then check its response.status_code . That means I'm having to work with the underlying requests API at that point to get the info that I want. The fact that atlassian-python-api is backed by requests ought to be invisible for the most part.

[Spacetown]

I meant as code.

[stevecj]

I'm in the middle of some rewriting, but the last code I had looked basically like…

    try:
        client.delete_space(key)
    except ApiNotFoundError:
        # but this never happens
        pass
    except ApiError as e:
        if type(e) is not ApiError:
            raise  # Re-raise instance of subclass.
        requests_resp = getattr(e.reason, 'response', None)
        if getattr(requests_resp, 'status_code', None) != 404:
            raise  # Re-raise instance of ApiError if not for a 404.
        # Otherwise (ApiError is for 404 response) ignore.

[Spacetown]

In the delete_space call the API error is always the 404.

try:
        client.delete_space(key)
    except ApiError as e:
        # (ApiError is for 404 response) ignore.
        pass

[stevecj]

The docs specifically state that a 409 response may also be returned, and of course, regardless of what the docs say, there could always be a 500 error, etc.

[gonchik]

@stevecj how about wrap into error file?