dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.3k stars 9.96k forks source link

Model binding with non-default constructor #43737

Open Pilchard123 opened 2 years ago

Pilchard123 commented 2 years ago

Is there an existing issue for this?

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

It is not possible to bind a model using a constructor unless using record types.

Describe the solution you'd like

It would be good to allow model binding with non-default constructors on types that are not records. I don't have a full design to hand, but for a first draft:

Additional context

Is this something that a PR would be accepted for? I can't guarantee I'd ever be able to do it, but never say never, hm?

Pilchard123 commented 2 years ago

On looking through the ComplexObjectModelBinder and ComplexObjectModelBinderProvider which appear to be the relevant places, there's a comment saying "Only record types are allowed to have a BoundConstructor", but nothing about why that is the case. How come only record types can be bound using a constructor? Could this be done by removing the restriction and letting everything work as it does now but now with non-record complex types?

javiercn commented 2 years ago

@Pilchard123 thanks for contacting us.

Very likely the reason is because we can't deterministically decide what constructor to use when you have many of them, so this was scoped out to support records only.

I do not remember the exact details, but I'll put this issue in .NET 8.0 for consideration. We might consider it if it gets enough traction.

Pilchard123 commented 2 years ago

Could it be done by taking the single constructor if only one exists, and requiring an attribute if multiple exist?

If it is the case (and I'm not going to hold you to this) that a non-record type can be constructed just fine as long as a suitable constructor is somehow identified, would a PR that removes the restriction be accepted (even if left on the shelf for a while until after 7.0 goes live)? Or does it have to be confirmed for some version before a PR would even be considered? I'd be willing to give it a punt myself if it'd be helpful.

javiercn commented 2 years ago

@Pilchard123 Unfortunately we will need to better understand the problem space before doing something. There were reasons at the time why we avoided this, so we would prefer to get those details figured out before we spend time making or evaluating a change.

Pilchard123 commented 1 year ago

Would the primary constructors for non-record types in 8-preview-3 be relevant here?

javiercn commented 1 year ago

@Pilchard123 we would need to look at it in more detail.

But a first thought is that the main thing we would be able to do would be to call the primary constructor on the type and ignore any other constructor.

The challenge is always that we don't have a way to determine what properties should we try to initialize once we've called the primary constructor, as we don't know which properties are set by it.

alex-jitbit commented 1 year ago

Just make CreateModel public instead of internal like it is in ComplexTypeModelBinder which you marked obsolete

eyadmba commented 1 year ago

I don't have stats to back this up, but I think that in most cases there would only be one parameterized constructor for the model class. The edge case of having multiple parameterized constructors can continue to error out like it does today, but solving for the common use-case would be very helpful.

Pilchard123 commented 1 year ago

Even if there are multiple constructors, there are attributes for model-y things already - field types, validation, that sort of thing. In the case where there's multiple constructors, an attribute (name to be determined) could disambiguate. And then, at least for the first iteration of it, if the constructor that gets picked doesn't set all the properties properly - make a constructor that does set all the properties properly.

It might have trouble with types that aren't under your control, but as a workaround you could write a wrapper type that is under your control and that forward the parameters on to the underlying type. In any case, I'd suggest that using a type that isn't yours as a model to bind to is asking for trouble anyway.

broomfn commented 5 months ago

I guess this never happened for .net 8?

I also think this would also be useful, as things are at the moment, I have to mark all my Edm classes as #nullable disable and supply a parameterless constructor for controller endpoints to bind to. On an OData Web API project this largely defeats the purpose of trying to use nullable reference types.