This adds the `OriginalRequest` to the `ApiException` object,

gregsochanik commented 10 years ago

when using the default HttpClientMediator.

More importantly, this allows the exact data of the original request to be pulled from an ApiException to help with error logging from within a consuming client, which is not something that AFAIK has been available up to now.

I've only been able to find details of the original EndpointUrl, which does not contain any headers, querystring or request payload. (happy to be corrected on this!)

I have left the OriginalRequest as a mutable property within Response, as it allowed me to change as little of the code as possible, with an eye to the pending async work.

NOTE: I've noticed that Response translates a WebException into an ApiWebException, but delving in to HttpClientMediator shows that this is caught in TryGetResponse and only re-thrown if the WebException Response is empty.

I have added a 2nd commit to pull the originalRequest up to the base ApiResponse so the request exception can be viewed there, but this causes a little confusion with the ApiException.Uri property. As this ctor is public I have left it in, but would be good to remove this at some point as is quite confusing.

gregsochanik commented 10 years ago

Also, this test:

https://github.com/gregsochanik/SevenDigital.Api.Wrapper/blob/bd418eaca7f14181a1eb069d4443ad728d88ec22/src/SevenDigital.Api.Wrapper.Integration.Tests/Exceptions/OriginalRequestLoggingTests.cs

shows how this data can now be accessed in a consuming client. This is a really useful addition from my point of view.

AnthonySteele commented 10 years ago

No objection to the idea of it - if you need more than just the request url in the exception, then recoding the whole request object is the way to do it.

I have the same concerns:

in this or a later pull request the property ApiException.Uri should go as it will be duplicate data.
Response was entirely immutable, would prefer it to stay that way? I expect that we don't want to capture the request for success responses. Though if it's simpler and faster to always record the request, there's no big reason to avoid this.

gregsochanik commented 10 years ago

re ApiException.Uri - No probs, I'm happy to do it in this PR, only prob is it could cause an integration issue if a client is relying on that property being there, so we'd need to update the major version number.

Making OriginalRequest immutable and passed in via the ctor is not a problem, but because the other 2 Response ctors are public and they don't set the OriginalRequest, we have a possible NullRef issue here if instantiated externally. Should we remove them? Why would anyone be using these externally anyway? (they're only used in tests)

AnthonySteele commented 10 years ago

Sorry I haven't had time to look though this in more detail, but We do tests whenever we use the api, where we make an ApiException (or descendant) and throw it to test the calling code's error handling. We already have the annoying overhead that we have to make a Response to make the ApiException. I don't want to extend that into making the Request to make the Response to make the Exception!

gregsochanik commented 10 years ago

Yup - good point! I'll leave the ctors as they are :-)

Just removing the Uri and making OriginalRequest immutable now, will commit shortly

AnthonySteele commented 10 years ago

Looks good to me, can we merge?

7digital-Api-Wrapper-Notifications commented 10 years ago

TC tests all pass

On 30 April 2014 14:04, Anthony Steele notifications@github.com wrote:

Looks good to me, can we merge?

— Reply to this email directly or view it on GitHubhttps://github.com/7digital/SevenDigital.Api.Wrapper/pull/158#issuecomment-41793588 .

Greg Sochanik Developer Api Device Integration

T: +44(0)7968062483 E: greg.sochanik@7digital.com W: www.7digital.com http://www.7digital/

@gregsochanik

This email, including attachments, is private and confidential. If you have received this email in error please notify the sender and delete it from your system. Emails are not secure and may contain viruses. No liability can be accepted for viruses that might be transferred by this email or any attachment. Any unauthorised copying of this message or unauthorised distribution and publication of the information contained herein are prohibited.

7digital Limited. Registered office: 69 Wilson Street, London EC2A 2BB. Registered in England and Wales. Registered No. 04843573.

7digital / SevenDigital.Api.Wrapper

This adds the `OriginalRequest` to the `ApiException` object, #158