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] Add null-safety for dart 2.12+ #8278

Closed
harsimranmaan opened this issue Dec 28, 2020 · 30 comments
Closed

[DART] Add null-safety for dart 2.12+ #8278

harsimranmaan opened this issue Dec 28, 2020 · 30 comments

Comments

@harsimranmaan
Copy link
Contributor

Is your feature request related to a problem? Please describe.

Starting with DART 2.12 dart packages support sound null-safety making it easy to detect common pitfalls during static analysis.

The current implementation of DART2 does not follow null-safety principles.

Describe the solution you'd like

The generated code should follow sound null-safety as per https://dart.dev/null-safety.

If an openapi field is not marked as required, it should be nullable by default: eg:

    Customer:
      type: object
      required:
        - name
        - tenant_id
      properties:
        id:
          type: string
          format: uuid
          readOnly: true
        created_at:
          type: string
          format: date-time
          readOnly: true
        updated_at:
          type: string
          format: date-time
          readOnly: true
        created_by:
          type: string
          format: uuid
          readOnly: true
        tenant_id:
          type: string
          format: uuid
        name:
          type: string
          minLength: 3
          maxLength: 50

should generate a class Customer with fields like
String? id, String tenantId, String name, Datetime? createdAt ...

Describe alternatives you've considered

Use the legacy mode until null-safety is supported.

Additional context

Required by gibahjoe/openapi-generator-dart#26

@kuhnroyal
Copy link
Contributor

I have thought about this for a while. We first need to ensure OAS3 nullable/required work correctly and then we can add a switch for this. Otherwise all properties would just be nullable which doesn't help much.

@kuhnroyal
Copy link
Contributor

Besides that, there is no nullsafety version from either http or dio available yet.

@kuhnroyal
Copy link
Contributor

If an openapi field is not marked as required, it should be nullable by default

This is not always correct. In OAS3 a field can be required but nullable. Required meaning it has to be serialized but may be serialized as null depending on the nullable flag.

@harsimranmaan
Copy link
Contributor Author

This is not always correct. In OAS3 a field can be required but nullable. Required meaning it has to be serialized but may be serialized as null depending on the nullable flag.

Thanks @kuhnroyal . TIL.

@harsimranmaan
Copy link
Contributor Author

Besides that, there is no nullsafety version from either http or dio available yet.

Yes, it is a pain to adopt null-safety at the moment as most of the packages are not null-safe yet. I am hoping that we'll have null-safe versions of major packages by March/April timeframe. Maybe then, it would make sense to prioritize this

@kuhnroyal
Copy link
Contributor

Thinking about possible and allowed API combinations:

class Test {
  String? text; // optional and non-nullable
  String textWithDefault; // optional and non-nullable with default value
  String? textNullable; // optional but nullable
  String? textNullableWithDefault; // optional but nullable with default value
  String textRequired; // required and non-nullable
  String? textRequiredNullable; // required but nullable

  Test({
    this.text,
    this.textWithDefault = "foo",
    this.textNullable,
    this.textNullableWithDefault = "foo",
    required this.textRequired,
    this.textRequiredNullable,
  });
}

To achieve this, we need to first make sure that it works without NNBD.
We need to always serialize required fields, even if they are nullable.
Only optional fields can have default values (this is currently wrong in the generator).

@kuhnroyal
Copy link
Contributor

#8235 contains some work on required/nullable

@harsimranmaan
Copy link
Contributor Author

Thanks @kuhnroyal. I have also started some work to get the dio package to support null-safety but it would likely take a few iterations to stabilize. 🤞🏽
cfug/dio#1026

@kuhnroyal
Copy link
Contributor

@harsimranmaan I have created a PR which should generate the correct required/nullable fields as a precondition. Once this is merge I can start looking into getting your dio branch in here.

@harsimranmaan
Copy link
Contributor Author

Thanks @kuhnroyal . looking forward to it.

@kuhnroyal
Copy link
Contributor

kuhnroyal commented Feb 25, 2021

I have now spent some time on this. I now think that we need to support the current templates for a while in the future but we also can't reliable get the new optional types into the existing templates as this does not only related to some model fields.

I propose we copy the dart/dart-dio template folders and add a generator flag that switches the template folder path.
What do you think? @agilob @josh-burton @wing328

@agilob
Copy link
Contributor

agilob commented Feb 25, 2021

TBH I don't really care about NNBD in Dart generators right now. There's plenty of other issues that need to be solved first. NNBD is cool, but having stable and battle-tested code base is more important imo. Having stable code that we know is safe, is more important than introducing drastic changes and testing them again. Dart 2.12+ isn't as close as it seems really, 2 days ago you had a dependency hell issues with test and http packages, updating min. dart version to 2.12 will make it much worse. It'll take time before wider adoption is there, so I also see no rush to get it working.

I propose we copy the dart/dart-dio template folders and add a generator flag that switches the template folder path.

Yea, all for it, but it should live in new .mustache files as a new generator rather than a configuration flag to existing generator. Adding checks to put ? after type is going to add mess to existing templates. In the future one wide adoption is there and this issue is requested by more than 2 users, we could simply switch default generator (after v6.0?).

I'm leaving this fully with you @kuhnroyal and @harsimranmaan , I don't see urgent need to get it in 5.x

@kuhnroyal
Copy link
Contributor

Soo, we now have a beta version for Dio and it is not compatible with the current code. So we can not update the dependency ever unless we make breaking changes. And I think there will be more people looking for this soon once they notice they can't upgrade to the next Flutter version.

The primary goal now is not NNBD but a version that is compatible with all the new dependency versions, and once we are there, then null-safety is not much to do. I have it already laying around.

@kuhnroyal
Copy link
Contributor

Ok, I discussed this with @wing328. I will add a new dart-dio-experimental generator that supports null-safety and will replace the current dart-dio generator when 6.0.0 is released.

@kuhnroyal
Copy link
Contributor

#8869 is up for testing

@guitoof
Copy link

guitoof commented May 28, 2021

Hi everybody,
I am also really interested in having a sound null safety supported on the generated Dart Open API client.
Do you have any news on this being released in the coming weeks?
Thanks a lot for your work on the subject @kuhnroyal

@kuhnroyal
Copy link
Contributor

I don't think anyone is actively working on this for the dart-http client.

@agilob
Copy link
Contributor

agilob commented May 28, 2021

I did most of the work on dart native client and json_serializable, 2.12 is still not on my roadmap anytime soon, but I can help you if you want to work on it. 60% of work will probably require copying what @kuhnroyal did in dart-dio.

@shtreb
Copy link

shtreb commented Jul 3, 2021

Hi everyone.

When can we look at null safety?

@wibtu
Copy link

wibtu commented Aug 26, 2021

Any updates so far? @kuhnroyal : There was an update on the time_machine repository on 20th of july. Does that help you?

@trygvis
Copy link

trygvis commented Aug 26, 2021

This is still dependent on Dana-Ferguson/time_machine#55.

@kuhnroyal
Copy link
Contributor

I don't think anyone is actively working on the dart generator. There is dart-dio-next which is fully null-safe except for the time_machine option which still depends on the above mentioned issue.
I migrated all my code away from time_machine and am now just using the new Date class from the dart-dio-next generator to differentiate date and date-time.

@agilob
Copy link
Contributor

agilob commented Aug 27, 2021

If no one else contributes this fix, I'll do it sometime in September

@zakton5
Copy link

zakton5 commented Sep 28, 2021

@agilob Have you made any progress on this?

@agilob
Copy link
Contributor

agilob commented Sep 29, 2021

yup, will open PR very soon

@Mike278
Copy link

Mike278 commented Nov 16, 2021

Thinking about possible and allowed API combinations:

class Test {
  String? text; // optional and non-nullable
  String textWithDefault; // optional and non-nullable with default value
  String? textNullable; // optional but nullable
  String? textNullableWithDefault; // optional but nullable with default value
  String textRequired; // required and non-nullable
  String? textRequiredNullable; // required but nullable

  Test({
    this.text,
    this.textWithDefault = "foo",
    this.textNullable,
    this.textNullableWithDefault = "foo",
    required this.textRequired,
    this.textRequiredNullable,
  });
}

To achieve this, we need to first make sure that it works without NNBD. We need to always serialize required fields, even if they are nullable. Only optional fields can have default values (this is currently wrong in the generator).

does it make sense to model required × nullable with a wrapper type like this?:

                required=true  required=false
nullable=true   T?             Option<T?>?
nullable=false  T              Option<T>?

@kuhnroyal
Copy link
Contributor

@wing328 I think we can close this now

@LucaIaconelli
Copy link

LucaIaconelli commented Jan 9, 2022

Hey @kuhnroyal @agilob

But has it been completed? Is there a way to generate dart code with http using null safety?

@agilob
Copy link
Contributor

agilob commented Jan 9, 2022

#10637

@LucaIaconelli
Copy link

LucaIaconelli commented Jan 9, 2022

Oh, great job; I look forward to the 6.0 release then.
Thank you @agilob

@wing328 wing328 closed this as completed Jan 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests