JsonKey's unknownEnumValue allows null fallback, which will crash at runtime

[tp]

We just hit an issue at runtime, where a JSON field defined with unknownEnumValue: null, would crash during parsing at runtime, due to this check in the generated code:

  if (value == null && unknownValue == null) {
    throw ArgumentError('`$source` is not one of the supported values: '
        '${enumValues.values.join(', ')}');
  }

I think if we know for sure that an unknown value of null is not allowed, we shouldn't allow that to be declared on the JsonKey.

On the other hand, to me it seems perfectly fine to have null values for unknowns (if the field is properly document and in the future marked as nullable).

So to fix that, I would propose to use a sentinel value to denote "no unknownEnumValue is specified" and only err when there really is no value – else return null or whatever else was defined as fallback.

Any thoughts on this? If desired I could implement this as a PR.

 flutter --version
Flutter 1.10.15-pre.307 • channel unknown • unknown source
Framework • revision 2cedd559bb (8 days ago) • 2019-10-29 00:43:06 -0400
Engine • revision 419f5d594a
Tools • Dart 2.6.0 (build 2.6.0-dev.8.2 e1fce75301)

[kevmoo]

I'll take a look here...

[kevmoo]

@tp – would you copy-paste the generated code that calls _$enumDecodeNullable – trying to understand exactly what you're hitting

[tp]

Sure @kevmoo, here's the setup:

So we use this on a field like

@JsonSerializable(
  checked: true,
  nullable: false,
  createToJson: false,
)
class Order {
  Order({
    @required this.status,
  });

  static Order fromJson(
    dynamic json,
  ) =>
      _$OrderFromJson(json);

  @JsonKey(
    required: true,
    disallowNullValue: true, // We don't expect `null` from the API
    unknownEnumValue: null, // But if we get an unexpected value, we fall back to `null`
  )
  final OrderStatus status;
}

Which generates this code:

T _$enumDecode<T>(
  Map<T, dynamic> enumValues,
  dynamic source, {
  T unknownValue,
}) {
  if (source == null) {
    throw ArgumentError('A value must be provided. Supported values: '
        '${enumValues.values.join(', ')}');
  }

  final value = enumValues.entries
      .singleWhere((e) => e.value == source, orElse: () => null)
      ?.key;

  if (value == null && unknownValue == null) {
    throw ArgumentError('`$source` is not one of the supported values: '
        '${enumValues.values.join(', ')}');
  }
  return value ?? unknownValue;
}

const _$OrderStatusEnumMap = {
  OrderStatus.confirmed: 'confirmed',
 …
};

Order _$OrderFromJson(Map<String, dynamic> json) {
  return $checkedNew('Order', json, () {
    $checkKeys(json,
        requiredKeys: const ['status'], disallowNullValues: const ['status']);
    final val = Order(
      status: $checkedConvert(
          json, 'status', (v) => _$enumDecode(_$OrderStatusEnumMap, v)),
    );
    return val;
  });
}

If instead I set unknownEnumValue: OrderStatus.confirmed, in the attribute, this FromJson code gets generated:

…


Order _$OrderFromJson(Map<String, dynamic> json) {
  return $checkedNew('Order', json, () {
    $checkKeys(json,
        requiredKeys: const ['status'], disallowNullValues: const ['status']);
    final val = Order(
      status: $checkedConvert(
          json,
          'status',
          (v) => _$enumDecode(_$OrderStatusEnumMap, v,
              unknownValue: OrderStatus.confirmed)),
    );
    return val;
  });
}

So as you can see using unknownEnumValue in the attribute doesn't lead to the unknownValue parameter being passed to _$enumDecode.

[kevmoo]

Ah! The problem here is that we can't tell the difference between you providing null and you not providing a value at all.

We'd need to provide some sentinel value – JsonKey.nullUnknown or similar to make this work.

[stevsimo]

Is there an update for this issue?

[Nico04]

It would be very handy to be able to use it like this :
#778 (comment)

[jeiea]

unknownToNull may be more clear. Or we may add unknownValueTo having a wrapping type for disallowing ambiguity and deprecate unknownEnumValue.

[jeiea]

I wish this issue has more attention. Why do I need Enum.unknown to every enum? Even my boss said just use string.

unknownToNull: true
Pros: Simple. No additional types are required. No breaking change.
Cons: Ambiguity like @JsonKey(unknownToNull: true, unknownEnumValue: Enum.something)

unknownValueTo: Some(null)
unknownValueTo: Some(Enum.what)
Pros: It replaces unknownEnumValue, so there's no ambiguous usage.
Cons: It needs deprecation, requires new data type. Not sure about complexity.

Personally I prefer the unknownValueTo way.

[jeiea]

@kevmoo It is labeled as help wanted. How can I help this?

[via-guy]

I created my own solution for this in #839. If nullable == true and unknownEnumValue == null then I handle unknown values gracefully. I can't think of any real-world situation where an app would want the entire decoding of their data to be broken because a new enum value was introduced in the backend code.

I had to create a crazy hacky solution for Swift because it was also throwing an error for an unknown value and it drove me crazy. Please let's not make the same mistake here...

[rmahmadkhan]

If the enum is not nullable in the class, it should not give such error. Any lead on this issue?

[kevmoo]

I'm starting on this now!

[kevmoo]

Landed in https://pub.dev/packages/json_serializable/versions/6.0.0