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

Implement Swagger/OpenAPI definition for client generation #255

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

fenix-hub
Copy link

This PR adds the Swagger/OpenAPI definition for the AssetLib APIs in order to test them through SwaggerUI and generate clients with different programming languages.

@fenix-hub
Copy link
Author

For now only CORE APIs have been mapped, next commits will implement the rest of them.

@fenix-hub
Copy link
Author

fenix-hub commented Apr 25, 2022

Added the rest of exposed APIs. Some descriptions/summaries are missing (related to parameters, request body and responses). It would be nice if developers/maintainers could add details to make the definition complete.
image
The definition is being tested. I'm generating a go client for my project Gibson directly using Swagger Editor.

The only known issue (already discussed in Discord) is related to the modify_date parameter in the AssetDetails object.
Since it is described as a string with format: date-time most client generators will treat it as a timestamp (relative to the programming language) most of the time using Time classes which adopt by default a format compliant to both ISO8601 and RFC3339 (2020-12-09T16:09:53+00:00), but not all of them are able to parse an RFC3339 only dates (2020-12-09 16:09:53+00:00) like the Asset Library currently uses, resulting in a parsing exception for most of the clients.

In this case the options are:
a) define the modify_date parameter as a simple string
b) update the library with a replacement of character with a T character before sending the response, in order to not create conflicts with how the timestamp is stored in the database

@Frontrider
Copy link

How does the authentication work?

@fenix-hub
Copy link
Author

The /login endpoint will return an Authentication and Authorization token.
However the usage is not standardized and to authenticate requests it must be passed in the body of requests that require authentication.

@Frontrider
Copy link

I see.
Doesn't the /asset's post method need a token field then for now? :)

@Frontrider
Copy link

I guess I start carpet bombing with questions until I can publish/update a plugin :)

Is download_provider in AssetSummary an enum containing github, gitlab,bitbucket, gogs,cgit and custom?
Same with category, and godot_version might also be one.

@fenix-hub
Copy link
Author

I'll make sure to update the specs with the "token" parameter in all the requests that require it. However I'm unsure about the use of enums in OpenAPI specs, since some client/server generators do not handle language-specific enums in the same way, and since the are just Strings on the frontend but actually a tinyint on the backend adding or removing one won't require to update the specs too.

@Frontrider
Copy link

Frontrider commented Dec 22, 2022

However I'm unsure about the use of enums in OpenAPI specs, since some client/server generators do not handle language-specific enums in the same way, and since the are just Strings on the frontend but actually a tinyint on the backend adding or removing one won't require to update the specs too.

On the frontend they are a fixed set of strings. An enum is how it shows up when I look at it from my side, that int behind the curtain is not relevant.

From a consumer perspective it is a breeding ground for annoying errors. If not then just more code to try to validate a fixed set of strings based on cross referencing the website, that will break in annoying ways. Instead of just knowing what it wants me to hand over, and building around it.




The official doc specifically states what enums are, and it fits for what the api is trying to do with it. The only way to change it is to have more api endpoints to retrieve them which is overkill for now.
https://swagger.io/docs/specification/data-models/enums/

If a client does not understand enums that match the specification then that is not the api's fault.

@Frontrider
Copy link

Good lord. Do I have to send both category AND category id? Only one of them is not sufficient, while they seem to mean the same thing.
Yeah, that is needed an enum 100% I will never get the ids (that is probably the int you're talking about, the enum's ordinal) right any other way without a dedicated endpoint telling them to me.

@fenix-hub
Copy link
Author

Good lord. Do I have to send both category AND category id? Only one of them is not sufficient, while they seem to mean the same thing.

At least for POST requests, it doesn't seem so.
https://github.com/godotengine/godot-asset-library/blob/aa7f9b57396563c78d5e38acc38ab90d521256ca/API.md

For GET requests, category == category_id.
https://github.com/godotengine/godot-asset-library/blob/aa7f9b57396563c78d5e38acc38ab90d521256ca/API.md#get-asset

Where is it failing for you just by providing the category_id ?

@Frontrider
Copy link

Frontrider commented Dec 23, 2022

I understand it now then. Remove the category name from places where sending it is invalid. I'm not sure how it could be documented then.
That is the bit I messed up, because you gave me both fields for an update/creation. Okay.

The other enums work out now.

The best I can think of is to type out the ids in the description but that is also a bit out there. Still better than nothing.

@Frontrider
Copy link

Well, I feel like I should apologize to that beautiful person who will have to look at my 5 "useless" edits on my addon, but It works now. Everything but the categories are coming from the api definition, and validated according to whatever is in that.

https://plugins.gradle.org/plugin/io.github.frontrider.godle-publish

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

Successfully merging this pull request may close these issues.

3 participants