schultek / dart_mappable

Improved json serialization and data classes with full support for generics, inheritance, customization and more.
https://pub.dev/packages/dart_mappable
MIT License
165 stars 23 forks source link

Can single fields be ignored if not null? #105

Open BlackCatDev-IO opened 1 year ago

BlackCatDev-IO commented 1 year ago

Hi, thanks for the work on this package. Its really nice to have the freezed functionality with a much simpler syntax.

I see the ignoreNull option, but one thing I didn't see in the docs is if its possible to ignore a single field even if it isn't null?

The json_annotation includes the option @JsonKey(includeFromJson: false, includeToJson: false) to omit that value from all serialization attempts whether its null or not.

A freezed example of that use case would look like this

enum TestStatus {
  initial,
  loading,
  success,
  failure,
}

@freezed
class TestState with _$TestState {
  factory TestState({
    @JsonKey(includeFromJson: false, includeToJson: false) // this is what I'm specifically referring to
    @Default(TestStatus.initial) TestStatus status,
    @Default('')
    String name,
  }) = _TestState;

  factory TestState.fromJson(Map<String, dynamic> json) =>
      _$TestStateFromJson(json);
}

Is there an equivalent to @JsonKey(includeFromJson: false, includeToJson: false) in dart_mappable?

If not, any chance of including that in the future?

schultek commented 1 year ago

Currently the ignoreNull is only implemented for a single class, not a single field.

But it seems like a nice feature to add.

2x2xplz commented 1 year ago

I'm new to this package so take this with a grain of salt, however this page explains the philosophy: _When analysing your code, dartmappable never looks at the fields of your model, but rather only at the constructor arguments.

Therefore your solution should be to create a TestState constructor that doesn't include the ignored field, and annotate that constructor with @MappableConstructor() Something like:

@MappableClass()
class TestState with TestStateMappable {
  TestStatus status,
  String name,

  TestState(this.status, this.name)

  @MappableConstructor()
  TestState(this.name) : status = TestStatus.initial
}
schultek commented 1 year ago

Sorry I misread the issue the first time. @2x2xplz is right, ignoring fields is as simple as leaving them out of the constructor.

BlackCatDev-IO commented 1 year ago

Sorry, I should have been more clear when I opened the issue. The example I provided only ignores that field for json serialization, but still includes that field in copyWith, and equality comparisons etc...After a quick test, it appears you lose all of that when leaving it out of the constructor with DartMappable. So, that is not the same functionality as the freezed example I provided.

Right now I can get around it in my use case with HydradedBloc by just specifying whatever I want the initial value to be in the fromJson overrides.

eg.

/// this assumes usage of HydradedBloc
...
  @override
  TestState? fromJson(Map<String, dynamic> json) {
    // with freezed this copyWith isn't necessary here when `includeFromJson` is set to false
    return TestState.fromMap(json).copyWith(status: TestStatus.initial); 
  }

Again, this package is great. I think this may be a feature worth considering for the future. Thanks.

schultek commented 1 year ago

I see. Your right I think this is currently not possible.

boostmerlin commented 1 year ago

I also came across this situation and wante this feature too.

AhmedLSayed9 commented 1 year ago

I'd like to see this feature too "migrating from Freezed".

tgrushka commented 5 months ago

Yes, please, this would be awesome. Something like a @MappableIgnore() annotation (or even the more esoteric Java-like name, @MappableTransient() -- but I prefer "ignore" as it seems more user-friendly) on the field. Most other serialization libraries offer this. (Also, I really like that this library tries to play nice and not conflict with names in other libraries by using the @Mappable... prefix. It makes it really easy to swap in/out database packages.)

stact commented 2 months ago

lol I tried to fork and to contribute: results: not my level :)

So to share what I got in mind and tried to implement is

@MappableField(includeFrom: false, includeTo: false) to be able to use the same logics as JsonKey but here for toJson and toMap

Hope someone will be able to be more efficient than me on this repo 😅

stact commented 2 months ago

So good news I finally get time to go deeper on the change. You will be able to find a working PR #236

@MappableClass(ignore: ['id'])
class BaseDto with BaseDtoMappable {
  BaseDto({
    required this.id,
    required this.createdAt,
    required this.modifiedAt,
  });

  final String id;
  final DateTime createdAt;
  final DateTime modifiedAt;
}

To test:

static void testFun() {
   final base = BaseDto(id: '123', createdAt: DateTime.now(), modifiedAt: DateTime.now());
   print(base.toJson());
   // Results: flutter: {"createdAt":"2024-09-24T16:27:16.591556Z","modifiedAt":"2024-09-24T16:27:16.591556Z"}
}

Magic :) are we ready to migrate?

schultek commented 2 months ago

Thanks for looking into this. I have a few comments:

I think this would be nicer as an option on @MappableField(ignore: true) instead of repeating the names on top.

How would you expect this to behave for deserialization. In your example the field is required, but shouldn't it be ignored for deserialization too? That would mean only nullable fields or ones with default values can be ignored.

If we want to give more granular control, like ignoring only for serializing or deserializing (and not both) how could that look like.

Do we want to allow ignoring fields also for toString or equality?


I want to avoid adding something to the package that fixes only a specific case but opens up 10 more new ones.

stact commented 2 months ago

Thank you for raising these issues.

I think this would be nicer as an option on @MappableField(ignore: true) instead of repeating the names on top.

Yes you're right, here I just tried to manage with a quick win and no breaking change. Here we need maybe also refacto ignoreNull to manage it in the @MappableField

If we want to give more granular control, like ignoring only for serializing or deserializing (and not both) how could that look like.

The ignore is only for serialization(toJson,toMap), maybe the name ignore is too generic and must be renamed ignoredFieldsOnSerialization

How would you expect this to behave for deserialization. In your example the field is required, but shouldn't it be ignored for deserialization too? That would mean only nullable fields or ones with default values can be ignored.

In the example I'm receiving fromJson that containts the id of the field (in database) and the object is correct and contains the id (what we expect)

const json = '{"id": "123", "createdAt":"2024-09-25T05:19:22.440733Z","modifiedAt":"2024-09-25T05:19:22.440733Z"}';
final fromJson = BaseDtoMapper.fromJson(json);
print(fromJson);
// Results: flutter: BaseDto(id: 123, createdAt: 2024-09-25 05:19:22.440733Z, modifiedAt: 2024-09-25 05:19:22.440733Z)
print(fromJson.toJson());
// Results: flutter: {"createdAt":"2024-09-25T05:19:22.440733Z","modifiedAt":"2024-09-25T05:19:22.440733Z"}

Here is the correct behavior, we won't push this information in database, so that's why ignore: ['id'] enters in the game. It can be also another technical fields, calculated on the fly that we won't push too

schultek commented 2 months ago

I'm thinking about re-using the GenerationMethods system: https://pub.dev/documentation/dart_mappable/latest/topics/Configuration-topic.html#generation-methods

Then this could be either excluding specific methods, or including specific methods.

stact commented 2 months ago

Yes this is great, I was not aware about this capacity. And your idea is to go from @MappableField instead of @MappableClass, right? If yes, definetely it will solve this issue on managing granularly.

tgrushka commented 2 months ago

EDIT: This does not work actually. LedgerItemMappable still has the computedBalance field but removes it from copyWith so I'm back to square 1.

Yes, @stact , I agree I would like to see it in an annotation as @schultek suggested. I keep coming back to this issue. Let's say I have a class like:

@MappableClass()
class LedgerItem with LedgerItemMappable {
  const LedgerItem({
    required this.date,
    this.amount,
    this.balance,
    this.note,
  })  : computedBalance = null,
        assert(amount != null || balance != null,
            'Balance must not be null if amount is null');
  final DateTime date;
  final double? amount;
  final double? balance;
  final double? computedBalance;
  @MappableField(hook: NullEmptyStringMappingHook())
  final String? note;
...
}

As it is now, I cannot add computedBalance in copyWith(). However, I obviously don't want to serialize computedBalance as it is a field computed outside of the class (as the class represents one row / record). So the only way right now is to add a redundant constructor:

  @MappableConstructor()
  const LedgerItem({
...
  const LedgerItem.computed({
    required this.date,
    this.amount,
    this.balance,
    required double this.computedBalance,
    this.note,
  }) : assert(amount != null || balance != null,
            'Balance must not be null if amount is null');

  LedgerItem withComputedBalance(double computedBalance) => LedgerItem.computed(
        date: date,
        amount: amount,
        balance: balance,
        computedBalance: computedBalance,
        note: note,
      );
tgrushka commented 2 months ago

Also, could we do something like:

@MappableField(serialize: true, deserialize: false)

(both would default to true, but this way you can control both directions -- as opposed to ignore)?

mrasityilmaz commented 2 weeks ago

So, are you planning to add this feature, or should we go ahead and implement it ourselves and release a new package?

We’re open to either approach, but a clear direction would be great. Thanks!

schultek commented 2 weeks ago

I do want to add this eventually, but I probably won't get to it anytime soon.

As a workaround I could see a custom hook that removes the fields from the input or output maps.