Skip to content

Include more error data in HubspotClientError #3

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.

Sign up for GitHub

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

Open
frankoid wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Include more error data in HubspotClientError #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
1888951
Include complete error data in HubspotClientError
Jan 21, 2016
1d49195
Include failureMessages in HubspotClientError message (if present)
Jan 21, 2016
b7d94ed
Avoid mutating argument
Jan 21, 2016
68c0482
Include the plain error_message in HubspotClientError
Jan 21, 2016
501cd7c
Improve error message
Jan 25, 2016
5a53d7d
Restore previous interface.
Jan 25, 2016
a3ce40c
Fixing test failure
Jan 25, 2016
4e92c07
Add doc for new param
Jan 26, 2016
4c97e4d
Add http status code to HubspotClientError creation.
Jan 26, 2016
9340ab8
Add commas to last elements of dicts
Mar 16, 2016
80dbe71
Add failureMessages to _HUBSPOT_ERROR_RESPONSE_SCHEMA
Mar 16, 2016
0d940f5
Format string using .format() for consistency with existing code
Mar 16, 2016
4e9e7d1
Split string to avoid line longer than 80 chars
Mar 16, 2016
31d35b6
Rename test
Mar 16, 2016
74c7ce1
Update HubspotClientError docstring
Mar 16, 2016
5e260fd
Fix param order in docstring
Mar 17, 2016
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
5 changes: 4 additions & 1 deletion hubspot/connection/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
from requests.adapters import HTTPAdapter
from requests.auth import AuthBase
from requests.sessions import Session
from voluptuous import Schema
from voluptuous import Optional, Schema

from hubspot.connection._validators import Constant
from hubspot.connection.exc import HubspotAuthenticationError
Expand All @@ -48,6 +48,7 @@
'status': Constant('error'),
'message': unicode,
'requestId': unicode,
Optional('failureMessages'): list,
},
required=True,
extra=True,
Expand Down Expand Up @@ -204,6 +205,8 @@ def _require_successful_response(response):
raise exception_class(
error_data['message'],
error_data['requestId'],
error_data,
Copy link
Contributor

@LuRsT LuRsT Mar 16, 2016

Choose a reason for hiding this comment

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

We are explicitly including only the data that we need in the _HUBSPOT_ERROR_RESPONSE_SCHEMA schema variable, since this commit is about adding more error data, it should be included in the schema so it can be validated.

Copy link
Author

@frankoid frankoid Mar 16, 2016

Choose a reason for hiding this comment

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

The main point of this change is to expose the whole of error_data, i.e. the whole of the error response body, to users of the library. The reason I want to do this is because I have code in my app that depends on the specifics of particular errors and HubSpot returns quite a few different things in the error response depending on which API endpoint is being used and what kind of error occurs so it would be very difficult to enumerate all of the possible properties that can occur inside error_data. In fact it would probably be impossible without access to HubSpot's internal code because we can only know about properties that we've seen in errors that we've encountered, and it'd probably be impossible for us to be confident that we'd triggered every single possible kind of error from every HubSpot API endpoint.

Including the whole of error_data like this does break encapsulation but I think that the flexibility it gives to users of the library makes it worth it.

However, the code in HubspotClientError does explicitly look at the failureMessages property so I'll add that to the schema (once I've read up on voluptuous - I'm not familiar with it).

Copy link
Author

@frankoid frankoid Mar 16, 2016

Choose a reason for hiding this comment

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

I've added failureMessages to the schema in 672019f

Copy link
Contributor

@LuRsT LuRsT Mar 16, 2016

Choose a reason for hiding this comment

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

@frankoid That sounds good to me, I believe at the time we even thought about doing something like this even thought it does indeed break encapsulation, but I completely understand the pain of working with Hubspot's API and their documentation...

I'll discuss with my colleagues this specific part of your implementation, but in my opinion, that's great. The only thing that I'm thinking is whether to make the schema for failureMessages a little bit more complete, but without checking their API documentation it's hard to tell what should the schema be, but since the idea is to have flexibility with whatever failureMessages you get, I'd say that the Optional('failureMessages'): list should suffice.

Copy link
Contributor

@LuRsT LuRsT Mar 17, 2016

Choose a reason for hiding this comment

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

Hi @frankoid, could you give me some example output for the failureMessages or a way for me to replicate them, just so I can see the structure for it, I'm thinking that if we have a proper structure, we could use a Record to store it instead of just a list straight out of their response in order to have a higher level of abstraction.

I'll look at their Documentation in the interim.

Copy link
Author

@frankoid frankoid Mar 17, 2016

Choose a reason for hiding this comment

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

Hi @LuRsT, failureMessages is used when there are errors during a bulk contact update - it contains the specific error that happened for each contact that failed to be updated (and the index of it so that you can figure out which of the contacts that you sent the error applies to).

If you look in test_client_error_response then you can see an example of what failureMessages looks like. I think you can reproduce a similar error by using save_contacts (in hubspot-contacts) with a contact with no email address (or if that doesn't work then try a contact with the empty string as email address).

If you think that giving failureMessages special treatment isn't the right way to go then I can remove it - client code would still be able to get at the failureMessages by poking around in error_data.

Copy link
Contributor

@LuRsT LuRsT Mar 21, 2016

Choose a reason for hiding this comment

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

Hi @frankoid, sorry, I didn't mean the failureMessages, I actually meant the error_data, right now I don't have much time to deal with this (since we're in the middle a sprint right now) but I can tell you what I have in mind so you can think about this too.

My "issue" with passing error_data is that it would give too much low level data to hubspot-connection clients, which is not great since it's a different level of abstraction. I see the ideal situation for this being a structure that we can pass to clients with the same level of abstraction as message and code (meaning, in a proper structure) so that the client can deal with it accordingly, without having to parse through undocumented Hubspot data. Now, in order to do this, of course I'd need some time to check the format and all the error possibilities that Hubspot could get so that I can find an appropriate Schema for it.

Once I have some time, that's what I'll do. I'll let you know once I resume working on your pull request.

Copy link
Author

@frankoid frankoid Mar 21, 2016

Choose a reason for hiding this comment

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

Hi @LuRsT, I don't think that the detailed contents of error_data are documented. As an example, failureMessages isn't mentioned on the page that documents the resource that returns it.

Without such documentation I'm not sure how you would compile a list of all of the error possibilities. You could try to do it by experimenting with the API, but there'd always be the possibility that a user of the library might come across an error situation (and corresponding HubSpot response) that your experiments hadn't uncovered.

Copy link
Contributor

@LuRsT LuRsT Mar 22, 2016

Choose a reason for hiding this comment

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

Hi @frankoid, I imagined that that would be the case 😞, which means that a list of all error possibilities is not a possibility, but we could still get an idea of what kind os structure could we use to represent the information that Hubspot is passing, and if they really have a very unstructured way of displaying those errors (which I doubt) we could add something like error_data now, but I'd prefer to keep it as structured as possible, but trying to of course, avoid the situation that you described where a user gets something from Hubspot that hubpost-connection is not ready for.

It's not easy work, which is why I'll need to back to you once I have some proper time so we can decide on the next steps, feel free to explore possibilities and share, I'm more than happy to discuss alternative solutions 😃.

response.status_code,
)
elif 500 <= response.status_code < 600:
raise HubspotServerError(response.reason, response.status_code)
Expand Down
12 changes: 11 additions & 1 deletion hubspot/connection/exc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,23 @@ class HubspotClientError(HubspotException):
HubSpot deemed the request invalid. This represents an HTTP response code
of 40X, except 401

:param unicode error_message: The error message returned by HubSpot
Copy link
Contributor

@LuRsT LuRsT Mar 17, 2016

Choose a reason for hiding this comment

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

Sorry for being pedantic but the order is wrong, error_message should be before request_id

Copy link
Author

@frankoid frankoid Mar 17, 2016

Choose a reason for hiding this comment

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

Fixed :)

:param unicode request_id:
Copy link
Contributor

@LuRsT LuRsT Mar 16, 2016

Choose a reason for hiding this comment

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

The docstring should be updated with error_message, I don't understand why it wasn't there for msg in the first place 😕

:param dict optional error_data: The response data returned by HubSpot
:param int optional http_status_code:

"""
def __init__(self, msg, request_id):
def __init__(self, error_message, request_id, error_data=None, http_status_code=None):
if error_data and 'failureMessages' in error_data:
msg = u'{0} ({1!s})'.format(error_message, error_data['failureMessages'])
else:
msg = error_message
super(HubspotClientError, self).__init__(msg)

self.http_status_code = http_status_code
self.error_message = error_message
self.request_id = request_id
self.error_data = error_data


class HubspotAuthenticationError(HubspotClientError):
Expand Down
3 changes: 1 addition & 2 deletions hubspot/connection/testing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,8 @@ def __exit__(self, exc_type, exc_value, traceback):
return

expected_api_call_count = len(self._expected_api_calls)
pending_api_call_count = expected_api_call_count - self._request_count
error_message = \
'{} more requests were expected'.format(pending_api_call_count)
'{} requests were expected, but only {} calls were received'.format(expected_api_call_count, self._request_count)
assert expected_api_call_count == self._request_count, error_message

def send_get_request(self, url_path, query_string_args=None):
Expand Down
38 changes: 36 additions & 2 deletions tests/test_connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,12 +244,25 @@ def test_server_error_response(self):

def test_client_error_response(self):
request_id = get_uuid4_str()
error_message = 'Json node is missing child property'
error_message = 'Errors found processing batch update'
failure_messages = [
{
'index': 0,
'error': {
'status': 'error',
'message': 'Email address is invalid',
}
}
]
invalid_emails = ['']
body_deserialization = {
'status': 'error',
'message': error_message,
'correlationId': '2ebc27ce-cc2d-4b81-99f3-01aa96e05206',
'invalidEmails': invalid_emails,
'failureMessages': failure_messages,
'requestId': request_id,
}
}
response_data_maker = _ResponseMaker(400, body_deserialization)
connection = _MockPortalConnection(response_data_maker)

Expand All @@ -258,7 +271,28 @@ def test_client_error_response(self):

exception = context_manager.exception
eq_(request_id, exception.request_id)
eq_("Errors found processing batch update "
"([{u'index': 0, u'error': "
"{u'status': u'error',"
" u'message': u'Email address is invalid'}}])",
str(exception))
eq_(error_message, exception.error_message)
eq_(invalid_emails, exception.error_data['invalidEmails'])
eq_(failure_messages, exception.error_data['failureMessages'])
eq_(400, exception.http_status_code)

def test_str_client_exception_without_failure_messages(self):
request_id = get_uuid4_str()
error_message = 'Json node is missing child property'
error_data = {
'status': 'error',
'message': error_message,
'requestId': request_id,
}

exception = HubspotClientError(error_message, request_id, error_data)
eq_(error_message, str(exception))
eq_(error_message, exception.error_message)


class TestAuthentication(object):
Expand Down
4 changes: 2 additions & 2 deletions tests/test_testing_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ def test_multiple_api_calls(self):

def test_unsuccessful_api_call(self):
exception = \
HubspotAuthenticationError('Must authenticate', get_uuid4_str())
HubspotAuthenticationError('Must authenticate', get_uuid4_str(), {})
expected_api_call = UnsuccessfulAPICall(
_STUB_URL_PATH,
'GET',
Expand All @@ -189,7 +189,7 @@ def test_unsuccessful_api_call(self):
def test_too_few_requests(self):
connection = \
self._make_connection_for_expected_api_call(_STUB_API_CALL_1)
error_message = '1 more requests were expected'
error_message = '1 requests were expected, but only 0 calls were received'
with assert_raises_substring(AssertionError, error_message):
with connection:
# Do not make any requests in the connection
Expand Down