arenanet / api-cdi

Collaborative Development Initiative for Public APIs
253 stars 41 forks source link

Missing skins under /v2/skins #78

Closed codemasher closed 8 years ago

codemasher commented 9 years ago

https://api.guildwars2.com/v2/skins/2984 yields an ErrBadParam while this id is listed under /v2/skins. Also, the HTTP error should rather be a 204/No Content than a 400/Bad Request - the latter sounds so passive aggressive... :P

tivac commented 9 years ago

That's a generic error message, and a 20x response code would be inappropriate for bad parameters from a client.

That said, this appears to be a bug & I'll try to poke at it tomorrow.

codemasher commented 9 years ago

A HTTP/20x response as generic is for sure inappropirate. However, the errors should at least indicate if it was a user error or a backend failure. Although my above example might be the result of a bug, the HTTP/400 was clearly wrong and caused me to review and test my code a couple times before i opened this issue because it implied that the error was on my side.

sliekens commented 8 years ago

It's not just 2984. Here's the full list.

https://api.guildwars2.com/v2/skins?ids=1042,1043,1047,1193,2329,2385,2388,2393,2394,2395,2984,3046,6074

I would argue that these are not bad parameters -- each and every one of these is listed in the API. Maybe 500 is more appropriate.

darthmaim commented 8 years ago

Really, who cares that much about the response codes. Its an error. The bug is with serializing the skin for the api (or listing it when it shouldn't be listed). This should be fixed. Changing the response code doesn't help anyone. Half of the comments on issues here in this repo or just about response codes, and not about the bug itself. Changing the response code makes sense when its something that is expected to happen (requires authentication, rate limiting, partial content, ...), this is a bug with the implementation underneath, any error code tells the consumer its not what they requested. If its 400 for requesting a broken entry or a 500 because the api server had some error when processing doesn't matter in this case, once its fixed it is fixed, until then it throws an exception in the client.

/rant over

sliekens commented 8 years ago

Lol jeez

lye commented 8 years ago

It's not just 2984. Here's the full list.

I just removed all of those skin IDs from the whitelist -- ErrBadParam usually means the server which reads the .dat file is refusing to serve the content. There's one flag which, looking at it now, might be the cause -- if ShowInWardrobe isn't set the skin is always suppressed. I'll keep an eye on it -- those skins will come back through the whitelist if that hypothesis is correct and I'll be able to fix the issue more correctly.

I should drink some coffee.

darthmaim commented 8 years ago

5898 is also returning ErrBadParam.

darthmaim commented 8 years ago

I'll keep an eye on it -- those skins will come back through the whitelist if that hypothesis is correct and I'll be able to fix the issue more correctly.

2329 is back.

freema commented 8 years ago

Skin id 5898 still return status 400 :(

lye commented 8 years ago

Just re-blacklisted 2329 and 5898. Haven't had time to look into what exactly wrong with these, but given that it came back I really think it's some bad content somewhere.

lye commented 8 years ago

I'm gonna close this one out for now. smh.