Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing skins under /v2/skins #78

Closed
codemasher opened this issue Aug 8, 2015 · 11 comments
Closed

Missing skins under /v2/skins #78

codemasher opened this issue Aug 8, 2015 · 11 comments

Comments

@codemasher
Copy link
Contributor

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
Copy link
Contributor

tivac commented Aug 9, 2015

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
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor

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
Copy link

Lol jeez

@lye
Copy link
Contributor

lye commented Sep 28, 2015

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
Copy link
Contributor

5898 is also returning ErrBadParam.

@darthmaim
Copy link
Contributor

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
Copy link

freema commented Oct 11, 2015

Skin id 5898 still return status 400 :(

@lye
Copy link
Contributor

lye commented Oct 14, 2015

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
Copy link
Contributor

lye commented Dec 7, 2015

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

@lye lye closed this as completed Dec 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants