Include more error data in HubspotClientError #3
Conversation
Including all error information returned by HubSpot aids troubleshooting and allows better error reporting to the user.
This is for use by code that wants to show a simple error message to the user, without technical details. Exception.message still contains the technical details (i.e. failureMessages) so that generic exception logging code will log the details needed for diagnosing the failure.
| 'index': 0, | ||
| 'error': { | ||
| 'status': 'error', | ||
| 'message': 'Email address is invalid' |
There was a problem hiding this comment.
It's easier to add new items to a dictionary/list if the last element is followed by a trailing comma, could you add one here?
There was a problem hiding this comment.
Yes that makes it nicer, I've added the trailing commas.
|
Thank you very much for your contribution, I'll be doing a review of the commits, hold tight 😄 |
| @@ -204,6 +204,7 @@ def _require_successful_response(response): | |||
| raise exception_class( | |||
| error_data['message'], | |||
| error_data['requestId'], | |||
| error_data, | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
I've added failureMessages to the schema in 672019f
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😃.
frankoid
force-pushed
the
exception-error-data
branch
from
49640b9
to
de6f02d
Compare
hubspot-connection relies on the existing interface.
The status code is significant in indicating the reason for failure. This is neccesary for clients to act appropriately depending on the failure.
Makes it easier to add new items.
...so that we validate that it is a list (if it is present).
frankoid
force-pushed
the
exception-error-data
branch
from
de6f02d
to
74c7ce1
Compare
|
I think I've now addressed all comments so far |
| @@ -30,7 +30,8 @@ class HubspotClientError(HubspotException): | |||
| of 40X, except 401 | |||
|
|
|||
| :param unicode request_id: | |||
| :param dict optional error_data: | |||
| :param unicode error_message: The error message returned by HubSpot | |||
There was a problem hiding this comment.
Sorry for being pedantic but the order is wrong, error_message should be before request_id
|
@frankoid, sorry for not getting back to you, we haven't had any time on the last sprints to spend time on this PR. This is just to let you know that we still remember your PR and will get back to it ASAP. Thanks for your patience |
|
Thanks for looking at this. I'm actually no longer working for the company where I developed this change so I won't be able to change this branch directly, however @JordanBright, @kevinbennet and @marco-primiceri might be interested in continuing with it. If not and there are small changes you'd like me to make in order to get it merged then I can have a go. |
Including all error information returned by HubSpot aids troubleshooting and allows better error reporting to the user.
This branch also includes some other tweaks, for example a change to an error message. If you'd like to merge some of these changes but not others please let me know.