JohnnyCrazy / SpotifyAPI-NET

:sound: A Client for the Spotify Web API, written in C#/.NET
http://johnnycrazy.github.io/SpotifyAPI-NET/
MIT License
1.5k stars 305 forks source link

Retry-After Header Lookup #82

Closed TheUltimateC0der closed 8 years ago

TheUltimateC0der commented 8 years ago

Would be nice to check the time given in the "Retry-After" handler, to not overload the Spotify API.

It would be clever to have that in the "SpotifyAPI.Web.Models.Error" as Property or as a Property in an "SpotifyAPI.Web.SpotifyWebAPI" instance

For reference please check this : https://developer.spotify.com/web-api/user-guide/#rate-limiting

JohnnyCrazy commented 8 years ago

Sounds like a good idea, will have a look at it tomorrow! :+1:

erinxocon commented 8 years ago

I was thinking the same thing the other day!

JohnnyCrazy commented 8 years ago

Turns out this won't be as easy as I thought.

My idea was adding a HeaderCollection to the BasicModel which will be automatically filled once the client got the response-headers. To achieve this, we would need to add where T : BasicModel to all the Download-Methods so the Method can set the HeaderCollection appropriately. This works for 90% of the API-Calls, but sadly we also have those weird array API-Calls CheckSavedTracks CheckSavedAlbums etc.. Their response can either be a JArray or an Error-Object. I couldn't find a solution for creating a class for this kind of responses which will deserialize correctly and suits T DownloadData<T> where T : BasicModel.

So, you guys got any ideas for a clean implementation or a workaround for my approach?

(Maybe you have some thoughts @JimmyAppelt )

TheUltimateC0der commented 8 years ago

I think it will be totally okay if we have the headers only on the Error-Object. In my opinion you don't have to look at the headers when everything is okay.

JohnnyCrazy commented 8 years ago

I just pushed an implementation to the headers branch. Every Model now contains a ResponseInfo Response-property, which currently only holds WebHeaderCollection Headers but may contain more in the future.

Usage:

FullAlbum album = _spotify.GetAlbum("1KFttYHEjNkDMYQrV6qYMA");

//'Response' is marked as JsonIgnore
//Will contain response headers
album.Response.Headers 

I'm not quite happy with it, but I can't think of a nicer solution currently. I plan to move to HttpClient in the near future, then this hopefully will get a makeover.

Thoughts? @S3NS4Ti0N @erinxocon @JimmyAppelt

JohnnyCrazy commented 8 years ago

Implemented in 15d8fd87e22a9e0ae4a9a5f0071152eefcf6d384

string model.Header(string headerName)
WebHeaderCollection model.Headers()
petrroll commented 8 years ago

Just out of curiosity what's the point of WebHeaderCollection? I can see it being passed around but not really used anywhere...

JohnnyCrazy commented 8 years ago

@petrroll Rate limit detection. If your exceeding the rate limit, a header will be set by the web api and users can detect it via that header collection

pentiumii400mhz commented 8 years ago

I can't get the Header to work, can you please add a example, the Header and the Headers Collection is allways empty? One problem is also that when Header("Retry-After") is "Nothing" I get a exception if I don't check if the Headers Collection is Nothing. Can this be more simple?

I really would like to have a simple way of getting how long time I should wait befor retrying agin?

JohnnyCrazy commented 8 years ago

@pentiumii400mhz

Can't reproduce it, headers are working fine: img

Header(string name) will return null if there is no header with that name, so there should be everything you need for a Retry-After-check

pentiumii400mhz commented 8 years ago

Hi

I'm sorry but when I'm getting a Error 429 the headers is Empty!

image

image

image

pentiumii400mhz commented 8 years ago

Anyone?

JohnnyCrazy commented 8 years ago

I will have a look at this the next days, kinda got out of my sight

pentiumii400mhz commented 8 years ago

Thanks, created a new Issue for this.