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

Razor Pages ComplexTypeModelBinder fails to handle the suggestion of "Alternatively, set the 'X' property to a non-null value in the 'YModel' constructor" #21916

Closed
jlchavez opened this issue May 17, 2020 · 1 comment · Fixed by #39353
Assignees
Labels
affected-very-few This issue impacts very few customers area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels bug This issue describes a behavior which is not expected - a bug. feature-model-binding investigate severity-nice-to-have This label is used by an internal tool
Milestone

Comments

@jlchavez
Copy link

Version

.NET Core 3.1
Microsoft.AspNetCore Nuget Packages 3.1.4 (latest)

Context of the problem

I have a page model with five properties that are a complex type, which is need to be bound on post back, this is for making an async upload mechanism, that will show a button for selecting the file and a button for uploading, when this one is clicked we show a progress bar, lock the buttons, finish the upload and roll back those UI changes, and also change a feedback icon so that the user knows that the file had been uploaded. When the file is completely loaded to the server, a hidden checkbox is marked, that's the only thing need to be validated by the JQuery Unobtrusive validation (a custom one). The Filename is just so that the user can read what was the file that he uploaded.

The objective of all this is the abstraction of how I upload a file, and not customizing code for every file upload i need to build, knowing a mapping to the internal file i need to upload, and some feedback to the user. The DocCode is part of ajax posting the file to another page handler.

I also need the Name property because of that passing a property as the new model for the partial view, if that property has a DisplayAttribute it can't be used so i have to send it in someway to the View for rendering.

Sample Code

** some things have been cut out for simplicity **

public class DocumentsModel : PageModel
{
    public DocumentsModel(IBizRegistrarion bizRegistrarion)
    {
        _bizRegistration = bizRegistrarion;
        IDDoc = new Document("PERID", "Identity Document");
    }

    [BindProperty]
    [Display(Name = "Identity Document")]
    [FileRequired]
    public Document IDDoc { get; }
}

the Document type is defined this way:

[Serializable]
public sealed class Document
{
    public Document(string docCode, string name)
    {
        DocCode = docCode;
        Name = name;
    }
    public string Name { get; }
    public string FileName { get; set; }
    public bool HasFile { get; set; }
    public string PlaceHolder { get => HasFile ? Loaded : Pending; }
    public string DocCode { get; }
}

I have a View that receives this model

@model Document
@functions {
    public string FileIcon(Document document)
    {
        return document?.HasFile ?? false ? "fas fa-check text-success" : "fas fa-times text-danger";
    }
}
<div class="form-group">
    <label class="control-label">@Model.Name</label>
    <div class="input-group mb-3">
        <input type="text" asp-for="FileName" class="form-control filename" placeholder="@Model?.PlaceHolder" readonly data-docCode="@Model.DocCode" />
        <div class="input-group-append">
            <span class="input-group-text statusIcon"><i class="@FileIcon(Model)"></i></span>
            <button type="button" class="btn btn-outline-secondary fileSelect"><i class="fas fa-copy"></i> @Select</button>
            <button type="button" class="btn btn-outline-secondary upload disabled"><i class="fas fa-cloud-upload-alt"></i> @Upload</button>
        </div>
    </div>
    <input type="checkbox" asp-for="HasFile" class="d-none" />
    <input type="file" class="fileUpload d-none" style="width:0px" />
    <span asp-validation-for="@Model" class="text-danger"></span>
    <progress style="display:none" value="50" max="100"></progress>
</div>

and the page, includes this partial view five times in this way:

@page
<form method="post">
    <partial name="_Upload" for="IDDoc" />
    <partial name="_Upload" for="IDDocR" />
    <partial name="_Upload" for="TaxIDDoc" />
    <partial name="_Upload" for="ComIDDoc" />
    <partial name="_Upload" for="BankDoc" />
</form>

Tracking the problem

When posting back II receive a binding error on a Razor Page when posting a form (post method): "Could not create an instance of type 'Document'. Model bound complex types must not be abstract or value types and must have a parameterless constructor. Alternatively, set the 'IDDoc' property to a non-null value in the 'DocumentsModel' constructor."

The property has an instance set to it, but the ParameterBinder gets to this line:

modelBindingContext.Model = value;

And creates a DefaultModelBindingContext which the Model is set to NULL, then when calling, on ComplexTypeModelBinder.BindModelCoreAsync we get to this line:

which is annotated with a comment as:

Create model first (if necessary) to avoid reporting errors about properties when activation fails.

on the next line, it is called the CreateModel, since the provided Model is null, but the property really has a value, this method tries to create a new instance, where:

  • If the constructor is not a parameterless one, it fails
  • If there is a parameterless constructor and the property is read only it won't assign it (OBVIOUSLY), but it won't even read the current property value
  • If the property has a setter, the original object would be overridden, so it is USELESS the idea of "Alternatively, set the 'X' property to a non-null value".

Correct functionality

If the property doesn't have a setter, then just validate that it has an instance, or else then try to constructor the object and fail if there is no parameterless constructor.

This is really dissapointing, I'd have to do a workaround with a ModelBinderAttribute or something like that.

@pranavkm pranavkm added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label May 17, 2020
@mkArtakMSFT mkArtakMSFT added the bug This issue describes a behavior which is not expected - a bug. label May 18, 2020
@mkArtakMSFT mkArtakMSFT added this to the Backlog milestone May 18, 2020
@mkArtakMSFT
Copy link
Member

We've moved this issue to the Backlog milestone. This means that it is not going to happen for the coming release. We will reassess the backlog following the current release and consider this item at that time. However, keep in mind that there are many other high priority features with which it will be competing for resources.

@pranavkm pranavkm added affected-very-few This issue impacts very few customers severity-nice-to-have This label is used by an internal tool labels Nov 6, 2020
@mkArtakMSFT mkArtakMSFT added area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Oct 14, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affected-very-few This issue impacts very few customers area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels bug This issue describes a behavior which is not expected - a bug. feature-model-binding investigate severity-nice-to-have This label is used by an internal tool
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants