schultek / dart_mappable

Improved json serialization and data classes with full support for generics, inheritance, customization and more.
https://pub.dev/packages/dart_mappable
MIT License
165 stars 23 forks source link

EmptyToNullHook doesn't solve certain issues. Need better empty string handling approach. #229

Open RohanSenguptaMantisPro opened 2 months ago

RohanSenguptaMantisPro commented 2 months ago

In a class like this if hook was not used, and json response returns emailVerified as an empty string, dart_mappable gives conversion error.

@MappableClass()
class UserDetails with UserDetailsMappable {
  const UserDetails({
    this.id,
    this.emailVerified,
  });

  @MappableField(key: '_id')
  final String? id;

  @MappableField(hook: NullableHook())
  final bool? emailVerified;

So to solve this we have to use hook

class NullableHook extends MappingHook {
  const NullableHook();

  @override
  Object? beforeDecode(Object? value) {
    return (value is String && value.isEmpty) ? null : value;
  }
}

But then for all the non-String fields we would have to use the hook with @MappableField(hook: NullableHook()) . we have to repeat that for all the properties.

Rather, it would have been better if it was done the way .tryParse() handles the conversion :

void main() {

  DateTime? nullableInt = DateTime.tryParse('');
  print(nullableInt);
}

If error in conversion then it will return null , if variable is nullable.

My Suggestion:

  1. dart_mappable should also handle conversion like .tryParse() so that Custom Hooks isn't required for this empty string conversion case at least,
  2. or, provide a better approach in hooks where we don't have to repeat applying the hooks for every non string property.
  3. also the inbuilt EmptyToNullHook produce errors while trying to achieve the same that I did with the NullableHook due to some reason ( also mentioned in the issue : #217 ) .
schultek commented 2 months ago

Not sure I understand the issue.

Do you want any field to be null if the deserialization of it throws?

RohanSenguptaMantisPro commented 2 months ago

yes. If an empty string is received it should be converted to null when deserializing directly, rather than throwing conversion error ( trying to convert an empty string to Datetime, int etc ) and if we want any customization we can use hooks according to our wants.

schultek commented 2 months ago

The current (and intended) behavior is:

The raw value must be in a format that is valid for deserialization, else an exception is throw.

As a default behavior I don't want to change this (I think for 99% of users this is the correct approach). It would also be breaking. But I'm open for an opt-in solution, like a custom hook or an annotation property.

RohanSenguptaMantisPro commented 2 months ago

Then I would say this CustomHook works pretty well.

class NullableHook extends MappingHook {
  const NullableHook();

  @override
  Object? beforeDecode(Object? value) {
    return (value is String && value.isEmpty) ? null : value;
  }
}

Only thing that has to be kept in mind is to add this hook to the fields wherever the empty string to null conversion functionality is desired.

thangmoxielabs commented 2 months ago

We need this behavior as well. If the field is nullable, when deserialization isn't valid it should return null. And of course if it's declared non-nullable then it should throw an exception.

A global flag, or maybe better a global custom hook would be nice