Skip to content

We need a solution for API Explorer + Custom ModelBinder #4825

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

Open
rynowak opened this issue Feb 23, 2017 · 10 comments
Open

We need a solution for API Explorer + Custom ModelBinder #4825

rynowak opened this issue Feb 23, 2017 · 10 comments
Labels
affected-few This issue impacts only small number of customers area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-openapi Needs: Design This issue requires design work before implementating. severity-major This label is used by an internal tool
Milestone

Comments

@rynowak
Copy link
Member

rynowak commented Feb 23, 2017

see: aspnet/Mvc#5673 (comment) and domaindrivendev/Swashbuckle.AspNetCore#309

The complaint here is that we 'expand' a parameter in the API definition that's being bound by a custom model binder. API explorer has no interaction with model binders/providers, and so no way to know that this isn't being handled by the default binder.

In this case we already have a metadata feature that would give the desired API explorer output, ModelMetadata.IsComplexType == false. However we don't allow user code to configure this value.

We should either make this configurable, or probe the model binders for information about types that have custom handling.

@rynowak
Copy link
Member Author

rynowak commented Feb 23, 2017

/cc @domaindrivendev

@pranavkm
Copy link
Contributor

Moving this to 3.0

@aspnet-hello aspnet-hello transferred this issue from aspnet/Mvc Dec 13, 2018
@aspnet-hello aspnet-hello assigned pranavkm and unassigned pranavkm Dec 13, 2018
@aspnet-hello aspnet-hello added this to the 3.0.0-preview2 milestone Dec 13, 2018
@aspnet-hello aspnet-hello added 1 - Ready area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates Needs: Design This issue requires design work before implementating. labels Dec 13, 2018
@tai-yi
Copy link

tai-yi commented Apr 3, 2019

Very useful, sometimes we use a complex type as an action input parameter(in controller) worked with ModelBindProvider, but the got a terrible experience when use API Explorer.

Please consider below points:

  1. Raw input type

  2. Multi-input as one action input parameter

    //Controller action 
    [HttpGet]
    public ActionResult<MyResult> WorkwithMyContext(MyFlowContext flow){
        ...
    }    
        
    //Complex Customer Model
    public class MyFlowContext{
        public MyFlow Flow {get;set;}
        public Data Data {get;set;}
    }
    
    public class MyOwnContextModelBinder{
        public Task BindModelAsync(ModelBindingContext bindingContext){
            ...
            //I need two parameters to load the Flow
            string flowId = ""; //from url
            string activityId = ""; //from url
            var flow = LoadMyFlow(flowId,activityId);
            var data = LoadMyData(flow);
            // validation
            var flowCtx =  new MyFlowContext(){                
                Flow = flow,
                Data = data
            }
            
            ...
        }
    }
  3. ModelValidation

    No need skip auto model validation after CustomerModelBinder Success.

    You know the sub properties are too many, validating them is too expensive, most validations are unnecessary.

@mkArtakMSFT mkArtakMSFT modified the milestones: 3.0.0-preview6, Backlog Apr 19, 2019
@pranavkm pranavkm removed their assignment Aug 21, 2019
@jwisener

This comment has been minimized.

@pranavkm pranavkm added affected-few This issue impacts only small number of customers enhancement This issue represents an ask for new feature or an enhancement to an existing one severity-major This label is used by an internal tool and removed cost: S labels Nov 6, 2020
@statler
Copy link

statler commented Nov 23, 2020

This was last linked to the v3 preview and backlogged. Any update on when it might be worked on? It seems like a relatively small fix to something that completely breaks the API Explorer. The issue isnt just that it doesnt work, the issue is that it creates incorrect API signatures that cannot be used as specified in the API Explorer.

In addition to the proposed solution, an alternative may be do allow an attribute on a contoller method parameter - something similar to ProducesResponseType, except on the input side. E.g.

public async Task<IActionResult> GetApprovalCategory([RequiresType(typeof(MyProxyType))] DataSourceLoadOptions loadOptions)

This would enable us to define a lightweight type that mirrored the expected parameter structure, and we could have complete control over what was documented. It is verbose, but in the absence of other solutions it would at least get the job done - unless something like this exists that I have missed somewhere?

In absence of a solution, is there any workaround?

@statler
Copy link

statler commented Mar 30, 2021

Nothing?

@jcracknell
Copy link

jcracknell commented Apr 15, 2021

Edit: replaced my earlier hacky solution.

I've done some investigation and the essence of the problem appears to be that the DefaultApiDescriptionProvider automatically expands parameters/properties according to default modelbinding conventions whenever they are (a) "complex" or (b) associated to a "non-greedy" BindingSource.

var source = bindingContext.BindingSource;
if (source != null && source.IsGreedy)
{
// We have a definite answer for this model. This is a greedy source like
// [FromBody] so there's no need to consider properties.
Context.Results.Add(CreateResult(bindingContext, source, containerName));
return;
}

There's a whole can of worms here with regards to whether or not it makes sense to claim that an input is greedy, but we're not going there.

A type is considered "complex" if it lacks a TypeConverter handling strings:

IsComplexType = !TypeDescriptor.GetConverter(ModelType).CanConvertFrom(typeof(string));

A TypeConverter is a reasonable way to deal with this, as generally your parameters should be a serialization model anyways. The problem you run into in non-trivial cases is that it is very difficult to access your IServiceProvider (and by extension e.g. your System.Text.Json.JsonSerializationOptions).

What you can do is to install a patched version of SimpleTypeModelBinder via MvcOptions.ModelBinderProviders, specifically amending this call to provide an ITypeDescriptorContext implementing IServiceProvider by delegating to ModelBindingContext.ActionContext.HttpContext.RequestServices:

model = _typeConverter.ConvertFrom(
context: null,
culture: valueProviderResult.Culture,
value: value);

@pranavkm pranavkm added old-area-web-frameworks-do-not-use *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 19, 2021
@ghost
Copy link

ghost commented Dec 28, 2021

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@vyrotek
Copy link

vyrotek commented Sep 8, 2022

Was this still being looked at? All the workarounds I've seen are hacky. e.g. Adding a dummy TypeConverter just to add "string" support when we already know a custom ModelBinder exists to handle our custom struct type.

Unfortunately, this suggestion doesn't really work well for us.
domaindrivendev/Swashbuckle.AspNetCore#309 (comment)

We still need to create a custom ModelBinder because we need to inject some services to perform the conversions. ModelBinders have access to bindingContext.HttpContext.RequestService.

It would be great if this SimpleTypeModelBinder provided a context reference instead of passing null here.

@ghost
Copy link

ghost commented Oct 11, 2022

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@captainsafia captainsafia added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates and removed old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Jun 20, 2023
@captainsafia captainsafia modified the milestones: .NET 8 Planning, Backlog Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected-few This issue impacts only small number of customers area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-openapi Needs: Design This issue requires design work before implementating. severity-major This label is used by an internal tool
Projects
No open projects
Status: No status
Development

No branches or pull requests