OrchardCore.Forms throws InvalidOperationException when restoring checkboxes from ModelState

[rjpowers10]

Describe the bug

OrchardCore.Forms leverages ModelStateHelpers.SerializeModelState and ModelStateHelpers.DeserializeModelState to preserve ModelState across web requests. This is broken for checkboxes since System.Text.Json was introduced in 2.0.0.

InvalidOperationException: The parameter conversion from type 'System.Collections.Generic.List`1[System.Object]' to type 'System.Boolean' failed because no type converter can convert between these types

Orchard Core version

2.0.2

To Reproduce

In my app I'm not actually using the form content item, but I do use OrchardCore.Forms solely for the ModelState preservation "trick". But I imagine you can reproduce this in OC with a simple form content item with a checkbox. You also need the form to be invalid so the ModelState preservation trick gets invoked and the user gets redirected back to the form.

Here's a simple console app that demonstrates the issue by comparing what happens with Newtonsoft.Json to System.Text.Json.

using Microsoft.Extensions.Primitives;

var checkbox = new ModelStateTransferValue
{
    Key = "MyCheckbox",
    AttemptedValue = bool.FalseString,
    RawValue = new StringValues(bool.FalseString) // Checkboxes in .NET set the RawValue to a StringValues object
};

// ModelStateHelpers.SerializeModelState
string newtonsoftJson = Newtonsoft.Json.JsonConvert.SerializeObject(checkbox);
string stjJson = System.Text.Json.JsonSerializer.Serialize(checkbox);

// ModelStateHelpers.DeserializeModelState
var newtonsoftModel = Newtonsoft.Json.JsonConvert.DeserializeObject<ModelStateTransferValue>(newtonsoftJson);
var stjModel = System.Text.Json.JsonSerializer.Deserialize<ModelStateTransferValue>(stjJson);

Console.WriteLine($"Type of Newtonsoft RawValue: {newtonsoftModel.RawValue.GetType()}"); // Outputs: Newtonsoft.Json.Linq.JArray
Console.WriteLine($"Newtonsoft RawValue is JArray: {newtonsoftModel.RawValue is Newtonsoft.Json.Linq.JArray}"); // Outputs: True
Console.WriteLine();
Console.WriteLine($"Type of STJ RawValue: {stjModel.RawValue.GetType()}"); // Outputs: System.Text.Json.JsonElement
Console.WriteLine($"STJ RawValue is JsonArray: {stjModel.RawValue is System.Text.Json.Nodes.JsonArray}"); // Output: False

public sealed class ModelStateTransferValue
{
    public string Key { get; set; }
    public string AttemptedValue { get; set; }
    public object RawValue { get; set; }
}

Running this console app produces the following output:

Type of Newtonsoft RawValue: Newtonsoft.Json.Linq.JArray
Newtonsoft RawValue is JArray: True

Type of STJ RawValue: System.Text.Json.JsonElement
STJ RawValue is JsonArray: False

The key is this line in ModelStateHelpers.

The Newtonsoft.Json version of the key line is

item.RawValue = item.RawValue is JArray jarray ? jarray.ToObject<object[]>() : item.RawValue;

The STJ version of the key line is

item.RawValue = item.RawValue is JsonArray jarray ? jarray.ToObject<object[]>() : item.RawValue;

• With Newtonsoft.Json is JArray is true for a checkbox, so the value is converted to object[].
• With STJ is JsonArray is false for a checkbox, so the value is left as JsonArray.

Over in .NET we look at ModelBindingHelper.UnwrapPossibleArrayType. With STJ we end up passing down List<object>, which it does not know how to unwrap and so it falls into case 4. Then we get an exception because it doesn't know to convert a list to a checkbox.

[rjpowers10]

Just remembered something like this happened before #7256

[sebastienros]

Do you think you can provide a PR fixing the conversion?
And if there a way to add a test (unit or functional) to ensure we don't regress that?

[github-actions]

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.

[rjpowers10]

I started looking into a fix, however I was unable to reproduce my problem exactly in stock OC. I didn't get the exception, but also the checkbox state was not preserved. If I checked the box and submitted the form, I got redirected back but the checkbox remained unchecked. Might be a different symptom but the same underlying cause.

Need to look into it more.

[rjpowers10]

Stock OC example from the main branch

1. Create a new site using the blog recipe.
2. Enable the OrchardCore.Forms feature.
3. Add a new form widget to the "content" zone and "always" layer. In the form's flow:
   1. Add the Validation Summary widget.
   2. Add the Input widget.
      1. Name: Name
      2. ID: Name
      3. Label option: Standard
      4. Label text: Name
      5. Type: Input
      6. Validation option: Standard
   3. Add the Input widget.
      1. Name: MyCheckbox
      2. ID: MyCheckbox
      3. Label option: Standard
      4. Label text: Click me
      5. Type: Checkbox
      6. Validation option: Standard
   4. Add the Button widget
      1. Name: MyButton
      2. ID: MyButton
      3. Text: Submit
      4. Type: Submit
   5. Add a new workflow like the image below. The idea is to submit the form and force a validation error on the checkbox, then redirect back to the form to display the error.
   6. Copy the action from the Http Request Event and set that as the form action back in the form widget.
   7. Uncheck "validate antiforgery token" from the Http Request Event.

If you get stuck I modified the example "contact" form from the OC docs

View the form

Fill out the form

Submit the form. The text input state is preserved. The checkbox state is not preserved.

Maybe this is a different symptom of the same problem. I don't get the InvalidOperationException, although my checkbox state is also not preserved for the invalid form.

[rjpowers10]

Apologies everyone. The original issue I submitted may be in my own code. But I do think there is an OC bug with checkbox state not being preserved in OrchardCore.Forms. I'm going to submit a new issue and PR for that.

[sebastienros]

Thanks for investigating!