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

Swagger properties not marked as nullable should not generate nullable dart fields #650

Closed
Adam-Langley opened this issue Sep 13, 2023 · 8 comments · Fixed by #657 or #676
Closed
Assignees
Labels
bug Something isn't working Triage needed

Comments

@Adam-Langley
Copy link

Adam-Langley commented Sep 13, 2023

Describe the bug
Model properties marked as nullable, are producing nullable dart properties.
It appears the generator is relying solely on the 'required' list to determine nullability.

To Reproduce
Please attach swagger and code snippet of generated value

      "MrControlDto": {
        "required": [
          "id"
        ],
        "type": "object",
        "properties": {
          "clonedFromEntityId": {
            "type": "string",
            "format": "uuid",
            "nullable": true
          },
          "hazardId": {
            "type": "string",
            "format": "uuid"
          },
          "id": {
            "type": "string",
            "format": "uuid",
            "x-primary-key": true
          }
        },
        "additionalProperties": false
      },
class Control extends DataClass implements Insertable<Control> {
  final UuidValue? clonedFromEntityId;
  final UuidValue? hazardId;
  final UuidValue id;

Expected behavior
In the above dart example, the 'hazardId' field should be of type 'UuidValue', not 'UuidValue?' because it is not nullable in the respective swagger.

Swagger specification link
Link to swagger/OpenApi documentation

Library version used:
2.11.10

Additional context
The differences between 'required' and 'nullable' are explained fairly well here:
https://stackoverflow.com/questions/45575493/what-does-required-in-openapi-really-mean

With the current implementation, it is possible for the JsonSerializable code to generate non-compliant json (i.e, with null assigned to fields that do not allow null)

@Vovanella95
Copy link
Collaborator

Hi @Adam-Langley , we generate all properties as nullable, because often swagger file 'lies' and do not returns such properties. Also, if all properties are nullable, we can send it as we wish (we can send or not send any property). It was our decision to generate it like this. It's expected behavior.

If you're sure that backend returns not nullable value - just use bang operator.

Thanks for raising issue!

@TortillaZHawaii
Copy link

I feel like it should be an option whether or not all properties should be nullable. Null safety can be found in many modern language like Kotlin, Rust or C#. Generated swagger files can be set up to respect nullability. It's a shame that it is lost in this library.

Also note that it seems like current implementation will treat:

"MrControlDto": {
  "required": [
    "id"
  ],
  "type": "object",
  "properties": {
    "id": {
      "type": "string",
      "nullable": "true"
    }
  },
  "additionalProperties": false
}

as final int id;, but in fact based on spec 3.0 id value could be null.
https://stackoverflow.com/questions/45575493/what-does-required-in-openapi-really-mean

@Vovanella95
Copy link
Collaborator

@TortillaZHawaii ok fine reopening the issue. Will provide option to generate all nullable or not

@Vovanella95 Vovanella95 reopened this Sep 27, 2023
@Vovanella95
Copy link
Collaborator

Vovanella95 commented Sep 29, 2023

@TortillaZHawaii @Adam-Langley I've implemented not nullable fields. It works on my 50+ swaggers, but I can not guarantee that there are no border cases. Please raise issues in case you face it. Thanks!

You can also use nullable_fields option to make all fields nullable.

@nsvetlakov
Copy link

hello,
In the released version 2.11.13, the error has recurred. You seem to have lost the fix somewhere.
Please do it as it was in version 12.

@Vovanella95 Vovanella95 reopened this Oct 22, 2023
@Vovanella95
Copy link
Collaborator

@nsvetlakov on which swagger file you have error?

@nsvetlakov
Copy link

this

@nsvetlakov on which swagger file you have error?

this one unde_seller.swagger.dart

@Vovanella95
Copy link
Collaborator

Vovanella95 commented Nov 10, 2023

Hi @nsvetlakov @TortillaZHawaii @Adam-Langley

Let's summarize:

  • Generator respects two properties to define, is property nullable, or not. Properties are: nullable and required
  • Properties, marked as nullable: true will be nullable
  • If property not marked as nullable, but it's not required - it will be nullable
  • Properties can be required and nullable at the same time

Example 1

"MrControlDto": {
  "required": [
    "id"
  ],
  "type": "object",
  "properties": {
    "id": {
      "type": "string",
      "nullable": "true"
    }
  },
  "additionalProperties": false
}
@JsonSerializable(explicitToJson: true)
class MrControlDto {
  const MrControlDto({
    required this.id,
  });

string, dynamic> toJson() => _$MrControlDtoToJson(this);

  @JsonKey(name: 'id', includeIfNull: false, defaultValue: '')
  final String? id; // nullable and required
  static const fromJsonFactory = _$MrControlDtoFromJson;

Example 2

"MrControlDto": {
            "required": [
                "id"
            ],
            "type": "object",
            "properties": {
                "id": {
                    "type": "string"
                }
            },
            "additionalProperties": false
        }
@JsonSerializable(explicitToJson: true)
class MrControlDto {
  const MrControlDto({
    required this.id,
  });

  @JsonKey(name: 'id', includeIfNull: false, defaultValue: '')
  final String id; // not nullable, required
  static const fromJsonFactory = _$MrControlDtoFromJson;

Example 3

"MrControlDto": {
            "type": "object",
            "properties": {
                "id": {
                    "type": "string"
                }
            },
            "additionalProperties": false
        }
@JsonSerializable(explicitToJson: true)
class MrControlDto {
  const MrControlDto({
    this.id,
  });

  factory MrControlDto.fromJson(Map<String, dynamic> json) =>
      _$MrControlDtoFromJson(json);

  static const toJsonFactory = _$MrControlDtoToJson;
  Map<String, dynamic> toJson() => _$MrControlDtoToJson(this);

  @JsonKey(name: 'id', includeIfNull: false, defaultValue: '')
  final String? id; // nullable not required

That's how it should work. If something broken - please raise new issue. This one will be closed by my next PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Triage needed
Projects
None yet
5 participants