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

Bump dart2 code compatibility to Dart 2.14 and introduce sound null-safety #10541

Closed

Conversation

provokateurin
Copy link
Contributor

@provokateurin provokateurin commented Oct 6, 2021

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

I actually started working on this a few weeks ago, before any of the current PRs against the dart2 generator were made.
I'm unsure what to do now, but I want to put out my work, so that we can have a great dart2 generator.
Anyway I update the Dart version to the latest stable and thus implemented null-safety. I wasn't able to actually test the generated code yet, but there are no errors shown anymore (due to the project I'm generated a client for using oneOf).
I deleted the tests, because they didn't do anything at all and with null-safety there were errors because of uninitialized fields.

Related PRs:
#10532
#10536

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.0), 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.

@agilob
Copy link
Contributor

agilob commented Oct 6, 2021

This PR does more than adding null safety to dart generator, I think it should be split into a few smaller PRs. A few general comments:

  • I would go as far as dart 2.14, but something older, this might be too big change for some
  • ignore and remove all changes related to json_serializable, consider this generator gone. Ideally, [dart] json_serializable: remove experimental generator #10532 would be merged before this one
  • we tend to keep changes in dart-dio and dart-dio-next and dart in separate PRs
  • with removal of generated test files this is even more braking change, this risks deleting user developed tests

}
}
if (millis != null) {
return DateTime.fromMillisecondsSinceEpoch(millis, isUtc: true);
}
}
return null;
return DateTime(1970);
Copy link
Contributor

Choose a reason for hiding this comment

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

user of this API can't distinguish between incorrect date and intentionally provided 1970

@@ -4,16 +4,16 @@ class QueryParam {
const QueryParam(this.name, this.value);

final String name;
final String value;
final String? value;
Copy link
Contributor

Choose a reason for hiding this comment

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

can query have name but no value? and for the line below, it has to be empty string not null string, as end user won't be able to distinguish intentional null string from no value. I think both fields should be not-null in this class

String message;
Exception innerException;
StackTrace stackTrace;
String? message;
Copy link
Contributor

@agilob agilob Oct 6, 2021

Choose a reason for hiding this comment

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

another design question, when does it make sense to raise ApiException without a message?

@@ -188,7 +183,7 @@ class ApiClient {
}

{{#native_serialization}}
static dynamic _deserialize(dynamic value, String targetType, {bool growable}) {
static dynamic _deserialize(dynamic value, String targetType, {bool? growable}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to make growable false or true by default? this would avoid doing growable == true a few times below, consider it micro-optimisation. I would also make it false, since other dart apis use immutable structures


@Deprecated('Scheduled for removal in OpenAPI Generator 6.x. Use serializeAsync() instead.')
String serialize(Object value) => value == null ? '' : json.encode(value);
String serialize(Object? value) => value == null ? '' : json.encode(value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it ever make sense to serialise null object?

@@ -1,8 +1,8 @@
{{>header}}
{{>part_of}}
class HttpBasicAuth implements Authentication {
String username;
String password;
String? username;
Copy link
Contributor

Choose a reason for hiding this comment

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

by making these not-null you will avoid defaulting to empty string below, another micro optimisation on our side

@@ -3,7 +3,7 @@
class OAuth implements Authentication {
OAuth({this.accessToken});

String accessToken;
String? accessToken;
Copy link
Contributor

@agilob agilob Oct 6, 2021

Choose a reason for hiding this comment

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

by making this not-null you will avoid null check below. does it make sense to use OAuth without access token? Technically yes, but practically that's a sec vuln on endpoint to accept empty code

@@ -69,7 +69,7 @@
public String defaultValue;
public String arrayModelType;
public boolean isAlias; // Is this effectively an alias of another simple type
public boolean isString, isInteger, isLong, isNumber, isNumeric, isFloat, isDouble, isDate, isDateTime, isShort, isUnboundedInteger, isBoolean;
public boolean isString, isInteger, isLong, isNumber, isNumeric, isFloat, isDouble, isDate, isDateTime, isShort, isUnboundedInteger, isPrimitiveType, isBoolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there really no other method for this already? Im really surprised this needs to be introduced as new code @wing328 ?

@@ -225,7 +214,6 @@ class {{{classname}}} {
{{/isArray}}
{{/json_serializable}}
}
return Future<{{{returnType}}}>.value();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this was added recently to satisfy stricter linter config, so pls revert #10263

@@ -66,10 +61,10 @@ class ApiClient {
String path,
String method,
List<QueryParam> queryParams,
Object body,
Object? body,
Map<String, String> headerParams,
Copy link
Contributor

Choose a reason for hiding this comment

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

headerParams and formParams are nullable in other openapi generators, so I would prefer to keep it this way for language agnostic users

}
if (value is Set && (match = _regSet.firstMatch(targetType)) != null) {
targetType = match[1]; // ignore: parameter_assignments
targetType = match![1]!; // ignore: parameter_assignments
Copy link
Contributor

Choose a reason for hiding this comment

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

yea, that's better than my defauling to Object? 👍

@agilob
Copy link
Contributor

agilob commented Oct 19, 2021

This branch was only passing tests because github actions have not kicked in and many tests didn't run...

@provokateurin
Copy link
Contributor Author

Sorry, at our company we decided it is currently not worth enough to fix the requested stuff but also the still missing stuff. Other people can you pickup these changes if they want to. Maybe we will come back to this, but currently it's not our plan to do so in a timely manner.

@agilob
Copy link
Contributor

agilob commented Oct 20, 2021

I've already rebased your branch and started fixing the above

@agilob
Copy link
Contributor

agilob commented Oct 27, 2021

Can you close this PR pls?

@provokateurin
Copy link
Contributor Author

Sure!

@agilob
Copy link
Contributor

agilob commented Oct 27, 2021

#10637

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants