Add support for AdvanceErrorHandling when sending a message
#89
Conversation
|
In addition, I was wondering why the |
|
@tsidei Could you give me quick response about the two open questions above so I can finalise the PR? |
There was a problem hiding this comment.
Hi, apologies for late response
As for error fields, we should add the two missing ones (ErrorCode and ErrorIdentifier), ErrorClass is not used so yeah, let's mark it as deprecated with a comment.
I think go.sum wasn't checked in because the go tool doesn't generate it since there are no external dependencies. You can still create it, but it will be empty.
Overall the PR looks good, let's add the missing fields and it should be good to go. Sorry again for the long waiting. Tests also look better now, thanks!
mailjet_client_test.go
Outdated
| // TODO: Missing ErrorIdentifier in struct | ||
| // TODO: Missing ErorrCode in struct, but ErrorClass exists | ||
| // -> Correct format not defined in API docs |
There was a problem hiding this comment.
yeah, we should add those missing fields. Please add them
There was a problem hiding this comment.
I added them. Thanks!
mariuskiessling
force-pushed
the
advanced-error-handling-support
branch
from
f2b64ab
to
a051626
Compare
In addition to the support of the new field, this commit improves the testing of the SendMailV31 method. The tests now validate that the request payload matches the passed input and that encoding and authorization information are present. The APIErrorDetailsV31 struct now also supports the ErrorCode and ErrorIdentifier fields. The ErrorClass field of that struct is now deprecated. Signed-off-by: Marius Kießling <marius.kiessling@metro.digital>
mariuskiessling
force-pushed
the
advanced-error-handling-support
branch
from
a051626
to
0a7dc9e
Compare
Thanks for the feedback! I adapted your notes by adding the two new fields and marking the but no dependencies listed as the module doesn't have any. This is why I was asking about it. To the best of my knowledge, it is also common to check the file in even if no dependencies are used. But this shouldn't stop us from going ahead with this PR. I prepared my final commit and rebased onto master. You should now be able to finally review the code. Cheers! |
|
yes, the The rest of the PR looks good! Thanks for the contribution. |
|
Ah sorry, my bad. I don't have any strong feelings about the topic. Just go ahead without including the |
|
released in 4.0.0. Thank you! |
Background
The API documentation of Mailjet's API v3.1 specifies a parameter called
AdvanceErrorHandling(not a typo, this is actually what it is called) for itssendaction. If this option is set, the server will perform additional validations on the request before it marks a transaction as successful. This is e.g. required to catch emails that should be sent with a unauthorised sender email address. Without this flag, the server would accept a request with an unauthorised sender email address and return a payload to the client that contains message information for a resource that was not actually dispatched.Changes
MessagesV31struct now supports theAdvanceErrorHandlingfieldSendMailV31method has been re-written into a table test. The test now also validates that the client transmits the correct payload and sets the right encoding and authorization headers. More crucially, the test now also validates that the client can deal with both successful as well as non-successful responses from the server. This should have been done in the first place as there is specific error handling code for different HTTP status codes. If the client implementation was messed up, this could lead to error decoding problems because different kind of errors are decoded into different structs.Open Questions
Whenever the server returns an error with the HTTP status code
400or403, theAPIErrorDetailsV31struct is chosen to hold the decoded error contents. The server however returns two additional properties (ErrorIdentifierandErorrCode) that are currently not defined in theAPIErrorDetailsV31struct. In addition, the API documentation that is linked here under theAdvanceErrorHandlingparameter description does not seem to exist. Hence, I could not find any formal definition of the error's structure.ErrorClassstill a valid response field for an error? The API does not seem to return it for any faulty requests. If not, I will mark it as deprecated. We should not remove it to not break backwards-compatibility within the major module version.