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

Can single fields be ignored if not null? #105

Open
BlackCatDev-IO opened this issue Jun 18, 2023 · 16 comments
Open

Can single fields be ignored if not null? #105

BlackCatDev-IO opened this issue Jun 18, 2023 · 16 comments

Comments

@BlackCatDev-IO
Copy link

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

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

But it seems like a nice feature to add.

@2x2xplz
Copy link

2x2xplz commented Jun 25, 2023

I'm new to this package so take this with a grain of salt, however this page explains the philosophy: When analysing your code, dart_mappable 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
Copy link
Owner

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

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

schultek commented Jul 4, 2023

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

@boostmerlin
Copy link

boostmerlin commented Sep 14, 2023

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

@AhmedLSayed9
Copy link

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

@tgrushka
Copy link

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

stact commented Sep 23, 2024

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

stact commented Sep 24, 2024

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

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

stact commented Sep 25, 2024

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

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

stact commented Sep 25, 2024

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

tgrushka commented Sep 27, 2024

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

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)?

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

No branches or pull requests

7 participants