google / built_collection.dart

Immutable Dart collections via the builder pattern.
https://pub.dev/packages/built_collection
BSD 3-Clause "New" or "Revised" License
275 stars 53 forks source link

Shouldn't BuiltMap<String, BuiltSet<int>> work out of the box? #263

Open larssn opened 2 years ago

larssn commented 2 years ago

So this throws a DeserializationError.

@nullable
BuiltMap<String, BuiltSet<int>> get entries;

Using this raw data: [wv8P7YM21ryHwcvtCoGR, [688, 689, 690, 691, 692, 693, 694, 695, 696, 697]]

Thought it was supported. Also tried making a custom builder factory for it, like the error message suggests - same error.

Any hints?

EDIT: This is auto-generated in serializers.g.dart:

..addBuilderFactory(
          const FullType(BuiltMap, const [
            const FullType(String),
            const FullType(BuiltSet, const [const FullType(int)])
          ]),
          () => new MapBuilder<String, BuiltSet<int>>())

Looks correct? Doesn't work

The exception message:

══╡ EXCEPTION CAUGHT BY FLUTTER FRAMEWORK ╞═════════════════════════════════════════════════════════
I/flutter (17084): The following DeserializationError was thrown:
I/flutter (17084): 'Turnover' failed due to: Deserializing '[wv8P7YM21ryHwcvtCoGR, [688, 689, 690, 691, 692, 693, 694,
I/flutter (17084): 695, 696, 697]]' to 'BuiltMap<String, BuiltSet<int>>' failed due to: Deserializing '[688, 689, 690,
I/flutter (17084): 691, 692, 693, 694, 695, 696, 697]' to 'BuiltSet<int>' failed due to: Bad state: No builder factory
I/flutter (17084): for BuiltSet<int>. Fix by adding one, see SerializersBuilder.addBuilderFactory.
davidmorgan commented 2 years ago

It has generated the builder factory for BuiltMap<String, BuiltSet<int>>, but it also needs one for BuiltSet<int>, and this is the one you'll need to add by hand.

FYI, there is also BuiltSetMultimap.

larssn commented 2 years ago

FYI, there is also BuiltSetMultimap.

Ah cool! Seems like an implicit Map<T, Set>? Lemme try that out first.

davidmorgan commented 2 years ago

Yes, for most purposes it beats the plain Map.

larssn commented 2 years ago

Thanks, BuiltSetMultimap was the way to go! ❤️

larssn commented 2 years ago

@davidmorgan Any way around this?

Invalid argument(s): Standard JSON cannot serialize type BuiltSetMultimap<dynamic, dynamic>

The data is:

@nullable
  BuiltSetMultimap<String, int> get economicEntries;

I don't see why that wouldn't serialize.

EDIT: I assume it's not supported: https://github.com/google/built_value.dart/blob/master/built_value/test/standard_json_plugin_test.dart#L51

davidmorgan commented 2 years ago

Looks like it's not supported; to be honest I don't remember why; if you like you could try changing the plugin

third_party/dart/built_value/lib/standard_json_plugin.dart

larssn commented 2 years ago

@davidmorgan So it seems that it is actually supported, which can be demonstrated by removing both BuiltSetMultimap, and BuiltListMultimap from _unsupportedTypes in StandardJsonPlugin.

Maybe it's just an oversight that these weren't removed.

I've tested this using the below model and test, but it would be nice if you could just double check my findings! 🙂

The model:

library model;

import 'package:built_collection/built_collection.dart';
import 'package:built_value/built_value.dart';
import 'package:built_value/serializer.dart';

import 'serializers.dart';

part 'model.g.dart';

abstract class Model implements Built<Model, ModelBuilder> {

  BuiltSetMultimap<String, int>? get builtSet;
  BuiltListMultimap<String, int>? get builtList;

  Model._();
  factory Model([Function(ModelBuilder b) updates]) = _$Model;
  factory Model.from(Map<String, dynamic> map) => serializers.deserializeWith(serializer, map) as Model;
  Map<String, dynamic> /*!*/ toMap() => serializers.serializeWith(serializer, this) as Map<String, dynamic>;

  @BuiltValueSerializer(serializeNulls: true)
  static Serializer<Model> get serializer => _$modelSerializer;
}

The test:

import 'package:built_collection/built_collection.dart';
import 'package:dummy/model.dart';
import 'package:flutter_test/flutter_test.dart';

const setData = [1, 2, 3, 3, 3];
const listData = [1, 1, 1];

Model _createModel() {
  return Model((b) => b
    ..builtSet = SetMultimapBuilder(<String, List<int>>{'hello': setData})
    ..builtList = ListMultimapBuilder(<String, List<int>>{'hello': listData}));
}

Map<String, dynamic> _createMap() {
  return <String, dynamic>{
    'builtSet': {
      'hello': setData,
    },
    'builtList': {
      'hello': listData,
    }
  };
}

void main() {
  test('BuiltSetMultimap should serialize', () {
    final instance = _createModel();

    expect(instance.toMap()['builtSet']['hello'].length, 3);
  });

  test('BuiltListMultimap should serialize', () {
    final instance = _createModel();

    expect(instance.toMap()['builtList']['hello'].length, 3);
  });

  test('Model should deserialize', () {
    expect(Model.from(_createMap()), _createModel());
  });
}

00:07 +3: All tests passed!

davidmorgan commented 2 years ago

Hmmmm it might be relate to needing additional / more complex support if there is a discriminator, meaning that the type is written in the json. For example this would happen if there is a BuiltSetMultimap inside a BuiltList<Object>.

One way to land this might be to narrow the restriction to only throw if there is a discriminator, i.e. to allow serialization/deserialization when the type is fully specified and to throw if it's not.

You could see this in tests by adding that BuiltList<Object> then adding BuiltSetMultimap to it :)

larssn commented 2 years ago

One way to land this might be to narrow the restriction to only throw if there is a discriminator, i.e. to allow serialization/deserialization when the type is fully specified and to throw if it's not.

Sounds like a pretty good compromise for now. How to best proceed? It's not really urgent for us, as we have our workaround, but would obvously be nice to have something merged for official support.