SuperEvilMegacorp / vainglory-assets

Community provided art, schemas, and other assets that make using the Vainglory API easier
https://developer.vainglorygame.com
MIT License
54 stars 40 forks source link

Bug: Lifetime player stats being returned with matches #145

Open bguilder opened 7 years ago

bguilder commented 7 years ago

We have been returning player stats within the match object. This object is actually life-to-date-of-match rather than life-to-now. It's super confusing.

The intention was to have snapshot info on the participant record. To fix this, we are considering removing the player data from the match object. We will be keeping participant data (data relevant to the match). However, in order to retrieve player lifetime data, the players endpoint will need to be used, and matches will no longer store lifetime player information.

If anybody foresees their apps breaking with this update, please let us know and we can try to accommodate. Thanks y'all!

ClarkThyLord commented 7 years ago

This is gonna break multiple things in my bot, EZLBot. 😓 When is this change happening? So I can build a work around can't have a broken bot serving 500+ servers 😬

turindev commented 7 years ago

Yup this will break my site as well, we will just need to coordinate. Also this will increase API calls so I'll have to consider the implications :)

bguilder commented 7 years ago

Yup, this is sure to break some sites. We'll definitely be sure everyone is notified and has time to make the necessary changes before deploying anything 👍

schneefux commented 7 years ago

This will break our service in parts. This means that in order to display a proper "players a b c played against d e f" view, we will need up to 5 * 50 + 1 requests per page from /matches, to get player's names. Will our rate limits be increased accordingly? Even with a limit of 50, there is no way to download all relevant data for one user within the average time of 1~2s it takes at the moment.

PierreAndreis commented 7 years ago

Players Name are being stored on player data, while the participant data is only giving player id. Does that mean I will have to make a call to /player endpoint to get the player names in a match? so 1 match with 6 participants = 1 match call + 5 players call ?

svperfecta commented 7 years ago

Noted: We need to keep player.name somewhere to handle name changes.

kvahuja commented 7 years ago

we need playername, apiId at the least in same response to avoid making recurring calls

PierreAndreis commented 7 years ago

@cklugewicz https://developer.vainglorygame.com/docs#get-a-collection-of-players

schneefux commented 7 years ago

@cklugewicz made a very good suggestion/comment, I'll quote it even if he/she decided to delete it:

The current /players endpoint only allows us to retrieve 1 player per call. I propose increasing that to 6, so that we can retrieve all players in a match with a single call.

Actually, this is a possible solution to this issue. player.id will be returned in relationships, and filter[playerIds] works on /players, it is currently undocumented:

http 'https://api.dc01.gamelockerapp.com/shards/eu/players' "Accept":"application/json" "X-Title-Id":"semc-vainglory" "Authorization":"" "filter[playerIds]"=="8f454688-2fe5-11e6-9433-06d90c28bf1a"
# 200

It does not work for an array of ids though:

http 'https://api.dc01.gamelockerapp.com/shards/eu/players' "Accept":"application/json" "X-Title-Id":"semc-vainglory" "Authorization":"" "filter[playerIds]"=="8f454688-2fe5-11e6-9433-06d90c28bf1a,727fa644-036e-11e7-8b59-062cb73cdbb1"
# 404

If we could get up to 6 players at once from /players, applications that need participant's stats could do it with 1 call per match, which is much more reasonable, especially if it happens on demand.


I have some concerns about including player in /matches' results. It is bad design to have more attributes for the same object (object type) on one endpoint than on the other. A player from /players would be !== a player from /matches. A clean solution to this could be using meta in relationships to store the player name (http://jsonapi.org/format/#document-meta), but I don't know enough about JSONAPI to want to recommend or suggest anything. A reason why I'm opposed to encourage searching by player.attributes.name on /matches completely is that player names are not unique on this endpoint, but they are on the other; which means that data for a player vaingloryPlayer from /matches does not necessarily belong to the player vaingloryPlayer in the case that players changed their name - as far as I understand, data in /matches is "frozen" and has a snapshot of the player's name at that time, which does not mean that the player <-> id mapping is current or unique, which is misleading. Either the API needs to map name to id internally before starting the search, which means returning matches that don't include the name that has been searched, or application developers need to map name to id via an additional call to /players first (which is not a good idea until #151 is solved).


tl;dr: API is in Alpha, this change will break applications anyways, better do more big breaking changes now than have more problems with player in the future.

svperfecta commented 7 years ago

Still not sure what we want to do here. Still need to think about this more. Ideally I think the answer is that we return only the latest version of the player.

schneefux commented 7 years ago

I have a solution:

participant is already a record that contains player's data until the start of the match. You make the bug a documented feature.

ghost commented 7 years ago

Yup. I suggest moving player.id (uuid) and player.name to participants and getting rid of players. Everything else is irrelevant from an immutable match perspective.

svperfecta commented 7 years ago

@schneefux Yeah, I think that's a pretty good solution. We can do this. I'll add to the API list now.

svperfecta commented 7 years ago

Tasks:

svperfecta commented 7 years ago

This issue was moved to gamelocker/platform#8