Carapacik / swagger_parser

Dart package that takes an OpenApi definition file and generates REST clients based on retrofit and data classes for your project.
https://pub.dev/packages/swagger_parser
MIT License
90 stars 38 forks source link

unknownEnumValue support for json_serializable #106

Closed savage7 closed 10 months ago

savage7 commented 10 months ago

Could you please add an option that enums are generated with unkown value? This enables enums to be more safe for future additions.

json_serializable does unfortunately not support null values for unkown cases, but you can define a fallback value like unkown. The configuration in json_serializable for this looks like this:

@JsonEnum()
enum MenuItemType {
...
  @JsonValue('unkown')
  unkown;
}

@JsonKey(
   name: 'type',
   unknownEnumValue: MenuItemType.unkown,
)
 final MenuItemType type;
Sancene commented 10 months ago

I do not see why that would be necessary. We are using enums to specifically escape implementing default behaviour and this addition will go against it.

savage7 commented 10 months ago

@Sancene It's just a way to be more robust regarding interfaces changes. When a new Enum Value is added the client has the possibility to handle this case. Otherwise the the request will just fail without updated generated code.

Lets say I have this version of the enum and a request which returns an array:

enum MenuItemType {
 foo
}
class MenItem {
 var title,
 var type
}
List<MenuItem> getMenuItems()

When I change the backend implementation by ie. adding "anotherFoo" and send a type with anotherFoo, the getMenuItems will fail. With the fallback to an unkown type the json response can still be deserialized and the interface change can be handled gracefully:

enum MenuItemType {
 foo,
anotherFoo
}
StarProxima commented 10 months ago

I'm thinking about such a solution, in idea it would catch all unknown values and set $unknown as default.

@JsonEnum()
enum Groups {
  @JsonValue(1)
  value1,
  @JsonValue(2)
  value2,

  /// Default value for all unparsed values, allows backward compatibility when adding new values on the backend
  @JsonValue(null)
  $unknown;
}
savage7 commented 10 months ago

@StarProxima Have you tried if json_serializable decodes this correctly?

I tried:

@JsonKey(
   name: 'type',
   unknownEnumValue: null,
)
 final MenuItemType type;

json_serializable threats unknownEnumValue: null as an error. There's an open PR for that https://github.com/google/json_serializable.dart/pull/1326

StarProxima commented 10 months ago

Yes, in its purest form, it unfortunately doesn't work.

We can define fromJson in enum so that we don't have to search for all occurrences of this enum and set unknownEnumValue.

The json_serializable would use fromJson instead of generating its own maps and using $enumDecode.

This approach seems promising to me, just need to check if using fromJson Retrofit will work. @savage7 Do you have a way to test this?

@JsonEnum()
enum Groups {
  @JsonValue(1)
  value1(1),
  @JsonValue(2)
  value2(2),

  /// Default value for all unparsed values, allows backward compatibility when adding new values on the backend
  $unknown(null);

  const Groups(this.json);

  final int? json;

  factory Groups.fromJson(String? json) => values.firstWhere(
        (e) => e.json == json,
        orElse: () => $unknown,
      );
}

Here's what json_serializable generates for a class that has an after with a list of enums:

Before (for 3 will throw an error):

      // ...
      groups: (json['groups'] as List<dynamic>?)
              ?.map((e) => $enumDecode(_$GroupsEnumMap, e))
              .toList() ??
          const [],
      // ...

After (for 3 it will give $unknown):

     // ...
     groups: (json['groups'] as List<dynamic>?)
              ?.map((e) => Groups.fromJson(e as String?))
              .toList() ??
          const [],
      // ...
StarProxima commented 10 months ago

It works with retrofit

Before:

    final value = ApiProviders.values.firstWhere(
      (e) => e.name == _result.data,
      orElse: () => throw ArgumentError(
        'ApiProviders does not contain value ${_result.data}',
      ),
    );

After (with fromJson):

final value = ApiProviders.fromJson(_result.data!);

@Carapacik What do you think about this approach, any other ideas?

StarProxima commented 10 months ago

If we apply this approach for toJson as well, we basically don't need annotations for enums from json_serializable at all, everything is also generated correctly without them.

enum Groups {
  value1(1),
  value2(2),

  /// Default value for all unparsed values, allows backward compatibility when adding new values on the backend
  $unknown(null);

  const Groups(this.json);

  final int? json;

  factory Groups.fromJson(String? json) => values.firstWhere(
        (e) => e.json == json,
        orElse: () => $unknown,
      );

  int? toJson() => json;
}