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.

Sign up for GitHub

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

Jump to bottom

Erlang server overhaul #19465

Merged
merged 12 commits into from
Sep 7, 2024
Merged

Erlang server overhaul #19465

merged 12 commits into from
Sep 7, 2024

Conversation

NelsonVides
Copy link
Contributor

This is really a total overhaul of the 8 years old submitted erlang generator. It is not only very buggy but also quite incomplete.

Changes are many, and they're breaking. First thing, is that I'm requiring the latest OTP27, which includes a json parser and therefore using this and dumping the very old jsx. The new one on the standard library is 5-20x faster according to all public benchmarks. I'm here also upgrading the versions of cowboy and ranch to the latest ones, for way more performance improvements and related bug fixes.

There's plenty of formatting changes, to actually follow the linked inaka guidelines (of which I am part of their team).

There's also bug fixes like generating duplicated is_authorized handlers.

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples: 
    ./mvnw clean package 
./bin/generate-samples.sh ./bin/configs/*.yaml
./bin/utils/export_docs_generators.sh
    (For Windows users, please run the script in Git BASH)
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.6.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
    • Technically this breaks the generator for Erlang so, pointing to 8.0.x? Not sure about it so pointing to master for now, please confirm me if I need to change it, that'd be easy.
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.
    • Tagging the committee for erlang here, though I suspect they might not really be actively supporting the project anymore. Please correct me if I'm wrong (and I hope a little bit that I am!).
      @tsloughter @jfacorro @robertoaloi

@wing328
Copy link
Member

wing328 commented Aug 28, 2024

thanks for refactoring the erlang sever generator 👍

There's plenty of formatting changes, to actually follow the linked inaka guidelines (of which I am part of their team).

in erlang, is there auto code formatter/linter to automatically format the code?

that's what we usually recommend to users to do as it's not easy to format all the code perfectly in mustache template

@wing328 wing328 added this to the 7.9.0 milestone Aug 28, 2024
@wing328
Copy link
Member

wing328 commented Aug 28, 2024

I'll mark the old (existing) erlang generator as deprecated (in another PR) so that we can include this PR in the upcoming release which allows breaking changes with fallback.

@NelsonVides
Copy link
Contributor Author

thanks for refactoring the erlang sever generator 👍

There's plenty of formatting changes, to actually follow the linked inaka guidelines (of which I am part of their team).

in erlang, is there auto code formatter/linter to automatically format the code?

that's what we usually recommend to users to do as it's not easy to format all the code perfectly in mustache template

Unfortunately (or fortunately?), there's like 4 different standard formatters. I could pick my favourite and just apply, though it would require plenty of manual tweaking taking into account it'd be running against mustache templates and not real erlang code. But it might at least ensure the generated code would just comply (?). However I'm not sure it's worth the effort, as, as I said, there are several tool out there anyway. But I can just try that maybe? 🥲

@NelsonVides
Copy link
Contributor Author

@wing328 I have a question as well, there are more issues related to mime types I'd like to fix (not sure if in this PR or already on a separate one), however the mustache templates give me booleans only for json and xml mime types (

openapi-generator/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java

Line 6887 in 4330b2f

if (isJsonMimeType(key)) {
). What's the best way in java to insert booleans for all the possible mime-types? Other than a 20-clauses else-if, that'd be a stinky code smell 😄

@wing328
Copy link
Member

wing328 commented Aug 28, 2024

Unfortunately (or fortunately?), there's like 4 different standard formatters.

users can pick whatever formatter they want.

my point is we don't need to make sure the auto-generated code is perfectly formatted as such task can be delegated to code formatter/linter instead.

@wing328
Copy link
Member

wing328 commented Aug 28, 2024

). What's the best way in java to insert booleans for all the possible mime-types? Other than a 20-clauses else-if, that'd be a stinky code smell 😄

another way is to check the value during runtime (in erlang code). not sure if it's something common in erlang.

@wing328
Copy link
Member

wing328 commented Aug 28, 2024

fyi. filed #19474 to mark current erlang-server as deprecated (just in case users need a fallback)

@tsloughter
Copy link
Contributor

Great to see!

One question, should jesse preparation/validation be option? I may be wrong but a quick search looks like OpenAPI supports back to version 07 but jesse only supports up to 06. So its possible a user may want to build something using an OpenAPI schema that jesse does not support and would rather ignore validation rather than have to attempt forking the schema and porting to an older jsonschema version?

@NelsonVides
Copy link
Contributor Author

). What's the best way in java to insert booleans for all the possible mime-types? Other than a 20-clauses else-if, that'd be a stinky code smell 😄

another way is to check the value during runtime (in erlang code). not sure if it's something common in erlang.

Can do that, it only will look ugly from the perspective of Erlang code, but this is autogenerated code anyway so nobody should be reading it much 🙈

@NelsonVides
Copy link
Contributor Author

Great to see!

One question, should jesse preparation/validation be option? I may be wrong but a quick search looks like OpenAPI supports back to version 07 but jesse only supports up to 06. So its possible a user may want to build something using an OpenAPI schema that jesse does not support and would rather ignore validation rather than have to attempt forking the schema and porting to an older jsonschema version?

Hello there! 😄

That's a very good point, one which I'm not familiar with 🥲 How hard it'd be to add 07 to jesse? Otherwise I'll find out how to set up some flag to ignore validation 🤔

@tsloughter
Copy link
Contributor

I don't know how much changed between jsonschema versions. Maybe its not a big task, I sort of just assumed it would be a decent amount of work.

@NelsonVides
Copy link
Contributor Author

I'm honestly thinking we could get away with validation here. I'm reading a bit about it and it seems like this requires a very smart generator. 3.0.x require actually draft 5, while 3.1.0 requires 2020-12. Mustache is very limited in what it allows you to template (logic-less, ya'know) and jesse is yet another limiting dependency here, and it's quite hard to template all these options, I think this could be left to the final user of the code rather 😕

@NelsonVides
Copy link
Contributor Author

@tsloughter I've pushed a big change, also solving #10393, but with some caveats, see the commit message for details 🙂

@NelsonVides
Rework handler and API
21b0130 
With this work, json validation becomes optional, fully implemented in
the `_api` module as it was before, but without being forcibly called by
the `_handler`. It is instead left as optional for the user to take
advantage of the exposed callbacks. Jesse also chooses draft-06 as a
default, but these can be chosen manually by the user too, as long as
jesse implements them.

`_handler` also becomes lighter, it now handles all mime types
transparently by forwarding to a user-given module that must implement
`accept_callback/4` and `provide_callback/4` as described in the
`_logic_handler` callbacks. These will simply be the return values of
cowboy_rest's `content_types_accepted` and `content_types_provided`
respectively, and should simply comply with their defined APIs. They
only get two parameters extending the behaviour, so that the user-given
callback can pattern-match on them: the path prefix of the logic
handler, and the operationID of the call.
@NelsonVides NelsonVides force-pushed the erlang_server branch 2 times, most recently from 5c40296 to 27eee49 Compare August 29, 2024 20:52
@NelsonVides
Copy link
Contributor Author

Pushed a fix for the typo that came out on CI

@NelsonVides
Copy link
Contributor Author

@wing328 what's the timeline for this PR? It's ready from my side, I've already been using it in local builds 🙂

@NelsonVides
Copy link
Contributor Author

Ok, pushed a check to make dialyzer (static typing analysis tool) stricter. Still, code is ready, looking forward to get this released :)

@wing328
Copy link
Member

wing328 commented Sep 6, 2024

https://github.com/OpenAPITools/openapi-generator/actions/runs/10724043335/job/29762060240?pr=19465

can you please update the samples when you've time?

@NelsonVides
Copy link
Contributor Author

https://github.com/OpenAPITools/openapi-generator/actions/runs/10724043335/job/29762060240?pr=19465

can you please update the samples when you've time?

Ouch, a single space went wrong 🥲 Updated!

@wing328 wing328 merged commit 596d446 into OpenAPITools:master Sep 7, 2024
19 checks passed
@wing328 wing328 mentioned this pull request Sep 7, 2024
Merged
5 tasks
@wing328
Copy link
Member

wing328 commented Sep 7, 2024

FYI. Merged #19547 to provide erlang-server-deprecated as a fallback

@NelsonVides NelsonVides deleted the erlang_server branch September 7, 2024 13:43
@NelsonVides
Copy link
Contributor Author

Amazing! Thank you so much!

@NelsonVides NelsonVides mentioned this pull request Sep 10, 2024
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Breaking change (with fallback) Enhancement: New generator Server: Erlang
Projects
None yet
Milestone
7.9.0
Development

Successfully merging this pull request may close these issues.
3 participants
@NelsonVides @wing328 @tsloughter