google / built_value.dart

Immutable value types, enum classes, and serialization.
https://pub.dev/packages/built_value
BSD 3-Clause "New" or "Revised" License
861 stars 184 forks source link

Support "pretty" JSON output #65

Closed davidmorgan closed 7 years ago

davidmorgan commented 7 years ago

A few people have asked about supporting "standard" JSON, meaning some kind of key->value maps with fieldnames.

It's not yet clearly exactly what would make sense here, because that isn't sufficient to serialize data models that are using interfaces rather than concrete types. Please expand on requirements here if this is what you'd like.

vadimtsushko commented 7 years ago

I would be very interested in such solution. In my case I have to implement client for some protocol (based on json-rpc 2.0), specifically Qlik Engine API. Methods of that protocol use deeply nested Map structure's, encoded in JSON as parameters. They looks pretty common for JavaScript code too. See for example: http://help.qlik.com/en-US/sense-developer/3.1/Subsystems/EngineAPI/Content/GenericObject/PropertyLevel/HyperCubeDef.htm

Such objects are obviously pain to work with, so I initially wrapped them with dart classes (and that was very boring and error prone), and afterwards use dogma and dogma_json_schema code generators to generate dart classes for such structures. That works well, but I would be glad to switch to built_value as with it is very pleasant to work with deeply nested structures (Code looks much better without nested constructors) and for many other reasons. Especially attractive for me is potential to mix manual and generated code as for validation, derived getters and so on. And Builder pattern itself (though not very intuitive at first) is very powerfull and pretty on the client side...

So my use case is very similar to that Anders quoted in https://github.com/google/built_json.dart/issues/7 Probably in both cases strict subset of overall built_value functionality - with added requirement that all fields and collection elements should be of concrete classes (not abstract) would do.

Actually I believe that in use cases when we do not control network protocol on both sides but trying to use built_value with existing API such restriction is frequently imposed upon real APIs.

On the other hand I can see, that built_value current serialization format is better in terms of performance and with it support of polymorphic collections and fields of abstract types - for use cases where we can fully control both sides of communication.

In my view best solution would be to keep current serialization format in a base default implementation and add such (key: value) serialization format as a first serialization plugin or extension.

Package documentation mention pluggability:

It's pluggable. Arbitrary extensions can be added to give custom JSON serialization for your own types.

But it is not obvious how such extensions could be added. Many classes, repositories, serializers, code-generators and so on are intermingled very intimately in built_value, what should be extended or overwritten?

If we have serialization plugin for key: value serialization of not-polymorphic, not-abstract models, that coud be used out of the box in many if not majority of real case scenarios of interaction with existing API.

But what is not less important we would have reference implementation of serialization plugin so we could more less easily add other plugins for specific use cases of interfaces with concrete API with polymorpich/abstract models on case by case base.

davidmorgan commented 7 years ago

Thanks for the detailed explanation. Yes, I think this makes sense to support.

The current "pluggable" code isn't really good for this, though: you can add your own serializers for your own (non-built-value) types, but what we want here is a different way of serializing all types. You'd essentially be throwing away all the built_value code and adding your own.

I'm considering the best way to go here. Options:

I'm going to add support for generic types first, since it's a necessary feature and may affect serialization. Then I'll take a look at this issue.

vadimtsushko commented 7 years ago

Great. I'll definitely be waiting for this feature with interest.

Considering available options: If I understand you right, third variant suppose conversion from/to map-based structures in runtime? I tried that approach briefly and to me it looks that in runtime there would not be enough metadata information for this transformation. So for me your second option looks as most promising one.

On the other hand I guess only implementation of such plugin will make obvious what compromises and constraints would be necessary for that.

davidmorgan commented 7 years ago

@vadimtsushko I had a go at this today:

89

As far as I can see it works, but I'm lacking a real use case to try it out on. Are you able to patch + try it please?

No rush :) ... not expecting to commit this before the new year.

vadimtsushko commented 7 years ago

Great. I've tried it on sample data, so far it looks like it works perfectly for my use case. Apparently I not even have to specify type on every serialize operation, if I add onliner like standardSerialize

    standardSerialize(Object data) => serializersWithPlugin.serialize(data, specifiedType: new FullType(data.runtimeType));
    print(standardSerialize(data));

Pluggabiliry is great. I immediately have scenario for a second plugin: Serialization to and from JsObject (with dart_utils package). For example Qlik Sense have not only JSON-RPC API but some JavaScript API too. They share big part of structures that these API's use as function parameters and return values - so I could just use two different serialization plugins for common models for these two API's. And having StandardJsonPlugin as a reference I'm relatively confident that I can make such plugin myself if/when it would be necessary.

davidmorgan commented 7 years ago

Great, thanks! As I said I'll aim to land this in January.

One detail I didn't get into yet: how should I handle special double values -- NaN, +inf, -inf? By default JSON doesn't support these. For built_value's encoding I was planning on adding custom string values for them. Maybe there's some standard I can use?

Other questions: does it work for classes containing arrays, maps?

Thanks!

vadimtsushko commented 7 years ago

I'm not actually switched to built_value on my projects yet. In Sense project I currently have 54 models (described in 3K LoC of JSON-Schema). So currently I think about best conversion path. Is it worth to make some pre-generator JSON-Schema to buit_value, and so on.

About double values - I do not know about standards, but in the same Qlik Sense JSON-RPC API I do get "NaN" string as a value of otherwise double typed fields when numeric value is unavailable.

I think I'll try to convert couple models with maps and arrays manually and check them against QS API. I did have some problems with int arrays in models before - but only with JsObject and Js-interop. Not with JSON encoded strings.

vadimtsushko commented 7 years ago

Hi, David. I've successfully converted all my Sense models to built_value and for me all works just fine. List fields definitely work for me, with map fields I can not say definitely - I have no models with map fields.

My code looks much better for my eyes with built_value bilders - see for new example

    var props = new GenericObjectProperties((b) => b
      ..qExtendsId = ''
      ..qInfo.qId = 'Report1'
      ..qInfo.qType = 'mashup'
      ..qHyperCubeDef.qDimensions.add(new NxDimension((b) => b
        ..qLibraryId = ''
        ..qDef.qFieldDefs.add('Склад')))
      ..qHyperCubeDef
          .qMeasures
          .add(new NxMeasure((b) => b..qDef.qDef = 'Sum(Сумма)')));
    var cubeHandle = await app.createSessionObject(props);

and old one

    var cube = new QHyperCubeDef()
      ..qDimensions = [
        new NxDimension()
          ..qDef = (new NxInlineDimensionDef()..qFieldDefs = ['Склад'])
      ]
      ..qMeasures = [
        new NxMeasure()..qDef = (new NxInlineMeasureDef()..qDef = 'Sum(Сумма)')
      ];

    var prop = new GenericObjectProperties()
      ..qExtendsId = ''
      ..qInfo = (new NxInfo()
        ..qId = 'Report1'
        ..qType = 'mashup')
      ..qHyperCubeDef = cube;

    var cubeHandle = await app.createSessionObject(prop);

In new code I got rid of three constructors :) That's a bonus.

Serialized format works with Qlik Sense JSON-RPC protocol just well.

Thank you very much. For now I would use patched local version of built_value , will switch on pub version as soon as that PR will be applied.

davidmorgan commented 7 years ago

Great! Thanks for confirming. And thanks for asking for the change in the first place -- it's a lot easier to make improvements if someone needs them :)

Andersmholmgren commented 7 years ago

Apologies for the late comment. To handle polymorphism it probably makes sense to target the same JSON schema format as swagger does http://swagger.io/specification/#schemaObject

It's both a subset and a superset of JSON Schema ;-)

For polymorphism it introduces a discriminator property.

Regardless, the output lgtm

vadimtsushko commented 7 years ago

Hi, David. Happy Christmas

I've stumbled upon a bug in such edge case as a BuiltList of BuiltList of some BuiltValue.

For example if I add a matrix field in your example CompoundValue class as

abstract class CompoundValue
    implements Built<CompoundValue, CompoundValueBuilder> {
  static final Serializer<CompoundValue> serializer = _$compoundValueSerializer;

  SimpleValue get simpleValue;
  @nullable
  ValidatedValue get validatedValue;

  BuiltList<String> get stringList;

  BuiltList<BuiltList<SimpleValue>> get matrix;

  factory CompoundValue([updates(CompoundValueBuilder b)]) = _$CompoundValue;
  CompoundValue._();
}

and try to serialize some value:


final _standardJsonSerializer =
    (serializers.toBuilder()..addPlugin(new StandardJsonPlugin())).build();

standardJsonEncode(Object data) => _standardJsonSerializer.serialize(data,
    specifiedType: new FullType(data.runtimeType));

standardJsonDecode(Type dataType, Object data) => _standardJsonSerializer
    .deserialize(data, specifiedType: new FullType(dataType));

void main() {
  final data = new CompoundValue((b) => b
    ..simpleValue.anInt = 1
    ..simpleValue.aString = 'two'
    ..stringList.add('365')
    ..matrix.add(new BuiltList<SimpleValue>([
      new SimpleValue((b) => b
        ..anInt = 2
        ..aString = 'sadfsdfsdfsdfsd')
    ]))
    ..validatedValue = new ValidatedValue((b) => b.anInt = 3).toBuilder());

  var jsonData = standardJsonEncode(data);
...
}

then seririalization breaks with error

Bad state: No builder for BuiltList<SimpleValue>, cannot serialize.

Looking into serializers.g.dart I see that indeed there is serilizer for outer (two dimensional) BuildList but not for inner (one dimensional) BuiltList of SimpleValue

dave26199 commented 7 years ago

Thanks! I'll be sure to add test coverage for that.

Not sure if it's related to this specific change or a general issue.

It should be possible to work around it by adding the builder factory to Serializers yourself with something along the lines of (can't easily check right now):

var realSerializers = (serializers.toBuilder()
    ..addBuilderFactory(
        const FullType(
            BuiltList,
            const [
                const FullType(BuiltList, const [SimpleValue])]),
        () => new ListBuilder<BuiltList<SimpleValue>>())).build()

Merry Christmas :)

vadimtsushko commented 7 years ago

Thank you, I did precisely that for a time being, along with unstructured NumSerializer with simple support for NaN String input values

davidmorgan commented 7 years ago

Now back from vacation -- will look at landing this, hopefully this week / next.

davidmorgan commented 7 years ago

Filed https://github.com/google/built_value.dart/issues/102 for followup on swagger-based polymorphism.