Skip to content

fix: more consistent naming of HTTP status code related attributes #185

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions ibm_cloud_sdk_core/api_exception.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.

import warnings
from http import HTTPStatus
from typing import Optional

Expand All @@ -39,15 +40,28 @@ def __init__(self, code: int, *, message: Optional[str] = None, http_response: O
# Call the base class constructor with the parameters it needs
super().__init__(message)
self.message = message
self.code = code
self.status_code = code
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use the same name in the DetailedResponse and I think it's a bit more clear than the simple code.

self.http_response = http_response
self.global_transaction_id = None
if http_response is not None:
self.global_transaction_id = http_response.headers.get('X-Global-Transaction-ID')
self.message = self.message if self.message else self._get_error_message(http_response)

# pylint: disable=fixme
# TODO: delete this by the end of 2024.
@property
def code(self):
"""The old `code` property with a deprecation warning."""

warnings.warn(
'Using the `code` attribute on the `ApiException` is deprecated and'
'will be removed in the future. Use `status_code` instead.',
DeprecationWarning,
)
return self.status_code

Comment on lines +50 to +62
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it would be good to not remove the old property completely, but mark it deprecated and give some time to the the users to update their code. Also, the date in the comment is arbitrary, there is no real reason behind it. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By renaming the field from code to status_code above, I think you've introduced a breaking change despite the addition of this new getter function.
What I mean is... previously a user would need to access the code field like this:

    print('The status code is: ', exception.code)

That will no longer work as the user would need to do one of the following instead now:

    print('The status code is: ', exception.status_code)

or

    print('The status code is: ', exception.code())

IOW, I don't think the user can continue to just use exception.code, can they???

Copy link
Member Author

@pyrooka pyrooka Jan 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fortunately that's not the case. In short, the @property decorator creates a valid attribute (well only with a getter by default), so it can be used as before. See the example below;

>>> ex = ApiException(404, message="Not found...")
>>> ex.code
404
>>> ex.status_code
404

The only difference is, when calling the code property, a deprecation message will be printed to the stderr at warning level.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha! I didn't notice the @property decorator, and I probably wouldn't have known what that was for even if I did 😂.

def __str__(self) -> str:
msg = 'Error: ' + str(self.message) + ', Code: ' + str(self.code)
msg = 'Error: ' + str(self.message) + ', Status code: ' + str(self.status_code)
if self.global_transaction_id is not None:
msg += ' , X-global-transaction-id: ' + str(self.global_transaction_id)
return msg
Expand Down
2 changes: 1 addition & 1 deletion ibm_cloud_sdk_core/detailed_response.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ def get_headers(self) -> Optional[dict]:
"""
return self.headers

def get_status_code(self) -> int:
def get_status_code(self) -> Union[int, None]:
"""The HTTP status code of the service request.
Returns:
Expand Down
2 changes: 1 addition & 1 deletion test/test_api_exception.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,4 +85,4 @@ def test_api_exception():
mock_response = requests.get('https://test-for-text.com', timeout=None)
exception = ApiException(500, http_response=mock_response)
assert exception.message == 'plain text error'
assert str(exception) == 'Error: plain text error, Code: 500 , X-global-transaction-id: xx'
assert str(exception) == 'Error: plain text error, Status code: 500 , X-global-transaction-id: xx'
8 changes: 7 additions & 1 deletion test/test_base_service.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# coding=utf-8
# pylint: disable=missing-docstring,protected-access,too-few-public-methods
# pylint: disable=missing-docstring,protected-access,too-few-public-methods,too-many-lines
import gzip
import json
import os
Expand Down Expand Up @@ -357,6 +357,7 @@ def test_request_server_error():
prepped = service.prepare_request('GET', url='')
service.send(prepped)
assert err.value.code == 500
assert err.value.status_code == 500
assert err.value.http_response.headers['Content-Type'] == 'application/json'
assert err.value.message == 'internal server error'

Expand Down Expand Up @@ -399,6 +400,7 @@ def test_request_success_invalid_json():
prepped = service.prepare_request('GET', url='')
service.send(prepped)
assert err.value.code == 200
assert err.value.status_code == 200
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to have a testcase that still uses the old way err.value.code to make sure that still works (see my previous comment about this above)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I'll add some!

assert err.value.http_response.headers['Content-Type'] == 'application/json; charset=utf8'
assert isinstance(err.value.__cause__, requests.exceptions.JSONDecodeError)
assert "Expecting ':' delimiter: line 1" in str(err.value.__cause__)
Expand Down Expand Up @@ -453,6 +455,7 @@ def test_request_fail_401_nonerror_json():
prepped = service.prepare_request('GET', url='')
service.send(prepped)
assert err.value.code == 401
assert err.value.status_code == 401
assert err.value.http_response.headers['Content-Type'] == 'application/json'
assert err.value.message == error_msg

Expand All @@ -473,6 +476,7 @@ def test_request_fail_401_error_json():
prepped = service.prepare_request('GET', url='')
service.send(prepped)
assert err.value.code == 401
assert err.value.status_code == 401
assert err.value.http_response.headers['Content-Type'] == 'application/json'
assert err.value.message == error_msg

Expand All @@ -492,6 +496,7 @@ def test_request_fail_401_nonjson():
prepped = service.prepare_request('GET', url='')
service.send(prepped)
assert err.value.code == 401
assert err.value.status_code == 401
assert err.value.http_response.headers['Content-Type'] == 'text/plain'
assert err.value.message == response_body

Expand All @@ -514,6 +519,7 @@ def test_request_fail_401_badjson():
prepped = service.prepare_request('GET', url='')
service.send(prepped)
assert err.value.code == 401
assert err.value.status_code == 401
assert err.value.http_response.headers['Content-Type'] == 'application/json'
assert err.value.message == response_body

Expand Down
2 changes: 1 addition & 1 deletion test/test_container_token_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ def test_authenticate_fail_iam():
with pytest.raises(ApiException) as err:
authenticator.authenticate(request)

assert str(err.value) == 'Error: Bad Request, Code: 400'
assert str(err.value) == 'Error: Bad Request, Status code: 400'


@mock_iam_response
Expand Down