[BUG][RUST] Regression: deriving Default (#10432) for Rust structs breaks custom type mappings for non-Default types

[ramosbugs]

Description

A recent change introduced in #10432 derived Default for all of the Rust models to improve ergonomics. This was assumed to be a non-breaking change, but it broke the build for any API specs that use the --type-mappings option to specify custom types (e.g., those that follow the NewType pattern) if those types don't implement the Default trait.

It looks like #10720 uncovered and fixed a separate breaking change related to that PR, but I think the problem is more fundamental here: the Default trait in Rust should only be used when there's a sensible default for a type. Consider, for example, an API endpoint that changes a user's password. What should the default struct values of ChangePasswordRequest be? I feel that for many API models, there is no obvious default, and this change has the potential to make code significantly more bug-prone by allowing types to be instantiated with arbitrary default values (e.g., empty strings) that don't make much sense semantically.

In cases where the --type-mappings option is used, this change also forces users either to abandon their custom types (e.g., a SecretUserPassword type that redacts the password in its Debug implementation), or to add a Default implementation that would be accessible not only within the OpenAPI-generated code, but across the user's entire codebase. 😢

openapi-generator version

5.3.0 regression

Related issues/PRs

#10432 Introduced the change
#10720 Fixed a related issue

cc: @PiDelport (author) @wing328 (reviewer)

Suggest a fix

I can see this change being useful in specific scenarios (e.g., structs with many optional types), but I think the problems it introduces in the general case are sufficient that Default shouldn't be derived by default. Is there some way we can (1) revert the change to unbreak existing code, and possibly (2) re-introduce the feature as an opt-in with some sort of toggle and/or extension on the relevant model types? One idea is to unconditionally generate the Default implementation in the Mustache template, but to put it behind a Cargo feature flag so that it only gets compiled if the user opts into the feature.

Thoughts?

[isimluk]

@ramosbugs, let me just start with saying I am rust newbie, nevertheless I agree with your conclusions.

My initial reaction would be to skip generation of Default trait for classes that very obviously won't have the sensible default for a type. I.e. Generic enums don't have sensible default. Classes containing enums don't have sensible defaults, etc.

[AlisCode]

I've been facing this issue as well, it very much looks to me like the Default trait has been abused here, for the sake of "better ergonomics". Some types just don't have good defaults by nature, and I think the initial feature request (#7501) is wrong to say that "a lot of boilerplate" is required when the derive isn't used. A trait implementation in this context is not boilerplate, an OpenAPI specification can't possibly give good defaults here.

My personal recommendation is to get rid of all of those Default derives, except maybe if it can be easily asserted that all fields are optional. Anything else is probably too complex.
Of course, removing the Default derive on everything (structs & enums) is a breaking change.

I'm not sure what the process to move forward on this issue is ? Should I just submit a PR here and let people discuss about it ?

[wing328]

I'm not sure what the process to move forward on this issue is ? Should I just submit a PR here and let people discuss about it ?

yes please.

[SorenHolstHansen]

I am running into a similar issue, but for me the compiler throws an error on the generated code, because a struct derives Default but one of its fields is an enum which does not derive Default. Hence the struct can't derive default correctly, and this is a compiler error

[SorenHolstHansen]

I have made a PR (#15856) That reverts the changes made in #10432. It is nice to have a Default derive, but at the moment it is breaking compilation (see #10720 (comment))