k-paxian / dart-json-mapper

Serialize / Deserialize Dart Objects to / from JSON
https://pub.dev/packages/dart_json_mapper
Other
399 stars 33 forks source link

Deserializing with String discriminatorProperty throws JsonFormatError(FormatException) #195

Closed izN8nu6RyeneG5XnBoBgyRMVGH6H43WF closed 1 year ago

izN8nu6RyeneG5XnBoBgyRMVGH6H43WF commented 1 year ago

Version: 2.2.5+3

I'm using a getter to runtimeType as a subclass discriminator, with the default discriminator subclass values, the class name. The value appears to be correctly serialized. However, when deserializing, an uncaught JsonFormatError is thrown.

Example:

import 'package:dart_json_mapper/dart_json_mapper.dart';

import 'main.mapper.g.dart' show initializeJsonMapper;

@JsonSerializable()
@Json(discriminatorProperty: 'type')
abstract class Parent {
    @JsonProperty(ignore: true)
    Type get type => runtimeType;

    var a = 1;
}

@JsonSerializable()
class Child extends Parent {
    var b = "test";
}

void main(List<String> arguments) {
    initializeJsonMapper();

    var obj = Child();
    var firstJson = JsonMapper.serialize(obj);
    // error gets thrown below
    var secondJson = JsonMapper.serialize(JsonMapper.deserialize<Parent>(firstJson));
}

Analysis:

The error is thrown in mapper.dart in _deserializeObject:

https://github.com/k-paxian/dart-json-mapper/blob/e954ddcff8ca675271cf518c107a2340949ab830/mapper/lib/src/mapper.dart#L892-L898

The logic (probably correctly) always attempts to decode jsonValue as JSON if it is a String, which makes sense, since we're supposed to be passing JSON. However, the calling function, _detectObjectType, is passing in the String value, not the JSON representation of it:

https://github.com/k-paxian/dart-json-mapper/blob/e954ddcff8ca675271cf518c107a2340949ab830/mapper/lib/src/mapper.dart#L327-L342

Maybe calling function valuates Strings one too many times, i.e. at line 331?

k-paxian commented 1 year ago

Finally, new real issue. Thank you for sharing it I'll take a look

k-paxian commented 1 year ago

Thank you so much for a good case, which is now covered. 🎉

izN8nu6RyeneG5XnBoBgyRMVGH6H43WF commented 1 year ago

@k-paxian The linked fix looks like it should work in the usual use case. However, since it's really just looking for valid JSON, it may still encounter issues for String discriminatorProperty fields representing something other than a Type, e.g. suppose we want to discriminate based on an arbitrary String. If that String just so happens to be JSON, it may not behave as the developer expects.

Most people probably won't use it that way, but it might be worth documenting.

k-paxian commented 1 year ago

@izN8nu6RyeneG5XnBoBgyRMVGH6H43WF No worries, String based discriminators are working as expected. Please take a look at 6d8da09ea7f034483fb00996d5a88fdeb7ee52a6 test. Thank you for your details oriented attitude and useful hints so far 👍