OpenAPITools / openapi-generator

OpenAPI Generator allows generation of API client libraries (SDK generation), server stubs, documentation and configuration automatically given an OpenAPI Spec (v2, v3)
https://openapi-generator.tech
Apache License 2.0
21.84k stars 6.59k forks source link

[REQ] [dart] Generate a copyWith help method on models #3858

Open jaumard opened 5 years ago

jaumard commented 5 years ago

Is your feature request related to a problem? Please describe.

It can be very useful to generate a copyWith help method on the generated models. Generated models are immutable so to change a properties you need to recreate an object, a copyWith method will be super helpful for that.

Describe the solution you'd like

Modify the template to generate a copyWith method

Describe alternatives you've considered

Find a dart package that can generate this for us and just use it, but it will add an external dep so I don't recommend it.

What do you think guys ? @ircecho (2017/07) @swipesight (2018/09) @nickmeinhold (2019/09)

auto-labeler[bot] commented 5 years ago

👍 Thanks for opening this issue! 🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

nickmeinhold commented 5 years ago

Sounds great!

I put up #3797 at some point, not the same thing but related (and I agree with your point about not adding a further dependency)

Please correct me if I'm wrong but I believe the Dart generator currently does not currently create immutable models. I notice the DartJaguar generator creates final fields - are collection types also immutable? I couldn't tell with a quick read sorry.

Is there any guidance around immutable models do you know? Do other generators create immutable models?

Btw, is there a copyWith method you have in mind? One I've seen around a lot uses optional named parameters, looks nice and clean but has the limitation of not allowing fields to be set to null. Do you know built_value? It uses the Builder pattern to achieve the same result but the syntax is pretty weird and I would imagine unfamiliar to most people. I'm not aware of a nice way to do it, so I would love to find out!

jaumard commented 5 years ago

Look like jaguar don't use immutable list (but they are final, just not immutable), but that's something I want to work on when I have some time as for me it's a must have.

I generally use the copyWith with named parameters but you're right you can't set value to null :/ I'm not familiar with built_value at all but if they have a solution that fix this problem I guess it should be the way to go :) I know built_value is used a lot so I'm not too worry about unfamiliarity. I'll try to take a look at how they fix that.

I have no knowledge about the pure dart generator ^^ I never really look at it so maybe you're right it doesn't generate immutable models. With jaguar it's immutable (yeah except list :/) and you don't have the choice. For me mutable models is a bad pattern ^^

jaumard commented 5 years ago

I've checked built_value and the notation is indeed weird lol But what we can do is this:


class User {
  final String name;
  final String email;
  const User({this.name, this.email});

  User copyWith({Optional<String> name, Optional<String> email}) {
    return User(
      name: name == null ? this.name : name.value,
      email: email == null ? this.email : email.value,
    );
  }
}

class Optional<T> {
  final T value;

  factory Optional.of(T value) {
    return Optional(value);
  }

  Optional(this.value);

  bool get hasValue => value != null;
}

Will be much easier to use and implement. Optional class must be generated once on his own file and we're done :)

nickmeinhold commented 5 years ago

Yeah it's super weird!

So this would be the "optional optional named parameters" approach :-) Yeah I've gotten around the named parameters limitation by wrapping any fields I wanted to be able to set to null in an extra object, same kind of approach and I agree it gets past the limitation. I don't agree that a copyWith that takes Optional objects is definitely the answer though. In my opinion it has its own disadvantages (as do the other options).

I wonder is it worth taking a step back and considering some of the upstream decisions as that could change how impactful any advantages and disadvantages of a particular approach are, eg:

I'm probably over analysing, my intention is try and be helpful, not to try and find problems, I hope that's clear.

On a side note: is this just for the DartJaguar generator? (because if so I'll shut up ;-) and more generally do you have any thoughts on merging dart-jaguar and dart vs. continuing as 2 different streams?

jaumard commented 5 years ago

as stated in https://github.com/OpenAPITools/openapi-generator/issues/3785#issuecomment-529293361 we'll go for merging as much as we can into one generator, but it will still be two different generators imho, as models and api will have to be generated differently.

I'm talking about what I know, so only dart-jaguar but it's a generic discussion to bring this on all dart generator if there is a need. If you say that the dart one don't have immutable models then the first step is to make them immutable and then talk about a copyWith ^^

To answer your different points:

How do you plan to get to immutable models? (eg. types from a library like built_value, or if not then how about collection types: Unmodifiable<List/Map>View, mixins, unmodifiable constructor, built_collection, etc.)

I'll probably push for full implementation rather than using built_value ^^ with unmodifiable constructor.

Before that even, what is the goal? Just immutable objects or something like a data class / value type?

I'll say here it's just immutable objects, as data class should probably come in next versions of dart :)

Is it worth considering that NNBD is (probably/hopefully) not far away and will presumably make a simple copyWith possible?

I don't know what NNBD is ^^ but I'll search :)

nickmeinhold commented 5 years ago

I think either merging or keeping separate would make sense but trying to do both doesn't.

The first step before making them both immutable would be to discuss it. I think there are many cases where immutable models are great but I'm not sure it's reasonable to assume everyone will want immutable models.

Maybe we could get some input from the core team on whether the project have a best practice / opinion on immutable models by default?

jaumard commented 5 years ago

I don't see any valid arguments to have mutable models :) For me mutable models are bad practice so let's not allow bad practice for our users.
But let's ear the arguments in favor of mutable models , I'm curious maybe I miss something.

What do you think @wing328 ? Any common practice across generators around this ?

nickmeinhold commented 5 years ago

The main arguments as I understand for allowing mutation are about speed and memory. Don't get me wrong, I think there are problems that come with mutation and many great arguments for having immutable objects in many circumstances and I usually make that choice. But different circumstances have different requirements and priorities so each has its place and I don't think it's fair to call mutation bad practice. A terribly oversimplified view might be that it's a tradeoff b/w speed and safety and depending on your situation you might prioritise one over the other.

There is some guidance in Effective Dart where they state "Of course, it is often useful to have mutable data. But, if you don’t need it, your default should be to make fields and top-level variables final". What I'm saying is I'm not sure we should be making the decision for people that they don't need it. To be clear I'm not arguing that we definitely should have mutable models, just that I think it needs consideration.

A separate argument (not pro mutation but more anti immutability in this context only) would be that there are no immutable types in Dart so the only options are to roll with your own custom immutable types or use a 3rd party library (and there are no standout options imo). So different conversation maybe but I think it's an important consideration here.

jaumard commented 5 years ago

A separate argument (not pro mutation but more anti immutability in this context only) would be that there are no immutable types in Dart so the only options are to roll with your own custom immutable types or use a 3rd party library (and there are no standout options imo). So different conversation maybe but I think it's an important consideration here.

What do you mean by that ? Dart as immutable types: https://api.dartlang.org/stable/2.5.0/dart-core/List/List.unmodifiable.html https://api.flutter.dev/flutter/dart-core/Map/Map.unmodifiable.html

I think that's all we need for immutability :)

nickmeinhold commented 5 years ago

Maybe I should have said it differently, I meant Dart doesn't have language level support for making classes immutable. Some languages do but Dart isn't one of them. When you create your own classes and choose how you're going to make them immutable you'll be making your own assumptions and decisions that won't necessarily match other peoples. For example I don't agree that final fields and unmodifiable lists/maps are all you need for immutability.

The unmodifiable lists/maps aren't types - the unmodifiable constructors return a List/Map so you don't get compile time checking. You can use in combination with the Unmodifiable<List/Map>View type but that gets cumbersome really quickly and isn't an actual immutable type.

There are other difficulties and tradeoffs that you will be making decisions about too and even if you feel that you've made all the best decisions it's still a custom implementation.

Each approach has its strengths and weaknesses. I've tried lots of approaches and never felt there was a clear winner. Right now I'm using built_value for my projects and pretty happy with it but I wouldn't necessarily argue for it. Each option comes with its advantages and disadvantages and they will be different in different contexts. One option here is to have mutable models by default and let users choose via cli option if they want immutable models and if so what type. I've been working on a built_value models cli option, maybe you could make a jaumard-models cli option ;-)

jaumard commented 5 years ago

Ho ok got it :)

For jaguar you don't have much of a choice ^^ either you make your models mutable, or you make them "immutable" with final fields. Jaguar-serializer doesn't support any other kind of models right now.

I agree that in pure dart you have multiple ways of achieving it ^^ I should have specify the context ^^

vasilich6107 commented 3 years ago

Hi. Any updates on this?

kuhnroyal commented 3 years ago

No, PRs are welcome :)

0xNF commented 2 years ago

What are the current thoughts on this issue? With NNBD in now, it's pretty easy to make copyWith methods.

The only thing that requires special attention is that one needs a work-around for how to assign nullable items. This is how I approach it in my own projects:

class Nullable<T> {
  T? item;
}

Using this class, we can then easily add copyWith:

class Cat {
  /// Returns a new [Cat] instance.
  Cat({
    required this.className,
    this.color = 'red',
    this.declawed,
  });

  final String className;

  final String color;

  final bool? declawed;

  Cat copyWith({
    String? className,
    String? color,
    Nullable<bool>? declawed,
  }) {
    return Cat(
      className: className ?? this.className,
      color: color ?? this.color,
      declawed: declawed == null ? this.declawed : declawed.item,
    );
  }
}

I'd be happy to add this to the Dart generator if it still seems like a good idea. I know that Dart-Dio has its own thing going on with BuiltValue and data classes in there.

MichalNemec commented 1 year ago

Hello, just stumbled upon this, any updates?

bubalopetar commented 5 months ago

@MichalNemec Hi, I am also interested if there is some news about adding copyWith method to openapi generated models automatically?

vasilich6107 commented 5 months ago

@bubalopetar you could subscribe to this PR updates https://github.com/OpenAPITools/openapi-generator/pull/13047 But it is over 2 years old so...