Critical correctness issue: action model binding is inside out and backward

[kjkrum]

I'm experimenting with actions. Unbound, collection, and entity actions all behave the same way with regard to ParameterConfiguration.Required(). The problem could be my expectation of what Required() is supposed to do.

	var action = builder.Action(nameof(UnboundController.UnboundAction));
	action.Parameter<string>("foo").Required();
	action.Parameter<string>("bar").Required();

If you underpost to this action, ModelState.IsValid is true and the ODataActionParameters parameter is non-null. The action will even accept a completely empty JSON object. But if you overpost, ModelState.IsValid is false and the ODataActionParameters is null.

My expectation is that this action should not accept underposting, but it should accept { "foo" : null, "bar" : null }.

If you make the properties non-nullable instead of required, then underposting causes ModelState.IsValid to be false:

	var action = builder.Action(nameof(UnboundController.UnboundAction));
	action.Parameter<string>("foo").Nullable = false;
	action.Parameter<string>("bar").Nullable = false;

Basically, Required() seems to do nothing.

[julealgon]

Can you show your controller action signature?

Also, does toggling the required value have any effect on the API $metadata?

[xuzhg]

@kjkrum @julealgon See the codes at: https://github.com/OData/ModelBuilder/blob/main/src/Microsoft.OData.ModelBuilder/Operations/ParameterConfiguration.cs#L71-L75

Please allow me to clarify the nullable vs optional for parameter:

1. nullable means we can or can't accept 'null' value, in the metadata, you will see nullable=false attribute, or omitted.
2. optional means the parameter can be omitted or not. in the metadata, you will see an <annotation .... /> element.

[kjkrum]

Calling Required() does not change the metadata. Calling Optional does change the metadata by adding this annotation:

          <Parameter Name="foo" Type="Edm.String">
            <Annotation Term="Org.OData.Core.V1.OptionalParameter" />
          </Parameter>

So it seems that parameters are required by default.

My controller action signature is public IActionResult UnboundAction(ODataActionParameters op). I used ODataActionParameters because I was following this doc that says that's how controller actions receive params. Route debug shows this as an OData route. If I overpost, causing ModelState.IsValid to be false, then serializing the model state shows an error that op is required; no mention of foo and bar. The construction of ODataActionParameters does not seem to respect parameters being required.

I changed the action signature to public IActionResult UnboundAction(string foo, string bar). This causes the parameters to be passed as URL parameters instead of as JSON in the post body. This makes them required; the model state is invalid if I don't provide them. Changing the signature also causes the Swagger UI to recognize the params as separate named inputs instead of presenting a generic JSON post body. Swagger still doesn't treat them as required, but that can be remedied with a redundant [Required] on the method param.

[kjkrum]

My general expectation for what required means was set by Model Validation in ASP.NET Web API. Overposting does not cause the model state to be invalid; unexpected properties are ignored. Underposting causes the model state to be invalid if and only if the missing property is [Required]. Of course, it isn't necessarily wrong for OData to behave differently. It's just surprising.

[xuzhg]

@kjkrum

1. parameters are required by default ==> Yes.

2. Required/Optional was used to nullability configuration. However, optional has a different meaning for parameters. So, we made a decision to change that in the new bigger version.

3. However, the optional parameter in 'OData Action' looks weird, because the action parameter is transferred in the request payload, and there's no difference whether you omit it in the payload or set it as null. But, it does impact the 'OData Function`, with or without the parameter in Function call could route to different controller/action.

4. public IActionResult UnboundAction(ODataActionParameters op) is the correct syntax to handle 'OData Action'. Can you show the errors in ModelState?

Hope it can help.

[kjkrum]

I got the behavior I wanted with this controller action signature: public IActionResult UnboundAction([Required, FromForm] string foo, [Required, FromForm] string bar). If I remove Required so the Swagger UI allows me to omit a param that's required by the EDM, then the model state is invalid as expected. FromForm causes the parameters to be passed in the body as multipart/form-data instead of JSON, but the values are passed to the controller action as expected. Route debug still shows it as an OData route.

[xuzhg]

if you don't want it to be OData route, you can change 'UnboundAction' action name to others random name.

public IActionResult UnboundAction([Required, FromForm] string foo, [Required, FromForm] string bar) is not fully supported in 8.x in OData.

[kjkrum]

@xuzhg The names of unbound actions don't seem to matter. It's only recognized as an OData route if I decorate it with [HttpPost($"/{EntityDataModel.RoutePrefix}/{nameof(UnboundAction)}")].

In what way is public IActionResult UnboundAction([Required, FromForm] string foo, [Required, FromForm] string bar) not supported? It seems to work as expected. Controller actions that receive ODataActionParameters do not seem to be fully supported, in that they do not respect parameters being required.

[kjkrum]

Correction: although the signature with separate FromForm parameters allows me to achieve what I want, it does not entirely work as expected. I just noticed that I forgot to remove Optional() from the params in the EDM. Removing [Required] on a method param so the Swagger UI allows it to be omitted exposes the opposite problem: the parameter is still treated as required even if it's optional in the EDM.

[julealgon]

@kjkrum note that, as @xuzhg mentioned, what you are doing is not supported by OData: you can't use FromForm and expect OData to detect your endpoint.

Just keep that in mind (that you are in standard MVC territory).

[kjkrum]

Conventional routing won't detect it. But if you use explicit attribute routing (which AFAIK you must always do for unbound actions anyway) the route is listed as an OData route in the $odata debug.

[julealgon]

Conventional routing won't detect it. But if you use explicit attribute routing (which AFAIK you must always do for unbound actions anyway) the route is listed as an OData route in the $odata debug.

Oh I'm very surprised to hear that.... I wonder then if you are just hitting some undefined behavior boundary, or if it really is supported at the end of the day.

@xuzhg can you clarify a bit more on this one?

[kjkrum]

To be clear, I'd prefer to use ODataActionParameters if there's a way to make it treat required parameters as required. I'm looking at ODataBodyModelBinder...

[kjkrum]

I think the issue is in ODataActionPayloadDeserializer. It iterates over the properties that are found in the payload and looks up the corresponding IEdmOperationParameter. This is backward. It should iterate over IEdmOperation.Parameters and find each one in the payload. If a required parameter is not found, it should explode. There's no "is optional" in IEdmOperationParameter; optional parameters implement IEdmOptionalParameter. That interface includes what the default value should be if the parameter isn't present in the payload.

The relevant section of the spec is 11.5.5.1, Invoking an Action.

When invoking an action through an action import all parameter values MUST be passed in the request body according to the particular format.

This is not currently enforced.

Section 6.2 suggests that unrecognized properties in the payload should be ignored. Currently, they cause binding to fail because of the inside-out iteration and the assertion on line 87.

[kjkrum]

This is a lock the doors, order pizza, nobody goes home until it's fixed level bug, and it's been over a year. What is going on at Microsoft?

[wgibbons1]

I'm getting similar issues. When I send a payload with an extra field to my POST endpoint, instead of ignoring the field it responds with an error payload saying "Product is required". When I send an incorrect type for one of my payload fields, the response error message is "product is required" instead of "name is invalid". It's unfortunately looking like I'm going to have to put all my POSTs and PUTs in a normal ControllerBase controller instead.

[julealgon]

This is a lock the doors, order pizza, nobody goes home until it's fixed level bug, and it's been over a year. What is going on at Microsoft?

I don't personally understand why you consider this issue that critical... you can just add your own validation temporarily until it is resolved, can't you?

Again, not disagreeing with you on the fact that this should be looked at, but to me this is just not that critical compared to a multitude of other issues in the OData repo.

[kjkrum]

@julealgon The incorrect implementation of the payload deserializer means that OData does not and fundamentally cannot implement its own spec. Every OData 8.x API in the world is failing to enforce its requirements, unless you think a bunch of people noticed this and worked around it but never reported it. It's been this way for years. When it's finally fixed, a whole lot of APIs are suddenly going to start returning errors for invalid payloads. But that's better than their current state of letting those payloads pass.