google / json_serializable.dart

Generates utilities to aid in serializing to/from JSON.
https://pub.dev/packages/json_serializable
BSD 3-Clause "New" or "Revised" License
1.55k stars 397 forks source link

enum as Map keys causing key type is int #1362

Open SpeedReach opened 11 months ago

SpeedReach commented 11 months ago

Problem

@JsonSerializable()
class MyClass {
  final Map<Slot, String> inv;

  MyClass({required this.inv});

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

  Map<String, dynamic> toJson() => _$MyClassToJson(this);
}

enum Slot {
  @JsonValue(1)
  slot1,
  @JsonValue(2)
  slot2,
  @JsonValue('3')
  slot3,
}

generated enum map

const _$SlotEnumMap = {
  Slot.slot1: 1,
  Slot.slot2: 2,
  Slot.slot3: '3',
};

Generated toJson:

instance.inv.map((k, e) => MapEntry(_$SlotEnumMap[k]!, e))

Since enums can be Map keys, and enums can be tranfered to int with JsonValue. This produces a out come that the json key is a int.

Generated fromJson:

(json['inv'] as Map<String, dynamic>).map(
        (k, e) => MapEntry($enumDecode(_$SlotEnumMap, k), e as String),
      )

I don't think this should happen, since json's key should always be a String.

A solution would be:

Generated toJson:

instance.inv.map((k, e) => MapEntry(_$SlotEnumMap[k]!.toString(), e)) //added toString()

Generated fromJson:

(json['inv'] as Map<String, dynamic>).map(
        (k, e) => MapEntry($enumDecodeJsonKey(_$SlotEnumMap, k), e as String), //uses a different decode function.
      )

enumDecodeJsonKey would be a helper specific for enum map keys, would have something like.

  for (var entry in enumValues.entries) {
    if (entry.value == source || entry.value.toString() == entry.value) {
      return entry.key;
    }
  }

If this looks nice , I'd be happy to work on this and submit a pr!

kevmoo commented 11 months ago

I think I just have a bug in the @JsonValue case. If you remove the @JsonValue(1) annotation, how does the map change?

weeb-destroyer commented 11 months ago

I think I just have a bug in the @JsonValue case. If you remove the @JsonValue(1) annotation, how does the map change?

The generated enum map will use their values name as keys

const _$SlotEnumMap = {
  Slot.slot1: 'slot1',
  Slot.slot2: 'slot2',
  Slot.slot3: 'slot3',
};
SpeedReach commented 11 months ago

@kevmoo yes, it works fine in other cases. It only happens when enums are used as json keys and is annotated with int values. It's an edgy case that might need specific handling. The solution I proposed will only transfer to the code when atleast one enum is annotated with int.

kevmoo commented 11 months ago

I think the code is correct, honestly!

The input value can be either a String '3' or an int 1 or 2 and it's converted correctly.

SpeedReach commented 11 months ago

It converts to a int, shouldn't json's keys always be strings?

SpeedReach commented 11 months ago

It causes wierd errors like this.

  final object = MyClass(inv: {Slot.slot1: "ww", Slot.slot3: "ww"});
  final json = object.toJson();
  final deserialized = MyClass.fromJson(json);

throws type '_Map<Object, String>' is not a subtype of type 'Map<String, dynamic>' in type cast in the fromJson method.

Even the jsonEncode in standard lib doesn't work.

  final object = MyClass(inv: {Slot.slot1: "ww", Slot.slot3: "ww"});
  final json = object.toJson();
  final deserialized = jsonEncode(json);

throws Converting object to an encodable object failed: _Map len:2

kevmoo commented 11 months ago

Hrm...let me look again...

kevmoo commented 11 months ago

Yeah. This is crazy complex.

Effectively, (now) you CANNOT use an enum as a Map key AND set a @JsonValue to anything other than a String.

We'd need to create some funky custom version of _$SlotEnumMap that's SPECIFIC to an enum used as a Map key where the values are pre-converted to the corresponding String values.

We CAN'T just to toString because some values are encoded with something other than their toString – like DateTime.

kevmoo commented 11 months ago

We could/should throw an error here and say "hey, you can't use an enum here" to be more clear this is not supported.

SpeedReach commented 11 months ago

I read the doc and swa that only int and String are the supported enum values. This should make it less complex?

kevmoo commented 11 months ago

I read the doc and swa that only int and String are the supported enum values. This should make it less complex?

Not really. I could just throw in a toString but that's...cheating. And would make things more complex later.

SpeedReach commented 11 months ago

https://github.com/google/json_serializable.dart/blob/e2c8badaee4e36893e13c4073375a8ea035f2403/json_serializable/README.md?plain=1#L112-L113 The doc referenced above

I could open a pr for throwing the error, when should we throw it?

kevmoo commented 11 months ago

@SpeedReach not outdated, but it's not specific enough to handle the case where @JsonValue is used.

You could try for a PR, but it's CRAZY complex to do it cleanly. I wrote most of this package. To do the plumbing correctly will be quite tough. I appreciate the enthusiasm, but it'd be a LOT of work.