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

[dart][dart-dio] Next-gen dart-dio generator #8869

Merged
merged 31 commits into from
Mar 20, 2021

Conversation

kuhnroyal
Copy link
Contributor

@kuhnroyal kuhnroyal commented Mar 2, 2021

WIP!

This is work in progress but examples compile and tests run locally. This needs an updated Dart version in CI.

Description

This PR establishes a next-gen dart-dio generator with breaking changes around null-safety which can not be implemented in backwards-compatible ways.

Features & changes

TODO

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/utils/export_docs_generators.sh
    
    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*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master, 5.1.x, 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@auto-labeler
Copy link

auto-labeler bot commented Mar 2, 2021

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

@kuhnroyal
Copy link
Contributor Author

CC @agilob @josh-burton

@robrbecker
Copy link

robrbecker commented Mar 3, 2021

I'm having a heck of a time getting this to build locally. I have tried to build with openjdk8 and oracle java 14.0.2.
Had to comment out the javadocs plugin from pom.xml as that was crashing with an exception.
I'm able to run mvn clean package successfully though.
Still getting an exception when running ./bin/generate-samples.sh bin/configs/dart-dio-next*

[main] INFO  o.o.codegen.TemplateManager - writing file samples/openapi3/client/petstore/dart-dio-next/petstore_client_lib_fake/doc/DogAllOf.md
[main] WARN  o.o.c.languages.AbstractDartCodegen - 200_response (model name starts with number) cannot be used as model name. Renamed to Model200Response
[main] ERROR o.o.c.api.TemplatingEngineAdapter - Failed to read full template enum_inline.mustache, enum_inline.mustache
[main] ERROR o.o.c.api.TemplatingEngineAdapter - Failed to read full template enum_inline.mustache, enum_inline.mustache
Exception in thread "main" java.lang.RuntimeException: Could not generate model 'EnumArrays'
	at org.openapitools.codegen.DefaultGenerator.generateModels(DefaultGenerator.java:544)
	at org.openapitools.codegen.DefaultGenerator.generate(DefaultGenerator.java:875)
	at org.openapitools.codegen.cmd.Generate.execute(Generate.java:440)
	at org.openapitools.codegen.cmd.OpenApiGeneratorCommand.run(OpenApiGeneratorCommand.java:32)
	at org.openapitools.codegen.OpenAPIGenerator.main(OpenAPIGenerator.java:66)
Caused by: org.openapitools.codegen.templating.TemplateNotFoundException: enum_inline
	at org.openapitools.codegen.templating.MustacheEngineAdapter.findTemplate(MustacheEngineAdapter.java:79)
	at org.openapitools.codegen.templating.MustacheEngineAdapter.lambda$compileTemplate$0(MustacheEngineAdapter.java:61)
	at com.samskivert.mustache.Mustache$IncludedTemplateSegment.execute(Mustache.java:756)
	at com.samskivert.mustache.Mustache$BlockSegment.executeSegs(Mustache.java:845)
	at com.samskivert.mustache.Mustache$InvertedSegment.execute(Mustache.java:910)
	at com.samskivert.mustache.Mustache$BlockSegment.executeSegs(Mustache.java:845)
	at com.samskivert.mustache.Mustache$SectionSegment.execute(Mustache.java:870)
	at com.samskivert.mustache.Mustache$BlockSegment.executeSegs(Mustache.java:845)
	at com.samskivert.mustache.Mustache$SectionSegment.execute(Mustache.java:866)
	at com.samskivert.mustache.Template.executeSegs(Template.java:157)
	at com.samskivert.mustache.Mustache$IncludedTemplateSegment.execute(Mustache.java:774)
	at com.samskivert.mustache.Mustache$BlockSegment.executeSegs(Mustache.java:845)
	at com.samskivert.mustache.Mustache$SectionSegment.execute(Mustache.java:881)
	at com.samskivert.mustache.Template.executeSegs(Template.java:157)
	at com.samskivert.mustache.Mustache$IncludedTemplateSegment.execute(Mustache.java:774)
	at com.samskivert.mustache.Mustache$BlockSegment.executeSegs(Mustache.java:845)
	at com.samskivert.mustache.Mustache$InvertedSegment.execute(Mustache.java:910)
	at com.samskivert.mustache.Mustache$BlockSegment.executeSegs(Mustache.java:845)
	at com.samskivert.mustache.Mustache$SectionSegment.execute(Mustache.java:881)
	at com.samskivert.mustache.Mustache$BlockSegment.executeSegs(Mustache.java:845)
	at com.samskivert.mustache.Mustache$SectionSegment.execute(Mustache.java:866)
	at com.samskivert.mustache.Template.executeSegs(Template.java:157)
	at com.samskivert.mustache.Template.execute(Template.java:134)
	at com.samskivert.mustache.Template.execute(Template.java:125)
	at org.openapitools.codegen.templating.MustacheEngineAdapter.compileTemplate(MustacheEngineAdapter.java:65)
	at org.openapitools.codegen.TemplateManager.write(TemplateManager.java:163)
	at org.openapitools.codegen.DefaultGenerator.processTemplateToFile(DefaultGenerator.java:1017)
	at org.openapitools.codegen.DefaultGenerator.generateModel(DefaultGenerator.java:382)
	at org.openapitools.codegen.DefaultGenerator.generateModels(DefaultGenerator.java:535)

@robrbecker
Copy link

I think I got it running enough to generate my project by running something like
java -jar openapi-generator/modules/openapi-generator-cli/target/openapi-generator-cli.jar generate ... <params>

@kuhnroyal
Copy link
Contributor Author

Hmm, the error looks like I might have messed something up with the templates but the example generates, not sure.

I think I got it running enough to generate my project by running something like
java -jar openapi-generator/modules/openapi-generator-cli/target/openapi-generator-cli.jar generate ... <params>

That is the way I am using it outside of the build in examples.

@vishna
Copy link

vishna commented Mar 7, 2021

Just stumbled upon this, looks promising - good job 👍 I was wondering if it would be easier to use freezed instead of build_value?

@kuhnroyal
Copy link
Contributor Author

Good question and one I have evaluated before. freezed does not support the level of enums that are required for OpenAPI - see rrousselGit/freezed#74

But this new version of the generator will allow to add different serialization libraries in the future. You are welcome to start working on a freezed implementation and see where that leads you.

}

class BasicAuthInterceptor extends AuthInterceptor {
Map<String, BasicAuthInfo> authInfo = {};
Map<String, BasicAuthInfo> authInfo = {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, this was just a copy/paste job from dart-dio.

import 'package:dio/dio.dart';
import 'package:{{pubName}}/src/auth/auth.dart';

class OAuthInterceptor extends AuthInterceptor {
Map<String, String> tokens = {};
Map<String, String> tokens = {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final for consistency?

This is much faster than individual files. The committed samples should be formatted since Dart is very opinionated and it makes diffs in PRs much easier to read.
Due to this, we also need to format in CI, otherwise there is a git diff.
@wing328 wing328 marked this pull request as ready for review March 19, 2021 17:02
@wing328 wing328 added this to the 5.1.0 milestone Mar 20, 2021
@wing328 wing328 merged commit 3d038b7 into OpenAPITools:master Mar 20, 2021
@kuhnroyal kuhnroyal deleted the dio-next-gen branch March 20, 2021 12:29
@jannikrh
Copy link

jannikrh commented Mar 26, 2021

If the additional library timemachine is not used, the Date format cannot be used. A complete DateTime string is always generated. Is it possible to distinguish between Date and DateTime like in the "normal" Dart generator?

As an example the following formatter is used in the Dart generator: final _dateFormatter = DateFormat('yyyy-MM-dd');
The example of the distinction between Date and DateTime is in the following file starting at line 36.
openapi-generator/modules/openapi-generator/src/main/resources/dart2/serialization/native/native_class.mustache

@agilob
Copy link
Contributor

agilob commented Mar 26, 2021

If the additional library timemachine is not used, the Date format cannot be used. A complete DateTime string is always generated. Is it possible to distinguish between Date and DateTime like in the "normal" Dart generator?

Could you open an issue for that and call me and @kuhnroyal?

@kuhnroyal
Copy link
Contributor Author

Don't think this is possible unfortunately, but I have not tried. built_value can only have one serializer per class and date/datetime is the same class DateTime in plain Dart.

@jannikrh
Copy link

I will open an issue later today and tag you. The implementation might be possible by extending Iso8601DateTimeSerializer in combination with FullType parameters. The DateTime serializer would be called in each case. Once as now and once with the parameter date or similar.

@agilob
Copy link
Contributor

agilob commented Mar 26, 2021

lets not comment here again, we will lose context of the problem. call us to the issue

@TimWhiting
Copy link

@kuhnroyal
I'd love to have freezed support. I'm not understanding the relation of rrousselGit/freezed#74 to enum support in OpenAPI. Is it a problem with modeling enums or serializing? Serializing should be taken care of by JsonSerializable shouldn't it? Can you elaborate more on the issues with freezed support?

@Abhilash-Chandran
Copy link

@TimWhiting I already have a version that uses freezed. I just need to clean it up a bit and raise a PR which probably @kuhnroyal could validate. I am using normal enum and seems to work well. Will create a PR today... 😄

@TimWhiting
Copy link

@Abhilash-Chandran I'm happy to test it on my apis to see if it makes sense. Can you push what you have so far to a branch?

@dastein1
Copy link

@Abhilash-Chandran I'd be interested in this as well.

@TimWhiting
Copy link

TimWhiting commented Jun 22, 2022

@dastein1 @Abhilash-Chandran
Abhilash, I've taken your branch that you gave me the link to, and squashed your commits, and rebased on master. It has a much cleaner PR / diff now. However, I think I broke something in the rebase. Anyways, for those who are interested, Abhilash's branch does work even if it is a bit out of date / sync with the rest of the repo:

Here is a link to @Abhilash-Chandran's branch:
https://github.com/bahag-chandrana/openapi-generator
Here is the link to the rebased changes:
https://github.com/TimWhiting/openapi-generator/tree/freezed_2

If anyone wants to try to get the rebased branch working & a PR opened, that would probably be a good place to start. I'll see what time I have, but I can't make any promises. I was hoping to see allOf and oneOf support with inheritance & freezed unions, but unfortunately that looks like it will be more work.

Here are the instructions I got from @Abhilash-Chandran to run his branch:
(Build the project from source as explained in the readme)
You can find the jar inside modules/openapi-generator-cli/target/openapi-generator-cli.jar

java -jar ../openapi-generator-cli.jar generate \
      -i api_spec.yaml \
      -g dart-dio-next \
      -c generator_config.yaml 

with the generator_config.yaml containing the additional property to enable freezed.

additionalProperties:
  serializationLibrary: freezed

@Abhilash-Chandran
Copy link

I saw some simiallar issues while building after rebasing. I will try to rework on this from the fresh branch. But no promises yet. Especially the way the templates are referred to has changed since I began working on this. So my idea is to port my changes directly on a fresh fork and hence avoid a host of other errors.

@TimWhiting
Copy link

@Abhilash-Chandran I did end up copying the json_serializable/api folder to the freezed folder. That seemed to be the only major change needed to get it to compile & generate valid dart files. However, I have not tried using the generated api yet. The changes are on my branch.

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.

None yet

9 participants