[Dart-Dio-Next]#9082 Add json_serializable serialization option

[MichaelMarner]

This is a WIP/PoC to add json_serializable as an alternative to built_value in the Dart-Dio-Next generator.

For context: We would like to migrate our Flutter app to full null safety, and so we are migrating from the old Dart generator to Dart-Dio-Next.

We have found that moving from models with to/fromJson methods to using built_value has made the job of migrating bigger and riskier than we'd like. We need to rewrite large chunks of our existing persistence layer and other parts.

Adding json_serializable as an option has mostly involved reworking existing templates in the old Dart generator to generate null safe code.

Fixes #9082

@swipesight @jaumard @josh-burton @amondnet @sbu-WBT @kuhnroyal @agilob

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.3.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.

[kuhnroyal]

Nice job! Was hoping someone gets the ball rolling on this and prepared the codebase somewhat.
We gonna need a sample project. I hope I can pitch in a little bit if you need help.

[jbrisko]

I am also very interested in this. I would love to test it. Migrating our project to built_value would be super inconvenient.

[agilob]

I think this should be new generator extendin dartdio.java not a config flag on dio, it's a big change in generator and there's a lot of potential for generator conflicts and bugs in generated code if they life together. If you decide to keep them together under one generator at least add tests for dio-json_serialisabable

[jaumard]

no sure that's wanted, why changing this ? if it's a requirement for the lib please do it only for it

[MichaelMarner]

Sorry this wasn't meant to be committed. The code generated by json_serializable includes a lot of implicit dynamic and so this option throws up a lot of errors in the generated code.

How do we feel about excluding *.g.dart instead, so only the generated code is excluded? Building on that, only exclude generated code if we're using json_serializable?

[jaumard]

to me doesn't make sens to analyze generated code, so I'm fine to exclude *.g.dart for all

[kuhnroyal]

I am not sure you can exclude files from strong-mode. I thought this only works for the lints. But if it works then go for it.

[elliots]

This seems to work? google/json_serializable.dart#557 (comment)

[jaumard]

I think this should be new generator extendin dartdio.java not a config flag on dio, it's a big change in generator and there's a lot of potential for generator conflicts and bugs in generated code if they life together. If you decide to keep them together under one generator at least add tests for dio-json_serialisabable

We will not make a new generator each time we want to change the serialization library. It doesn't make sens and will be a nightmare to maintain. It need to be done properly to avoid conflicts and bugs, here it's on different files for the most parts so it's fine. But sure it would be better with tests.

I'm interested to use freezed at some point so I really think the config flag is the way to go to centralize all the Dio related stuff.

[kuhnroyal]

I think this should be new generator extendin dartdio.java not a config flag on dio, it's a big change in generator and there's a lot of potential for generator conflicts and bugs in generated code if they life together. If you decide to keep them together under one generator at least add tests for dio-json_serialisabable

I disagree, we should not start a new generator. We end up with no one maintaining it. I prepared the dart-dio-next generator exactly for this use case. And yes we need a sample and tests :)

[MichaelMarner]

Hey thanks for the feedback so far.

Just to give an indication of where I'm at with this. We spent most of last week migrating our mobile app from Swagger/codegen to this branch of Dart-Dio-Next. So far all appears to have worked well with no bugs introduced to our app. The mobile app is scheduled for a release next week, and so this week is mostly internal testing and QA. So far though we're happy with the code this branch is generating.

Once our app update has been released I'll have a bit of time to come back and tidy up this PR. Agreed tests and samples are needed to ensure both this works, and that it doesn't break the built_value serialisation. I pushed this PR early as a draft just so people can be aware it's happening and to provide early feedback.

Cheers

[zbarbuto]

Hey @MichaelMarner - I had some time so I had a go at adding a sample project and some tests to your changes.

It uncovered a couple of issues in the templates which I've addressed wherever possible however I'm unsure how to progress it any further as I'm not familiar enough with this Java/OpenAPI/maven to wire up the automated testing (although I gave it a go based on the existing projects).

Happy to keep pushing it forward if I can get some feedback on it.

[kuhnroyal]

Good work, i'lll try to checkout the sample and see if I can help.

One thing I noticed is the use of mutable models. I would like to see immutable models by default and if anyone requires mutable models, we could add a flag for it.

Can you merge master when you get a chance?

[zbarbuto]

Thanks @kuhnroyal - merged master into my branch but makes the PR into Michael's fork pretty noisy so might be better to just rebase and cherry pick once happy with it.

[MichaelMarner]

Hey so it turns out that real work kind of took priority over this PR, but I've just made a few changes to fix some things up:

• Merged master back in
• Changed properties to final to make the models non-mutable
• Modified the property declarations so non-required properties are marked as nullable, since Dart nullable means something different to OpenAPI nullable.

[elliots]

Hi, I'm using a version before these latest commits and was relying on mutable models (when sending back to the rest API for creation/update). I'm quite new with dart, is there a different way I should be doing this? ... or could properties be made optionally final?

[MichaelMarner]

Hi @elliots all properties are named parameters in the model constructor, so you can set them that way:

new MyModel(property1: "asdf", property2: "etc");

I can look at adding an option for optional final if people think it is a worthwhile addition.

[elliots]

I'm updating individual properties on the model as the user performs actions, if they are final I would need a second model for the ui that I then use to create the values to post back to the server.

Worth adding an option if you think others would do similar, but I've forked with the 'final' removed for my use so no worries.

[ayushin]

is this still active? how about adding https://pub.dev/packages/freezed

[MichaelMarner]

We've been using this in our production app since August of last year without any issues. At this point I think it's ready to go but I'm not sure what the next steps are, as I'm new to this project

[agilob]

needs includeIfNull to be used in conjunction with required. If field is required, then null value must be included

[MichaelMarner]

I believe I have addressed this now

[agilob]

enum parameter can have a value, so field FIELD_1 can be represented by field-1 and so field-1 must be mapped to FIELD_1

[agilob]

This supported from 4.2.0 https://pub.dev/documentation/json_annotation/latest/json_annotation/JsonEnum/fieldRename.html

[MichaelMarner]

Latest commits have fixed up generation of enums, and I've used @JsonValue() to specify the enum values

[agilob]

same mapping here

[agilob]

Please add sample project for this generator. I think it should target 6.0.x branch btw

[wing328]

Please target current master, which will be released as v6.0.0.

By the way, shall we replace dart-dio with dart-dio-next as part of the v6.0.0 release?

cc @jaumard (2018/09) @josh-burton (2019/12) @amondnet (2019/12) @sbu-WBT (2020/12) @kuhnroyal (2020/12) @agilob (2020/12) @ahmednfwela (2021/08)

[MichaelMarner]

I created a PR in your repository, fixing some enum problems, regenerated tests and added the sample to maven/CI.

Have merged your PR back into this

[kuhnroyal]

I'll check later for the failures, must have a typo in the POM.

[ayushin]

Is this not a good time to add an option for freezed as well?

[kuhnroyal]

Is this not a good time to add an option for freezed as well?

If you write it sure :)

[MichaelMarner]

Is this not a good time to add an option for freezed as well?

I think it might be a bit of scope creep to also introduce freezed to this PR. I'd really like to see this merged so I don't have to keep rebasing against master as new changes come in. It will be easy enough and cleaner to extend this branch to include freezed as an option later on.

My 2c anyway.

[kuhnroyal]

Yea definitely not as part of this PR.
@MichaelMarner Can you check my PR in your repo?

[kuhnroyal]

Needs a merge again

[kuhnroyal]

@MichaelMarner Can you merge again please, want to get this in

[MichaelMarner]

Sorry, was a long weekend in South Australia and I was out of town.

Have merged master back into this branch, and have also made the following changes:

• Changed the GitHub action for Dart tests to use Dart 2.14.0 (was previously 2.13). This is a requirement for json_annotation >= 4.2 which this branch uses. See discussion above regarding enums.
• Made minor changes to the templates to generate code that passes dart analyze

[kuhnroyal]

Big thanks @MichaelMarner !  🤘

[MichaelMarner]

Woo! Thanks for your patience everyone.