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

[FEATURE] copyWith extension methods that allow setting a field to null #438

Closed
vipw opened this issue Jul 29, 2022 · 13 comments · Fixed by #445 or #457
Closed

[FEATURE] copyWith extension methods that allow setting a field to null #438

vipw opened this issue Jul 29, 2022 · 13 comments · Fixed by #445 or #457
Assignees
Labels
enhancement New feature or request

Comments

@vipw
Copy link
Contributor

vipw commented Jul 29, 2022

Is your feature request related to a problem? Please describe.
I have an API that allows unsetting a value by POSTing a JSON document with a field set to NULL.

For example a person's age can be unset with by POSTing a Person object to /person/personId

{
  "name": "bob",
  "age": null
} 

The autogenerated Person class has

extension $PersonExtension on Person {
  Person copyWith(
      {String? name,
      int? age) {
  return Person(
        name: name?? this.name,
        age: age?? this.age);
  }
}

This is usually very helpful but doesn't allow setting either field to null.

This is a simplified example, I actually have dozens of entities, each with many members. Otherwise I would just do the obvious thing:

newPerson = Person(name: oldPerson.name, age: null);

Describe the solution you'd like
I'm not sure it's the best solution, but I built my own extension method based on this post: https://stackoverflow.com/a/71732563/759140

class Wrapped<T> {
  final T value;
  const Wrapped.value(this.value);
}

extension PersonExtensionWrapped on Person {
  Person copyWrapped(
      {Wrapped<String?>? name,
      Wrapped<int?>? age) {
  return Person(
        name: (name != null ? name.value : this.name),
        age: (age != null ? age.value : this.age)
      );
  }
}

It seems bloated to make copyWith and copyWrapped methods. Maybe it could be a configuration flag whether the copyWith uses wrapping or not.

I also had a thought to add a copyButNullSomeFields method that would take bools, and null the specified fields:

extension PersonExtensionNull on Person {
  Person copyButNullSomeFields(bool? name, bool? age) {
  return Person(
   name: (name == null || name == false) ? this.name : null;
   age: (age == null || age == false) ? this.name : null;
  }
}

But I haven't tried this one out yet. It might be a better for autogenerated code because it would be easy to chain it with copyWith().

Swagger specification link
N/A

@vipw vipw added the enhancement New feature or request label Jul 29, 2022
@Vovanella95
Copy link
Collaborator

@vipw try new version :) swagger_dart_code_generator: ^2.7.2

@nguyenloki258
Copy link

@vipw try new version :) swagger_dart_code_generator: ^2.7.2
I got error
A member named 'copyWith' is defined in extension '$AccountResponseModelExtension' and extension '$AccountResponseModelWrappedExtension', and none are more specific • lib/modules/user_profile/user_profile_controller.dart:214:26 • ambiguous_extension_member_access

@Vovanella95 Vovanella95 reopened this Aug 15, 2022
@Vovanella95
Copy link
Collaborator

@nguyenloki258 could you please share your swagger file? Because my 50 swaggers has been generated well

@vipw
Copy link
Contributor Author

vipw commented Aug 19, 2022

I get the same issue as @nguyenloki258. The generated code compiles, but since both extension classes have a method with the same name, the compiler can't infer which copyWith should be called.

An easy solution is to change the SwaggerModelsGenerator::generateCopyWithWrappedContent method to make methods named copyWithWrapped. I've changed that in my local environment and it works fine.

Here it is in a PR:
#450

@vipw
Copy link
Contributor Author

vipw commented Aug 19, 2022

There's one other thing that comes to my mind about this feature. Do you think it's good to allow setting required fields to null?

Maybe there should only be one copyWith method that takes raw types for required members and Wrapped for the optional members. That would correspond more closely to the swagger file definition, but it would break backward compatibility and make code using the generated class more verbose. I'm curious what you think about it.

@Vovanella95
Copy link
Collaborator

Good evening @vipw! I think it will make models more flexible. Because who knows how swagger file is configures.

That's correct your PR fix this issue?

@ko16a46
Copy link

ko16a46 commented Aug 19, 2022

Hello! I'm receiving the same issue as @nguyenloki258

The issue is that I have multiple swagger files, which each create two different versions of the Wrapped class.

Later, I have a barrel file that exports the generated models, and there's a conflict because both model files have their own Wrapped.

As a stopgap fix, I downgraded my version of the package. Technically, you can also work around it by doing export 'my_api_models.swagger.dart' hide Wrapped on one of the conflicting models, but it's not ideal since copyWith won't work for those models.

Perhaps a better solution would be to copy how freezed does copyWith. Freezed's way is less verbose since there's no wrapper object needed.

It keeps a private placeholder object, and copyWith checks if the given value is equal to the placeholder object. If it's the same placeholder, use the existing; if it's different (even null), use the given value. It does some override and casting shenanigans to deal with type safety.

@Vovanella95
Copy link
Collaborator

Hi @ko16a46 , good idea. Previously, we copy-pasted freezed approach. Bun looks like a lot changed since :D I will investigate it and make changes. Thanks!

@dJani97
Copy link

dJani97 commented Aug 26, 2022

Getting the same error:

A member named 'copyWith' is defined in extension '$LoginResponseExtension' 
and extension '$LoginResponseWrappedExtension', and none are more specific.

For anyone looking for a workaround, this solves it for me. Wrap your API response in one of the extension classes like so, and then use copyWith:

$LoginResponseExtension(response.body!).copyWith( .... )

@Vovanella95
Copy link
Collaborator

@dJani97 that would be really helpfull if you share you swagger file

@dJani97
Copy link

dJani97 commented Aug 26, 2022

@Vovanella95 it's not an open-source project, can I share it with you without making it public?

@dJani97
Copy link

dJani97 commented Aug 26, 2022

I'm sending it by email

@Vovanella95
Copy link
Collaborator

Hi @dJani97 , @vipw , Fixed your issue. Now copyWith works fine.

void main() {
  final user = LoginRequest(
    emailAddress: 'vovanella95@mail.ru',
    password: 'password',
  );

  final updatedUser = user.copyWithWrapped(password: null);
}

Will release new version later today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
6 participants