-
Notifications
You must be signed in to change notification settings - Fork 6k
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] - Rework Dart client generator to be flutter-compatible #7418
[Dart] - Rework Dart client generator to be flutter-compatible #7418
Conversation
I don't understand the error log of AppVeyor. Since I removed the flutter generator it shouldn't search for that. 🤔 |
OK, nevermind, I think I found the problem |
Very good indeed! I want to merge this as soon as possible! Maybe you try to remove all the dev_dependencies, because in my understanding neither "browser" nor "guinness" is used anywhere at all. And the dartdevc problem seems to originate in the guiness library. |
Thanks @ircecho, I'm happy to hear that! You are talking about these lines, right? These are needed for the testcases in https://github.com/swagger-api/swagger-codegen/tree/master/samples/client/petstore/dart/petstore/test Instructions for running these tests: https://github.com/swagger-api/swagger-codegen/blob/master/samples/client/petstore/dart/petstore/README.md |
Yes, that is true! It is very unfortunate, that the tests have been written with a now unmaintained library, which is no longer usable with dartdevc. One day, they will have to be rewritten with proper dart testing libraries. However, I think we should leave the situation alone and ignore dartdevc for the moment, and when there is time, fix it in another pull request. However, I think that the dependencies on browser and guinness can safely be dropped from Other than that, I think this is merge-ready. @wing328 |
Unfortunately there is a problem when serializing maps. "strings": {
"type": "object",
"additionalProperties": {
"$ref": "#/definitions/ArticleSpecificationStrings"
}
}, This apparently creates a map in the dart code which is correct: Map<String, ArticleSpecificationStrings> strings = {}; However, the serialization/deserialization code is erroneous. @joernahrens could you fix this, please? There's another thing as well, which makes fixing this problem a lot easier. You should rename the method toMap to toJson, and the encoder will automatically use it when encoding complex objects, which should save you a lot of complexity because 'articleRepresentations': articleRepresentations == null ? null : ArticleRepresentation.toMapList(articleRepresentations), can become 'articleRepresentations': articleRepresentations, and you can throw away all the toMapList and so on methods and JSON.encode(.) should take care of everything. I did a small proof-of-concept and it seems to work, however, there may be problems when implementing it in the whole library. To deserialize the maps I created a method like this static Map<String, ProductStrings> mapFromJson(Map<String, Map<String, dynamic>> json) {
var map = new Map<String, ProductStrings>();
if (json != null && json.length > 0) {
json.forEach((String key, Map<String, dynamic> value) => map[key] = new ProductStrings.fromJson(value));
}
return map;
} It has to be used in the right places, but that should fix everything. |
@ircecho I already was expecting something.. 😉 |
379615c
to
fdecd30
Compare
Made some changes @ircecho, thx for the |
@joernahrens thanks for the quick update based on the feedback. @ircecho I wonder if you can take another look and I'll merge it if it looks good to you. |
@@ -103,7 +93,7 @@ class ApiClient { | |||
} else if (obj is String) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if, as a whole, should be unnecessary. JSON.convert(.) should be able to cope with all those types out of the box.
{{name}} = | ||
{{#complexType}} | ||
{{#isListContainer}}{{complexType}}.listFromJson(json['{{name}}']){{/isListContainer}}{{^isListContainer}} | ||
{{#isMapContainer}}new {{complexType}}.mapFromJson(json['{{name}}']){{/isMapContainer}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This leads to a problem because mapFromJson rightly is not a constructor.
Example code generated:
strings =
new ProductStrings.mapFromJson(json['strings'])
;
Problems detected:
error • A value of type 'ProductStrings' can't be assigned to a variable of type 'Map<String, ProductStrings>' at lib/model/product_representation.dart:57:9 • invalid_assignment
error • The class 'ProductStrings' doesn't have a constructor named 'mapFromJson' at lib/model/product_representation.dart:57:28 • new_with_undefined_constructor
new
needs to be removed. We should also add a test that uses maps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi again, sorry, that new
was wrong of course..
I fixed that, and also removed the line for string "serialization". How would you add a test for this in general? All the testing is based on the petstore.yml at the moment. Would be best, to extend that API to include an API-call including a Map
, or did you have a simpler test in mind @ircecho ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
petstore.yaml only covers the basic while petstore-with-fake-endpoints-models-for-testing.yaml contains a lot more edge cases.
You may consider updating ./bin/dart-petstore.sh
to use petstore-with-fake-endpoints-models-for-testing.yaml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wing328 Thanks, for the hint, I could have found this by myself 😁
I gave it a short try and saw, that there are a LOT of errors to fix. There are things like:
- Classes starting with numbers
- Other names not matching dart requirements
- Maps in maps don't work at the moment
- maybe many more..
I started this a bit, but would provide a further PR if I have the time to fix these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes absolutely! Better fix the crazy stuff later!
Oh and I found another bug: "debugInformation" : {
"type" : "object"
}, When I'm using a field like this, which correctly leads to field Object debugInformation = null; but Object does not have a toJson or fromJson method. debugInformation =
new Object.fromJson(json['debugInformation'])
; @wing328 How do you think we should handle this situation? Because we cannot properly deserialize or serialze an object if we do not know what type it is. |
We encounter similar issues with other generators based on the same reason that there's no way the API client can serialize/deserialize the payload without knowing anything (e.g. properties) about the object. My suggestion is to avoid using |
a79bb8b
to
d99040e
Compare
@wing328 That is exactly that kind of behaviour from a code generator that bums people out. If it is valid code in the specification, it should also produce valid code in the generator. @joernahrens On the curious case of "debugInformation" : {
"type" : "object"
}, I suggest we use dynamic instead of Object, e.g. dynamic debugInformation = null; and in the fromJson method just use debugInformation = json['debugInformation']; And then I hope we can finally merge this PR! This will fix so many problems! |
Yup, I agreed the spec is correct but most generators do not work well in this case and users may find it surprising. I'll try to add an FAQ to educate the users about clearly documenting the "object" with properties. |
@ircecho I'd agree with the solution, but what would be the best way to handle this |
@joernahrens Is there no way to check, if it is a plain object? I assumed as much. Well if it is not that simple, I guess we can keep it as it is for the moment and fix it later. |
Agreed with fixing it later. If there's no other pending item, I'll merge this PR tomorrow. |
The circleci build failed, but I don't really see if this PR caused that (I think not). Otherwise, yes, sounds good, thanks for all feedback. |
No worry. It fails more frequently for other jobs too. I'll restart it later |
Thanks a lot for this PR, I would also like to use Flutter. Was just trying this, and got a problem when using enums. Its starting to use dartson again and does some strange quoting:
|
@cputoaster Sorry, I didn't put work into enums. As far I understood this also wasn't completely supported before. Since I didn't change the mustache template for it, this weird |
@cputoaster please open a separate issue to track it. The |
string enums need to generated with triple handlebars. swagger-api#7418 (comment)
string enums need to generated with triple handlebars. swagger-api#7418 (comment)
string enums need to generated with triple handlebars. swagger-api#7418 (comment)
string enums need to generated with triple handlebars. swagger-api#7418 (comment)
Hey, i am just curious and a little confused. This PR definitely seems to be something I am looking for which is to generate a Flutter 1.0 compatible Swagger Client Library. I am unsure which branch/jar of swagger-codgen-cli to use to be able to use the work the that has been done in this PR. Thank you. |
Any update on the question from @aroraenterprise ? |
Just add the option After that, follow the install instructions in the generated folder README.md |
Where to run this command? |
Easiest way is to use the online generators. |
PR checklist
./bin/
to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.sh
and./bin/security/{LANG}-petstore.sh
if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in.\bin\windows\
.3.0.0
branch for changes related to OpenAPI spec 3.0. Default:master
.Description of the PR
Resolves #7367
The dart code generated by the
DartClientCodegen
is not working with Flutter due to the usage of dartson, which uses dart-mirrors. This PR introduces a major change to the Dart generator which generates flutter-compatible Dart code.This is a new PR, but based on my first try, where I created a new flutter generator (based on the one for Dart).
The existing test cases for web are still working, but I didn't work for me with
dartdevc
, I am getting compile errors when trying that:PTAL @ircecho
Sample
In
/samples/client/petstore/flutter
I created a flutter app which is basically doing some petstore API calls and logging the output.This is not to see as a complete solution, I didn't test every feature of swagger, for example I left out authentication completely for now. There might be still a decent amount of work to be done for that.
But the sample shows that there are some things working already. If you want to test that you will have to setup the Flutter SDK. Afterwards running the app from the folder
/samples/client/petstore/flutter/flutter_petstore
should be working and show you some API responses in the console output.Output
Btw, the output of the inventory looks really weird, but it looks the same when using Swagger UI