-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
Feat: Add freezed serialization to dart-dio generator #13047
base: master
Are you sure you want to change the base?
Feat: Add freezed serialization to dart-dio generator #13047
Conversation
can you base your PR on #12295 ? It will help with oneOf/anyOf integration |
I thought it is BuiltValue specific changes..! Also haven't quite understood yet what is happening in the |
my PR affects only |
Wouldn't it make sense for me to rebase it from main once your PR is merged to main. I was thinking more in that line. |
if you are willing to wait until mine is merged, then yeah sure |
I think we could rebase this now and get freezed finally going. |
I will work on it this week to rebase and see if could also include oneOf/anyOf support in freezed. |
…freezed # Conflicts: # modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/DartDioClientCodegen.java # modules/openapi-generator/src/main/resources/dart/libraries/dio/serialization/freezed/api/imports.mustache # modules/openapi-generator/src/main/resources/dart/libraries/dio/serialization/freezed/class.mustache # modules/openapi-generator/src/main/resources/dart/libraries/dio/serialization/freezed/enum.mustache
}} | ||
|
||
{{#required}} | ||
{{#required}}required {{/required}}{{#isContainer}}{{baseType}}<{{#isMap}}String, {{/isMap}}{{#items}}{{>serialization/built_value/variable_type}}{{/items}}>{{/isContainer}}{{^isContainer}}{{{datatypeWithEnum}}}{{/isContainer}}{{#isNullable}}?{{/isNullable}} |
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.
if this is going to depend on serialization/built_value/variable_type
, shouldn't we move it outside the built_value
directory ?
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.
Already changed it locally.. 😄 will be up in the next merge.. 👍
Looks like this approval failed because of a test failure? When looking at the logs, it seems to be a temporary Maven failure. Can someone re-run the test again? |
@ahmednfwela Could you explain me how the new vendor extensions are to be used for the oneOf and anyOf types. I assume allOf all ready works as it should. In this freezed version I deserialize the models based on the descriminator property. This should be sufficient for both anyOf and oneOf as I understood. |
Also anybody know how to map response code to response models. Which mustache template variables can I use to create a union typed response for api endpoints. Meaning if response code is 200 map to model A and response code is 400 map to error model B. |
@bahag-chandrana the vendor extensions only describe parent/child relations, and they are:
a oneOf and anyOf describe a different problem however.
This is why I made the but since Freezed supports unions, you can just do @freezed
class MyModelUnion with _$MyModelUnion {
const factory MyModelUnion.asNum(num value) = AsNum;
const factory MyModelUnion.asString(String value) = AsString;
} |
they exist in the "responses": [
{
"headers": [],
"responseHeaders": [],
"code": "200",
"is1xx": false,
"is2xx": true,
"is3xx": false,
"is4xx": false,
"is5xx": false,
"message": "Bar created",
"dataType": "Bar",
"baseType": "Bar",
"hasHeaders": false,
"isString": false,
"isNumeric": false,
"isInteger": false,
"isShort": false,
"isLong": false,
"isUnboundedInteger": false,
"isNumber": false,
"isFloat": false,
"isDouble": false,
"isDecimal": false,
"isByteArray": false,
"isBoolean": false,
"isDate": false,
"isDateTime": false,
"isUuid": false,
"isEmail": false,
"isModel": true,
"isFreeFormObject": false,
"isAnyType": false,
"isDefault": false,
"simpleType": false,
"primitiveType": false,
"isMap": false,
"isArray": false,
"isBinary": false,
"isFile": false,
"isNull": false,
"schema": {
"$ref": "#/components/schemas/Bar"
},
"jsonSchema": "{\n \"description\" : \"Bar created\",\n \"content\" : {\n \"application/json\" : {\n \"schema\" : {\n \"$ref\" : \"#/components/schemas/Bar\"\n }\n }\n }\n}",
"vendorExtensions": {},
"uniqueItems": false,
"exclusiveMinimum": false,
"exclusiveMaximum": false,
"vars": [],
"requiredVars": [],
"hasValidation": false,
"additionalPropertiesIsAnyType": false,
"hasVars": false,
"hasRequired": false,
"hasDiscriminatorWithNonEmptyMapping": false,
"hasMultipleTypes": false,
"content": {
"application/json": {
"schema": {
"openApiType": "Bar",
"baseName": "SchemaFor200ResponseBodyApplicationJson",
"complexType": "Bar",
"getter": "getSchemaFor200ResponseBodyApplicationJson",
"setter": "setSchemaFor200ResponseBodyApplicationJson",
"dataType": "Bar",
"datatypeWithEnum": "Bar",
"name": "schemaFor200ResponseBodyApplicationJson",
"defaultValueWithParam": " = data.SchemaFor200ResponseBodyApplicationJson;",
"baseType": "Bar",
"example": "null",
"jsonSchema": "{\n \"$ref\" : \"#/components/schemas/Bar\"\n}",
"exclusiveMinimum": false,
"exclusiveMaximum": false,
"required": false,
"deprecated": false,
"hasMoreNonReadOnly": false,
"isPrimitiveType": false,
"isModel": true,
"isContainer": false,
"isString": false,
"isNumeric": false,
"isInteger": false,
"isShort": false,
"isLong": false,
"isUnboundedInteger": false,
"isNumber": false,
"isFloat": false,
"isDouble": false,
"isDecimal": false,
"isByteArray": false,
"isBinary": false,
"isFile": false,
"isBoolean": false,
"isDate": false,
"isDateTime": false,
"isUuid": false,
"isUri": false,
"isEmail": false,
"isNull": false,
"isFreeFormObject": false,
"isAnyType": false,
"isArray": false,
"isMap": false,
"isEnum": false,
"isInnerEnum": false,
"isReadOnly": false,
"isWriteOnly": false,
"isNullable": false,
"isSelfReference": false,
"isCircularReference": false,
"isDiscriminator": false,
"vars": [],
"requiredVars": [],
"vendorExtensions": {},
"hasValidation": false,
"isInherited": false,
"nameInCamelCase": "SchemaFor200ResponseBodyApplicationJson",
"nameInSnakeCase": "SCHEMA_FOR200_RESPONSE_BODY_APPLICATION_JSON",
"uniqueItems": false,
"isXmlAttribute": false,
"isXmlWrapped": false,
"additionalPropertiesIsAnyType": false,
"hasVars": false,
"hasRequired": false,
"hasDiscriminatorWithNonEmptyMapping": false,
"hasMultipleTypes": false,
"ref": "#/components/schemas/Bar",
"schemaIsFromAdditionalProperties": false,
"datatype": "Bar",
"iexclusiveMaximum": false,
"hasItems": false
},
"testCases": {}
}
},
"schemaIsFromAdditionalProperties": false,
"range": false,
"isPrimitiveType": false,
"wildcard": false,
"complexType": "Bar"
}
] |
I kinda avoided the usual approach in order to utilize the existing models. In freezed we can't use existing data models to build a union. So as an alternative I map oneOf/anyOf as following. @freezed
class MyModelUnion with _$MyModelUnion {
const factory MyModelUnion.asObj1({
required Obj1 obj1;
}) = AsObj1;
const factory MyModelUnion.asObj2({
required Obj2 obj2;
}) = AsObj2;
} Then in the fromJson i use the discriminator to map it to the correct model. We use this approach in our project a lot and it works pretty good. |
Hi @bahag-chandrana Could we make this PR work for current |
Well This PR already works for current dart-dio. It's been waiting approval since long time and not sure what's holding things back. 😊 |
it's mainly missing tests, which will be hard to add without the dart next generator decoupling things |
Hi I understand your concerns. For me it makes sense to add this feature so any one could use it and contribute in case of bugs rather than holding the merge for unknown amount of time. |
it can be added as a beta feature, if you want to use it early without tests |
Makes sense from my point of view |
I could add tests for sure. Only as of now I am not clear how. But on the other hand I have created an entire repository with all the different cases for this pr along with the sample generated code. Maybe that helps. https://github.com/bahag-chandrana/test-json-serialize |
Thanks for the work with the freezed generator @bahag-chandrana! While using it, I noticed an issue when generating lists of items, here's a branch which hopefully fixes the problem too. Given the example spec below, which contains an array: components:
schemas:
TestObj:
type: object
properties:
testElements:
type: array
items:
type: string
required:
- testElements the following Dart code is generated, notice the nullable @freezed
class TestObj with _$TestObj {
const TestObj._();
const factory TestObj({
@JsonKey(name: r'testElements') required List<String?> testElements,
}) = _TestObj;
factory TestObj.fromJson(Map<String, dynamic> json) =>
_$TestObjFromJson(json);
}
With the changes on this branch, this now generates: @freezed
class TestObj with _$TestObj {
const TestObj._();
const factory TestObj({
@JsonKey(name: r'testElements') required List<String> testElements,
}) = _TestObj;
factory TestObj.fromJson(Map<String, dynamic> json) =>
_$TestObjFromJson(json);
} Which in inline with the types created by other generators, such as dart-dio with built_value serialiser, or the typescript-axios generator (i.e. non-nullable): export interface TestObj {
testElements: Array<string>;
} My hypothesis about where this issue comes from is the original variable_type.mustache is recursive and so is being called for the outer The outer To solve this, I've added a separate As a disclaimer, I've not heavily tested this (e.g. with nested collections) - there may be other issues - so it would be great to get initial thoughts on this behaviour. |
I noticed this issue. However I am not sure assuming all the entries in the list wont contain null could be lead to runtime issues when working with those list. Hence I opted to mark them nullable when the schema-component itself is marked as nullable. If there is a counter argument for this point of view then I think we can change it. I am just not sure which is right. |
Hey, folks, it will be 2 years since this one is open next month, so I'm curious what this PR's state is. Thanks! |
please resolve the merge conflicts (by updating this PR or filing a new one based on this one) once that's done. please PM me via Slack and we will discuss how to move it forward https://join.slack.com/t/openapi-generator/shared_invite/zt-12jxxd7p2-XUeQM~4pzsU9x~eGLQqX2g |
…criminator and with discriminator mapping.
for tests, i think you will need to add a new config file similar to and generate the samples (e.g. run and add the new sample folder to a new dart-dio workflow file similar to https://github.com/OpenAPITools/openapi-generator/blob/master/.github/workflows/samples-dart.yaml |
some mustache files contain tabs which should be replaced with spaces instead
|
…ing an if else block.
…ld samples of freezed.
@wing328 Thanks a lot for the feedback. I have added the samples generated files and the created the workflow yaml as requested. Also fixed the tab issues. |
I am thinking of a better solution for implementing unions using freezed for the oneOf, anyOf cases. So I need some more time to work on this PR. |
This PR is aimed at adding the freezed serialization to the dart-dio generator. The changes were actually copied from changes made on old branch which was tested extensively in personal projects. Since the branches diverged I have migrated the changes to the freshly cloned branch.
The new changes could be tested by simply changing the serialization library to
freezed
.PR checklist
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.
master
(6.1.0) (minor release - breaking changes with fallbacks),7.0.x
(breaking changes without fallbacks)@jaumard (2018/09) @josh-burton (2019/12) @amondnet (2019/12) @sbu-WBT (2020/12) @kuhnroyal (2020/12) @agilob (2020/12) @ahmednfwela (2021/08)
TODO's: