Fix exception handling in ApiMethodWrapper.handle_neptune_http_errors

[AleksanderWWW]

…tune_http_errors

Before submitting checklist

• Did you update the CHANGELOG? (not for test updates, internal changes/refactors or CI/CD setup)
• Did you ask the docs owner to review all the user-facing changes?

[codecov]

Codecov Report

Patch coverage is 85.71% of modified lines.

Files Changed	Coverage
...eptune/internal/backends/hosted_file_operations.py	ø
src/neptune/common/backends/utils.py	50.00%
...eptune/internal/backends/swagger_client_wrapper.py	91.66%

📢 Thoughts on this report? Let us know!.

[aniezurawski]

Just make source_exception an Optional (source_exception: Optional[Exception]) and check if it's not None.

[aniezurawski]

This if seems to be unnecessary.

[aniezurawski]

Please don't expose bravado exceptions. ;)

How about using NeptuneApiException?

[aniezurawski]

We still may want to raise a default_exception. Just without a cause (from).

[AleksanderWWW]

@aniezurawski
BTW
Shouldn't we switch places of those two if statements?

    if response.status_code >= 300:
        ApiMethodWrapper.handle_neptune_http_errors(response)
    if response.status_code in (
        HTTPUnprocessableEntity.status_code,
        HTTPPaymentRequired.status_code,
    ):
        raise NeptuneLimitExceedException(reason=response.json().get("title", "Unknown reason"))

They have 422 and 402 codes.

[aniezurawski]

Again let's wrap it in exept AttributeError and just stringify all arguments if somthing goes wrong.

[aniezurawski]

@aniezurawski BTW Shouldn't we switch places of those two if statements?

    if response.status_code >= 300:
        ApiMethodWrapper.handle_neptune_http_errors(response)
    if response.status_code in (
        HTTPUnprocessableEntity.status_code,
        HTTPPaymentRequired.status_code,
    ):
        raise NeptuneLimitExceedException(reason=response.json().get("title", "Unknown reason"))

They have 422 and 402 codes.

Huh. Good point. In this case it look like handle_neptune_http_errors was meant to not raise anything if there is no source or default excpetion. Now I see the bug was introduced in 1dfa30a. In that case we should just restore the old behaviour.

[AleksanderWWW]

@aniezurawski BTW Shouldn't we switch places of those two if statements?

    if response.status_code >= 300:
        ApiMethodWrapper.handle_neptune_http_errors(response)
    if response.status_code in (
        HTTPUnprocessableEntity.status_code,
        HTTPPaymentRequired.status_code,
    ):
        raise NeptuneLimitExceedException(reason=response.json().get("title", "Unknown reason"))

They have 422 and 402 codes.

Huh. Good point. In this case it look like handle_neptune_http_errors was meant to not raise anything if there is no source or default excpetion. Now I see the bug was introduced in 1dfa30a. In that case we should just restore the old behaviour.

I don't follow. What's then the point of calling this with None as exception if in that case it's not supposed to do anything? Or am I missing something?