intercom-dotnet icon indicating copy to clipboard operation
intercom-dotnet copied to clipboard

ApiException doesn't have any Message set

Open nvivo opened this issue 8 years ago • 4 comments

The ApiException should set the ApiResponseBody as the Message property.

Any logger will use the Message property or ToString to display information about the exception. Not have any message prevents the user from finding out what happened.

nvivo avatar May 22 '17 21:05 nvivo

Hi @nvivo, OK, I think I understand, at the moment you do have access to the message via the exception body attribute but you are saying that some loggers will automatically just grab the message property so it would not be included then. Is that correct? i.e. you can handle the exception fine manually but not with some loggers? It does make sense and we should get to it but there might be a few other changes ahead of it in terms of priority. If you have an idea on this it would be great if you could submit a small PR for it? That would defo speed it up Thanks Cathal

choran avatar Aug 29 '17 13:08 choran

Hey @nvivo! We are currently using the ApiResponseBody to present most of the data and throwing a 404 with this code will return:

try
    {
        Company company = companyClient.View(new Company()
            {
                company_id = "232153"
             }); 
        } catch (ApiException e)
            {
                Console.WriteLine(e.ApiErrors);
                Console.WriteLine(e.ApiResponseBody);
            }
Intercom.Data.Errors
{"type":"error.list","request_id":"b2g8i49goo1364losjjg","errors":[{"code":"not_found","message":"Company Not Found"}]}

Do you mean you wanted the Exception to throw a Message where it would show just Company Not Found?

kmossco avatar Apr 01 '18 17:04 kmossco

Hi, I was going to do a PR and ended up working around and forgot about it.

The thing here is just about defaults, what should happen if the user doesn't explicitly look at ApiErrors/ApiResponseBody. By default, all libraries I have ever worked with set the .Message property to something meaningful, so that any generic handler or logger can see at lease some information about the error.

Of course anyone can handle the specific exception and look at the property. But when someone is not expecting that, the current log message is meaningless.

So, .Message could by default return the ApiResponseBody, so people at least can see what happened without changing the code. Something simple like this would be enough:

public override string Message => "Api Error: " + ApiResponseBody;

nvivo avatar Apr 02 '18 15:04 nvivo

@nvivo obrigado! That makes total sense and really wanted to make sure we captured exactly what you meant before we work on this. I'll look into making this override soon. 🙌

kmossco avatar Apr 03 '18 07:04 kmossco