7digital / SevenDigital.Api.Wrapper

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

Json deserialization endpoint by endpoint #104

Closed gregsochanik closed 8 years ago

gregsochanik commented 10 years ago

I'm going to need to start adding json de-serialisation to the ApiWrapper soon and wanted to discuss approaches.

My idea is to carry on with the Attribute based markup of the Schema library by including something like:

[Accept("application/json")] [Accept("application/xml")]

or even just

[Xml] [Json]

or similar. Which we can add to the top of a schema class like we do with the [ApiEndpoint] attribute.

This would then tell the wrapper what accept header to request with and also what serialization library to use the other end.

The important this is that it would default to Xml, so that we could switch over to json deserialisation endpoint by endpoint.

Thoughts?

raoulmillais commented 10 years ago

Attributes sound sensible to me. My instinct is to favour the first option as we might want to add some sort of feeds support to the wrapper in the future or start using a custom media type etc etc and the parameterisation leaves it open to extension.

I wonder whether "ContentType" would be better? Accept(-encoding) is what we send in the request and ContentType is what we get back. I guess it boils down to whether we are defining what is supported by the endpoint or what the client should ask for.

Assuming we go the "ContentType" route, we could default to XML unless there is the attribute, in which case we request JSON? This would:

If we discover a use-case where you'd be likely explicitly want XML if the endpoint supports both. We could always add XML ContentTypeAttributes and make preferred contenttype configurable somehow.

gregsochanik commented 10 years ago

Agreed, [ContentType] would be a better option for the attribute. They are designed to show what the api endpoint supports so does make more sense.

Assuming we go the "ContentType" route, we could default to XML unless there is the attribute, in which case we request JSON?

Yeah, this was the idea, you'd only need to mark the schema objects if you explicitly wanted them to be handled using json, in order to reduce noise and remove the need to decorate all the schema objects.

The api returns XML by default so not having an Attribute would be the equivalent of requesting with "Accept:*", hence currently returning xml. We can future proof this by having the wrapper make the de-serialisation decision based on the Content-type header in the response.

AnthonySteele commented 10 years ago

Let's talk about responses first.

The response should come back with a Content-Type: header. So it won't be hard for the API wrapper to deal with either format on the fly, i.e. in pseudo-code:

if (contenttype is xml) { return ParseXml(response.body) } else if (content type is json) { return ParseJson(response.body) } else { throw an error };

I don't think that Api wrapper json decoding support is hard - just add json.net, right ;)

So given that the wrapper can theoretically cope with both, and our apis all serve xml by default (among potentially other content types), why does a client ever need to specify that it wants json?

I can think of 2 possible reasons:
1) We believe that the json response is significantly smaller and faster
2) We might have an api that can only serve json.

Either of these are valid reasons, I am in favour of flexibility.

The client sends an accept header listing one or more content types that it can accept in order of preference. Currently it just says "I want xml" but this can change.

We could consider marking a dto as [PreferJson] to signal that on an endpoint the wrapper prefers json but can parse XML (as in truth it can). e.g. application/json;q=1.0,application/xml;q=0.9

The key to content negotiation is that the client and server both can potentially support multiple content types (e.g. json or xml encoding of the same dto) and they negotiate the best fit between the types that they both support. We are heading towards a situation where the server and client most likely both support xml and json; so we might need "hints" to know which one to prefer when both are allowed.

AnthonySteele commented 10 years ago

Lets talk about POST body formats (and PUT body too)

These are related to response formats in that a server may likely do both the same way due to technologies used – e.g. a server that only knows how to respond with json is likely to be one that can only read json too.

They are different in that there is no round trip and no negotiation. The client sends a body and a content type. Essentially it says “here’s some form-url encoded data” or “here’s some json data” and the server can only respond with an error code if it cannot cope with that.

An attribute could still work here, but the semantics are different, there is no “preference”. Perhaps mark up the dto as [HttpPost] [JsonBody] or as [PostJson] ?

gregsochanik commented 10 years ago

We believe that the json response is significantly smaller and faster

It is slightly smaller and faster, but most of our endpoints offer http gzip, which virtually negates this. It's a bigger deal for https only calls which have http gzip turned off (not that there are any). It is faster though (de/serialization is a concern too, json again faster than xml - esp the XmlSerializer which we have to use over the DataContract one - I've not looked into perf comparisons), and as it's all handled within the wrapper, a consuming client should never really care about how the data is transferred.

We might have an api that can only serve json.

That could be a vague possibility. We don't, but there was talk of the playlist-api being json only (again, not that there's any point, as setting up different response content types is pretty trivial).

I like the idea of simply specifying the Accept header application/json;q=1.0,application/xml;q=0.9 and reading the rather than having to deal with attributes at all for requesting! I think that would solve most of the issues for now wouldn't it, and would result in less code?

With regards to POSTing, it would be preferable to send Json if the endpoint it supports it as it results in smaller payloads. Hence as you say, an attribute would work, it could simply be a property of the already existing [HttpPost]. i.e. [HttpPost(ContentType = "application/json")]

AnthonySteele commented 10 years ago

I expect that this issue is parked for the time being?

AnthonySteele commented 9 years ago

Api wrapper can now request "prefer json". We may want to use this for testing deserialisation into dtos some time?

gregsochanik commented 9 years ago

Have just had a brief look at this issue. It pretty much works at the moment using ResponseAs<T>, but we need to take into account the non standard json setup for single dtos as the current implementation will be expecting:

{
  "id": 123,
  "title": "An album"
}

where our json response would be

{
  release: {
      "id": 123,
      "title": "An album"
   }
}

It's possible to create a custom json deserializer for Json.Net (JsonConverter) which would be a more elegant solution that having to add directive attributes to the Schema library. I've done this within the playlist-api, so when I have a mo can add this functionality to the wrapper.

AnthonySteele commented 9 years ago

The class ResponseDeserializer handles the actual reading of xml or json. In the xml case it it told when to "unwrap" the response, i.e. start deserializing with the first node under the <response> node instead of the whole document for ResponseAs<T>.

The issue is that the case where content type = json, unwrapResponse=true isn't implemented yet. The json unwrap case isn't implemented here yet - is it possible to do something similar to the xml? For json, I think that we want to unwrap 1 level of object hierarchy not 2.

gregsochanik commented 9 years ago

Sure - we can do the same thing, use a JsonTextReader to read into the first element and then deserialize. Will have a go now.