google / built_value.dart

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

Separate validation from model building #606

Open moodysalem opened 5 years ago

moodysalem commented 5 years ago

Background

In the generated built value builder, anything not marked with @nullable throws an error on build.

Request

I want to be able to separate the validation from the build method. Sometimes I want to use built_value for immutability and type safety only, e.g. when editing a model on a form. Validation doesn't make sense in this context because often the user will still be manipulating the form and the form can transiently be in a state where the model has nullable fields. However I need to build the model to emit events to the parent angular component as the user makes changes.

Example

class Value {
  /// ...
}

class BuiltThing implements Built<BuiltThing, BuiltThingBuilder> {
  /// ...other bv stuff
  /// Not nullable, a valid BuiltThing has a value.
  Value get value;
}

@Component(
  template:'''<value-selector [(value)]="value"/>''',
  directives: [ValueSelectorComponent]
)
class ThingForm {
  @Input
  BuiltThing thing;

  @Output
  Stream<BuiltThing> thingChange => _thingChangeController.stream;

  StreamController<BuiltThing> _thingChangeController = StreamController();

  String get value => thing.value;

  /// newValue can be null, causing an error
  set value(String newValue) => _thingChangeController.add(thing.rebuild((b) => b..value = newValue));
}

Workaround

Sometimes the call to build is wrapped in a try catch and the builder is stored as state in the form. This requires more boilerplate.

The other solution is using a model just for the form, which allows nullable for all the fields that can be transiently nullable, and writing converters to translate between the two models. This also requires more boilerplate.

Solutions

The validation concern may make sense to be moved into a separate package and @nullable validation could be entirely removed from built_value.

Or this could be exposed as an optional flag validate: true on the annotation.

I would expect additional validations could be added (empty, minLength, min for numbers, etc.), including custom validation for custom types, for example... @customValidate((value) => value.startsWith('abc'))

joshua-i commented 5 years ago

I think this is the correct behavior for built_value. Autovalue in java does the same thing: https://github.com/google/auto/blob/master/value/userguide/howto.md#nullable In general, it feels correct to me to do null checking before building a value. I think nullability is a special kind of constraint, which is why some languages (including possibly dart in the future) make it part of the type system.

So for me, this is more a question of the right way to use built_value with angular forms, and whether we should mark things as nullable in order to be able to represent some transient state of the form in which a field is not filled in. And if there was a separate validation-by-annotation framework for built_value, I don't know that we'd prefer it over angular forms validations.

moodysalem commented 5 years ago

That's fair, in TypeScript I might use a Partial<ThingInterface> or Nullable<ThingInterface> type to represent what I edit in the form to make all the fields optional. Maybe we can generate additional models with nullables and converters for forms?

davidmorgan commented 5 years ago

Thanks! This is a tricky problem. I'm definitely open to suggestions.

FWIW, the convention for additional validation is to write it in the constructor of the value type. This is then checked on instantiation, in the same manner.

The plan for nullability going forward is that we will switch to using the upcoming language feature, i.e. @nullable today will map onto a nullable type. So we don't have a whole lot of freedom to experiment right now. Maybe it makes sense to take another look at this when the language changes have landed.

micimize commented 4 years ago

Sometimes the call to build is wrapped in a try catch and the builder is stored as state in the form. This requires more boilerplate.

This seems like the right abstraction to me. It can be generalized, and bounds the scope of an invalid value.

Though, one issue for me is I'd much prefer for build to collect BuiltValueNullFieldErrors, and then throw, so that it could actually be used for field validation.