search

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

Cannot rebuild a BuiltSet entry in BuiltMap #249

Closed lukepighetti closed 3 years ago

lukepighetti commented 3 years ago

It appears that if you try to rebuild a BuiltSet which is an entry of BuiltMap, it does not rebuild as expected. The changes are not applied.

main.dart

import 'package:built_collection/built_collection.dart';
import 'package:built_value_map_entry_rebuilder/models.dart';
import 'package:flutter_test/flutter_test.dart';

void main() {
  test('BuiltMap entry update', () {
    final joe = Person((b) => b..name = 'Joe');
    final luke = Person((b) => b..name = 'Luke');

    final org = Organization((b) => b
      ..name = 'Foolandia'
      ..people = MapBuilder({
        'mobile': BuiltSet<Person>({joe, luke}),
      }));

    final orgBuilder = org.toBuilder();

    /// Initial works
    expect(orgBuilder.people['mobile'], equals(BuiltSet<Person>({joe, luke})));

    /// Weird rebuild works
    orgBuilder.people['mobile'] = (orgBuilder.people['mobile']!.toBuilder()
          ..removeWhere((e) => e.name == 'Joe'))
        .build();
    expect(orgBuilder.people['mobile'], equals(BuiltSet<Person>({luke})));

    /// Rebuild fails, expected to work
    orgBuilder.people['mobile']!
        .rebuild((b) => b..removeWhere((e) => e.name == 'Luke'));

    expect(orgBuilder.people['mobile'], equals(BuiltSet<Person>({})));
  });
}

models.dart

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

part 'models.g.dart';

abstract class Person implements Built<Person, PersonBuilder> {
  static Serializer<Person> get serializer => _$personSerializer;

  Person._();
  factory Person([void Function(PersonBuilder) updates]) = _$Person;

  String get name;
}

abstract class Organization
    implements Built<Organization, OrganizationBuilder> {
  static Serializer<Organization> get serializer => _$organizationSerializer;

  Organization._();
  factory Organization([void Function(OrganizationBuilder) updates]) =
      _$Organization;

  String get name;

  BuiltMap<String, BuiltSet<Person>> get people;
}

built_value_test.dart

import 'package:built_collection/built_collection.dart';
import 'package:built_value_map_entry_rebuilder/models.dart';
import 'package:flutter_test/flutter_test.dart';

void main() {
  test('BuiltMap entry update', () {
    final joe = Person((b) => b..name = 'Joe');
    final luke = Person((b) => b..name = 'Luke');

    final org = Organization((b) => b
      ..name = 'Foolandia'
      ..people = MapBuilder({
        'mobile': BuiltSet<Person>({joe, luke}),
      }));

    final orgBuilder = org.toBuilder();

    /// Initial works
    expect(orgBuilder.people['mobile'], equals(BuiltSet<Person>({joe, luke})));

    /// Weird rebuild works
    orgBuilder.people['mobile'] = (orgBuilder.people['mobile']!.toBuilder()
          ..removeWhere((e) => e.name == 'Joe'))
        .build();
    expect(orgBuilder.people['mobile'], equals(BuiltSet<Person>({luke})));

    /// Rebuild fails, expected to work
    orgBuilder.people['mobile']!
        .rebuild((b) => b..removeWhere((e) => e.name == 'Luke'));

    expect(orgBuilder.people['mobile'], equals(BuiltSet<Person>({})));
  });
}

Log

Expected: _BuiltSet<Person>:[]
  Actual: _BuiltSet<Person>:[
            _$Person:Person {  
            name=Luke,  
          }
          ]
   Which: at location [0] is _BuiltSet<Person>:[
            _$Person:Person {  
            name=Luke,  
          }
          ] which longer than expected

package:test_api                                   expect
expect
package:flutter_test/src/widget_tester.dart:441
main.<fn>
test/built_value_test.dart:38
2

✖ BuiltMap entry update
Exited (1)
dave26199 commented 3 years ago

BuiltSet is immutable, so rebuild does not change an existing instance; you need to also assign:


orgBuilder.people['mobile'] =

orgBuilder.people['mobile']!
        .rebuild((b) => b..removeWhere((e) => e.name == 'Luke'));

Usually a BuiltSetMultimap provides a nicer API here.

On Tue, Jul 27, 2021, 23:46 Luke Pighetti @.***> wrote:

It appears that if you try to rebuild a BuiltSet which is an entry of BuiltMap, it does not rebuild as expected. The changes are not applied.

main.dart

import 'package:built_collection/built_collection.dart';import 'package:built_value_map_entry_rebuilder/models.dart';import 'package:flutter_test/flutter_test.dart'; void main() { test('BuiltMap entry update', () { final joe = Person((b) => b..name = 'Joe'); final luke = Person((b) => b..name = 'Luke');

final org = Organization((b) => b
  ..name = 'Foolandia'
  ..people = MapBuilder({
    'mobile': BuiltSet<Person>({joe, luke}),
  }));

final orgBuilder = org.toBuilder();

/// Initial works    expect(orgBuilder.people['mobile'], equals(BuiltSet<Person>({joe, luke})));

/// Weird rebuild works    orgBuilder.people['mobile'] = (orgBuilder.people['mobile']!.toBuilder()
      ..removeWhere((e) => e.name == 'Joe'))
    .build();
expect(orgBuilder.people['mobile'], equals(BuiltSet<Person>({luke})));

/// Rebuild fails, expected to work    orgBuilder.people['mobile']!
    .rebuild((b) => b..removeWhere((e) => e.name == 'Luke'));

expect(orgBuilder.people['mobile'], equals(BuiltSet<Person>({})));

}); }

models.dart

import 'package:built_value/built_value.dart';import 'package:built_value/serializer.dart';import 'package:built_collection/builtcollection.dart'; part 'models.g.dart'; abstract class Person implements Built<Person, PersonBuilder> { static Serializer get serializer => $personSerializer;

Person.(); factory Person([void Function(PersonBuilder) updates]) = $Person;

String get name; } abstract class Organization implements Built<Organization, OrganizationBuilder> { static Serializer get serializer => _$organizationSerializer;

Organization.(); factory Organization([void Function(OrganizationBuilder) updates]) = $Organization;

String get name;

BuiltMap<String, BuiltSet> get people; }

built_value_test.dart

import 'package:built_collection/built_collection.dart';import 'package:built_value_map_entry_rebuilder/models.dart';import 'package:flutter_test/flutter_test.dart'; void main() { test('BuiltMap entry update', () { final joe = Person((b) => b..name = 'Joe'); final luke = Person((b) => b..name = 'Luke');

final org = Organization((b) => b
  ..name = 'Foolandia'
  ..people = MapBuilder({
    'mobile': BuiltSet<Person>({joe, luke}),
  }));

final orgBuilder = org.toBuilder();

/// Initial works    expect(orgBuilder.people['mobile'], equals(BuiltSet<Person>({joe, luke})));

/// Weird rebuild works    orgBuilder.people['mobile'] = (orgBuilder.people['mobile']!.toBuilder()
      ..removeWhere((e) => e.name == 'Joe'))
    .build();
expect(orgBuilder.people['mobile'], equals(BuiltSet<Person>({luke})));

/// Rebuild fails, expected to work    orgBuilder.people['mobile']!
    .rebuild((b) => b..removeWhere((e) => e.name == 'Luke'));

expect(orgBuilder.people['mobile'], equals(BuiltSet<Person>({})));

}); }

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/google/built_collection.dart/issues/249, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADAZMCR6C6WOYRFF75WXFJ3TZ4SLFANCNFSM5BDC7IGQ .

lukepighetti commented 3 years ago

Does that mean that BuiltSet.rebuild() is always no-op? If so, should it throw an error or a warning similar to when you try to mutate an UnmodifiableListView?

davidmorgan commented 3 years ago

It returns a new result, it doesn't modify the existing collection.

I think there's an annotation in package:meta we could use to catch cases where you ignore the result, but I didn't look at that yet--it seems a good idea.

lukepighetti commented 3 years ago

It seems like there is an API divergence between built values and built collections. If you rebuild a value it mutates the existing instance, while if you rebuild a collection it does NOT mutate the existing instance. Am I understanding that right?

davidmorgan commented 3 years ago

They're the same, built_value classes are also immutable :) ... only builders are mutable.

lukepighetti commented 3 years ago

Brain fart on my part. Thanks for sticking with me!

davidmorgan commented 3 years ago

No worries, this question comes up regularly :) ... I should try that annotation ...