7digital / SevenDigital.Api.Wrapper

A fluent c# wrapper for the 7digital API
MIT License
16 stars 29 forks source link

Empty responses cause Please() method to throw NonXmlException #224

Closed gregsochanik closed 8 years ago

gregsochanik commented 9 years ago

As the Please() method is supposed to perform de-serialisation, this is valid behaviour, as it's expecting a response body to de-serialise. But in certain cases, this is misleading, for example:

await _api.Create<Playlist>()
                .WithMethod(HttpMethod.Delete)
                .ForPlaylistUrl(playlistUrl)
                .Please();

...fires the delete correctly and the playlist is deleted, but the api endpoint ~/playlists/{playlistId} does not return a content body. This causes the wrapper to throw a NonXmlResponseException.

Using Response() works correctly as it doesn't attempt de-serialisation.

Is there a way we can handle this a little more gracefully for these kind of edge case situations?

The problem is that the api router converts the 204 NoContent response the playlist-api returns (which MUST NOT have a response body) into a 200 OK response.

I'm having a look at the RFC, but I'm not 100% whether a 200 OK response MUST have a response body, if it's a MAY, then the current behaviour is incorrect.

AnthonySteele commented 9 years ago

It would be nice if this worked. If you issue a GET then you want a response body. For other HTTP verbs, you might just want to know if the http request succeeded.

Problem is, how do you know if it succeeded? The Http specification would say look at the response status code. 7digital api router as originally designed would say look at the response payload. If you have neither then you're out of luck. The api router's lack of normal standard http response codes is really the problem here.

gregsochanik commented 9 years ago

"The api router's lack of normal standard http response codes is really the problem here."

Yup, agree 100% with this. Does the router allow >=400 status codes through yet? If not then yes it'll never work.

As the wrapper is supposed to represent how our api works (i.e. "RFC-7d" over and above RFC-2616), maybe it's indicative of a wider issue; that api endpoints MUST return a content body (for application/xml) no matter what....!

Therefore the DELETE ~playlist/{playlistId} endpoint should probably return at least the <response status="ok|error"></response> element for xml, rather than nothing.

AnthonySteele commented 9 years ago

AFAIK, the router allows >=500 status codes through.