PHP - Add ResponseStatusCode to API error/exception

[baywet]

related to #2216
To improve the error handling experience we need to add the status code the the main api error type:

• add a ResponseStatusCode field to the api exception class
• add corresponding accessors (if needed)
• populate the information in the request adapter when an error is encountered

Examples
microsoft/kiota-dotnet#68
microsoft/kiota-http-dotnet#82

[Ndiritu]

@baywet we added the native response some time back -https://github.com/microsoft/kiota-abstractions-php/blob/dev/src/ApiException.php#L14

Thoughts?

[baywet]

Thanks for sharing this. The only benefit I can see from doing this is the consumer has access to the response headers. But it muddies the water by requiring the consumer to know about the implementation type.
Those kind of changes should be discussed more broadly before they are implemented, we should have a consistent experience across languages.
Do you have more context about why it was added at the first place?

[Ndiritu]

Apologies, we should have discussed this more broadly.

This came up as we were testing the first preview of the Kiota-generated SDK and we ran into issues with our requests during testing. We thought it would be helpful to add the native response to the exception to make it easy to debug failed requests.

The native response type we opted to use is the PSR HTTP response type that is agreed & used across PHP HTTP clients, making it intuitive to handle for developers.

[Ndiritu]

I think adding more than the response code would be helpful for developers debugging failed requests

[baywet]

no worries, it happens to make quick calls sometimes.
Do you see people needing to access the response body? The only case when they might want to do that is if they didn't have an error mapping, and the body is present but wasn't parsed. But it'd a bit of an anti-pattern, we'd rather have them add the error mappings in the description.

[Ndiritu]

No I don't foresee the need to access the response body but think it wouldn't hurt to provide the full response object for debugging. Perhaps for less standardized APIs than Graph, the schema may not perfectly align with the response? Idk

[darrelmiller]

I have two concerns with including the response body by default in the exception object. The first is from a performance perspective, we shouldn't make the default behaviour more expensive than it needs to be. I realize this is only for exception scenarios, but it isn't that uncommon for HTTP responses to also include HTML renderings of the error message. We don't want folks allocating memory to hold onto those payloads when they don't care about them. If they explicitly want to access the body then there should be a way to express that in code.
The other concern is the fact that that response may contain EUII information. We really don't want people accidentally logging that information.

[Ndiritu]

Makes sense. Making the change. Does it make sense to expose response headers as well?

[baywet]

Let's wait until we get demands for the headers at this point. It's not going to be a breaking change anyway so I'm comfortable doing it Post GA

[Ndiritu]

@darrelmiller @baywet how should we handle error responses in Graph core's Large File Upload, Page Iterator and Batch Requests considering the ODataError models are generated in the service lib? Should we define custom ODataError models in Core?

Realised we had added the full native response to the exception to provide response payload visibility during those errors.

[baywet]

Why do the tasks need to deserialize errors instead of letting them bubble through and let the consumers, who will have the errors definitions, handle them? Can you give specific examples please?

[Ndiritu]

In a case like the Batch request builder, we'd need to specify error mappings for a failed request to make sense, otherwise the developer gets an ApiException that only provides the status code.
The ODataError models are only present in the service lib.

[baywet]

This is where we might need a bit of plaster, and have the core one be a base type that needs the mappings in the constructor, and the derived one in the service lib set things up. What do you think?

[Ndiritu]

I don't understand how the core base type will initialise a derived type in the service lib. Or what the base type would look like. Would you mind sharing an example?

[baywet]

classDiagram
    BatchBase <|-- BatchV1
    class BatchBase {
            +constructor(map[string]factory errorMappings)
     }
     class BatchV1 {
             + constructor()
     }

Loading

In the BatchV1 constructor, we just call the batch base constructor and pass a list of error mappings. The same kind of map that gets passed from the executor method to the request adapter send method.

Does that clarify the suggestion?

[Ndiritu]

Makes sense. I wasn't clear whether we were refactoring the error types or the batch request builders.

Will create another task to refactor our batch request builder. We initially provided it in the Core lib.