[Announcement] [Gdscript] Godot 4.x Client Work in Progress

Goutte commented 1 year ago

Hello !

I started experimenting with a Godot client (GDScript), and albeit there's not much to show right now I wanted to write something here in case anyone else is interested in joining forces. I looked here for the keywords Godot and GDScript.

Here's the working branch.

Rationale

Being able to quickly draw a pure GDScript client for any OAS would be amazing for some online games, even though Godot has its own network engine, because in HTML5 exports only the HTTP client is available (for now). Anyway, there's plenty of uses for HTTP connections to a server in games (auth, leaderboards, news, etc.).

I don't know if GDScript will have all the features we need, we'll see along the way.

Y U NO C ?

Having to compile Godot itself and distribute it to all collaborators is no small task. And I'd rather not have Mono if I can help it.

Design

Some ideas, constraints inherent to Godot, etc. Nothing is frozen yet.

Indent using Tabs

Godot uses tabs by default, although it is configurable.

Java code should be indented like the other generators.

Try not to pollute global space (with `class_name`) ?

Godot does not have namespaces. (for now)

What should we add in the global namespace (via class_name, or perhaps singleton nodes) ?

If we do decide to use class_name (and pollute global space) for models and api, which I suspect we'll need to get nice type defs, we need to set up for all class names a prefix that will act as a namespace.

We can rather easily do this for models with --model-name-prefix, just need to make sure our users know about it. I found --api-name-suffix but no prefix, although it is used in toApiName(). Probably because of lack of support across targets ? Should I add my own --api-name-prefix CLI option or wait ?

snake_case vs camelCase

I'd love some configuration but I won't bother with it until at least I have a working client.

Static typing

Godot does not allow nullable types (we're waiting for them).

This means we either skip types, or try to figure out how to replace null by the appropriate pseudo-null value for the given type. ("" for String, etc.) But it becomes awkward for int values, where 0 might not be a sensible pseudo-null.

Async

In HTML5 exports, only async is available. Since it is my personal target, I'll favor it.

Godot 4 has Callables and lambda defs, but no Promises yet. I think the best method signature for our generated async API method endpoints would be to return a Promise. But if we start waiting on the promises of Godot… ;)

Therefore, for the first major version of the generator, we'll use Callables in that fashion, unless you have a better idea :


func createUser(
    # … ← generated params from the endpoint schema
    on_success: Callable,  # func(result: User)
    on_failure: Callable  # func(error: ApiError)
):
    pass  # ← method implementation

This function signature is actually very cumbersome to use (see the current state of the demo in samples), because we'll often need to chain requests. We need to find an implementation of Promise in pure GDScript, and fast.

To Singleton or not to Singleton

I tried to avoid using Singletons (aka. Autoloads), so XxxxApi classes are simple RefCounted (plain ol' Godot Object with garbage collection), but that means each XxxxApi holds its own (host, port) configuration to lazily create its client when none was provided. Same goes for extra headers the user wants to send.

You can create the configuration once and pass it to each API class.

State of Things

I managed to bootstrap the gdscript target, compile and generate the petstore sample. Now we need to fill the templates with working code, one feature at a time.

[x] Models template basic bones
[x] Apis template basic bones
[x] Basic Java CodeGen
[x] Enums
[x] Datetimes ← using String, shifting timezone responsibility on user
[x] Model Collections ← using Arrays with dubious recursion support
[ ] Reserved Words ← mostly done, never done
[x] Configure enabled/disabled features
[x] Demo / Integration Tests
[x] Normalization
[x] Serialization (JSON)
[x] Model validation of required
[x] Model validation of constraints (length, etc.)
[x] Deserialization (JSON) ← my focus
[x] ~~Deserialization (JSON+LD)~~ ← skipped (ouch)
[x] ~~Serialization (XML)~~ ← skipped
[x] ~~Deserialization (XML)~~ ← skipped
[x] RequestBodies Components
[ ] SecuritySchemes ← hmm
[x] Response Errors ← may skip
[ ] Unit Tests
[x] Header customization
[x] Single host configuration from OAS
[ ] Multiple host configuration from OAS
[x] HTTPClient async constraint
[x] HTTPClient reuse through Apis
[x] Override endpoints for easier template overriding (perhaps via template partials?)
[x] Bunch of configuration (YAML)
[x] Bunch of configuration (CLI)

Probably won't do everything on this list myself. Will do enough for my own use, hopefully.

I'm looking at the other language targets and throwing darts. Wish me luck !

Burning questions

Is there somewhere a docker image (or whatever) for a server with a working petstore I can use for my demo during dev ?

Yes → https://github.com/OpenAPITools/openapi-generator/wiki/Integration-Tests (thanks @wing328)
Should I use handlebars straight away or keep using mustache, as mildly recommended ? I'm used to (and fond of) jinja2 and Twig (Pebble, for Java).

Moved to handlebars because the truthy value of mustache includes "" and "null" and that does not play well with {{#example}}.
I made a small python script to grab the reserved keywords from Godot's XML. Since it might be useful again when updating the generator to account for changes in Godot, I'd like to keep it close. Where can I store it ? (right now it's in modules/openapi-generator/src/main/resources/gdscript, schmoozing with the templates)

wing328 commented 1 year ago

Should I use handlebars straight away or keep using mustache, as mildly recommended ? I'm used to (and fond of) jinja2 and Twig (Pebble, for Java).

It's up to you and I think the community gets used to mustache.

wing328 commented 1 year ago

Is there somewhere a docker image (or whatever) for a server with a working petstore I can use for my demo during dev ?

Would this help? https://github.com/OpenAPITools/openapi-generator/wiki/Integration-Tests

Goutte commented 1 year ago

The backbone is here and as I'm getting slowly more proficient in OAS code gen, it's time to delve into tests.

Goutte commented 1 year ago

The doc states:

A simple test script/app to test Petstore (e.g. test.php) to serve as a starting point for developers to easily play with the auto-generated SDK

I can't find any test.php in the source, and I'm not sure where in the repo to put my own test file. (same question for the python util to extract reserved words).

Also, for integration testing, I think I can make a single (or more later as needed) GDScript test file and run it from the CLI, and return an error code > 0 if any test failed.

I can wrap the call(s) into a shell script for convenience. Where should I put such files ?

Goutte commented 1 year ago

godot4 --headless samples/client/petstore/gdscript

:tada: I have a test-suite in the form of a Godot project in samples, that I can run in --headless mode and return a meaningful exit code.

Now, the serious business can finally start…

Goutte commented 1 year ago

I'm glad to announce that the integration test can create a user, log in, and create a pet. It's very rough around the edges, but it works.

I'm stuck, though

I'm now using that test to do some refactoring and exploration into namespaces.

I can rather easily prefix and suffix the Api and Models with their already-defined parameters. I'm not doing anything, it just works®.

Yet I have some core files (base class, error) that I want to be able to customize the same way, using for example coreNamePrefix in the YAML config:

generatorName: gdscript
outputDir: samples/client/petstore/gdscript
inputSpec: modules/openapi-generator/src/test/resources/3_0/petstore.yaml
templateDir: modules/openapi-generator/src/main/resources/gdscript
additionalProperties:
  hideGenerationTimestamp: "true"
  apiNamePrefix: Demo
  modelNamePrefix: Demo
  coreNamePrefix: Demo

The property coreNamePrefix shows up in the templates, even if not defined in GdscriptClientCodegenOptionsProvider, but even if defined there I can't figure out how to access it from GdscriptClientCodegen's constructor or processOpts().

additionalProperties.containsKey(CORE_NAME_PREFIX) yields false

I want to change the filename when defining my SupportedFile like so :

supportingFiles.add(new SupportingFile("ApiBee.handlebars", "core", toCoreFilename("ApiBee") + ".gd"));

The method toCoreFilename tries to find the coreNamePrefix additional property in additionalProperties and fails. I'm guessing I'm not looking in the right place, or at the right time, or perhaps I overlooked the adequate override.

This probably ties into GdscriptClientCodegenOptionsProvider whose purpose I do not understand anymore, as I thought was used for precisely such custom parameters like coreNamePrefix.

Any lights on these would be appreciated. @wing328 ?

Goutte commented 1 year ago

It was too baffling ; I should have noticed my confusion earlier, but it's only after re-reading my message above that I thought of the wrong assumption I was making.

public static final String CORE_NAME_PREFIX = "coreNamePréfix";

When I wrote this line, I checked against presence in the template, and io and behold the variable was there, so I went merrily on my way.

Textbook confirmation bias.

Got stuck way too long on a sneaky diacritic, but at least I learned a lot about the internals of the generator along the way :fox_face: !

Goutte commented 1 year ago

I'm going to take some time and consume a real-world OAS with this, before I carry on and freeze things for MR. Might take a few months. I'm also not convinced by the Callable approach, since it yields pretty ugly code on usage. (see the demo)

~~I'm also not comfortable with the feature set configuration, if you have any tips, or any other specific gen I should mirror, I'm all ears.~~

wing328 commented 1 year ago

@Goutte thanks for your work on this. Can you please PM via Slack me when you've time?

https://join.slack.com/t/openapi-generator/shared_invite/zt-12jxxd7p2-XUeQM~4pzsU9x~eGLQqX2g

Frontrider commented 1 year ago

At least Godot 3 has a style guide, that is based on how the c++ code is formatted. I don't think it changed for 4, would be the best to follow this.

https://www.reddit.com/r/godot/comments/yngda3/gdstyle_naming_convention_and_code_order_cheat/ https://www.gdquest.com/docs/guidelines/best-practices/godot-gdscript/

iamarkdev commented 1 year ago

Any updates on this @Goutte ?

saatsazov commented 1 year ago

Hello, I found a bug with generating default values for strings. More details: https://github.com/Goutte/openapi-generator/pull/1

The problem: Screenshot 2023-03-09 104838

wing328 commented 1 year ago

@Goutte can you please submit a PR so that we can incorporate this new generator into the official repo so that more Godot users can benefit from this and submit PRs to further enhance it?

saatsazov commented 1 year ago

@wing328 Shall we implement more features or we can merge current WIP? I started using this generator for my pet project. So I may found more problem in future. And may address them.

saatsazov commented 1 year ago

Actually there is one more bug, which I haven't fix for now. Just modified generated code. Godot 4 was under active development. Now they seem to freeze changes. (At least they mark some unstable api which they are going to modify in future releases)

The method connect_to_host now accept 3 parameters. Generator currently have 4 parameters here: https://github.com/saatsazov/openapi-generator/blob/feat-gdscript/modules/openapi-generator/src/main/resources/gdscript/ApiBee.handlebars#L86

And this is a latest documentation: https://docs.godotengine.org/en/stable/classes/class_httpclient.html#class-httpclient-method-connect-to-host

Basically as I understood they merge ssl_enabled and verify_host to single tls_options

Goutte commented 1 year ago

Oops ; something unexpected is happening, and I'm not receiving github's notification emails anymore. Sorry for not responding

Thanks @saatsazov for the bug finds ! ~~Do you have a branch with your fixs that I should rebase/merge ?~~ (done, thanks!)

I'll look into wrapping this up for a PR now, it will help me procrastinate the writing of a CV. :roll_eyes:

Goutte commented 1 year ago

@saatsazov I've added support for TLS and some underscores to method names, to mark them as protected. It's here : https://github.com/Goutte/openapi-generator/pull/2

It's a breaking change ; there might be more before the MR, since I frown at the current API. (even the bee prefix is bad, since :bee: may be a legit model)

saatsazov commented 1 year ago

Hello @Goutte,

No worries about breaking changes. My project is not on a stage to worry about that. But thanks for worried mentioning breaking changes.

I actually started doing some quick and dirty fixes in my fork. One of the biggest problem I encountered is connected with connection. When you start doing request. the whole game is very laggy. So what I did is started a separate thread. I suppose it should be some better way to do so.

I tracked problem to this loop https://github.com/Goutte/openapi-generator/blob/13d3c5c79d1f2a4bf4a464a00a59d1b9d68f1793/modules/openapi-generator/src/main/resources/gdscript/ApiBee.handlebars#L100

The delay seems to block main thread. So as a quick solution I added a thread.

As you mentioned in a comments here https://github.com/Goutte/openapi-generator/blob/13d3c5c79d1f2a4bf4a464a00a59d1b9d68f1793/modules/openapi-generator/src/main/resources/gdscript/ApiBee.handlebars#L64

there is no such thing as idle frame. So we should think how to approach this. I may provide an example project later if you want.

Goutte commented 1 year ago

Good point !

Whatever the await, it's probably not a good idea to do network on the main thread, right ? We can provide a func xxxx_threaded(…) -> Thread that wraps the api call and returns a Thread, perhaps ?

Inline objects hurdle

TLS works, but now I need to support inline objects like these:

  /authentication_token:
    post:
      operationId: login_check_post
      tags:
        - 'Login Check'
      responses:
        200:
          description: 'User token created'
          content:
            application/json:
              schema:
                type: object
                properties:
                  token: { readOnly: true, type: string, nullable: false }
                required:
                  - token
      summary: 'Creates a user token.'
      requestBody:
        description: 'The login data'
        content:
          application/json:
            schema:
              type: object
              properties:
                username:
                  type: string
                  nullable: false
                password:
                  type: string
                  nullable: false
              required:
                - username
                - password
        required: true
    parameters: []

The login_check_post API method will require a GoasLoginCheckPostRequest object.

I looked at how typescript did it in modules/openapi-generator/src/main/resources/typescript/types/ObjectParamAPI.mustache, but Godot works best with one class per file (things get pretty weird & buggy for inline public classes).

saatsazov commented 1 year ago

At the moment I can't say how would be better to work with async. So we can address this issues later when we have more details.

Speaking of inline object I use this generator for PHP project and this is how it looks like. It generate a bunch of of classes per each inline request and response. This may mitigate the problem you mention one class per file.

Goutte commented 1 year ago

I've added rudimentary support for inline objects (request/response, mainly).

Still not sure what to put in the await… I'm also considering adding the Config to the Api's init() instead of injecting bee_config. Godot recommends not using init params, but that's only for Resources, if I remember correctly. (and Api is a Reference)

Goutte commented 1 year ago

Breaking

replaced bee for bzz, less collision chance
Apis now receive config and client via constructor

@saatsazov let's break as much as we can before MR, if you have concerns or refacto ideas, go full EAFP while nothing is frozen !

Testing

I remember managing to set up a test project with a custom MainLoop or something, for automated integration testing, but it's complicated to set up as it requires Godot to run, and docker for the petstore server. Unit tests don't feel as relevant, though.

Frontrider commented 1 year ago

Breaking
* replaced `bee` for `bzz`, less collision chance

* Apis now receive `config` and `client` via constructor
@saatsazov let's break as much as we can before MR, if you have concerns or refacto ideas, go full EAFP while nothing is frozen !

Testing

I remember managing to set up a test project with a custom MainLoop or something, for automated integration testing, but it's complicated to set up as it requires Godot to run, and docker for the petstore server. Unit tests don't feel as relevant, though.

Gdunit+docker should be fine imo.

Goutte commented 1 year ago

Gdunit+docker should be fine imo.

Thanks for caring ;)
I meant unit tests in java ; I've looked at what other targets did, and we can have some but they will provide little value
I usually favor GUT over GdUnit for my games, but it's not a strong preference ; do you have reason to believe GdUnit is more appropriate in our case ?

I'll look today into refactoring the test project (in samples/client/petstore/gdscript) to put the generated client into an addon instead of the root, and adding a proper assertion engine.

Frontrider commented 1 year ago

I usually favor GUT over GdUnit for my games, but it's not a strong preference ; do you have reason to believe GdUnit is more appropriate in our case ?

I like the assertions more, to me they are more readable. Nothing else.

I meant unit tests in java ; I've looked at what other targets did, and we can have some but they will provide little value

Executing the generated code in godot as a functional test would have value imo. That can also be launched from java if needed.

Goutte commented 1 year ago

Executing the generated code in godot as a functional test would have value

Definitely ! So far we can register, authenticate and CRUD a monkey on the petstore. I tried using the new echo server, first, but there's a collision with the word color, so I'm using ye olde petstore for now.

The command line is still a bit complex, but it passes (provided the dockerized petstore is running):

godot --debug --headless --path samples/client/petstore/gdscript --script addons/gut/gut_cmdln.gd Yes, it's GUT, sorry about that ; started hacking around before you answered ; we can have both, or switch

Breaking soon

I'm currently wrapping the object sent back into the on_success callback into an ApiResponse which will hold the response HTTP details (code, headers, etc.) as well as the deserialized data. (if the client detects a model and manages to deserialize it)

Even if we don't provide the full response analysis for now, changing the callback signature now will help us implement these features later on without breaking.

Frontrider commented 1 year ago

Yes, it's GUT, sorry about that ; started hacking around before you answered ; we can have both, or switch

I don't feel anyhow about it. I'll most likely just be a consumer anyways. :sweat_smile:

Goutte commented 1 year ago

Now I remember why I had not added a test engine but made a minimalist (disgusting) one. Oh well… The review of this target is going to be more painful, but I guess it's worth it ?

@saatsazov Does it still lag a lot when you set polling_interval_ms = 0 in your ApiConfig instance ?

So far I'm using the client in menus, but in a few months I will start using it during heavy 3D rendering as well, so I will inspect the async/threading issue more closely then.

I'll let this sit until then (I fixed my email notification issue), perhaps I will fix/implement the response's character set detection, but that should not be a breaking change.

Latest draft has been merged in feat-gdscript.

Goutte commented 1 year ago

Also, this warrants interest : https://github.com/D2klaas/Godot-4-HTTPManager

jchu231 commented 1 year ago

I am interested in helping test out this generator. I am trying to utilize feat-gdscript via @openapitools/openapi-generator-cli, but am having trouble enabling your generator with it, any guidance would be awesome!

And thanks @Goutte for your work on this generator!

wing328 commented 1 year ago

@jchu231 you will need to build https://github.com/Goutte/openapi-generator/tree/feat-gdscript locally for the time being as the generator has not been merged into master.

jchu231 commented 1 year ago

@Goutte , I am testing your generator (thanks for your awesome work btw!).

Ran into this error when generating the first api: Caused by: org.openapitools.codegen.templating.TemplateNotFoundException: partials/api_base_class_name.handlebars, partials/api_base_class_name.hbs

Looks like there is an api_bee_class_name.handlebars file that used to be named api_base_class_name.handlebars, is this a typo?

I think either renaming api_bee_class_name.handlebars to api_base_class_name.handlebars, or renaming references to api_bee_class_name.handlebars fixes it. I applied those changes locally and it worked. I am not sure which route is preferrable though.

Goutte commented 1 year ago

@jchu231 Thanks ! I was not very comfortable with the bee semantic, and am trying to move to a more generic word, like base. I've pushed a fix.

jchu231 commented 1 year ago

Thanks for pushing out the fix, @Goutte !!

I've been running into another issue, not sure if you've encountered this, but I am seeing some issues with ApiBee._bzz_do_request_text() where I am getting a 200 response from my server, but the request then goes from HttpClient.STATUS_BODY to HttpClient.STATUS_CONNECTION_ERROR. Root cause might be with Godot's HttpClient, but I haven't narrowed it down yet. My same server endpoint has been working fine with Unity and Typescript client. Was curious if you had any thoughts or advice, any guidance would be super helpful!

Error in Godot console looks like:

My ApiBee.gd code where the error happens

Goutte commented 1 year ago

I bet that by now you understand why this generator ain't a merge request yet. :)

This data retrieval inner loop needs some more love.

I suspect the error is raised by the call to poll(), but I don't see it right now (although I have seen it in the past at some point).

I'll keep an eye out ; meanwhile, I invite you to experiment with that loop and break things, since the current implementation can be qualified as shenanigans more than code.

I also notice that the OS.delay_usec looks wrong by a factor of a thousand, but that's not our problem right now.

jchu231 commented 1 year ago

I think I was able to fix the issue I was seeing! (the 200 -> STATUS_CONNECTION_ERROR when waiting for body)

I had to move the call to self._bzz_client.poll() AFTER the interval delay (see attachment). CleanShot 2023-05-31 at 14 39 24

I had a call that was failing 100% of the time (I think the larger the response body, the more likely this issue surfaces), and after this change it now works every time.

My (very rough) theory is that .poll() was being called too rapidly which (for some reason) was causing the underlying connection to freak out and return !connected status. Related code in http_client.cpp https://github.com/godotengine/godot/blob/master/core/io/http_client_tcp.cpp#L426-L436

Goutte commented 1 year ago

@jchu231 That's great news !

You can make a MR to my branch, or have me fix this on my end. Your pick. Our commits are probably going to get squashed anyway.

I'll run the test-suite with the change (but I expect it to pass).

jchu231 commented 1 year ago

@Goutte , do you might making this change? I'm not super comfortable with the generator codebase yet (I've just been editing the generated files), but happy to review and then I can learn how to make changes for the next time.

I also tweaked the order of the code in the while self._bzz_client.get_status() == HTTPClient.STATUS_REQUESTING: block. Not sure if this was actually needed, but did it to be consistent. CleanShot 2023-05-31 at 17 23 25

saatsazov commented 1 month ago

I think it's time to merge this not finished work to master of openapi. As this is the only app that exists. And also the openapi have some problems solved but I can't use directly your branch. as it still have some bugs.

I actually tried to take latest master from OpenAPITools/openapi-generator and lates feat-gdscript branch. And still got some bug generating my schema.

OpenAPITools / openapi-generator