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

The model is null in POST method when api-version is not specified and AssumeDefaultVersionWhenUnspecified = true #745

Closed
alisovoy opened this issue May 26, 2021 · 3 comments

Comments

@alisovoy
Copy link

alisovoy commented May 26, 2021

I have a small OData Api with one Controller:

[ApiVersion("1.0")]
[ApiVersion("2.0")]
public class AssetsController : ODataController
{

	public IEnumerable<Asset> Get()
	{
		return Assets;
	}

	public Asset Get(string key)
	{
		return Assets.FirstOrDefault(a => a.Id == key);
	}

	public IActionResult Post([FromBody] Asset asset, ApiVersion version)
	{
		Assets.Add(asset);
		return Ok();
	}

	[MapToApiVersion("2.0")]
	public IActionResult Patch([FromODataUri] string key, Delta<Asset> assetDelta, ApiVersion version)
	{
		var asset = Assets.FirstOrDefault(a => a.Id == key);
		if (asset != null)
		{
			assetDelta.Patch(asset);
		}
		return Ok();
	}

	private static readonly List<Asset> Assets = new List<Asset>()
	{
		new Asset() { Id = "1", Author = "Johny", Name = "Car", Data = "some data 01" },
		new Asset() { Id = "2", Author = "Karl", Name = "Boat", Data = "some data 02" },
		new Asset() { Id = "3", Author = "Ben", Name = "Plane", Data = "some data 03" },
	};
}

Model:

public class Asset
{
	public string Id { get; set; }
	public string Name { get; set; }
	public string Data { get; set; }
	public string Author { get; set; }
}

public class AssetModelConfiguration : IModelConfiguration
{
	private EntityTypeConfiguration<Asset> ConfigureCurrent(ODataModelBuilder builder)
	{
		return builder.EntitySet<Asset>("Assets").EntityType;
	}

	public void Apply(ODataModelBuilder builder, ApiVersion apiVersion, string routePrefix)
	{
		ConfigureCurrent(builder);
	}
}

and the setup:

	public void ConfigureServices(IServiceCollection services)
	{

		services.AddControllers();

		services.AddApiVersioning(options =>
		{
			options.ReportApiVersions = true;
			options.DefaultApiVersion = new ApiVersion(1, 0);
			options.AssumeDefaultVersionWhenUnspecified = true;
			options.ApiVersionReader = ApiVersionReader.Combine(new QueryStringApiVersionReader(), new HeaderApiVersionReader("api-version"));
		});

		services.AddOData().EnableApiVersioning();
	}

	public void Configure(IApplicationBuilder app, IWebHostEnvironment env, VersionedODataModelBuilder modelBuilder)
	{
		if (env.IsDevelopment())
		{
			app.UseDeveloperExceptionPage();
		}
		app.UseHttpsRedirection();
		app.UseRouting();

		app.UseEndpoints(endpoints =>
		{
			endpoints.MapControllers();
			endpoints.Select().Expand().Filter().OrderBy().MaxTop(100).Count();
			endpoints.MapVersionedODataRoute("ODataRoute", "odata", modelBuilder);
		});
	}

If I do a post JSON without specifying the api-version:

	POST https://localhost:44354/odata/assets HTTP/1.1
	Content-Type: application/json
	{"id":"4","name":"Car4","data4":"some data 01","author":"Jan4"}

the default version is taken correctly, BUT model is null:

NULL

If I change the type of parameter to object:

image

the input parameter is not null but of type "System.Text.Json.JsonElement".

Why the json cannot be deserialized without specifying explicitly the api version?

if I add ?api-version=X.X everything works as expected and the input is nicely deserialized to the Asset model

@commonsensesoftware
Copy link
Collaborator

This ultimately has to do with OData model binding. The type should never be System.Text.Json.JsonElement. OData uses its own JSON serialization/deserialization code, though it might use some of the common libraries under the hood. object isn't supported by OData, but it is supported by vanilla ASP.NET Core. Despite how it might seem, neither path actually worked by going through OData.

Since you say everything works as expected when the API version is explicitly specified, it seems the default version is not being set or resolved early enough. This would affect the resolution of the corresponding EDM and could produce the result you are seeing. I'll have to investigate.

To unblock yourself, there are a few options. This is almost certainly a result of enabling AssumeDefaultVersionWhenUnspecified = true. This feature is highly misused and abused. It's only meant for backward compatibility. If you don't need that, you shouldn't use it. If you really need this feature, then you will likely get things to work with very, very simple middleware that explicitly sets the default API version. It needs to be registered before endpoint routing.

var options = context.RequestServices.GetRequires<IOptions<ApiVersioningOptions>>();
var feature = context.Features.Get<IApiVersioningFeature>();
feature.RequestedApiVersion ??= options.DefaultApiVersion;
app.UseMyMiddleware();
app.UseEndpoints(endpoints => { });

@alisovoy
Copy link
Author

Hello Chris,
It works!
Thank you very much for your fast reply and help with the provided workaround.

@commonsensesoftware
Copy link
Collaborator

Glad you have a functioning workaround. I have confirmed that this behavior is, in fact, a bug. This issue only happens when options.AssumeDefaultVersionWhenUnspecified = true; is configured.

There is a strong affinity between API version and EDM. The IEdmModelSelector chooses the best EDM based on the incoming request. If there is no API version available, it will now fallback to resolving the API version via IApiVersionSelector similar to how actions are selected. When all else fails, the selection process defaults to the highest (e.g. most current) EDM. This typically only happens when EDM model selection is outside of a request.

This is why the model was null. The EDM 3.0 was being selected, but it doesn't exist for Assets. The action selection process correctly used the IApiVersionSelector to determine that the 1.0 version of the action should be used for an unspecified version. The consequence is a correctly routed action with a mismatched EDM, which results in null.

This behavior will be corrected in the next patch. Thanks for all the input. The workaround should suffice in the meantime.

commonsensesoftware added a commit that referenced this issue Jun 24, 2021
commonsensesoftware added a commit that referenced this issue Mar 21, 2022
commonsensesoftware added a commit that referenced this issue Mar 21, 2022
commonsensesoftware added a commit that referenced this issue Mar 21, 2022
commonsensesoftware added a commit that referenced this issue Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants