QueryFieldAttribute doesn't respect the default value set for optional parameters

[joaomariomz]

Describe the bug If a parameter is set as optional on the controller method and it's missing in the query string of a request, QueryFieldAttribute logic will default that parameter to the Type's default instead of using the default that is set in the method.

To Reproduce

1. Add the following optional parameter to a controller: [QueryField]bool myOptionalBool = true
2. Make a request to that endpoint not passing myOptionalBool in the query string.

Expected behavior myOptionalBool parameter should be defaulted to true

Actual behavior myOptionalBool parameter is set to false

[geoperez]

Makes sense, thanks for the PR. Let's wait for @rdeago approval.

[rdeago]

Sorry, I didn't approve the PR. Explanation is here.

[rdeago]

@joaomariomz please don't take this personally. It's not entirely your fault: web API classes are so complicated that I keep forgetting how they work in detail - and I wrote them! 🙈 🤦‍♂

Please join us on Slack. 😉

[rdeago]

From a review of the current code, it turns out this is not the only problem with QueryFieldAttribute and related classes.

Parameter default values are never respected. This affects FormDataAttribute, FormFieldAttribute, QueryDataAttribute, and QueryFieldAttribute, as well as any user-defined mechanism based on the two IRequestDataAttribute generic interfaces.

To fix this issue, said interfaces have to be changed so that their GetRequestDataAsync methods take an additional defaultValue parameter. The only class that actually uses those methods is WebApiModuleBase, in a context where it can easily retrieve the default value for the relevant parameter or, in its absence, pass the default value for the parameter type.

It's a breaking change, but it's OK to do it now that the master branch is "in flux", on its way to version 4.0.

Oh, but wait... A default value doesn't always make sense. FormDataAttribute.GetRequestDataAsync and QueryDataAttribute.GetRequestDataAsync return an empty NameValueCollection if the request lacks any data; this is actually better than returning null.

A distinction should probably be made between parameters that get the whole of a kind of data in a request, and parameters that get a single field. For the former, a default value doesn't seem to make sense. We might need separate interfaces (IRequestDataAttribute and IRequestFieldAttribute) to deal with both kinds of parameters correctly.

While we're at it, I'll confess that I even named those interfaces incorrectly. Interface names should not end in Attribute. Better names could be IRequestDataProvider and IRequestFieldProvider; please feel free to suggest something more appropriate.

---

Let's do specs!

Before matters get out of control, I'm laying down a sort of formal specification. Please @geoperez, @joaomariomz, @wbarrionuevo, @russplaysguitar, and actually anyone reading this and using WebApiModule, please read and comment. Once the spec is sound, we can think about fixing the issue once and for all.

I'll assume that IRequestDataProvider and IRequestFieldProvider interfaces already exist. The former are the current INonNullRequestDataAttribute interfaces, properly renamed; the latter are similar to the current IRequestDataAttribute interfaces, but add a defaultValue parameter to their GetRequestDataAsync methods (probably renamed to GetRequestFieldAsync).

Definitions

Term	Definition
Web API module	A subclass of EmbedIO.WebApi.WebApiModuleBase.
Web API controller	A subclass of EmbedIO.WebApi.WebApiController.
Web API method	A public, non-generic, instance method of a web API controller, having an attribute of type EmbedIO.Routing.RouteAttribute.
Web API parameter	A parameter specification, part of the signature of a web API method.
Request data provider	A subclass of System.Attribute, applicable to method parameters, that implements at least one of the IRequestDataProvider interfaces defined in the EmbedIO.WebApi namespace.
Request field provider	A subclass of System.Attribute, applicable to method parameters, that implements at least one of the IRequestFieldProvider interfaces defined in the EmbedIO.WebApi namespace.

Categories of web API parameters

Upon registering a web API controller in a web API module, every parameter of every web API method of the former is classified into one of the following categories.

Request data parameters

Request data parameters are web API parameters that have an attribute which is a request data provider.

These parameters do not need a default value.

Example:

[Route(HttpVerb.Get, "/survey/submit")]
public async Task SubmitSurvey([FormData] NameValueCollection data)
{
    // If the request has no form data, data will be empty, not null.
}

OPEN QUESTION: Should we throw an exception if a request data parameter has a default value?

Request field parameters

Request field parameters are web API parameters that have an attribute which is a request field provider.

These parameters can have a default value. The default value is passed to the web API method if the desired field is not present in the request, unless the request field provider defines another way to handle such situation (such as a BadRequestIfMissing property that, if true, causes a HttpBadRequestException to be thrown).

If the parameter has no default value, the default value for the type of the parameter takes its place in the above procedure.

Example:

[Route(HttpVerb.Get, "/login")]
public async Task SubmitSurvey([FormField(true)] string name, [FormField("pwd")] string password = string.Empty)
{
    // No "name" field -> error 400
    // No "pwd" field -> password is empty
}

OPEN QUESTION: Should we throw an exception if a request field parameter has a default value and specifies that a missing field is an error? If so, this would require moving the BadRequestIfMissing property in the IRequestFieldProvider interface, so WebApiModuleBase.CompileHandler knows about it and can take appropriate measures.

Route-based parameters

Route-based parameters get their values from route parameters. They don't need any attribute, but their names must be equal to a route parameter. An example:

[Route(HttpVerb.Get, "/customer/{id}")]
public async Task<Customer> GetCustomer(long id)
{
    // "id" in route is not parsable as a long -> error 400
}

Other parameters

Any web API parameter that doesn't belong to any one of the categories above... shouldn't probably be there to start with. It is probably a typo in the name of a route-based parameter.

Currently, these parameters are accepted and their value is always the default value for their respective types.

OPEN QUESTION: Should we throw an exception if there are one or more parameters like this in a web API method?

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.