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

unknownEnumValue support for json_serializable #106

Closed
savage7 opened this issue Oct 13, 2023 · 7 comments · Fixed by #126
Closed

unknownEnumValue support for json_serializable #106

savage7 opened this issue Oct 13, 2023 · 7 comments · Fixed by #126
Assignees
Labels
enhancement New feature or request question Further information is requested

Comments

@savage7
Copy link

savage7 commented Oct 13, 2023

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;
@Carapacik Carapacik added the enhancement New feature or request label Oct 13, 2023
@Carapacik Carapacik self-assigned this Oct 13, 2023
@Sancene Sancene assigned Sancene and Carapacik and unassigned Carapacik Oct 13, 2023
@Sancene
Copy link
Collaborator

Sancene commented Oct 13, 2023

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.

@Sancene Sancene added question Further information is requested and removed enhancement New feature or request labels Oct 13, 2023
@savage7
Copy link
Author

savage7 commented Oct 13, 2023

@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
Copy link
Collaborator

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
Copy link
Author

savage7 commented Oct 14, 2023

@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 google/json_serializable.dart#1326

@StarProxima
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

StarProxima commented Oct 14, 2023

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;
}

@Carapacik Carapacik added the enhancement New feature or request label Oct 19, 2023
@Carapacik Carapacik linked a pull request Oct 26, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants