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

Malformed swagger spec #19

Open
J-Swift opened this issue Aug 15, 2019 · 11 comments
Open

Malformed swagger spec #19

J-Swift opened this issue Aug 15, 2019 · 11 comments

Comments

@J-Swift
Copy link

J-Swift commented Aug 15, 2019

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

J-Swift commented Aug 16, 2019

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

J-Swift commented Aug 17, 2019

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

Zer0xFF commented Oct 23, 2019

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

J-Swift commented Oct 23, 2019

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

J-Swift commented Jan 3, 2020

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

Zer0xFF commented Jan 3, 2020

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

J-Swift commented Jan 4, 2020

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

J-Swift commented Jan 4, 2020

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

Zer0xFF commented Jan 5, 2020

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

J-Swift commented Jan 5, 2020

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

Zer0xFF commented Jan 5, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants