TheGamesDB / TheGamesDBv2

Version 2 of TGDB
https://thegamesdb.net/
GNU General Public License v3.0
54 stars 16 forks source link

Malformed swagger spec #19

Open J-Swift opened 5 years ago

J-Swift commented 5 years ago

I haven't looked too far into the underlying issue, but it looks like the swagger spec (https://api.thegamesdb.net/) references explicit items in the models. For example, Publishers > Data > Publishers has 4271 subobjects because it explicitly lists every single publisher.

When trying to auto-generate API clients (e.g. using something like openapi-generator), this ends up with 12k files/types generated.

Would it be possible to generalize the swagger definitions for the models so that the actual values aren't encoded into it? I really like that you are offering the swagger interface, and it will greatly ease migration from the old style clients if the swagger spec gets cleaned up.

Thanks!

J-Swift commented 5 years ago

I couldn't find where the spec is being generated in this repo, so for now I'm just going to hand-clean the currently hosted version. You can see that here for now.

In cleaning this up I noticed that there is a mix of array responses and object responses keyed by object id. I think it would be good to standardize on the array style since all the objects already embed their ids. Of interest is that swagger doesn't actually support dynamic object keys (even in v3.0). That is probably why this issue came up in the first place actually.

J-Swift commented 5 years ago

Update for anyone else that might want to auto-generate a client, I've cleaned up the spec a bit more and generated a go client using openapi-generator. You can find it here. It's my first foray into golang so is subject to change in the future if I learn I did anything especially poor.

That's a good enough stopgap for now so I can continue moving forward on a GamesDBv2 migration, but generating a new client isn't easily sustainable when changes arise in the future. I'd still really like to see the source spec get fixed so manual cleanup isn't needed. I'm happy to try and assist if needed!

Zer0xFF commented 5 years ago

@J-Swift thanks for this, I've finally had time to look into this, and I can see your swagger yaml working really well.

I'm going to update our current reference to this.

I couldn't find where the spec is being generated in this repo this wasn't pushed out, since it was hacky enough code, as apparent from the resulting yaml

if you have a suggestion as to how we can auto generate this properly, that will be appreciated, as we also currently have 1 undocumented api end point https://github.com/TheGamesDB/TheGamesDBv2/blob/dev/API/include/routes.php#L20 and this would be needed to ensure consistent/timely update as the API evolve.

J-Swift commented 5 years ago

Auto generation is going to differ per platform. My knowledge is limited mostly to ruby, where there is a really good package that generates it based on your unit test suite (so that it gets programmatically verified).

The other approach would be to invert the relationship of the system, and generate the server/clients from the spec. That's obviously a far reaching change, so don't think you necessarily would want to do that now but good to keep in mind for long term.

This might work for you (I haven't looked too far into it): https://github.com/zircote/swagger-php

If you are googling for packages, keep in mind that swagger is the old (v2) name. OpenApi is the new (v3+) name.

I can try and update that endpoint in my spec for now though. I'll look into it tonight.

J-Swift commented 4 years ago

@Zer0xFF - man, completely forgot about this! I am getting to this now to update for the new /v1 prefix, but I'm not sure what exactly changed for the /v1.1/Games/ByGameName to be able to edit it. Could you let me know how it differs from /v1/Games/ByGameName?

Zer0xFF commented 4 years ago

/v1.1/Games/ByGameName arguments works just like /v1/Games/ByGameName, but return would look like /v1/Games/ByGameID (since all /v1/Games/* should have the same return structure, there was just a mistake when i wrote that endpoint)

J-Swift commented 4 years ago

Interesting, I missed that in the original spec, and so the /v1/Games/ByGameName definition is actually incorrect. So then the real question is.. how does /v1/Games/ByGameName differ from /v1/Games/ByGameID 😃 ?

J-Swift commented 4 years ago

Doing a quick curl against current API looks like the responses are actually the same for both endpoints. Tested against these urls:

https://api.thegamesdb.net/v1/Games/ByGameName?apikey=TMP_TEST_KEY&name=halo https://api.thegamesdb.net/v1.1/Games/ByGameName?apikey=TMP_TEST_KEY&name=halo

Zer0xFF commented 4 years ago

sorry for the delay, the issue is when you add &include=platform more details can be found on the forum, https://forums.thegamesdb.net/viewtopic.php?f=11&p=1208, but to quickly summarise, the v1 endpoint doesn't include data node before include the data, while v1.1 endpoint does. it might actually be best to removed the /v1/ from spec to avoid usage

J-Swift commented 4 years ago

OK, I think this is all updated then. I also added the /v1 prefix to all endpoints. I haven't looked any further into integrating the spec generation into the php project, but things should be good for now until you start editing the API

Zer0xFF commented 4 years ago

auto generation isn't strictly required since we wont be making any breaking changes, but its a nice feature to have.