Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
larssn opened this issue Jun 14, 2022 · 9 comments
Open

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

larssn opened this issue Jun 14, 2022 · 9 comments
Assignees
Labels

Comments

@larssn
Copy link

larssn commented Jun 14, 2022

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 davidmorgan self-assigned this Jun 14, 2022
@davidmorgan
Copy link
Contributor

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
Copy link
Author

larssn commented Jun 14, 2022

FYI, there is also BuiltSetMultimap.

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

@davidmorgan
Copy link
Contributor

Yes, for most purposes it beats the plain Map.

@larssn
Copy link
Author

larssn commented Jun 14, 2022

Thanks, BuiltSetMultimap was the way to go! ❤️

@larssn larssn closed this as completed Jun 14, 2022
@larssn
Copy link
Author

larssn commented Jun 14, 2022

@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
Copy link
Contributor

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
Copy link
Author

larssn commented Jun 20, 2022

@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 davidmorgan reopened this Jun 20, 2022
@davidmorgan
Copy link
Contributor

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
Copy link
Author

larssn commented Jun 21, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants